Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall

[Julien Grall <julien@xxxxxxx>]

On 21/04/2020 06:54, Jan Beulich wrote:
> On 20.04.2020 19:53, Julien Grall wrote:
> > > --- a/xen/common/domain.c
> > > +++ b/xen/common/domain.c
> > > @@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc
> > >        return rc;
> > >    }
> > >    -#ifdef VM_ASSIST_VALID
> > > -long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
> > > -               unsigned long valid)
> > > +#ifdef arch_vm_assist_valid
> > How about naming the function arch_vm_assist_valid_mask?
> Certainly a possibility, albeit to me the gain would be marginal
> and possibly not outweigh the growth in length. Andrew, any
> preference?
You have a point regarding the length of the function.

> > > --- a/xen/include/asm-x86/domain.h
> > > +++ b/xen/include/asm-x86/domain.h
> > > @@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup
> > >       pv_inject_event(&event);
> > > }
> > > +#define PV_VM_ASSIST_VALID  ((1UL << VMASST_TYPE_4gb_segments)        | \
> > > +                             (1UL << VMASST_TYPE_4gb_segments_notify) | \
> > > +                             (1UL << VMASST_TYPE_writable_pagetables) | \
> > > +                             (1UL << VMASST_TYPE_pae_extended_cr3)    | \
> > > +                             (1UL << VMASST_TYPE_architectural_iopl)  | \
> > > +                             (1UL << VMASST_TYPE_runstate_update_flag)| \
> > > +                             (1UL << VMASST_TYPE_m2p_strict))
> > > +#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag)
> > > +
> > > +#define arch_vm_assist_valid(d) \
> > > +    (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \
> > > +                      : is_pv_32bit_domain(d) ? (uint32_t)PV_VM_ASSIST_VALID \
> > I understand this is matching the current code, however without
> > looking at the rest of patch this is not clear why the cast. May
> > I suggest to add a comment explaining the rationale?
> Hmm, I can state that the rationale is history. Many of the assists in
> the low 32 bits are for 32-bit guests only. But we can't start refusing
> a 64-bit kernel requesting them. The ones in the high 32 bits are, for
> now, applicable to 64-bit guests only, and have always been refused for
> 32-bit ones.
>
> Imo if anything an explanation on where new bits should be put should
> go next to the VMASST_TYPE_* definitions in the public header, yet then
> again the public headers aren't (imo) a good place to put
> implementation detail comments.
How about splitting PV_VM_ASSIST_VALID in two? One would contain 64-bit 
PV specific flags and the other common PV flags?
This should make the code more obvious and easier to read for someone 
less familiar with the area.
It also means we could have a BUILD_BUG_ON() to check at build time that 
no flags are added above 32-bit.
Cheers,

--
Julien Grall