[PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[Jan Beulich]

I've noticed the bugs fixed in patch 1 only while doing the other cleanup.

1: fix error code use
2: drop dead variable updates
3: fold duplicate vpset retrieval code

Jan

[PATCH 1/3] x86/Viridian: fix error code use

[Jan Beulich]

Both the wrong use of HV_STATUS_* and the return type of
hv_vpset_to_vpmask() can lead to viridian_hypercall()'s
ASSERT_UNREACHABLE() triggering when translating error codes from Xen
to Viridian representation.

Fixes: b4124682db6e ("viridian: add ExProcessorMasks variants of the flush hypercalls")
Fixes: 9afa867d42ba ("viridian: add ExProcessorMasks variant of the IPI hypercall")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -628,8 +628,8 @@ static unsigned int hv_vpset_nr_banks(st
     return hweight64(vpset->valid_bank_mask);
 }
 
-static uint16_t hv_vpset_to_vpmask(const struct hv_vpset *set,
-                                   struct hypercall_vpmask *vpmask)
+static int hv_vpset_to_vpmask(const struct hv_vpset *set,
+                              struct hypercall_vpmask *vpmask)
 {
 #define NR_VPS_PER_BANK (HV_VPSET_BANK_SIZE * 8)
 
@@ -919,10 +919,10 @@ static int hvcall_ipi_ex(const union hyp
          input_params.reserved_zero[0] ||
          input_params.reserved_zero[1] ||
          input_params.reserved_zero[2] )
-        return HV_STATUS_INVALID_PARAMETER;
+        return -EINVAL;
 
     if ( input_params.vector < 0x10 || input_params.vector > 0xff )
-        return HV_STATUS_INVALID_PARAMETER;
+        return -EINVAL;
 
     *set = input_params.set;
     if ( set->format == HV_GENERIC_SET_SPARSE_4K )

[PATCH 2/3] x86/Viridian: drop dead variable updates

[Jan Beulich]

Both hvcall_flush_ex() and hvcall_ipi_ex() update "size" without
subsequently using the value; future compilers may warn about such.
Alongside dropping the updates, shrink the variables' scopes to
demonstrate that there are no outer scope uses.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -776,7 +776,6 @@ static int hvcall_flush_ex(const union h
     {
         union hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
         struct hv_vpset *set = &vpset->set;
-        size_t size;
         int rc;
 
         *set = input_params.set;
@@ -784,8 +783,7 @@ static int hvcall_flush_ex(const union h
         {
             unsigned long offset = offsetof(typeof(input_params),
                                             set.bank_contents);
-
-            size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
+            size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
 
             if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
                  sizeof(*vpset) )
@@ -798,11 +796,7 @@ static int hvcall_flush_ex(const union h
                                           input_params_gpa + offset,
                                           size) != HVMTRANS_okay)
                 return -EINVAL;
-
-            size += sizeof(*set);
         }
-        else
-            size = sizeof(*set);
 
         rc = hv_vpset_to_vpmask(set, vpmask);
         if ( rc )
@@ -903,7 +897,6 @@ static int hvcall_ipi_ex(const union hyp
     } input_params;
     union 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. */
@@ -929,8 +922,7 @@ static int hvcall_ipi_ex(const union hyp
     {
         unsigned long offset = offsetof(typeof(input_params),
                                         set.bank_contents);
-
-        size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
+        size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
 
         if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
              sizeof(*vpset) )
@@ -943,11 +935,7 @@ static int hvcall_ipi_ex(const union hyp
                                       input_params_gpa + offset,
                                       size) != HVMTRANS_okay)
             return -EINVAL;
-
-        size += sizeof(*set);
     }
-    else
-        size = sizeof(*set);
 
     rc = hv_vpset_to_vpmask(set, vpmask);
     if ( rc )

[PATCH 3/3] x86/Viridian: fold duplicate vpset retrieval code

[Jan Beulich]

hvcall_{flush,ipi}_ex() use more almost identical code than what was
isolated into hv_vpset_to_vpmask(). Move that code there as well, to
have just one instance of it. This way all of HV_GENERIC_SET_SPARSE_4K
processing now happens in a single place.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -628,10 +628,14 @@ static unsigned int hv_vpset_nr_banks(st
     return hweight64(vpset->valid_bank_mask);
 }
 
-static int hv_vpset_to_vpmask(const struct hv_vpset *set,
+static int hv_vpset_to_vpmask(const struct hv_vpset *in, paddr_t bank_gpa,
                               struct hypercall_vpmask *vpmask)
 {
 #define NR_VPS_PER_BANK (HV_VPSET_BANK_SIZE * 8)
+    union hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
+    struct hv_vpset *set = &vpset->set;
+
+    *set = *in;
 
     switch ( set->format )
     {
@@ -643,6 +647,18 @@ static int hv_vpset_to_vpmask(const stru
     {
         uint64_t bank_mask;
         unsigned int vp, bank = 0;
+        size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
+
+        if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
+             sizeof(*vpset) )
+        {
+            ASSERT_UNREACHABLE();
+            return -EINVAL;
+        }
+
+        if ( hvm_copy_from_guest_phys(&set->bank_contents, bank_gpa,
+                                      size) != HVMTRANS_okay)
+            return -EINVAL;
 
         vpmask_empty(vpmask);
         for ( vp = 0, bank_mask = set->valid_bank_mask;
@@ -774,31 +790,13 @@ static int hvcall_flush_ex(const union h
         vpmask_fill(vpmask);
     else
     {
-        union hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
-        struct hv_vpset *set = &vpset->set;
-        int rc;
-
-        *set = input_params.set;
-        if ( set->format == HV_GENERIC_SET_SPARSE_4K )
-        {
-            unsigned long offset = offsetof(typeof(input_params),
+        unsigned int bank_offset = offsetof(typeof(input_params),
                                             set.bank_contents);
-            size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
-
-            if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
-                 sizeof(*vpset) )
-            {
-                ASSERT_UNREACHABLE();
-                return -EINVAL;
-            }
-
-            if ( hvm_copy_from_guest_phys(&set->bank_contents[0],
-                                          input_params_gpa + offset,
-                                          size) != HVMTRANS_okay)
-                return -EINVAL;
-        }
+        int rc;
 
-        rc = hv_vpset_to_vpmask(set, vpmask);
+        rc = hv_vpset_to_vpmask(&input_params.set,
+                                input_params_gpa + bank_offset,
+                                vpmask);
         if ( rc )
             return rc;
     }
@@ -895,8 +893,8 @@ static int hvcall_ipi_ex(const union hyp
         uint8_t reserved_zero[3];
         struct hv_vpset set;
     } input_params;
-    union hypercall_vpset *vpset = &this_cpu(hypercall_vpset);
-    struct hv_vpset *set = &vpset->set;
+    unsigned int bank_offset = offsetof(typeof(input_params),
+                                        set.bank_contents);
     int rc;
 
     /* These hypercalls should never use the fast-call convention. */
@@ -917,27 +915,8 @@ static int hvcall_ipi_ex(const union hyp
     if ( input_params.vector < 0x10 || input_params.vector > 0xff )
         return -EINVAL;
 
-    *set = input_params.set;
-    if ( set->format == HV_GENERIC_SET_SPARSE_4K )
-    {
-        unsigned long offset = offsetof(typeof(input_params),
-                                        set.bank_contents);
-        size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
-
-        if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
-             sizeof(*vpset) )
-        {
-            ASSERT_UNREACHABLE();
-            return -EINVAL;
-        }
-
-        if ( hvm_copy_from_guest_phys(&set->bank_contents,
-                                      input_params_gpa + offset,
-                                      size) != HVMTRANS_okay)
-            return -EINVAL;
-    }
-
-    rc = hv_vpset_to_vpmask(set, vpmask);
+    rc = hv_vpset_to_vpmask(&input_params.set, input_params_gpa + bank_offset,
+                            vpmask);
     if ( rc )
         return rc;
 

Re: [PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[Andrew Cooper]

On 18/11/2021 13:12, Jan Beulich wrote:
> I've noticed the bugs fixed in patch 1 only while doing the other cleanup.
>
> 1: fix error code use
> 2: drop dead variable updates
> 3: fold duplicate vpset retrieval code

Oh, nice.  This makes it rather easier to do the flush short-circuit for
HV_GENERIC_SET_ALL.

I'll try importing this patchset onto my branch and having a go at it.

~Andrew

Re: [PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[Jan Beulich]

On 18.11.2021 14:20, Andrew Cooper wrote:
> On 18/11/2021 13:12, Jan Beulich wrote:
>> I've noticed the bugs fixed in patch 1 only while doing the other cleanup.
>>
>> 1: fix error code use
>> 2: drop dead variable updates
>> 3: fold duplicate vpset retrieval code
> 
> Oh, nice.  This makes it rather easier to do the flush short-circuit for
> HV_GENERIC_SET_ALL.

To be honest I first thought it might, but now I'm not sure anymore.

Jan

Re: [PATCH 3/3] x86/Viridian: fold duplicate vpset retrieval code

[Andrew Cooper]

On 18/11/2021 13:14, Jan Beulich wrote:
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -643,6 +647,18 @@ static int hv_vpset_to_vpmask(const stru
>      {
>          uint64_t bank_mask;
>          unsigned int vp, bank = 0;
> +        size_t size = sizeof(*set->bank_contents) * hv_vpset_nr_banks(set);
> +
> +        if ( offsetof(typeof(*vpset), set.bank_contents[0]) + size >
> +             sizeof(*vpset) )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return -EINVAL;
> +        }
> +
> +        if ( hvm_copy_from_guest_phys(&set->bank_contents, bank_gpa,
> +                                      size) != HVMTRANS_okay)

Minor style issue - closing bracket.  I see it was a preexisting issue
from the old code.

~Andrew

Re: [PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[Andrew Cooper]

On 18/11/2021 13:34, Jan Beulich wrote:
> On 18.11.2021 14:20, Andrew Cooper wrote:
>> On 18/11/2021 13:12, Jan Beulich wrote:
>>> I've noticed the bugs fixed in patch 1 only while doing the other cleanup.
>>>
>>> 1: fix error code use
>>> 2: drop dead variable updates
>>> 3: fold duplicate vpset retrieval code
>> Oh, nice.  This makes it rather easier to do the flush short-circuit for
>> HV_GENERIC_SET_ALL.
> To be honest I first thought it might, but now I'm not sure anymore.

Just this delta:

diff --git a/xen/arch/x86/hvm/viridian/viridian.c
b/xen/arch/x86/hvm/viridian/viridian.c
index 658e68f7f2bb..c8c05bfb04a1 100644
--- a/xen/arch/x86/hvm/viridian/viridian.c
+++ b/xen/arch/x86/hvm/viridian/viridian.c
@@ -771,7 +771,8 @@ static int hvcall_flush_ex(const union
hypercall_input *input,
                                   sizeof(input_params)) != HVMTRANS_okay )
         return -EINVAL;
 
-    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
+    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS ||
+         input_params.set.format == HV_GENERIC_SET_ALL )
         vcpu_bitmap = NULL;
     else
     {

which now I come to think of it independent of your cleanup, and small
enough to be folded into my main IPI change.

~Andrew

Re: [PATCH 1/3] x86/Viridian: fix error code use

[Durrant, Paul]

On 18/11/2021 13:13, Jan Beulich wrote:
> Both the wrong use of HV_STATUS_* and the return type of
> hv_vpset_to_vpmask() can lead to viridian_hypercall()'s
> ASSERT_UNREACHABLE() triggering when translating error codes from Xen
> to Viridian representation.
> 
> Fixes: b4124682db6e ("viridian: add ExProcessorMasks variants of the flush hypercalls")
> Fixes: 9afa867d42ba ("viridian: add ExProcessorMasks variant of the IPI hypercall")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 2/3] x86/Viridian: drop dead variable updates

[Durrant, Paul]

On 18/11/2021 13:14, Jan Beulich wrote:
> Both hvcall_flush_ex() and hvcall_ipi_ex() update "size" without
> subsequently using the value; future compilers may warn about such.
> Alongside dropping the updates, shrink the variables' scopes to
> demonstrate that there are no outer scope uses.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 3/3] x86/Viridian: fold duplicate vpset retrieval code

[Durrant, Paul]

On 18/11/2021 13:14, Jan Beulich wrote:
> hvcall_{flush,ipi}_ex() use more almost identical code than what was
> isolated into hv_vpset_to_vpmask(). Move that code there as well, to
> have just one instance of it. This way all of HV_GENERIC_SET_SPARSE_4K
> processing now happens in a single place.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH 0/3] x86/Viridian: ExProcessorMasks handling improvements

[Jan Beulich]

On 18.11.2021 19:05, Andrew Cooper wrote:
> On 18/11/2021 13:34, Jan Beulich wrote:
>> On 18.11.2021 14:20, Andrew Cooper wrote:
>>> On 18/11/2021 13:12, Jan Beulich wrote:
>>>> I've noticed the bugs fixed in patch 1 only while doing the other cleanup.
>>>>
>>>> 1: fix error code use
>>>> 2: drop dead variable updates
>>>> 3: fold duplicate vpset retrieval code
>>> Oh, nice.  This makes it rather easier to do the flush short-circuit for
>>> HV_GENERIC_SET_ALL.
>> To be honest I first thought it might, but now I'm not sure anymore.
> 
> Just this delta:
> 
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 658e68f7f2bb..c8c05bfb04a1 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -771,7 +771,8 @@ static int hvcall_flush_ex(const union
> hypercall_input *input,
>                                    sizeof(input_params)) != HVMTRANS_okay )
>          return -EINVAL;
>  
> -    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS )
> +    if ( input_params.flags & HV_FLUSH_ALL_PROCESSORS ||
> +         input_params.set.format == HV_GENERIC_SET_ALL )
>          vcpu_bitmap = NULL;
>      else
>      {
> 
> which now I come to think of it independent of your cleanup, and small
> enough to be folded into my main IPI change.

FTAOD please keep my R-b there with this addition.

Jan