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 ?
> --- 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.
> --- 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.
> @@ -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.)
> @@ -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.
Jan
