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

From: Rahul Singh <Rahul.Singh@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Bertrand Marquis" <Bertrand.Marquis@arm.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file
Date: Thu, 3 Mar 2022 16:31:12 +0000	[thread overview]
Message-ID: <41F03A07-F8D6-423D-9E40-F4B89C611C87@arm.com> (raw)
In-Reply-To: <c11278f4-c902-0eb9-ac59-e27c0f2ba912@suse.com>

Hi Jan,

> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 01.03.2022 14:34, Rahul Singh wrote:
>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> 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.
> 
> That's a very abstract statement, which you can't really derive any
> judgement from. It'll be necessary to see in how far the code is
> indeed different. After all PCI, MSI, and MSI-X are largely arch-
> agnostic.
> 
>>>> --- 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. 
> 
> Question is: Is this just an effect resulting from different implementation,
> or an inherent requirement? In the former case, harmonizing things may be an
> alternative option.

This is an inherent requirement to provide a GPA when registering the MMIO handler.

For x86 msix mmio handlers is registered in init_msix(..) function as there is no requirement
on x86 to provide GPA when registering the handler. Later point of time when BARs are configured
and memory decoding bit is enabled vpci_make_msix_hole() will clear the identity mapping for msix
base table address so that access to msix tables will be trapped.

On ARM we need to provide GPA to register the mmio handler and MSIX table base
address is not valid when init_msix() is called as BAR will be configured later point in time.
Therefore on ARM mmio handler will be registered in function vpci_make_msix_hole() when
memory decoding bit is enabled.

> 
>> 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. 
> 
> Wait - if you pass in the guest address at registration time, you
> shouldn't have a need for a "find" function.

Yes you are right we don’t need to call msix_find() on ARM. In that case there is
need to move msix_find() function to x86 file as I did in v1.

Regards,
Rahul
> 
> Jan
> 