Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Jan Beulich <jbeulich@suse.com>]

On 15.10.2021 12:09, Bertrand Marquis wrote:
>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote:
>> On 14.10.2021 16:49, Bertrand Marquis wrote:
>>> @@ -752,6 +752,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>
>>>     check_pdev(pdev);
>>>
>>> +#ifdef CONFIG_ARM
>>> +    /*
>>> +     * On ARM PCI devices discovery will be done by Dom0. Add vpci handler when
>>> +     * Dom0 inform XEN to add the PCI devices in XEN.
>>> +     */
>>> +    ret = vpci_add_handlers(pdev);
>>> +    if ( ret )
>>> +    {
>>> +        printk(XENLOG_ERR "Setup of vPCI failed: %d\n", ret);
>>> +        goto out;
>>> +    }
>>> +#endif
>>> +
>>>     ret = 0;
>>>     if ( !pdev->domain )
>>
>> Elsewhere I did point out that you need to be careful wrt pdev->domain
>> being NULL. As the code in context clearly documents, you're now
>> adding handlers before that field was set. In comments to a prior
>> version I did already suggest to consider placing the new code inside
>> the if() in question (albeit at the time this was mainly to make clear
>> that error cleanup may not fiddle with things not put in place by the
>> iommu_enable_device() alternative path). This would have the further
>> benefit of making is crystal clear that here only handlers for Dom0
>> would get put in place (and hence their installing for DomU-s would
>> need to be looked for elsewhere).
> 
> I asked Oleksandr for confirmation here but on arm there will be 2 other use cases:
> - PCI own by a DomD so device enumeration done from there
> - dom0less with devices detection done inside Xen

Question is whether at this time you mean to handle all these cases.
Installing handlers when device detection happens in Xen might need
to be done differently, for example - e.g. more along the lines of
what x86 PVH Dom0 has done.

Anything you don't mean to handle (and is safe to be left out, i.e.
without breaking existing cases) will want spelling out in the
description.

>>> @@ -784,6 +797,9 @@ out:
>>>                    &PCI_SBDF(seg, bus, slot, func));
>>>         }
>>>     }
>>> +    else if ( pdev )
>>> +        pci_cleanup_msi(pdev);
>>
>> Have you thoroughly checked that this call is benign on x86? There's
>> no wording to that effect in the description afaics, and I can't
>> easily convince myself that it would be correct when the
>> iommu_enable_device() path was taken. (I'm also not going to
>> exclude that it should have been there even prior to your work,
>> albeit then adding this would want to be a separate bugfix patch.)
> 
> This was not in the original serie and requested by Stefano. I must admit
> I am not completely sure on the details here so I am really ok to remove this
> but this would go against what was requested on the previous versions (4 and 5).

I understand this, but a request to add something still requires that
it be checked that the addition can't have bad effects in other cases.
A compromise here (which I wouldn't like very much) might be to add
another #ifdef CONFIG_ARM, solely to leave the existing x86 case
undisturbed. Yet then, with the called function doing nothing on Arm,
not adding the code here might as well be an option (which it looked
like Stefano would be amenable to). A TODO item would then perhaps
best be left here in a comment.

>>> --- a/xen/include/xen/pci.h
>>> +++ b/xen/include/xen/pci.h
>>> @@ -40,6 +40,9 @@
>>> #define PCI_SBDF3(s,b,df) \
>>>     ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
>>>
>>> +#define ECAM_BDF(addr)         ( ((addr) & 0x0ffff000) >> 12)
>>> +#define ECAM_REG_OFFSET(addr)  ((addr) & 0x00000fff)
>>
>> The latter is fine to be put here (i.e. FTAOD I'm fine with it
>> staying here). For the former I even question its original placement
>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as
>> the bus portion of the address can be anywhere from 1 to 8 bits. And
>> in fact there is a reason why this macro was/is used in only a
>> single place, but not e.g. in x86'es handling of physical MCFG. It
>> is merely an implementation choice in vPCI that the entire segment 0
>> has a linear address range covering all 256 buses. Hence I think
>> this wants to move to xen/vpci.h and then perhaps also be named
>> VPCI_ECAM_BDF().
> 
> On previous version it was request to renamed this to ECAM and agreed
> to put is here. Now you want me to rename it to VPCI and move it again.
> I would like to have a confirmation that this is ok and the final move
> if possible.

A final confirmation is, unfortunately, only as final as it can be at
the very moment it is given. It was the MCFG vs ECAM naming discussion
which made me pull out again the section in the spec, reminding me of
aspects I didn't previously take into consideration. I'm sorry for
this, but it's an iterative process on all sides.

So, FTAOD, as a maintainer of this headed I will continue to object to
a non-spec-compliant construct to be put into here. It'll be Roger,
being the vPCI maintainer, to confirm that putting it in xen/vpci.h is
fine.

> Also if I have to do this I will do the same for REG_OFFSET of course.

As said - that one, being in line with the PCI spec, is fine to remain
as and where you have it.

Jan