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

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

Hi Jan,

Thanks for reviewing the code.

> On 14 Dec 2021, at 2:15 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> 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).
> 

Ack. I will split the changes in next version.

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

Yes I will not change the order I will fix this in next version.
> 
>> --- /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.

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

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

Ok . Let me try to fix this in next version

Regards,
Rahul
> 
> Jan
>