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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

H Jan,

> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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 ...

Code moved to x86 file is based on criteria that either the code will be 
unusable or will be different 
for ARM when MSIX  support will be introduce for ARM.
> 
>> --- 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.

I will maybe be wrong but what I understand from the code is that for x86 
if there is no p2m entries setup for the region, accesses to them will be 
trapped 
into the hypervisor and can be handled by specific MMIO handler.

But for ARM when we are registering the MMIO handler we have to provide 
the GPA also for the MMIO handler. 

For ARM arch vpci_make_msix_hole() will be used to register the MMIO handler 
for the MSIX MMIO region.

int vpci_make_msix_hole(const struct pci_dev *pdev)
{
    struct vpci_msix *msix = pdev->vpci->msix;
    paddr_t addr,size;

   for ( int i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
   {                                                                           
       addr = vmsix_table_addr(pdev->vpci, i);               
       size = vmsix_table_size(pdev->vpci, i) - 1;                              
                                           
       register_mmio_handler(pdev->domain, &vpci_msi_mmio_handler,             
                                              addr, size, NULL);                
                
    }                                                                           
                                      
    return 0;                                                                   
}

Therefore in this case there is difference how ARM handle this case.
 
> 
>> +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.

Yes, you are right here I can avoid this change if I will introduce 
"struct list_head msix_tables"  in "d->arch.hvm" for ARM also. 

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

Ok. Let me remove the x86_ prefixes in next version.  MMIO handler functions 
declaration is different 
for ARM and X86 therefore I need to move this code to arch specific file.

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

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

I will fix this in next version.

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

Ok. Let me move this change in separate patch in next series.

Regards,
Rahul
> 
> Jan
>