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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

Please recall that we try to avoid casts where possible.

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

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

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.

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

Jan