Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask operation from guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/13 13:39, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
>> Sent: Friday, November 22, 2013 6:49 PM
>> To: Wu, Feng
>> Cc: Jan Beulich (JBeulich@xxxxxxxx); xen-devel@xxxxxxxxxxxxx; Auld, Will;
>> Nakajima, Jun; Zhang, Xiantao
>> Subject: Re: [Xen-devel] [PATCH v4] x86: properly handle MSI-X unmask
>> operation from guests
>>
>> On 22/11/13 01:08, Wu, Feng wrote:
>>> patch revision history
>>> ----------------------
>>> v1: Initial patch to handle this issue involving changing the hypercall 
>>> interface
>>> v2:Totally handled inside hypervisor.
>>> v3:Change some logics of handling msi-x pending unmask operations.
>>> v4:Some changes related to coding style according to Andrew Cooper's
>> comments
>>> From 51c5b7f1f9c8f319da8adf021b39e18fbd3bf314 Mon Sep 17 00:00:00
>> 2001
>>> From: Feng Wu <feng.wu@xxxxxxxxx>
>>> Date: Wed, 13 Nov 2013 21:43:48 -0500
>>> Subject: [PATCH] x86: properly handle MSI-X unmask operation from guests
>>>
>>> For a pass-through device with MSI-x capability, when guest tries
>>> to unmask the MSI-x interrupt for the passed through device, xen
>>> doesn't clear the mask bit for MSI-x in hardware in the following
>>> scenario, which will cause network disconnection:
>>>
>>> 1. Guest masks the MSI-x interrupt
>>> 2. Guest updates the address and data for it
>>> 3. Guest unmasks the MSI-x interrupt (This is the problematic step)
>>>
>>> In the step #3 above, Xen doesn't handle it well. When guest tries
>>> to unmask MSI-X interrupt, it traps to Xen, Xen just returns to Qemu
>>> if it notices that address or data has been modified by guest before,
>>> then Qemu will update Xen with the latest value of address/data by
>>> hypercall. However, in this whole process, the MSI-X interrupt unmask
>>> operation is missing, which means Xen doesn't clear the mask bit in
>>> hardware for the MSI-X interrupt, so it remains disabled, that is why
>>> it loses the network connection.
>>>
>>> This patch fixes this issue.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
>>> ---
>>>  xen/arch/x86/hvm/io.c        |    3 +++
>>>  xen/arch/x86/hvm/vmsi.c      |   18 ++++++++++++++++++
>>>  xen/include/asm-x86/domain.h |    8 ++++++++
>>>  xen/include/xen/pci.h        |    1 +
>>>  4 files changed, 30 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
>>> index deb7b92..6c83c25 100644
>>> --- a/xen/arch/x86/hvm/io.c
>>> +++ b/xen/arch/x86/hvm/io.c
>>> @@ -297,6 +297,9 @@ void hvm_io_assist(ioreq_t *p)
>>>          break;
>>>      }
>>>
>>> +    if ( msix_post_handler(curr) )
>>> +        gdprintk(XENLOG_ERR, "msix_post_handler error\n");
>>> +
>>>      if ( p->state == STATE_IOREQ_NONE )
>>>          vcpu_end_shutdown_deferral(curr);
>>>  }
>>> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
>>> index 4826b4a..2cdd0dc 100644
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -293,7 +293,11 @@ static int msixtbl_write(struct vcpu *v, unsigned
>> long address,
>>>      /* exit to device model if address/data has been modified */
>>>      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
>>> +    {
>>> +        v->arch.pending_msix_unmask.valid = 1;
>>> +        v->arch.pending_msix_unmask.ctrl_address = address;
>>>          goto out;
>>> +    }
>>>
>>>      virt = msixtbl_addr_to_virt(entry, address);
>>>      if ( !virt )
>>> @@ -528,3 +532,17 @@ void msixtbl_pt_cleanup(struct domain *d)
>>>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>>>      local_irq_restore(flags);
>>>  }
>>> +
>>> +int msix_post_handler(struct vcpu *v)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( v->arch.pending_msix_unmask.valid == 0 )
>>> +        return 0;
>>> +
>>> +    v->arch.pending_msix_unmask.valid = 0;
>>> +
>>> +    rc = msixtbl_write(v, v->arch.pending_msix_unmask.ctrl_address, 4, 0);
>> Wont this corrupt other information which might be present in the register ?
> It will not corrupt other information in the register. In fact, no matter 
> what value is passed to msixtbl_write(), 
> it only modifies the mask bit (bit 0) in hardware, it reads the other bits 
> and writes them back.

But it will result in corruption iff any other bits in the word become
defined by the spec.

I will defer to others as to whether that should count against this
patch or not.

~Andrew

>
>> ~Andrew
>>
>>> +    return rc != X86EMUL_OKAY ? -1 : 0;
>>> +}
>>> +
>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>> index 9d39061..89b1adc 100644
>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -375,6 +375,12 @@ struct pv_vcpu
>>>      spinlock_t shadow_ldt_lock;
>>>  };
>>>
>>> +struct pending_msix_unmask_info
>>> +{
>>> +    unsigned long ctrl_address;
>>> +    bool_t valid;
>>> +};
>>> +
>>>  struct arch_vcpu
>>>  {
>>>      /*
>>> @@ -439,6 +445,8 @@ struct arch_vcpu
>>>
>>>      /* A secondary copy of the vcpu time info. */
>>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>>> +
>>> +    struct pending_msix_unmask_info pending_msix_unmask;
>>>  } __cacheline_aligned;
>>>
>>>  /* Shorthands to improve code legibility. */
>>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>>> index cadb525..ce8f6ff 100644
>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -147,5 +147,6 @@ struct pirq;
>>>  int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
>>>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>>>  void msixtbl_pt_cleanup(struct domain *d);
>>> +int msix_post_handler(struct vcpu *v);
>>>
>>>  #endif /* __XEN_PCI_H__ */
>>> --
>>> 1.7.1
>>>
>>> Thanks,
>>> Feng
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@xxxxxxxxxxxxx
>>> http://lists.xen.org/xen-devel
> Thanks,
> Feng


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel