Re: [PATCH v2 08/12] viridian: add ExProcessorMasks variants of the flush hypercalls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2020 10:48, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Just a couple of cosmetic remarks, apart from them this looks
good to me, albeit some of the size calculations are quite,
well, involved. I guess like with most parts if this series,
in the end Wei will need to give his ack.

> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -576,6 +576,70 @@ static unsigned int vpmask_nr(const struct 
> hypercall_vpmask *vpmask)
>      return bitmap_weight(vpmask->mask, HVM_MAX_VCPUS);
>  }
>  
> +#define HV_VPSET_BANK_SIZE \
> +    sizeof_field(struct hv_vpset, bank_contents[0])
> +
> +#define HV_VPSET_SIZE(banks)   \
> +    (sizeof(struct hv_vpset) + (banks * HV_VPSET_BANK_SIZE))

Personally I think this would be better done using
offsetof(struct hv_vpset, bank_contents), but you're the maintainer.
However, "banks" wants parenthesizing, just in case.

> +#define HV_VPSET_MAX_BANKS \
> +    (sizeof_field(struct hv_vpset, valid_bank_mask) * 8)
> +
> +struct hypercall_vpset {
> +    union {
> +        struct hv_vpset set;
> +        uint8_t pad[HV_VPSET_SIZE(HV_VPSET_MAX_BANKS)];
> +    };
> +};

A struct with just a union as member could be expressed as a simple
union - any reason you prefer the slightly more involved variant?

> +static DEFINE_PER_CPU(struct hypercall_vpset, hypercall_vpset);
> +
> +static unsigned int hv_vpset_nr_banks(struct hv_vpset *vpset)
> +{
> +    return hweight64(vpset->valid_bank_mask);
> +}
> +
> +static uint16_t hv_vpset_to_vpmask(struct hv_vpset *set,

const?

> @@ -656,6 +720,78 @@ static int hvcall_flush(union hypercall_input *input,
>      return 0;
>  }
>  
> +static int hvcall_flush_ex(union hypercall_input *input,

const again?

> +                           union hypercall_output *output,
> +                           unsigned long input_params_gpa,
> +                           unsigned long output_params_gpa)

Mainly for cosmetic reasons and to be in sync with
hvm_copy_from_guest_phys()'s respective parameter, perhaps both
would better be paddr_t?

Jan