Re: [XEN][PATCH 3/3] x86/hvm: vmx: refactor cache disable mode data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.10.2025 00:54, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> 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 <grygorii_strashko@xxxxxxxx>

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