RE: [PATCH 03/10] viridian: introduce a per-cpu hypercall_vpmask and accessor functions...

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx
> Sent: 12 November 2020 08:46
> 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 03/10] viridian: introduce a per-cpu hypercall_vpmask and 
> accessor functions...
> 
> 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
> > @@ -507,15 +507,41 @@ void viridian_domain_deinit(struct domain *d)
> >      XFREE(d->arch.hvm.viridian);
> >  }
> >
> > +struct hypercall_vpmask {
> > +    DECLARE_BITMAP(mask, HVM_MAX_VCPUS);
> > +};
> > +
> > +static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
> > +
> > +static void vpmask_empty(struct hypercall_vpmask *vpmask)
> 
> const?

Yes, I suppose that's ook for all these since the outer struct is not 
changing... It's a bit misleading though.

> 
> > +{
> > +    bitmap_zero(vpmask->mask, HVM_MAX_VCPUS);
> > +}
> > +
> > +static void vpmask_set(struct hypercall_vpmask *vpmask, unsigned int vp)
> > +{
> > +    __set_bit(vp, vpmask->mask);
> 
> Perhaps assert vp in range here and ...
> 
> > +}
> > +
> > +static void vpmask_fill(struct hypercall_vpmask *vpmask)
> > +{
> > +    bitmap_fill(vpmask->mask, HVM_MAX_VCPUS);
> > +}
> > +
> > +static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
> > +{
> > +    return test_bit(vp, vpmask->mask);
> 
> ... here?

Ok.

> 
> This also wants const again.
> 
> > @@ -567,13 +594,29 @@ static int hvcall_flush(union hypercall_input *input,
> >       * so err on the safe side.
> >       */
> >      if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
> > -        input_params.vcpu_mask = ~0ul;
> > +        vpmask_fill(vpmask);
> > +    else
> > +    {
> > +        unsigned int vp;
> > +
> > +        vpmask_empty(vpmask);
> > +        for (vp = 0; vp < 64; vp++)
> 
> Nit: Missing blanks.
> 

Oh yes. You can tell I was moving between this and libxl code :-)

> > +        {
> > +            if ( !input_params.vcpu_mask )
> > +                break;
> > +
> > +            if ( input_params.vcpu_mask & 1 )
> > +                vpmask_set(vpmask, vp);
> > +
> > +            input_params.vcpu_mask >>= 1;
> > +        }
> 
> Wouldn't bitmap_zero() + bitmap_copy() (in a suitable wrapper)
> be quite a bit cheaper a way to achieve the same effect?
> 

Yes, I guess that would work.

  Paul

> Jan