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

[Rahul Singh <Rahul.Singh@arm.com>]

Hi Roger,

Thanks for reviewing the code.

> On 14 Dec 2021, at 12:37 pm, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> 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@arm.com>
>> ---
>> 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.
Ok.
>> 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.
Ok.
> 
>> #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).

Ok.
> 
>> 
>> 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.

Ok.
> 
>> 
>> 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.
Ack. 
> 
>> +              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.

I need the wrapper because {read,write}{l,q} function argument is different for ARM and x86.
ARM {read,wrie}(l,q}  function argument is pointer to the address whereas X86  {read,wrie}(l,q} 
function argument is address itself.

> 
>>             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.

Ok.

Regards,
Rahul
> 
> Thanks, Roger.