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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi Jan,

On 30.10.25 13:23, Jan Beulich wrote:
> 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 ?
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