Re: [PATCH] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Dec 14, 2021 at 10:45:17AM +0000, Rahul Singh wrote:
> vpci/msix.c file will be used for arm architecture when vpci msix
> support will be added to ARM, but there is x86 specific code in this
> file.
> 
> Move x86 specific code to the x86_msix.c file to make sure common code
> will be used for other architecture.
> 
> No functional change intended.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
>  xen/arch/x86/msi.c                       |   2 +-
>  xen/drivers/passthrough/amd/iommu_init.c |   1 +
>  xen/drivers/vpci/Makefile                |   1 +
>  xen/drivers/vpci/msi.c                   |   3 +-
>  xen/drivers/vpci/msix.c                  | 134 +++++---------------
>  xen/drivers/vpci/x86_msix.c              | 155 +++++++++++++++++++++++

This should go into xen/arch/x86/hvm/vmsi.c there's already vPCI MSI
specific code in there.

>  xen/include/asm-x86/msi.h                |  28 ----
>  xen/include/xen/msi.h                    |  28 ++++
>  xen/include/xen/vpci.h                   |  21 +++
>  9 files changed, 239 insertions(+), 134 deletions(-)
>  create mode 100644 xen/drivers/vpci/x86_msix.c
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 5febc0ea4b..2b120f897f 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -23,7 +23,7 @@
>  #include <asm/io.h>
>  #include <asm/smp.h>
>  #include <asm/desc.h>
> -#include <asm/msi.h>
> +#include <xen/msi.h>

You likely need to move this up to the xen/ prefixed include block.

>  #include <asm/fixmap.h>
>  #include <asm/p2m.h>
>  #include <mach_apic.h>
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c 
> b/xen/drivers/passthrough/amd/iommu_init.c
> index 559a734bda..fc385959c7 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -20,6 +20,7 @@
>  #include <xen/acpi.h>
>  #include <xen/delay.h>
>  #include <xen/keyhandler.h>
> +#include <xen/msi.h>
>  
>  #include "iommu.h"

Might be better to replace the asm/msi.h in include in iommu.h with
xen/msi.h instead (or just add the xen/msi.h include instead of
replace).

>  
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 1a1413b93e..543c265199 100644
> --- a/xen/drivers/vpci/Makefile
> +++ b/xen/drivers/vpci/Makefile
> @@ -1,2 +1,3 @@
>  obj-y += vpci.o header.o
>  obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
> +obj-$(CONFIG_X86) += x86_msix.o
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index 5757a7aed2..8fc82a9b8d 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -16,12 +16,11 @@
>   * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/msi.h>
>  #include <xen/sched.h>
>  #include <xen/softirq.h>
>  #include <xen/vpci.h>
>  
> -#include <asm/msi.h>
> -
>  static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg,
>                               void *data)
>  {
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 846f1b8d70..7a9b02f1a5 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -17,15 +17,24 @@
>   * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/msi.h>
>  #include <xen/sched.h>
>  #include <xen/vpci.h>
>  
> -#include <asm/msi.h>
>  #include <asm/p2m.h>
>  
> -#define VMSIX_ADDR_IN_RANGE(addr, vpci, nr)                               \
> -    ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
> -     (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
> +/*
> + * The return value is different for the MMIO handler on ARM and x86
> + * architecture. To make the code common for both architectures create
> + * generic return code with architecture dependent values.
> + */
> +#ifdef CONFIG_X86
> +#define VPCI_EMUL_OKAY      X86EMUL_OKAY
> +#define VPCI_EMUL_RETRY     X86EMUL_RETRY
> +#else
> +#define VPCI_EMUL_OKAY      1
> +#define VPCI_EMUL_RETRY     VPCI_EMUL_OKAY
> +#endif

Since msix_{read/write} are no longer directly used by the MMIO
handlers you might as well just return an error code (or a boolean)
and let the caller translate that into the per-arch return code.

>  
>  static uint32_t control_read(const struct pci_dev *pdev, unsigned int reg,
>                               void *data)
> @@ -138,29 +147,6 @@ static void control_write(const struct pci_dev *pdev, 
> unsigned int reg,
>          pci_conf_write16(pdev->sbdf, reg, val);
>  }
>  
> -static struct vpci_msix *msix_find(const struct domain *d, unsigned long 
> addr)
> -{
> -    struct vpci_msix *msix;
> -
> -    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
> -    {
> -        const struct vpci_bar *bars = msix->pdev->vpci->header.bars;
> -        unsigned int i;
> -
> -        for ( i = 0; i < ARRAY_SIZE(msix->tables); i++ )
> -            if ( bars[msix->tables[i] & PCI_MSIX_BIRMASK].enabled &&
> -                 VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, i) )
> -                return msix;
> -    }
> -
> -    return NULL;
> -}
> -
> -static int msix_accept(struct vcpu *v, unsigned long addr)
> -{
> -    return !!msix_find(v->domain, addr);
> -}
> -
>  static bool access_allowed(const struct pci_dev *pdev, unsigned long addr,
>                             unsigned int len)
>  {
> @@ -182,21 +168,19 @@ static struct vpci_msix_entry *get_entry(struct 
> vpci_msix *msix,
>      return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
>  }
>  
> -static int msix_read(struct vcpu *v, unsigned long addr, unsigned int len,
> -                     unsigned long *data)
> +int msix_read(struct vpci_msix *msix, unsigned long addr, unsigned int len,

This now requires a vpci_ prefix, since it's a global function.
Plain msix_{read,write} is way to generic.

> +              unsigned long *data)
>  {
> -    const struct domain *d = v->domain;
> -    struct vpci_msix *msix = msix_find(d, addr);
>      const struct vpci_msix_entry *entry;
>      unsigned int offset;
>  
>      *data = ~0ul;
>  
>      if ( !msix )
> -        return X86EMUL_RETRY;
> +        return VPCI_EMUL_RETRY;
>  
>      if ( !access_allowed(msix->pdev, addr, len) )
> -        return X86EMUL_OKAY;
> +        return VPCI_EMUL_OKAY;
>  
>      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
>      {
> @@ -210,11 +194,11 @@ static int msix_read(struct vcpu *v, unsigned long 
> addr, unsigned int len,
>          switch ( len )
>          {
>          case 4:
> -            *data = readl(addr);
> +            *data = vpci_arch_readl(addr);

Why do you need a vpci wrapper around the read/write handlers? AFAICT
arm64 also has {read,write}{l,q}. And you likely want to protect the
64bit read with CONFIG_64BIT if this code is to be made available to
arm32.

>              break;
>  
>          case 8:
> -            *data = readq(addr);
> +            *data = vpci_arch_readq(addr);
>              break;
>  
>          default:
> @@ -222,7 +206,7 @@ static int msix_read(struct vcpu *v, unsigned long addr, 
> unsigned int len,
>              break;
>          }
>  
> -        return X86EMUL_OKAY;
> +        return VPCI_EMUL_OKAY;
>      }
>  
>      spin_lock(&msix->pdev->vpci->lock);
> @@ -256,22 +240,20 @@ static int msix_read(struct vcpu *v, unsigned long 
> addr, unsigned int len,
>      }
>      spin_unlock(&msix->pdev->vpci->lock);
>  
> -    return X86EMUL_OKAY;
> +    return VPCI_EMUL_OKAY;
>  }
>  
> -static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
> -                      unsigned long data)
> +int msix_write(const struct domain *d, struct vpci_msix *msix,
> +               unsigned long addr, unsigned int len, unsigned long data)
>  {
> -    const struct domain *d = v->domain;
> -    struct vpci_msix *msix = msix_find(d, addr);
>      struct vpci_msix_entry *entry;
>      unsigned int offset;
>  
>      if ( !msix )
> -        return X86EMUL_RETRY;
> +        return VPCI_EMUL_RETRY;
>  
>      if ( !access_allowed(msix->pdev, addr, len) )
> -        return X86EMUL_OKAY;
> +        return VPCI_EMUL_OKAY;
>  
>      if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
>      {
> @@ -281,11 +263,11 @@ static int msix_write(struct vcpu *v, unsigned long 
> addr, unsigned int len,
>              switch ( len )
>              {
>              case 4:
> -                writel(data, addr);
> +                vpci_arch_writel(data, addr);
>                  break;
>  
>              case 8:
> -                writeq(data, addr);
> +                vpci_arch_writeq(data, addr);
>                  break;
>  
>              default:
> @@ -294,7 +276,7 @@ static int msix_write(struct vcpu *v, unsigned long addr, 
> unsigned int len,
>              }
>          }
>  
> -        return X86EMUL_OKAY;
> +        return VPCI_EMUL_OKAY;
>      }
>  
>      spin_lock(&msix->pdev->vpci->lock);
> @@ -372,60 +354,7 @@ static int msix_write(struct vcpu *v, unsigned long 
> addr, unsigned int len,
>      }
>      spin_unlock(&msix->pdev->vpci->lock);
>  
> -    return X86EMUL_OKAY;
> -}
> -
> -static const struct hvm_mmio_ops vpci_msix_table_ops = {
> -    .check = msix_accept,
> -    .read = msix_read,
> -    .write = msix_write,
> -};
> -
> -int vpci_make_msix_hole(const struct pci_dev *pdev)
> -{
> -    struct domain *d = pdev->domain;
> -    unsigned int i;
> -
> -    if ( !pdev->vpci->msix )
> -        return 0;
> -
> -    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> -    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> -    {
> -        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> -        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> -                                     vmsix_table_size(pdev->vpci, i) - 1);
> -
> -        for ( ; start <= end; start++ )
> -        {
> -            p2m_type_t t;
> -            mfn_t mfn = get_gfn_query(d, start, &t);
> -
> -            switch ( t )
> -            {
> -            case p2m_mmio_dm:
> -            case p2m_invalid:
> -                break;
> -            case p2m_mmio_direct:
> -                if ( mfn_x(mfn) == start )
> -                {
> -                    clear_identity_p2m_entry(d, start);
> -                    break;
> -                }
> -                /* fallthrough. */
> -            default:
> -                put_gfn(d, start);
> -                gprintk(XENLOG_WARNING,
> -                        "%pp: existing mapping (mfn: %" PRI_mfn
> -                        "type: %d) at %#lx clobbers MSIX MMIO area\n",
> -                        &pdev->sbdf, mfn_x(mfn), t, start);
> -                return -EEXIST;
> -            }
> -            put_gfn(d, start);
> -        }
> -    }
> -
> -    return 0;
> +    return VPCI_EMUL_OKAY;
>  }
>  
>  static int init_msix(struct pci_dev *pdev)
> @@ -472,11 +401,10 @@ static int init_msix(struct pci_dev *pdev)
>          vpci_msix_arch_init_entry(&msix->entries[i]);
>      }
>  
> -    if ( list_empty(&d->arch.hvm.msix_tables) )
> -        register_mmio_handler(d, &vpci_msix_table_ops);
> +    register_msix_mmio_handler(d);
> +    vpci_msix_add_to_msix_table(msix, d);
>  
>      pdev->vpci->msix = msix;
> -    list_add(&msix->next, &d->arch.hvm.msix_tables);

You could likely do the registering of the handler and the addition of
the table in the same handler: vpci_msix_arch_register or similar.

Thanks, Roger.