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
