> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 12 November 2020 09:19
> To: Paul Durrant <p...@xen.org>
> Cc: Paul Durrant <pdurr...@amazon.com>; Wei Liu <w...@xen.org>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 06/10] viridian: add ExProcessorMasks variants of the
> flush hypercalls
>
> On 11.11.2020 21:07, Paul Durrant wrote:
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -553,6 +553,83 @@ static unsigned int vpmask_next(struct
> > hypercall_vpmask *vpmask, unsigned int
> vp
> > (vp) < HVM_MAX_VCPUS; \
> > (vp) = vpmask_next(vpmask, vp))
> >
> > +struct hypercall_vpset {
> > + struct hv_vpset set;
> > + uint64_t __bank_contents[64];
>
> gcc documents this to be supported as an extension; did you check
> clang supports this, too?
By 'this', do you mean the assumption that that memory layout is consecutive?
> (I'd also prefer if the leading
> underscores could be dropped, but as you know I'm not the maintainer
> of this code.)
>
> > +static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
> > +{
> > + uint64_t bank_mask;
> > + unsigned int nr = 0;
> > +
> > + for ( bank_mask = vpset->valid_bank_mask; bank_mask; bank_mask >>= 1 )
> > + if ( bank_mask & 1 )
> > + nr++;
> > +
> > + return nr;
> > +}
>
> Simply use hweight64()?
>
Ok.
> > +static uint16_t hv_vpset_to_vpmask(struct hv_vpset *set, size_t size,
> > + struct hypercall_vpmask *vpmask)
> > +{
> > + switch ( set->format )
> > + {
> > + case HV_GENERIC_SET_ALL:
> > + vpmask_fill(vpmask);
> > + return 0;
> > +
> > + case HV_GENERIC_SET_SPARSE_4K:
> > + {
> > + uint64_t bank_mask;
> > + unsigned int bank = 0, vp = 0;
> > +
> > + vpmask_empty(vpmask);
> > + for ( bank_mask = set->valid_bank_mask; bank_mask; bank_mask >>= 1
> > )
> > + {
> > + /* Make sure we won't dereference past the end of the array */
> > + if ( (void *)(set->bank_contents + bank) >=
> > + (void *)set + size )
> > + {
> > + ASSERT_UNREACHABLE();
> > + return -EINVAL;
> > + }
>
> Doesn't this come too late? I.e. don't you want to check instead
> (or also) that you won't overrun the space when copying in from
> the guest? And for the specific purpose here, doesn't it come too
> early, as you won't access any memory when the low bit of bank_mask
> isn't set?
>
I'll dry-run this again. It may make more sense to move the check.
Paul
> Jan
