[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v2 08/12] viridian: add ExProcessorMasks variants of the flush hypercalls
> -----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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |