diff mbox series

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

Message ID 20201111200721.30551-7-paul@xen.org (mailing list archive)
State Superseded
Headers show
Series viridian: add support for ExProcessorMasks | expand

Commit Message

Paul Durrant Nov. 11, 2020, 8:07 p.m. UTC 
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(+)

Comments

Jan Beulich Nov. 12, 2020, 9:19 a.m. UTC | #1 
On 11.11.2020 21:07, Paul Durrant wrote:
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -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
Paul Durrant Nov. 19, 2020, 4:11 p.m. UTC | #2 
> -----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
Jan Beulich Nov. 19, 2020, 4:44 p.m. UTC | #3 
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
Paul Durrant Nov. 19, 2020, 4:47 p.m. UTC | #4 
> -----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
diff mbox series

Patch

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