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;
 

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

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>