RE: [PATCH 06/10] viridian: add ExProcessorMasks variants of the flush hypercalls

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 12 November 2020 09:19
> 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 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