Re: [PATCH v2] interrupts: allow guest to set and clear MSI-X mask bit

[Malcolm Crossley <malcolm.crossley@citrix.com>]

On 19/07/13 16:07, Joby Poriyath wrote:
> Guest needs the ability to enable and disable MSI-X interrupts
> by setting the MSI-X control bit. Currently, a write to MSI-X
> mask bit by the guest is silently ignored.
>
> A likely scenario is where we have a 82599 SR-IOV nic passed
> through to a guest. From the guest if you do
>
>    ifconfig <ETH_DEV> down
>    ifconfig <ETH_DEV> up
>
> the interrupts remain masked.  The the mask bit for the VF is
> being set by the PF performing a reset (at the request of the VF).
> However, interrupts are enabled by VF driver by clearing the mask
> bit by writing directly to BAR3 region containing the MSI-X table.
>
>  From dom0, we can verify that
> interrupts are being masked using 'xl debug-keys M'.
>
> Initially, guest was allowed to modify MSI-X bit.
> Later this behaviour was changed.
> See changeset 74c213c506afcd74a8556dd092995fd4dc38b225.
>
> Signed-off-by: Joby Poriyath <joby.poriyath@citrix.com>

Reviewed-by: Malcolm Crossley <malcolm.crossley@citrix.com>

> ---
>   xen/arch/x86/hvm/vmsi.c |   32 +++++++++++++++++++-------------
>   1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index 36de312..97d9f93 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -169,6 +169,7 @@ struct msixtbl_entry
>           uint32_t msi_ad[3];	/* Shadow of address low, high and data */
>       } gentries[MAX_MSIX_ACC_ENTRIES];
>       struct rcu_head rcu;
> +    struct pirq *pirq;
>   };
>   
>   static DEFINE_RCU_READ_LOCK(msixtbl_rcu_lock);
> @@ -254,6 +255,9 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>       void *virt;
>       unsigned int nr_entry, index;
>       int r = X86EMUL_UNHANDLEABLE;
> +    unsigned long flags;
> +    struct irq_desc *desc;
> +    unsigned long orig;
>   
>       if ( len != 4 || (address & 3) )
>           return r;
> @@ -283,20 +287,20 @@ static int msixtbl_write(struct vcpu *v, unsigned long address,
>       if ( !virt )
>           goto out;
>   
> -    /* Do not allow the mask bit to be changed. */
> -#if 0 /* XXX
> -       * As the mask bit is the only defined bit in the word, and as the
> -       * host MSI-X code doesn't preserve the other bits anyway, doing
> -       * this is pointless. So for now just discard the write (also
> -       * saving us from having to determine the matching irq_desc).
> -       */
> -    spin_lock_irqsave(&desc->lock, flags);
> +    desc = pirq_spin_lock_irq_desc(entry->pirq, &flags);
> +    if ( !desc )
> +        goto out;
> +
> +   /* The mask bit is the only defined bit in the word. But we
> +    * ought to preserve the reserved bits. Clearing the reserved
> +    * bits can result in undefined behaviour (see PCI Local Bus
> +    * Specification revision 2.3).
> +    */
>       orig = readl(virt);
> -    val &= ~PCI_MSIX_VECTOR_BITMASK;
> -    val |= orig & PCI_MSIX_VECTOR_BITMASK;
> +    val &= PCI_MSIX_VECTOR_BITMASK;
> +    val |= ( orig & ~PCI_MSIX_VECTOR_BITMASK );
>       writel(val, virt);
>       spin_unlock_irqrestore(&desc->lock, flags);
> -#endif
>   
>       r = X86EMUL_OKAY;
>   out:
> @@ -328,7 +332,8 @@ const struct hvm_mmio_handler msixtbl_mmio_handler = {
>   static void add_msixtbl_entry(struct domain *d,
>                                 struct pci_dev *pdev,
>                                 uint64_t gtable,
> -                              struct msixtbl_entry *entry)
> +                              struct msixtbl_entry *entry,
> +                              struct pirq *pirq)
>   {
>       u32 len;
>   
> @@ -342,6 +347,7 @@ static void add_msixtbl_entry(struct domain *d,
>       entry->table_len = len;
>       entry->pdev = pdev;
>       entry->gtable = (unsigned long) gtable;
> +    entry->pirq = pirq;
>   
>       list_add_rcu(&entry->list, &d->arch.hvm_domain.msixtbl_list);
>   }
> @@ -404,7 +410,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t gtable)
>   
>       entry = new_entry;
>       new_entry = NULL;
> -    add_msixtbl_entry(d, pdev, gtable, entry);
> +    add_msixtbl_entry(d, pdev, gtable, entry, pirq);
>   
>   found:
>       atomic_inc(&entry->refcnt);