[PATCH 00/10] viridian: add support for ExProcessorMasks

[PATCH 00/10] viridian: add support for ExProcessorMasks

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Paul Durrant (10):
  viridian: move flush hypercall implementation into separate function
  viridian: move IPI hypercall implementation into separate function
  viridian: introduce a per-cpu hypercall_vpmask and accessor
    functions...
  viridian: use hypercall_vpmask in hvcall_ipi()
  viridian: use softirq batching in hvcall_ipi()
  viridian: add ExProcessorMasks variants of the flush hypercalls
  viridian: add ExProcessorMasks variant of the IPI hypercall
  viridian: log initial invocation of each type of hypercall
  viridian: add a new '_HVMPV_ex_processor_masks' bit into
    HVM_PARAM_VIRIDIAN...
  xl / libxl: add 'ex_processor_mask' into
    'libxl_viridian_enlightenment'

 docs/man/xl.cfg.5.pod.in             |   8 +
 tools/include/libxl.h                |   7 +
 tools/libs/light/libxl_types.idl     |   1 +
 tools/libs/light/libxl_x86.c         |   3 +
 xen/arch/x86/hvm/viridian/viridian.c | 595 +++++++++++++++++++++------
 xen/include/asm-x86/hvm/viridian.h   |   8 +
 xen/include/public/hvm/params.h      |   7 +-
 7 files changed, 506 insertions(+), 123 deletions(-)

-- 
2.20.1

[PATCH 01/10] viridian: move flush hypercall implementation into separate function

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This patch moves the implementation of HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST
that is currently inline in viridian_hypercall() into a new hvcall_flush()
function.

The new function returns Xen erro values which are then dealt with
appropriately. A return value of -ERESTART translates to viridian_hypercall()
returning HVM_HCALL_preempted. Other return values translate to status codes
and viridian_hypercall() returning HVM_HCALL_completed. Currently the only
values, other than -ERESTART, returned by hvcall_flush() are 0 (indicating
success) or -EINVAL.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 130 ++++++++++++++++-----------
 1 file changed, 78 insertions(+), 52 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index dc7183a54627..f1a1b6a8af82 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -518,6 +518,69 @@ static bool need_flush(void *ctxt, struct vcpu *v)
     return vcpu_mask & (1ul << v->vcpu_id);
 }
 
+union hypercall_input {
+    uint64_t raw;
+    struct {
+        uint16_t call_code;
+        uint16_t fast:1;
+        uint16_t rsvd1:15;
+        uint16_t rep_count:12;
+        uint16_t rsvd2:4;
+        uint16_t rep_start:12;
+        uint16_t rsvd3:4;
+    };
+};
+
+union hypercall_output {
+    uint64_t raw;
+    struct {
+        uint16_t result;
+        uint16_t rsvd1;
+        uint32_t rep_complete:12;
+        uint32_t rsvd2:20;
+    };
+};
+
+static int hvcall_flush(union hypercall_input *input,
+                        union hypercall_output *output,
+                        unsigned long input_params_gpa,
+                        unsigned long output_params_gpa)
+{
+    struct {
+        uint64_t address_space;
+        uint64_t flags;
+        uint64_t vcpu_mask;
+    } input_params;
+
+    /* These hypercalls should never use the fast-call convention. */
+    if ( input->fast )
+        return -EINVAL;
+
+    /* Get input parameters. */
+    if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
+                                  sizeof(input_params)) != HVMTRANS_okay )
+        return -EINVAL;
+
+    /*
+     * It is not clear from the spec. if we are supposed to
+     * include current virtual CPU in the set or not in this case,
+     * so err on the safe side.
+     */
+    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
+        input_params.vcpu_mask = ~0ul;
+
+    /*
+     * 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) )
+        return -ERESTART;
+
+    output->rep_complete = input->rep_count;
+
+    return 0;
+}
+
 int viridian_hypercall(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
@@ -525,29 +588,8 @@ int viridian_hypercall(struct cpu_user_regs *regs)
     int mode = hvm_guest_x86_mode(curr);
     unsigned long input_params_gpa, output_params_gpa;
     uint16_t status = HV_STATUS_SUCCESS;
-
-    union hypercall_input {
-        uint64_t raw;
-        struct {
-            uint16_t call_code;
-            uint16_t fast:1;
-            uint16_t rsvd1:15;
-            uint16_t rep_count:12;
-            uint16_t rsvd2:4;
-            uint16_t rep_start:12;
-            uint16_t rsvd3:4;
-        };
-    } input;
-
-    union hypercall_output {
-        uint64_t raw;
-        struct {
-            uint16_t result;
-            uint16_t rsvd1;
-            uint32_t rep_complete:12;
-            uint32_t rsvd2:20;
-        };
-    } output = { 0 };
+    union hypercall_input input;
+    union hypercall_output output = {};
 
     ASSERT(is_viridian_domain(currd));
 
@@ -580,41 +622,25 @@ int viridian_hypercall(struct cpu_user_regs *regs)
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE:
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST:
     {
-        struct {
-            uint64_t address_space;
-            uint64_t flags;
-            uint64_t vcpu_mask;
-        } input_params;
+        int rc = hvcall_flush(&input, &output, input_params_gpa,
+                              output_params_gpa);
 
-        /* These hypercalls should never use the fast-call convention. */
-        status = HV_STATUS_INVALID_PARAMETER;
-        if ( input.fast )
+        switch ( rc )
+        {
+        case 0:
             break;
 
-        /* Get input parameters. */
-        if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
-                                      sizeof(input_params)) !=
-             HVMTRANS_okay )
-            break;
-
-        /*
-         * It is not clear from the spec. if we are supposed to
-         * include current virtual CPU in the set or not in this case,
-         * so err on the safe side.
-         */
-        if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
-            input_params.vcpu_mask = ~0ul;
-
-        /*
-         * 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) )
+        case -ERESTART:
             return HVM_HCALL_preempted;
 
-        output.rep_complete = input.rep_count;
+        default:
+            ASSERT_UNREACHABLE();
+            /* Fallthrough */
+        case -EINVAL:
+            status = HV_STATUS_INVALID_PARAMETER;
+            break;
+        }
 
-        status = HV_STATUS_SUCCESS;
         break;
     }
 
-- 
2.20.1

[PATCH 02/10] viridian: move IPI hypercall implementation into separate function

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

This patch moves the implementation of HVCALL_SEND_IPI that is currently
inline in viridian_hypercall() into a new hvcall_ipi() function.

The new function returns Xen errno values similarly to hvcall_flush(). Hence
the errno translation code in viridial_hypercall() is generalized, removing
the need for the local 'status' variable.

NOTE: The formatting of the 'out' label also corrected as per CODING_STYLE
      and the code is adjusted to avoid a register copy-back if 'mode' is
      neither 8 nor 4.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 170 ++++++++++++++-------------
 1 file changed, 87 insertions(+), 83 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index f1a1b6a8af82..c4f720f58d6d 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -581,13 +581,72 @@ static int hvcall_flush(union hypercall_input *input,
     return 0;
 }
 
+static int hvcall_ipi(union hypercall_input *input,
+                      union hypercall_output *output,
+                      unsigned long input_params_gpa,
+                      unsigned long output_params_gpa)
+{
+    struct domain *currd = current->domain;
+    struct vcpu *v;
+    uint32_t vector;
+    uint64_t vcpu_mask;
+
+    /* Get input parameters. */
+    if ( input->fast )
+    {
+        if ( input_params_gpa >> 32 )
+            return -EINVAL;
+
+        vector = input_params_gpa;
+        vcpu_mask = output_params_gpa;
+    }
+    else
+    {
+        struct {
+            uint32_t vector;
+            uint8_t target_vtl;
+            uint8_t reserved_zero[3];
+            uint64_t vcpu_mask;
+        } input_params;
+
+        if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
+                                      sizeof(input_params)) != HVMTRANS_okay )
+            return -EINVAL;
+
+        if ( input_params.target_vtl ||
+             input_params.reserved_zero[0] ||
+             input_params.reserved_zero[1] ||
+             input_params.reserved_zero[2] )
+            return -EINVAL;
+
+        vector = input_params.vector;
+        vcpu_mask = input_params.vcpu_mask;
+    }
+
+    if ( vector < 0x10 || vector > 0xff )
+        return -EINVAL;
+
+    for_each_vcpu ( currd, v )
+    {
+        if ( v->vcpu_id >= (sizeof(vcpu_mask) * 8) )
+            return -EINVAL;
+
+        if ( !(vcpu_mask & (1ul << v->vcpu_id)) )
+            continue;
+
+        vlapic_set_irq(vcpu_vlapic(v), vector, 0);
+    }
+
+    return 0;
+}
+
 int viridian_hypercall(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
     int mode = hvm_guest_x86_mode(curr);
     unsigned long input_params_gpa, output_params_gpa;
-    uint16_t status = HV_STATUS_SUCCESS;
+    int rc = 0;
     union hypercall_input input;
     union hypercall_output output = {};
 
@@ -616,92 +675,18 @@ int viridian_hypercall(struct cpu_user_regs *regs)
          * See section 14.5.1 of the specification.
          */
         do_sched_op(SCHEDOP_yield, guest_handle_from_ptr(NULL, void));
-        status = HV_STATUS_SUCCESS;
         break;
 
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE:
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST:
-    {
-        int rc = hvcall_flush(&input, &output, input_params_gpa,
-                              output_params_gpa);
-
-        switch ( rc )
-        {
-        case 0:
-            break;
-
-        case -ERESTART:
-            return HVM_HCALL_preempted;
-
-        default:
-            ASSERT_UNREACHABLE();
-            /* Fallthrough */
-        case -EINVAL:
-            status = HV_STATUS_INVALID_PARAMETER;
-            break;
-        }
-
+        rc = hvcall_flush(&input, &output, input_params_gpa,
+                          output_params_gpa);
         break;
-    }
 
     case HVCALL_SEND_IPI:
-    {
-        struct vcpu *v;
-        uint32_t vector;
-        uint64_t vcpu_mask;
-
-        status = HV_STATUS_INVALID_PARAMETER;
-
-        /* Get input parameters. */
-        if ( input.fast )
-        {
-            if ( input_params_gpa >> 32 )
-                break;
-
-            vector = input_params_gpa;
-            vcpu_mask = output_params_gpa;
-        }
-        else
-        {
-            struct {
-                uint32_t vector;
-                uint8_t target_vtl;
-                uint8_t reserved_zero[3];
-                uint64_t vcpu_mask;
-            } input_params;
-
-            if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
-                                          sizeof(input_params)) !=
-                 HVMTRANS_okay )
-                break;
-
-            if ( input_params.target_vtl ||
-                 input_params.reserved_zero[0] ||
-                 input_params.reserved_zero[1] ||
-                 input_params.reserved_zero[2] )
-                break;
-
-            vector = input_params.vector;
-            vcpu_mask = input_params.vcpu_mask;
-        }
-
-        if ( vector < 0x10 || vector > 0xff )
-            break;
-
-        for_each_vcpu ( currd, v )
-        {
-            if ( v->vcpu_id >= (sizeof(vcpu_mask) * 8) )
-                break;
-
-            if ( !(vcpu_mask & (1ul << v->vcpu_id)) )
-                continue;
-
-            vlapic_set_irq(vcpu_vlapic(v), vector, 0);
-        }
-
-        status = HV_STATUS_SUCCESS;
+        rc = hvcall_ipi(&input, &output, input_params_gpa,
+                        output_params_gpa);
         break;
-    }
 
     default:
         gprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
@@ -714,17 +699,36 @@ int viridian_hypercall(struct cpu_user_regs *regs)
          * Given that return a status of 'invalid code' has not so far
          * caused any problems it's not worth logging.
          */
-        status = HV_STATUS_INVALID_HYPERCALL_CODE;
+        rc = -EOPNOTSUPP;
+        break;
+    }
+
+ out:
+    switch ( rc )
+    {
+    case 0:
+        break;
+
+    case -ERESTART:
+        return HVM_HCALL_preempted;
+
+    case -EOPNOTSUPP:
+        output.result = HV_STATUS_INVALID_HYPERCALL_CODE;
+        break;
+
+    default:
+        ASSERT_UNREACHABLE();
+        /* Fallthrough */
+    case -EINVAL:
+        output.result = HV_STATUS_INVALID_PARAMETER;
         break;
     }
 
-out:
-    output.result = status;
     switch (mode) {
     case 8:
         regs->rax = output.raw;
         break;
-    default:
+    case 4:
         regs->rdx = output.raw >> 32;
         regs->rax = (uint32_t)output.raw;
         break;
-- 
2.20.1

Re: [PATCH 02/10] viridian: move IPI hypercall implementation into separate function

[Jan Beulich]

On 11.11.2020 21:07, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> This patch moves the implementation of HVCALL_SEND_IPI that is currently
> inline in viridian_hypercall() into a new hvcall_ipi() function.
> 
> The new function returns Xen errno values similarly to hvcall_flush(). Hence
> the errno translation code in viridial_hypercall() is generalized, removing
> the need for the local 'status' variable.
> 
> NOTE: The formatting of the 'out' label also corrected as per CODING_STYLE

How about correcting the adjacent switch() at the same time as well?

>       and the code is adjusted to avoid a register copy-back if 'mode' is
>       neither 8 nor 4.

While you mention it here, isn't this an unrelated change wanting
its own justification?

Jan

RE: [PATCH 02/10] viridian: move IPI hypercall implementation into separate function

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2020 08:38
> To: Paul Durrant <paul@xen.org>
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> Subject: RE: [EXTERNAL] [PATCH 02/10] viridian: move IPI hypercall implementation into separate
> function
> 
> 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 11.11.2020 21:07, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > This patch moves the implementation of HVCALL_SEND_IPI that is currently
> > inline in viridian_hypercall() into a new hvcall_ipi() function.
> >
> > The new function returns Xen errno values similarly to hvcall_flush(). Hence
> > the errno translation code in viridial_hypercall() is generalized, removing
> > the need for the local 'status' variable.
> >
> > NOTE: The formatting of the 'out' label also corrected as per CODING_STYLE
> 
> How about correcting the adjacent switch() at the same time as well?
> 

Sure.

> >       and the code is adjusted to avoid a register copy-back if 'mode' is
> >       neither 8 nor 4.
> 
> While you mention it here, isn't this an unrelated change wanting
> its own justification?
> 

It was such small mod that I folded it but maybe it would be best to break it out into a separate patch and also do the format adjustment there.

  Paul

> Jan

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

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

... 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 <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 51 +++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

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;
-- 
2.20.1

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

[Jan Beulich]

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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com
> Sent: 12 November 2020 08:46
> To: Paul Durrant <paul@xen.org>
> Cc: Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> 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

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

[Jan Beulich]

On 19.11.2020 17:02, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com
>> 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

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

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 November 2020 16:41
> To: paul@xen.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Wei Liu' <wl@xen.org>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Roger Pau Monné' <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> 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 <jbeulich@suse.com
> >> 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

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

[Jan Beulich]

On 19.11.2020 17:44, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 November 2020 16:41
>> To: paul@xen.org
>> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Wei Liu' <wl@xen.org>; 'Andrew Cooper'
>> <andrew.cooper3@citrix.com>; 'Roger Pau Monné' <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
>> 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 <jbeulich@suse.com
>>>> 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 04/10] viridian: use hypercall_vpmask in hvcall_ipi()

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

A subsequent patch will need to IPI a mask of virtual processors potentially
wider than 64 bits. A previous patch introduced per-cpu hypercall_vpmask
to allow hvcall_flush() to deal with such wide masks. This patch modifies
the implementation of hvcall_ipi() to make use of the same mask structures,
introducing a for_each_vp() macro to facilitate traversing a mask.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 43 ++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 4ab1f14b2248..63f63093a513 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -533,6 +533,21 @@ static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
     return test_bit(vp, vpmask->mask);
 }
 
+static unsigned int vpmask_first(struct hypercall_vpmask *vpmask)
+{
+    return find_first_bit(vpmask->mask, HVM_MAX_VCPUS);
+}
+
+static unsigned int vpmask_next(struct hypercall_vpmask *vpmask, unsigned int vp)
+{
+    return find_next_bit(vpmask->mask, HVM_MAX_VCPUS, vp + 1);
+}
+
+#define for_each_vp(vpmask, vp) \
+	for ((vp) = vpmask_first(vpmask); \
+	     (vp) < HVM_MAX_VCPUS; \
+	     (vp) = vpmask_next(vpmask, vp))
+
 /*
  * Windows should not issue the hypercalls requiring this callback in the
  * case where vcpu_id would exceed the size of the mask.
@@ -624,15 +639,24 @@ static int hvcall_flush(union hypercall_input *input,
     return 0;
 }
 
+static void send_ipi(struct hypercall_vpmask *vpmask, uint8_t vector)
+{
+    struct domain *currd = current->domain;
+    unsigned int vp;
+
+    for_each_vp ( vpmask, vp )
+        vlapic_set_irq(vcpu_vlapic(currd->vcpu[vp]), vector, 0);
+}
+
 static int hvcall_ipi(union hypercall_input *input,
                       union hypercall_output *output,
                       unsigned long input_params_gpa,
                       unsigned long output_params_gpa)
 {
-    struct domain *currd = current->domain;
-    struct vcpu *v;
+    struct hypercall_vpmask *vpmask = &this_cpu(hypercall_vpmask);
     uint32_t vector;
     uint64_t vcpu_mask;
+    unsigned int vp;
 
     /* Get input parameters. */
     if ( input->fast )
@@ -669,17 +693,20 @@ static int hvcall_ipi(union hypercall_input *input,
     if ( vector < 0x10 || vector > 0xff )
         return -EINVAL;
 
-    for_each_vcpu ( currd, v )
+    vpmask_empty(vpmask);
+    for (vp = 0; vp < 64; vp++)
     {
-        if ( v->vcpu_id >= (sizeof(vcpu_mask) * 8) )
-            return -EINVAL;
+        if ( !vcpu_mask )
+            break;
 
-        if ( !(vcpu_mask & (1ul << v->vcpu_id)) )
-            continue;
+        if ( vcpu_mask & 1 )
+            vpmask_set(vpmask, vp);
 
-        vlapic_set_irq(vcpu_vlapic(v), vector, 0);
+        vcpu_mask >>= 1;
     }
 
+    send_ipi(vpmask, vector);
+
     return 0;
 }
 
-- 
2.20.1

Re: [PATCH 04/10] viridian: use hypercall_vpmask in hvcall_ipi()

[Jan Beulich]

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
> @@ -533,6 +533,21 @@ static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
>      return test_bit(vp, vpmask->mask);
>  }
>  
> +static unsigned int vpmask_first(struct hypercall_vpmask *vpmask)
> +{
> +    return find_first_bit(vpmask->mask, HVM_MAX_VCPUS);
> +}
> +
> +static unsigned int vpmask_next(struct hypercall_vpmask *vpmask, unsigned int vp)
> +{
> +    return find_next_bit(vpmask->mask, HVM_MAX_VCPUS, vp + 1);

Perhaps again assert on vp's range?

> +}
> +
> +#define for_each_vp(vpmask, vp) \
> +	for ((vp) = vpmask_first(vpmask); \
> +	     (vp) < HVM_MAX_VCPUS; \
> +	     (vp) = vpmask_next(vpmask, vp))

Nit again: Missing blanks here and ...

> @@ -669,17 +693,20 @@ static int hvcall_ipi(union hypercall_input *input,
>      if ( vector < 0x10 || vector > 0xff )
>          return -EINVAL;
>  
> -    for_each_vcpu ( currd, v )
> +    vpmask_empty(vpmask);
> +    for (vp = 0; vp < 64; vp++)

... here. I also wonder if the literal 64 couldn't be expressed in
some suitable way.

Jan

RE: [PATCH 04/10] viridian: use hypercall_vpmask in hvcall_ipi()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2020 08:50
> To: Paul Durrant <paul@xen.org>
> Cc: Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 04/10] viridian: use hypercall_vpmask in hvcall_ipi()
> 
> 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
> > @@ -533,6 +533,21 @@ static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
> >      return test_bit(vp, vpmask->mask);
> >  }
> >
> > +static unsigned int vpmask_first(struct hypercall_vpmask *vpmask)
> > +{
> > +    return find_first_bit(vpmask->mask, HVM_MAX_VCPUS);
> > +}
> > +
> > +static unsigned int vpmask_next(struct hypercall_vpmask *vpmask, unsigned int vp)
> > +{
> > +    return find_next_bit(vpmask->mask, HVM_MAX_VCPUS, vp + 1);
> 
> Perhaps again assert on vp's range?
> 

Ok.

> > +}
> > +
> > +#define for_each_vp(vpmask, vp) \
> > +	for ((vp) = vpmask_first(vpmask); \
> > +	     (vp) < HVM_MAX_VCPUS; \
> > +	     (vp) = vpmask_next(vpmask, vp))
> 
> Nit again: Missing blanks here and ...

Oh yes.

> 
> > @@ -669,17 +693,20 @@ static int hvcall_ipi(union hypercall_input *input,
> >      if ( vector < 0x10 || vector > 0xff )
> >          return -EINVAL;
> >
> > -    for_each_vcpu ( currd, v )
> > +    vpmask_empty(vpmask);
> > +    for (vp = 0; vp < 64; vp++)
> 
> ... here. I also wonder if the literal 64 couldn't be expressed in
> some suitable way.
> 

Ok. I'll see if I can suitably macrotize it.

  Paul

> Jan

[PATCH 05/10] viridian: use softirq batching in hvcall_ipi()

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

vlapic_ipi() uses a softirq batching mechanism to improve the efficiency of
sending a IPIs to large number of processors. This patch modifies send_ipi()
(the worker function called by hvcall_ipi()) to also make use of the
mechanism when there multiple bits set the hypercall_vpmask. Hence a `nr`
field is added to the structure to track the number of set bits.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 63f63093a513..765d53016c02 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -11,6 +11,7 @@
 #include <xen/hypercall.h>
 #include <xen/domain_page.h>
 #include <xen/param.h>
+#include <xen/softirq.h>
 #include <asm/guest/hyperv-tlfs.h>
 #include <asm/paging.h>
 #include <asm/p2m.h>
@@ -509,6 +510,7 @@ void viridian_domain_deinit(struct domain *d)
 
 struct hypercall_vpmask {
     DECLARE_BITMAP(mask, HVM_MAX_VCPUS);
+    unsigned int nr;
 };
 
 static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
@@ -516,21 +518,24 @@ static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
 static void vpmask_empty(struct hypercall_vpmask *vpmask)
 {
     bitmap_zero(vpmask->mask, HVM_MAX_VCPUS);
+    vpmask->nr = 0;
 }
 
 static void vpmask_set(struct hypercall_vpmask *vpmask, unsigned int vp)
 {
-    __set_bit(vp, vpmask->mask);
+    if ( !test_and_set_bit(vp, vpmask->mask) )
+        vpmask->nr++;
 }
 
 static void vpmask_fill(struct hypercall_vpmask *vpmask)
 {
     bitmap_fill(vpmask->mask, HVM_MAX_VCPUS);
+    vpmask->nr = HVM_MAX_VCPUS;
 }
 
 static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
 {
-    return test_bit(vp, vpmask->mask);
+    return vpmask->nr && test_bit(vp, vpmask->mask);
 }
 
 static unsigned int vpmask_first(struct hypercall_vpmask *vpmask)
@@ -644,8 +649,17 @@ static void send_ipi(struct hypercall_vpmask *vpmask, uint8_t vector)
     struct domain *currd = current->domain;
     unsigned int vp;
 
+    if ( !vpmask->nr )
+        return;
+
+    if ( vpmask->nr > 1 )
+        cpu_raise_softirq_batch_begin();
+
     for_each_vp ( vpmask, vp )
         vlapic_set_irq(vcpu_vlapic(currd->vcpu[vp]), vector, 0);
+
+    if ( vpmask->nr > 1 )
+        cpu_raise_softirq_batch_finish();
 }
 
 static int hvcall_ipi(union hypercall_input *input,
-- 
2.20.1

Re: [PATCH 05/10] viridian: use softirq batching in hvcall_ipi()

[Jan Beulich]

On 11.11.2020 21:07, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> vlapic_ipi() uses a softirq batching mechanism to improve the efficiency of
> sending a IPIs to large number of processors. This patch modifies send_ipi()
> (the worker function called by hvcall_ipi()) to also make use of the
> mechanism when there multiple bits set the hypercall_vpmask. Hence a `nr`
> field is added to the structure to track the number of set bits.

This is kind of unusual, i.e. we don't do so elsewhere. I take it the
assumption is that using bitmap_weight() is too much overhead?

> @@ -509,6 +510,7 @@ void viridian_domain_deinit(struct domain *d)
>  
>  struct hypercall_vpmask {
>      DECLARE_BITMAP(mask, HVM_MAX_VCPUS);
> +    unsigned int nr;
>  };
>  
>  static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
> @@ -516,21 +518,24 @@ static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
>  static void vpmask_empty(struct hypercall_vpmask *vpmask)
>  {
>      bitmap_zero(vpmask->mask, HVM_MAX_VCPUS);
> +    vpmask->nr = 0;
>  }
>  
>  static void vpmask_set(struct hypercall_vpmask *vpmask, unsigned int vp)
>  {
> -    __set_bit(vp, vpmask->mask);
> +    if ( !test_and_set_bit(vp, vpmask->mask) )
> +        vpmask->nr++;

If test_and_set_bit() is the correct thing to use here (rather
than __test_and_set_bit()), the counter also needs updating
atomically.

>  }
>  
>  static void vpmask_fill(struct hypercall_vpmask *vpmask)
>  {
>      bitmap_fill(vpmask->mask, HVM_MAX_VCPUS);
> +    vpmask->nr = HVM_MAX_VCPUS;
>  }
>  
>  static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
>  {
> -    return test_bit(vp, vpmask->mask);
> +    return vpmask->nr && test_bit(vp, vpmask->mask);

Is this in fact an improvement?

Jan

RE: [PATCH 05/10] viridian: use softirq batching in hvcall_ipi()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2020 08:53
> To: Paul Durrant <paul@xen.org>
> Cc: Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 05/10] viridian: use softirq batching in hvcall_ipi()
> 
> On 11.11.2020 21:07, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > vlapic_ipi() uses a softirq batching mechanism to improve the efficiency of
> > sending a IPIs to large number of processors. This patch modifies send_ipi()
> > (the worker function called by hvcall_ipi()) to also make use of the
> > mechanism when there multiple bits set the hypercall_vpmask. Hence a `nr`
> > field is added to the structure to track the number of set bits.
> 
> This is kind of unusual, i.e. we don't do so elsewhere. I take it the
> assumption is that using bitmap_weight() is too much overhead?

It just seemed wasteful in the circumstances. If I move to bitmap copy OTOH then I'll have to use bitmap_weight().

> 
> > @@ -509,6 +510,7 @@ void viridian_domain_deinit(struct domain *d)
> >
> >  struct hypercall_vpmask {
> >      DECLARE_BITMAP(mask, HVM_MAX_VCPUS);
> > +    unsigned int nr;
> >  };
> >
> >  static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
> > @@ -516,21 +518,24 @@ static DEFINE_PER_CPU(struct hypercall_vpmask, hypercall_vpmask);
> >  static void vpmask_empty(struct hypercall_vpmask *vpmask)
> >  {
> >      bitmap_zero(vpmask->mask, HVM_MAX_VCPUS);
> > +    vpmask->nr = 0;
> >  }
> >
> >  static void vpmask_set(struct hypercall_vpmask *vpmask, unsigned int vp)
> >  {
> > -    __set_bit(vp, vpmask->mask);
> > +    if ( !test_and_set_bit(vp, vpmask->mask) )
> > +        vpmask->nr++;
> 
> If test_and_set_bit() is the correct thing to use here (rather
> than __test_and_set_bit()), the counter also needs updating
> atomically.
> 

It doesn't need to be atomic, but I'll probably drop it.

> >  }
> >
> >  static void vpmask_fill(struct hypercall_vpmask *vpmask)
> >  {
> >      bitmap_fill(vpmask->mask, HVM_MAX_VCPUS);
> > +    vpmask->nr = HVM_MAX_VCPUS;
> >  }
> >
> >  static bool vpmask_test(struct hypercall_vpmask *vpmask, unsigned int vp)
> >  {
> > -    return test_bit(vp, vpmask->mask);
> > +    return vpmask->nr && test_bit(vp, vpmask->mask);
> 
> Is this in fact an improvement?
> 

I think so, but it's a moot point if I drop 'nr'.

  Paul

> Jan

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

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

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@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 147 +++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 765d53016c02..1226e1596a1c 100644
--- 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];
+};
+
+static DEFINE_PER_CPU(struct hypercall_vpset, hypercall_vpset);
+
+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;
+}
+
+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;
+            }
+
+            if ( bank_mask & 1 )
+            {
+                uint64_t mask = set->bank_contents[bank];
+                unsigned int i;
+
+                for ( i = 0; i < 64; i++, vp++ )
+                {
+                    if ( mask & 1 )
+                    {
+                        if ( vp >= HVM_MAX_VCPUS )
+                            return -EINVAL;
+
+                        vpmask_set(vpmask, vp);
+                    }
+
+                    mask >>= 1;
+                }
+
+                bank++;
+            }
+            else
+                vp += 64;
+        }
+        return 0;
+    }
+
+    default:
+        break;
+    }
+
+    return -EINVAL;
+}
+
 /*
  * Windows should not issue the hypercalls requiring this callback in the
  * case where vcpu_id would exceed the size of the mask.
@@ -644,6 +721,70 @@ static int hvcall_flush(union hypercall_input *input,
     return 0;
 }
 
+static int hvcall_flush_ex(union hypercall_input *input,
+                           union hypercall_output *output,
+                           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;
+        struct hv_vpset set;
+    } input_params;
+
+    /* These hypercalls should never use the fast-call convention. */
+    if ( input->fast )
+        return -EINVAL;
+
+    /* Get input parameters. */
+    if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
+                                  sizeof(input_params)) != HVMTRANS_okay )
+        return -EINVAL;
+
+    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
+        vpmask_fill(vpmask);
+    else
+    {
+        struct hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
+        struct hv_vpset *set = &vpset->set;
+        size_t size;
+        int rc;
+
+        *set = input_params.set;
+        if ( set->format == HV_GENERIC_SET_SPARSE_4K )
+        {
+            unsigned long offset = offsetof(typeof(input_params),
+                                            set.bank_contents);
+
+            size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
+            if ( hvm_copy_from_guest_phys(&set->bank_contents,
+                                          input_params_gpa + offset,
+                                          size) != HVMTRANS_okay)
+                return -EINVAL;
+
+            size += sizeof(*set);
+        }
+        else
+            size = sizeof(*set);
+
+        rc = hv_vpset_to_vpmask(set, size, vpmask);
+        if ( rc )
+            return rc;
+    }
+
+    /*
+     * A false return means that another vcpu is currently trying
+     * a similar operation, so back off.
+     */
+    if ( !paging_flush_tlb(need_flush, vpmask) )
+        return -ERESTART;
+
+    output->rep_complete = input->rep_count;
+
+    return 0;
+}
+
 static void send_ipi(struct hypercall_vpmask *vpmask, uint8_t vector)
 {
     struct domain *currd = current->domain;
@@ -767,6 +908,12 @@ int viridian_hypercall(struct cpu_user_regs *regs)
                           output_params_gpa);
         break;
 
+    case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX:
+    case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX:
+        rc = hvcall_flush_ex(&input, &output, input_params_gpa,
+                             output_params_gpa);
+        break;
+
     case HVCALL_SEND_IPI:
         rc = hvcall_ipi(&input, &output, input_params_gpa,
                         output_params_gpa);
-- 
2.20.1

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

[Jan Beulich]

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? (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()?

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

Jan

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2020 09:19
> To: Paul Durrant <paul@xen.org>
> Cc: Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@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

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

[Jan Beulich]

On 19.11.2020 17:11, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 12 November 2020 09:19
>>
>> 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?

No, rather the basic language aspect that in standard C a struct
member which is a struct ending in a flexible array member may
not be followed by any other field.

Jan

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 November 2020 16:45
> To: paul@xen.org
> Cc: 'Paul Durrant' <pdurrant@amazon.com>; 'Wei Liu' <wl@xen.org>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Roger Pau Monné' <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 06/10] viridian: add ExProcessorMasks variants of the flush hypercalls
> 
> On 19.11.2020 17:11, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 12 November 2020 09:19
> >>
> >> 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?
> 
> No, rather the basic language aspect that in standard C a struct
> member which is a struct ending in a flexible array member may
> not be followed by any other field.
>

Ah, ok, now I understand what you mean. I'll union it to reserve the space instead.

  Paul
 
> Jan

[PATCH 07/10] viridian: add ExProcessorMasks variant of the IPI hypercall

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

A previous patch introduced variants of the flush hypercalls that take a
'Virtual Processor Set' as an argument rather than a simple 64-bit mask.
This patch introduces a similar variant of the HVCALL_SEND_IPI hypercall
(HVCALL_SEND_IPI_EX).

NOTE: As with HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST_EX, a guest should
      not yet issue the HVCALL_SEND_IPI_EX hypercall as support for
      'ExProcessorMasks' is not yet advertised via CPUID.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 66 ++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 1226e1596a1c..e899dd1e9f55 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -865,6 +865,67 @@ static int hvcall_ipi(union hypercall_input *input,
     return 0;
 }
 
+static int hvcall_ipi_ex(union hypercall_input *input,
+                         union hypercall_output *output,
+                         unsigned long input_params_gpa,
+                         unsigned long output_params_gpa)
+{
+    struct hypercall_vpmask *vpmask = &this_cpu(hypercall_vpmask);
+    struct {
+        uint32_t vector;
+        uint8_t target_vtl;
+        uint8_t reserved_zero[3];
+        struct hv_vpset set;
+    } input_params;
+    struct hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
+    struct hv_vpset *set = &vpset->set;
+    size_t size;
+    int rc;
+
+    /* These hypercalls should never use the fast-call convention. */
+    if ( input->fast )
+        return -EINVAL;
+
+    /* Get input parameters. */
+    if ( hvm_copy_from_guest_phys(&input_params, input_params_gpa,
+                                  sizeof(input_params)) != HVMTRANS_okay )
+        return -EINVAL;
+
+    if ( input_params.target_vtl ||
+         input_params.reserved_zero[0] ||
+         input_params.reserved_zero[1] ||
+         input_params.reserved_zero[2] )
+        return HV_STATUS_INVALID_PARAMETER;
+
+    if ( input_params.vector < 0x10 || input_params.vector > 0xff )
+        return HV_STATUS_INVALID_PARAMETER;
+
+    *set = input_params.set;
+    if ( set->format == HV_GENERIC_SET_SPARSE_4K )
+    {
+        unsigned long offset = offsetof(typeof(input_params),
+                                        set.bank_contents);
+
+        size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
+        if ( hvm_copy_from_guest_phys(&set->bank_contents,
+                                      input_params_gpa + offset,
+                                      size) != HVMTRANS_okay)
+            return -EINVAL;
+
+        size += sizeof(*set);
+    }
+    else
+        size = sizeof(*set);
+
+    rc = hv_vpset_to_vpmask(set, size, vpmask);
+    if ( rc )
+        return rc;
+
+    send_ipi(vpmask, input_params.vector);
+
+    return 0;
+}
+
 int viridian_hypercall(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
@@ -919,6 +980,11 @@ int viridian_hypercall(struct cpu_user_regs *regs)
                         output_params_gpa);
         break;
 
+    case HVCALL_SEND_IPI_EX:
+        rc = hvcall_ipi_ex(&input, &output, input_params_gpa,
+                           output_params_gpa);
+        break;
+
     default:
         gprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
                 input.call_code);
-- 
2.20.1

[PATCH 08/10] viridian: log initial invocation of each type of hypercall

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

To make is simpler to observe which viridian hypercalls are issued by a
particular Windows guest, this patch adds a per-domain mask to track them.
Each type of hypercall causes a different bit to be set in the mask and
when the bit transitions from clear to set, a log line is emitted showing
the name of the hypercall and the domain that issued it.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/viridian/viridian.c | 21 +++++++++++++++++++++
 xen/include/asm-x86/hvm/viridian.h   |  8 ++++++++
 2 files changed, 29 insertions(+)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index e899dd1e9f55..670fec3a4870 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -930,6 +930,7 @@ int viridian_hypercall(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     struct domain *currd = curr->domain;
+    struct viridian_domain *vd = currd->arch.hvm.viridian;
     int mode = hvm_guest_x86_mode(curr);
     unsigned long input_params_gpa, output_params_gpa;
     int rc = 0;
@@ -957,6 +958,10 @@ int viridian_hypercall(struct cpu_user_regs *regs)
     switch ( input.call_code )
     {
     case HVCALL_NOTIFY_LONG_SPIN_WAIT:
+        if ( !test_and_set_bit(_HCALL_spin_wait, &vd->hypercall_flags) )
+            printk(XENLOG_G_INFO "d%d: VIRIDIAN HVCALL_NOTIFY_LONG_SPIN_WAIT\n",
+                   currd->domain_id);
+
         /*
          * See section 14.5.1 of the specification.
          */
@@ -965,22 +970,38 @@ int viridian_hypercall(struct cpu_user_regs *regs)
 
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE:
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST:
+        if ( !test_and_set_bit(_HCALL_flush, &vd->hypercall_flags) )
+            printk(XENLOG_G_INFO "%pd: VIRIDIAN HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST\n",
+                   currd);
+
         rc = hvcall_flush(&input, &output, input_params_gpa,
                           output_params_gpa);
         break;
 
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX:
     case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX:
+        if ( !test_and_set_bit(_HCALL_flush_ex, &vd->hypercall_flags) )
+            printk(XENLOG_G_INFO "%pd: VIRIDIAN HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE/LIST_EX\n",
+                   currd);
+
         rc = hvcall_flush_ex(&input, &output, input_params_gpa,
                              output_params_gpa);
         break;
 
     case HVCALL_SEND_IPI:
+        if ( !test_and_set_bit(_HCALL_ipi, &vd->hypercall_flags) )
+            printk(XENLOG_G_INFO "%pd: VIRIDIAN HVCALL_SEND_IPI\n",
+                   currd);
+
         rc = hvcall_ipi(&input, &output, input_params_gpa,
                         output_params_gpa);
         break;
 
     case HVCALL_SEND_IPI_EX:
+        if ( !test_and_set_bit(_HCALL_ipi_ex, &vd->hypercall_flags) )
+            printk(XENLOG_G_INFO "%pd: VIRIDIAN HVCALL_SEND_IPI_EX\n",
+                   currd);
+
         rc = hvcall_ipi_ex(&input, &output, input_params_gpa,
                            output_params_gpa);
         break;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index cbf77d9c760b..d176c4b9153b 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -59,6 +59,14 @@ struct viridian_domain
 {
     union hv_guest_os_id guest_os_id;
     union hv_vp_assist_page_msr hypercall_gpa;
+    unsigned long hypercall_flags;
+
+#define _HCALL_spin_wait 0
+#define _HCALL_flush 1
+#define _HCALL_flush_ex 2
+#define _HCALL_ipi 3
+#define _HCALL_ipi_ex 4
+
     struct viridian_time_ref_count time_ref_count;
     struct viridian_page reference_tsc;
 };
-- 
2.20.1

Re: [PATCH 08/10] viridian: log initial invocation of each type of hypercall

[Jan Beulich]

On 11.11.2020 21:07, Paul Durrant wrote:
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -59,6 +59,14 @@ struct viridian_domain
>  {
>      union hv_guest_os_id guest_os_id;
>      union hv_vp_assist_page_msr hypercall_gpa;
> +    unsigned long hypercall_flags;
> +
> +#define _HCALL_spin_wait 0
> +#define _HCALL_flush 1
> +#define _HCALL_flush_ex 2
> +#define _HCALL_ipi 3
> +#define _HCALL_ipi_ex 4

I'd like to suggest for this to either be unsigned int until
more than 32 bits are needed, or be using DECLARE_BITMAP()
right away.

Jan

RE: [PATCH 08/10] viridian: log initial invocation of each type of hypercall

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2020 09:23
> To: Paul Durrant <paul@xen.org>
> Cc: Paul Durrant <pdurrant@amazon.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 08/10] viridian: log initial invocation of each type of hypercall
> 
> On 11.11.2020 21:07, Paul Durrant wrote:
> > --- a/xen/include/asm-x86/hvm/viridian.h
> > +++ b/xen/include/asm-x86/hvm/viridian.h
> > @@ -59,6 +59,14 @@ struct viridian_domain
> >  {
> >      union hv_guest_os_id guest_os_id;
> >      union hv_vp_assist_page_msr hypercall_gpa;
> > +    unsigned long hypercall_flags;
> > +
> > +#define _HCALL_spin_wait 0
> > +#define _HCALL_flush 1
> > +#define _HCALL_flush_ex 2
> > +#define _HCALL_ipi 3
> > +#define _HCALL_ipi_ex 4
> 
> I'd like to suggest for this to either be unsigned int until
> more than 32 bits are needed, or be using DECLARE_BITMAP()
> right away.

Ok. I may just go straight for the bitmap then.

  Paul

> 
> Jan

[PATCH 09/10] viridian: add a new '_HVMPV_ex_processor_masks' bit into HVM_PARAM_VIRIDIAN...

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

... and advertise ExProcessorMasks support if it is set.

Support is advertised by setting bit 11 in CPUID:40000004:EAX.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/x86/hvm/viridian/viridian.c | 3 +++
 xen/include/public/hvm/params.h      | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c
index 670fec3a4870..4d570adc21c0 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -84,6 +84,7 @@ typedef union _HV_CRASH_CTL_REG_CONTENTS
 #define CPUID4A_MSR_BASED_APIC         (1 << 3)
 #define CPUID4A_RELAX_TIMER_INT        (1 << 5)
 #define CPUID4A_SYNTHETIC_CLUSTER_IPI  (1 << 10)
+#define CPUID4A_EX_PROCESSOR_MASKS     (1 << 11)
 
 /* Viridian CPUID leaf 6: Implementation HW features detected and in use */
 #define CPUID6A_APIC_OVERLAY    (1 << 0)
@@ -197,6 +198,8 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
             res->a |= CPUID4A_MSR_BASED_APIC;
         if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
             res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
+        if ( viridian_feature_mask(d) & HVMPV_ex_processor_masks )
+            res->a |= CPUID4A_EX_PROCESSOR_MASKS;
 
         /*
          * This value is the recommended number of attempts to try to
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 0e3fdca09646..3b0a0f45da53 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -164,6 +164,10 @@
 #define _HVMPV_hcall_ipi 9
 #define HVMPV_hcall_ipi (1 << _HVMPV_hcall_ipi)
 
+/* Enable ExProcessorMasks */
+#define _HVMPV_ex_processor_masks 10
+#define HVMPV_ex_processor_masks (1 << _HVMPV_ex_processor_masks)
+
 #define HVMPV_feature_mask \
         (HVMPV_base_freq | \
          HVMPV_no_freq | \
@@ -174,7 +178,8 @@
          HVMPV_crash_ctl | \
          HVMPV_synic | \
          HVMPV_stimer | \
-         HVMPV_hcall_ipi)
+         HVMPV_hcall_ipi | \
+         HVMPV_ex_processor_masks)
 
 #endif
 
-- 
2.20.1

[PATCH 10/10] xl / libxl: add 'ex_processor_mask' into 'libxl_viridian_enlightenment'

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Adding the new value into the enumeration makes it immediately available
to xl, so this patch adjusts the xl.cfg(5) documentation accordingly.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 docs/man/xl.cfg.5.pod.in         | 8 ++++++++
 tools/include/libxl.h            | 7 +++++++
 tools/libs/light/libxl_types.idl | 1 +
 tools/libs/light/libxl_x86.c     | 3 +++
 4 files changed, 19 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 0532739c1fff..3f0f8de1e988 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2318,6 +2318,14 @@ This set incorporates use of a hypercall for interprocessor interrupts.
 This enlightenment may improve performance of Windows guests with multiple
 virtual CPUs.
 
+=item B<ex_processor_masks>
+
+This set enables new hypercall variants taking a variably-sized sparse
+B<Virtual Processor Set> as an argument, rather than a simple 64-bit
+mask. Hence this enlightenment must be specified for guests with more
+than 64 vCPUs if B<hcall_remote_tlb_flush> and/or B<hcall_ipi> are also
+specified.
+
 =item B<defaults>
 
 This is a special value that enables the default set of groups, which
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 1ea5b4f446e8..eaffccb30f37 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -444,6 +444,13 @@
  */
 #define LIBXL_HAVE_DISK_SAFE_REMOVE 1
 
+/*
+ * LIBXL_HAVE_VIRIDIAN_EX_PROCESSOR_MASKS indicates that the
+ * 'ex_processor_masks' value is present in the viridian enlightenment
+ * enumeration.
+ */
+#define LIBXL_HAVE_VIRIDIAN_EX_PROCESSOR_MASKS 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 9d3f05f39978..05324736b744 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -238,6 +238,7 @@ libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
     (7, "synic"),
     (8, "stimer"),
     (9, "hcall_ipi"),
+    (10, "ex_processor_masks"),
     ])
 
 libxl_hdtype = Enumeration("hdtype", [
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index e18274cc10e2..86d272999d67 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -366,6 +366,9 @@ static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
     if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_HCALL_IPI))
         mask |= HVMPV_hcall_ipi;
 
+    if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_EX_PROCESSOR_MASKS))
+        mask |= HVMPV_ex_processor_masks;
+
     if (mask != 0 &&
         xc_hvm_param_set(CTX->xch,
                          domid,
-- 
2.20.1