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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2021 11:45, Rahul Singh wrote:
> --- 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

In addition to what Roger has said, at the example of the above I think
you want to split this change. The change in return value naming could
likely quite well be a separate thing. And then it'll be easier to see
which other suggested changes are really movement of x86-specific stuff
(looking over it I wasn't convinced everything you move really is).

> @@ -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);
>  
>      return 0;

May I ask that you don't alter the order of operations? I take it that
vpci_msix_add_to_msix_table() is the replacement of the list_add().
That should occur only after pdev->vcpi has been updated. I could in
fact imagine that in cases like this one for Arm barriers may need
adding.

> --- /dev/null
> +++ b/xen/drivers/vpci/x86_msix.c
> @@ -0,0 +1,155 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/vpci.h>
> +
> +#include <asm/msi.h>
> +#include <asm/p2m.h>
> +
> +u32 vpci_arch_readl(unsigned long addr)

Nit: No new uses of u<N> please; these are being phased out, with
uint<N>_t being the intended types.

> +{
> +    return readl(addr);
> +}
> +
> +u64 vpci_arch_readq(unsigned long addr)
> +{
> +    return readq(addr);
> +}
> +
> +void vpci_arch_writel(u32 data, unsigned long addr)
> +{
> +    writel(data, addr);
> +}
> +
> +void vpci_arch_writeq(u64 data, unsigned long addr)
> +{
> +    writeq(data, addr);
> +}

Functions like these (if, as Roger said, they need abstracting in the
first place) or ...

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

... these would imo better be inline helpers.

> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -148,34 +148,6 @@ int msi_free_irq(struct msi_desc *entry);
>   */
>  #define NR_HP_RESERVED_VECTORS       20
>  
> -#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 == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )
> -#define msi_mask_bits_reg(base, is64bit) \
> -     ( (is64bit == 1) ? base+PCI_MSI_MASK_BIT : base+PCI_MSI_MASK_BIT-4)
> -#define msi_pending_bits_reg(base, is64bit) \
> -     ((base) + PCI_MSI_MASK_BIT + ((is64bit) ? 4 : 0))
> -#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)
> -
>  /*
>   * MSI Defined Data Structures
>   */
> diff --git a/xen/include/xen/msi.h b/xen/include/xen/msi.h
> index c903d0050c..1c22c9a4a7 100644
> --- 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 == 1) ? base+PCI_MSI_DATA_64 : base+PCI_MSI_DATA_32 )

As you move this code, please tidy is style-wise. For the construct
here, for example this would mean

#define msi_data_reg(base, is64bit) \
    ((is64bit) ? (base) + PCI_MSI_DATA_64 : (base) + PCI_MSI_DATA_32)

or perhaps even

#define msi_data_reg(base, is64bit) \
    ((base) + ((is64bit) ? PCI_MSI_DATA_64 : PCI_MSI_DATA_32))

Further items would want similar adjustments.

Jan