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

Commit Message

Paul Durrant Nov. 11, 2020, 8:07 p.m. UTC

From: Paul Durrant <[e-mail]>

... and make use of them in hvcall_flush()/need_flush().

Subsequent patches will need to deal with virtual processor masks potentially
wider than 64 bits. Thus, to avoid using too much stack, this patch
introduces global per-cpu virtual processor masks and converts the
implementation of hvcall_flush() to use them.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Wei Liu <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>
---
 xen/arch/x86/hvm/viridian/viridian.c | 51 +++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

Comments

Jan Beulich Nov. 12, 2020, 8:45 a.m. UTC | #1

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?

> +{
> +    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?

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.

> +        {
> +            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?

Jan

Paul Durrant Nov. 19, 2020, 4:02 p.m. UTC | #2

> -----Original Message-----
> From: Jan Beulich <[e-mail]
> Sent: 12 November 2020 08:46
> To: Paul Durrant <[e-mail]>
> Cc: Paul Durrant <[e-mail]>; Wei Liu <[e-mail]>; Andrew Cooper
> <[e-mail]>; Roger Pau Monné <[e-mail]>; [e-mail]
> 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

Jan Beulich Nov. 19, 2020, 4:41 p.m. UTC | #3

On 19.11.2020 17:02, Paul Durrant wrote:
>> From: Jan Beulich <[e-mail]
>> Sent: 12 November 2020 08:46
>>
>> 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.

I'd be curious to learn about that "misleading" aspect.

Jan

Durrant, Paul Nov. 19, 2020, 4:44 p.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 19 November 2020 16:41
> To: [e-mail]
> Cc: Durrant, Paul <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'Roger Pau Monné' <[e-mail]>; [e-mail]
> Subject: RE: [EXTERNAL] [PATCH 03/10] viridian: introduce a per-cpu hypercall_vpmask and accessor
> functions...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 19.11.2020 17:02, Paul Durrant wrote:
> >> From: Jan Beulich <[e-mail]
> >> Sent: 12 November 2020 08:46
> >>
> >> 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.
> 
> I'd be curious to learn about that "misleading" aspect.
> 

Because the function is modifying (zero-ing) the bitmap... so implying the mask is const is measleading.

  Paul

> Jan

Jan Beulich Nov. 19, 2020, 4:46 p.m. UTC | #5

On 19.11.2020 17:44, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 19 November 2020 16:41
>> To: [e-mail]
>> Cc: Durrant, Paul <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Andrew Cooper'
>> <[e-mail]>; 'Roger Pau Monné' <[e-mail]>; [e-mail]
>> Subject: RE: [EXTERNAL] [PATCH 03/10] viridian: introduce a per-cpu hypercall_vpmask and accessor
>> functions...
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open
>> attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 19.11.2020 17:02, Paul Durrant wrote:
>>>> From: Jan Beulich <[e-mail]
>>>> Sent: 12 November 2020 08:46
>>>>
>>>> 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.
>>
>> I'd be curious to learn about that "misleading" aspect.
>>
> 
> Because the function is modifying (zero-ing) the bitmap... so implying
> the mask is const is measleading.

Oh, I was mislead by the name then; should have looked at the return
type (which I was implying to be bool, when it's void). Please
disregard my request(s) in such case(s).

Jan

Patch

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index c4f720f58d6d..4ab1f14b2248 100644
--- 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)
+{
+    bitmap_zero(vpmask->mask, HVM_MAX_VCPUS);
+}
+
+static void vpmask_set(struct hypercall_vpmask *vpmask, unsigned int vp)
+{
+    __set_bit(vp, vpmask->mask);
+}
+
+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);
+}
+
 /*
  * Windows should not issue the hypercalls requiring this callback in the
  * case where vcpu_id would exceed the size of the mask.
  */
 static bool need_flush(void *ctxt, struct vcpu *v)
 {
-    uint64_t vcpu_mask = *(uint64_t *)ctxt;
+    struct hypercall_vpmask *vpmask = ctxt;
 
-    return vcpu_mask & (1ul << v->vcpu_id);
+    return vpmask_test(vpmask, v->vcpu_id);
 }
 
 union hypercall_input {
@@ -546,6 +572,7 @@  static int hvcall_flush(union hypercall_input *input,
                         unsigned long input_params_gpa,
                         unsigned long output_params_gpa)
 {
+    struct hypercall_vpmask *vpmask = &this_cpu(hypercall_vpmask);
     struct {
         uint64_t address_space;
         uint64_t flags;
@@ -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++)
+        {
+            if ( !input_params.vcpu_mask )
+                break;
+
+            if ( input_params.vcpu_mask & 1 )
+                vpmask_set(vpmask, vp);
+
+            input_params.vcpu_mask >>= 1;
+        }
+    }
 
     /*
      * A false return means that another vcpu is currently trying
      * a similar operation, so back off.
      */
-    if ( !paging_flush_tlb(need_flush, &input_params.vcpu_mask) )
+    if ( !paging_flush_tlb(need_flush, vpmask) )
         return -ERESTART;
 
     output->rep_complete = input->rep_count;