Re: [PATCH v6] x86: properly handle MSI-X unmask operation from guests

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/11/13 09:31, Jan Beulich wrote:
> 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@intel.com>
>
> Only latch the address if the guest really is unmasking the entry.
>
> Clean up the entire change.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -300,7 +300,10 @@ void hvm_io_assist(ioreq_t *p)
>      }
>  
>      if ( p->state == STATE_IOREQ_NONE )
> +    {
> +        msix_write_completion(curr);
>          vcpu_end_shutdown_deferral(curr);
> +    }
>  }
>  
>  static int dpci_ioport_read(uint32_t mport, ioreq_t *p)
> --- 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,
>  
>      /* exit to device model if address/data has been modified */
>      if ( test_and_clear_bit(nr_entry, &entry->table_flags) )
> +    {
> +        if ( !(val & PCI_MSIX_VECTOR_BITMASK) )
> +            v->arch.hvm_vcpu.hvm_io.msix_unmask_address = address;
>          goto out;
> +    }
>  
>      virt = msixtbl_addr_to_virt(entry, address);
>      if ( !virt )
> @@ -528,3 +532,15 @@ void msixtbl_pt_cleanup(struct domain *d
>      spin_unlock(&d->arch.hvm_domain.msixtbl_list_lock);
>      local_irq_restore(flags);
>  }
> +
> +void msix_write_completion(struct vcpu *v)
> +{
> +    unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
> +
> +    if ( !ctrl_address )
> +        return;
> +
> +    v->arch.hvm_vcpu.hvm_io.msix_unmask_address = 0;
> +    if ( msixtbl_write(v, ctrl_address, 4, 0) != X86EMUL_OKAY )
> +        gdprintk(XENLOG_WARNING, "MSI-X write completion failure\n");
> +}
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -74,6 +74,8 @@ struct hvm_vcpu_io {
>       * necessary retry through other than function return codes.
>       */
>      bool_t mmio_retry, mmio_retrying;
> +
> +    unsigned long msix_unmask_address;
>  };
>  
>  #define VMCX_EADDR    (~0ULL)
> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -124,6 +124,7 @@ void hvm_interrupt_post(struct vcpu *v, 
>  void hvm_io_assist(ioreq_t *p);
>  void hvm_dpci_eoi(struct domain *d, unsigned int guest_irq,
>                    union vioapic_redir_entry *ent);
> +void msix_write_completion(struct vcpu *);
>  
>  struct hvm_hw_stdvga {
>      uint8_t sr_index;
>
>
>