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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.02.2022 16:25, 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/hvm/vmsi.c file to make sure common
> code will be used for other architecture.

Could you provide some criteria by which code is considered x86-specific
(or not)? For example ...

> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct vpci_msix *msix)
>  
>      return 0;
>  }
> +
> +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;
> +}

... nothing in this function looks to be x86-specific, except maybe
functions like clear_identity_p2m_entry() may not currently be available
on Arm. But this doesn't make the code x86-specific.

> +struct vpci_msix *vpci_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;
> +}

Or take this one - I don't see anything x86-specific in here. The use
of d->arch.hvm merely points out that there may be a field which now
needs generalizing.

> +static int x86_msix_accept(struct vcpu *v, unsigned long addr)
> +{
> +    return !!vpci_msix_find(v->domain, addr);
> +}
> +
> +static int x86_msix_write(struct vcpu *v, unsigned long addr, unsigned int 
> len,
> +                          unsigned long data)
> +{
> +    const struct domain *d = v->domain;
> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
> +
> +    return vpci_msix_write(msix, addr, len, data);
> +}
> +
> +static int x86_msix_read(struct vcpu *v, unsigned long addr, unsigned int 
> len,
> +                         unsigned long *data)
> +{
> +    const struct domain *d = v->domain;
> +    struct vpci_msix *msix = vpci_msix_find(d, addr);
> +
> +    return vpci_msix_read(msix, addr, len, data);
> +}
> +
> +static const struct hvm_mmio_ops vpci_msix_table_ops = {
> +    .check = x86_msix_accept,
> +    .read = x86_msix_read,
> +    .write = x86_msix_write,
> +};

I don't see the need to add x86_ prefixes to static functions while
you're moving them. Provided any of this is really x86-specific in
the first place.

> +void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
> +{
> +    if ( list_empty(&d->arch.hvm.msix_tables) )
> +        register_mmio_handler(d, &vpci_msix_table_ops);

This is perhaps the only thing which I could see would better live
in arch-specific code.

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -20,10 +20,10 @@
>  #include <xen/iocap.h>
>  #include <xen/keyhandler.h>
>  #include <xen/pfn.h>
> +#include <xen/msi.h>
>  #include <asm/io.h>
>  #include <asm/smp.h>
>  #include <asm/desc.h>
> -#include <asm/msi.h>

Just like you do here and elsewhere, ...

> --- a/xen/drivers/passthrough/amd/iommu.h
> +++ b/xen/drivers/passthrough/amd/iommu.h
> @@ -26,6 +26,7 @@
>  #include <xen/tasklet.h>
>  #include <xen/sched.h>
>  #include <xen/domain_page.h>
> +#include <xen/msi.h>
>  
>  #include <asm/msi.h>

... I think you want to remove this now redundant #include as well.

> --- a/xen/include/xen/msi.h
> +++ b/xen/include/xen/msi.h
> @@ -3,6 +3,34 @@
>  
>  #include <xen/pci.h>
>  
> +#define msi_control_reg(base)       (base + PCI_MSI_FLAGS)
> +#define msi_lower_address_reg(base) (base + PCI_MSI_ADDRESS_LO)
> +#define msi_upper_address_reg(base) (base + PCI_MSI_ADDRESS_HI)
> +#define msi_data_reg(base, is64bit) \
> +     ( (is64bit) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32 )
> +#define msi_mask_bits_reg(base, is64bit) \
> +     ( (is64bit) ? (base) + PCI_MSI_MASK_BIT : (base) + PCI_MSI_MASK_BIT - 4)
> +#define msi_pending_bits_reg(base, is64bit) \
> +     ( (is64bit) ? (base) + PCI_MSI_MASK_BIT + 4 : (base) + PCI_MSI_MASK_BIT)
> +#define msi_disable(control)        control &= ~PCI_MSI_FLAGS_ENABLE
> +#define multi_msi_capable(control) \
> +     (1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
> +#define multi_msi_enable(control, num) \
> +     control |= (((fls(num) - 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> +#define is_64bit_address(control)   (!!(control & PCI_MSI_FLAGS_64BIT))
> +#define is_mask_bit_support(control)    (!!(control & PCI_MSI_FLAGS_MASKBIT))
> +#define msi_enable(control, num) multi_msi_enable(control, num); \
> +     control |= PCI_MSI_FLAGS_ENABLE
> +
> +#define msix_control_reg(base)      (base + PCI_MSIX_FLAGS)
> +#define msix_table_offset_reg(base) (base + PCI_MSIX_TABLE)
> +#define msix_pba_offset_reg(base)   (base + PCI_MSIX_PBA)
> +#define msix_enable(control)        control |= PCI_MSIX_FLAGS_ENABLE
> +#define msix_disable(control)       control &= ~PCI_MSIX_FLAGS_ENABLE
> +#define msix_table_size(control)    ((control & PCI_MSIX_FLAGS_QSIZE)+1)
> +#define msix_unmask(address)        (address & ~PCI_MSIX_VECTOR_BITMASK)
> +#define msix_mask(address)          (address | PCI_MSIX_VECTOR_BITMASK)

Why did you put these not ...

>  #ifdef CONFIG_HAS_PCI_MSI

.. below here?

Also the movement of these is quite the opposite of what the title
says. IOW this likely wants to be a separate change.

Jan