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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@xxxxxxxx> 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

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

> 
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -374,7 +374,8 @@ static void bar_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>         if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>>             gprintk(XENLOG_WARNING,
>>                     "%pp: ignored BAR %lu write with memory decoding 
>> enabled\n",
>> -                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi);
>> +                    &pdev->sbdf,
>> +                    (unsigned long)(bar - pdev->vpci->header.bars + hi));
> 
> This looks like an entirely unrelated change which I'm afraid I don't
> even understand why it needs making. The description says this is for
> Arm32, but it remains unclear what the compilation error would have
> been. My best guess is that perhaps you really mean to change the
> format specifier to %zu (really this should be %td, but our vsprintf()
> doesn't support 't' for whatever reason).

Compilation error is about an invalid %lu print for an unsigned int value
when compiled for arm32.

I will use zu instead in the next version and remove the cast.

> 
> Please recall that we try to avoid casts where possible.

Sure I will.

> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -507,7 +507,7 @@ int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int 
>> reg, unsigned int len,
>> 
>>     vpci_write(sbdf, reg, min(4u, len), data);
>>     if ( len == 8 )
>> -        vpci_write(sbdf, reg + 4, 4, data >> 32);
>> +        vpci_write(sbdf, reg + 4, 4, (uint64_t)data >> 32);
> 
> I assume  the need for this change will go away with the use of
> CONFIG_64BIT in the earlier patch.

Yes

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

@Roger can you confirm this is what is wanted ?

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


> 
> Also, as already pointed out on a much earlier version, while the
> blank following the opening parenthesis was warranted in
> asm-x86/pci.h for alignment reasons, it is no longer warranted here.
> It was certainly gone in v4 and v5.

Yes my mistake during the process I did a copy and paste from the original
one and did not modify. I will fix that.

> 
> And one final nit: I don't think we commonly use full stops in patch
> titles. (Personally I therefore also think titles shouldn't start
> with a capital letter, but I know others think differently.)

Ok I will fix the patch name.

Cheers
Bertrand

> 
> Jan
>