RE: [PATCH v2 08/12] viridian: add ExProcessorMasks variants of the flush hypercalls

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 24 November 2020 16:56
> To: Paul Durrant <paul@xxxxxxx>
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 08/12] viridian: add ExProcessorMasks variants of the 
> flush hypercalls
> 
> On 20.11.2020 10:48, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > The Microsoft Hypervisor TLFS specifies variants of the already implemented
> > HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST hypercalls that take a 'Virtual
> > Processor Set' as an argument rather than a simple 64-bit mask.
> >
> > This patch adds a new hvcall_flush_ex() function to implement these
> > (HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST_EX) hypercalls. This makes use of
> > new helper functions, hv_vpset_nr_banks() and hv_vpset_to_vpmask(), to
> > determine the size of the Virtual Processor Set (so it can be copied from
> > guest memory) and parse it into hypercall_vpmask (respectively).
> >
> > NOTE: A guest should not yet issue these hypercalls as 'ExProcessorMasks'
> >       support needs to be advertised via CPUID. This will be done in a
> >       subsequent patch.
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> Just a couple of cosmetic remarks, apart from them this looks
> good to me, albeit some of the size calculations are quite,
> well, involved. I guess like with most parts if this series,
> in the end Wei will need to give his ack.
> 
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -576,6 +576,70 @@ static unsigned int vpmask_nr(const struct 
> > hypercall_vpmask *vpmask)
> >      return bitmap_weight(vpmask->mask, HVM_MAX_VCPUS);
> >  }
> >
> > +#define HV_VPSET_BANK_SIZE \
> > +    sizeof_field(struct hv_vpset, bank_contents[0])
> > +
> > +#define HV_VPSET_SIZE(banks)   \
> > +    (sizeof(struct hv_vpset) + (banks * HV_VPSET_BANK_SIZE))
> 
> Personally I think this would be better done using
> offsetof(struct hv_vpset, bank_contents), but you're the maintainer.
> However, "banks" wants parenthesizing, just in case.
> 

No, I actually prefer using offsetof() and yes I do indeed need to parenthesize 
'banks'.

> > +#define HV_VPSET_MAX_BANKS \
> > +    (sizeof_field(struct hv_vpset, valid_bank_mask) * 8)
> > +
> > +struct hypercall_vpset {
> > +    union {
> > +        struct hv_vpset set;
> > +        uint8_t pad[HV_VPSET_SIZE(HV_VPSET_MAX_BANKS)];
> > +    };
> > +};
> 
> A struct with just a union as member could be expressed as a simple
> union - any reason you prefer the slightly more involved variant?
> 

Not really... it's only that it was a struct in the original patch. I'll change 
to using a union.

> > +static DEFINE_PER_CPU(struct hypercall_vpset, hypercall_vpset);
> > +
> > +static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
> > +{
> > +    return hweight64(vpset->valid_bank_mask);
> > +}
> > +
> > +static uint16_t hv_vpset_to_vpmask(struct hv_vpset *set,
> 
> const?
> 

Ok.

> > @@ -656,6 +720,78 @@ static int hvcall_flush(union hypercall_input *input,
> >      return 0;
> >  }
> >
> > +static int hvcall_flush_ex(union hypercall_input *input,
> 
> const again?
> 

True, but I'll need to go back and do that for the others too.

> > +                           union hypercall_output *output,
> > +                           unsigned long input_params_gpa,
> > +                           unsigned long output_params_gpa)
> 
> Mainly for cosmetic reasons and to be in sync with
> hvm_copy_from_guest_phys()'s respective parameter, perhaps both
> would better be paddr_t?
> 

Ok. Again I'll fix the prior patches to match.

  Paul

> Jan