Hi Jan,
On 30.10.25 13:23, Jan Beulich wrote:
On 30.10.2025 00:54, Grygorii Strashko wrote:
From: Grygorii Strashko <[email protected]>
The Cache Disable mode data is used only by VMX code, so move it from
common HVM structures into VMX specific structures:
- move "uc_lock", "is_in_uc_mode" fields from struct hvm_domain to struct
vmx_domain;
- move "cache_mode" field from struct hvm_vcpu to struct vmx_vcpu.
Hence, the "is_in_uc_mode" field is used directly in mm/shadow/multi.c
_sh_propagate(), introduce the hvm_is_in_uc_mode() macro to avoid direct
access to this field and account for INTEL_VMX configuration.
Signed-off-by: Grygorii Strashko <[email protected]>
Requested-by: Andrew ?
ok
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -583,6 +583,7 @@ static int cf_check vmx_domain_initialise(struct domain *d)
int rc;
d->arch.ctxt_switch = &csw;
+ spin_lock_init(&d->arch.hvm.vmx.uc_lock);
I don't think this is the best place; in any event it wants to be separated from
adjacent code by a blank line. I'd prefer if it was put ...
/*
* Work around CVE-2018-12207? The hardware domain is already permitted
... below this CVE workaround.
ok
--- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
+++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
@@ -46,7 +46,9 @@ struct ept_data {
#define _VMX_DOMAIN_PML_ENABLED 0
#define VMX_DOMAIN_PML_ENABLED (1ul << _VMX_DOMAIN_PML_ENABLED)
+
struct vmx_domain {
+ spinlock_t uc_lock;
mfn_t apic_access_mfn;
/* VMX_DOMAIN_* */
unsigned int status;
Any reason to make this the very first field of the struct? It might better
live adjacent to the other field you move; there's going to be some padding
anyway, afaict.
I've tried to put fields in holes and checked with pahole.
With current change it is:
struct vmx_domain {
spinlock_t uc_lock; /* 0 8 */
mfn_t apic_access_mfn; /* 8 8 */
unsigned int status; /* 16 4 */
_Bool exec_sp; /* 20 1 */
_Bool is_in_uc_mode; /* 21 1 */
/* size: 24, cachelines: 1, members: 5 */
/* padding: 2 */
/* last cacheline: 24 bytes */
};
It seems can be grouped like below?:
struct vmx_domain {
mfn_t apic_access_mfn; /* 0 8 */
unsigned int status; /* 8 4 */
_Bool exec_sp; /* 12 1 */
_Bool is_in_uc_mode; /* 13 1 */
/* XXX 2 bytes hole, try to pack */
spinlock_t uc_lock; /* 16 8 */
/* size: 24, cachelines: 1, members: 5 */
/* sum members: 22, holes: 1, sum holes: 2 */
/* last cacheline: 24 bytes */
};
@@ -56,6 +58,12 @@ struct vmx_domain {
* around CVE-2018-12207 as appropriate.
*/
bool exec_sp;
+ /*
+ * If one of vcpus of this domain is in no_fill_mode or
+ * mtrr/pat between vcpus is not the same, set is_in_uc_mode.
+ * Protected by uc_lock.
+ */
+ bool is_in_uc_mode;
Imo while moving, the is_ prefix could also be dropped. It doesn't convey any
extra information on top of the in_, and I think we prefer is_*() also as
typically function(-like) predicates. (I.e. in hvm_is_in_uc_mode() I'm fine
with the name.)
ok
@@ -93,6 +101,9 @@ struct pi_blocking_vcpu {
spinlock_t *lock;
};
+#define NORMAL_CACHE_MODE 0
+#define NO_FILL_CACHE_MODE 2
As you necessarily touch all use sites, could we switch to the more common
CACHE_MODE_* at this occasion? Also imo these want to live ...
@@ -156,6 +167,9 @@ struct vmx_vcpu {
uint8_t lbr_flags;
+ /* Which cache mode is this VCPU in (CR0:CD/NW)? */
+ uint8_t cache_mode;
... right next to the field they belong to.
ok.
--
Best regards,
-grygorii
