Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.10.2021 19:40, Rahul Singh wrote:
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,102 @@
> +/*
> + * xen/arch/arm/vpci.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/sched.h>
> +
> +#include <asm/mmio.h>
> +
> +#define REGISTER_OFFSET(addr)  ( (addr) & 0x00000fff)

Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
Also isn't this effectively part of the public interface (along with
MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?

> +/* Do some sanity checks. */
> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
> +{
> +    /* Check access size. */
> +    if ( len > 8 )
> +        return false;

struct hsr_dabt's size field doesn't allow len to be above 8. I could
see that you may want to sanity check things, but that's not helpful
if done incompletely. Elsewhere you assume the value to be non-zero,
and ...

> +    /* Check that access is size aligned. */
> +    if ( (reg & (len - 1)) )

... right here you assume the value to be a power of 2. While I'm not
a maintainer, I'd still like to suggest consistency: Do all pertinent
checks or none of them (relying on the caller).

Independent of this - is bare metal Arm enforcing this same
alignment restriction (unconditionally)? Iirc on x86 we felt we'd
better synthesize misaligned accesses.

> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                          register_t *r, void *p)
> +{
> +    unsigned int reg;
> +    pci_sbdf_t sbdf;
> +    unsigned long data = ~0UL;

What use is this initializer? The error path further down doesn't
forward the value into *r, and subsequently the value gets fully
overwritten.

> +    unsigned int size = 1U << info->dabt.size;
> +
> +    sbdf.sbdf = MMCFG_BDF(info->gpa);

This implies segment to be zero. While probably fine for now, I
wonder if this wouldn't warrant a comment.

> +    reg = REGISTER_OFFSET(info->gpa);
> +
> +    if ( !vpci_mmio_access_allowed(reg, size) )
> +        return 0;
> +
> +    data = vpci_read(sbdf, reg, min(4u, size));
> +    if ( size == 8 )
> +        data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;

Throughout this series I haven't been able to spot where the HAS_VPCI
Kconfig symbol would get selected. Hence I cannot tell whether all of
this is Arm64-specific. Otherwise I wonder whether size 8 actually
can occur on Arm32.

> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> +                           register_t r, void *p)
> +{
> +    unsigned int reg;
> +    pci_sbdf_t sbdf;
> +    unsigned long data = r;

A little like in the read function - what use is this local variable?
Can't you use r directly?

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>      else
>          iommu_enable_device(pdev);

Please note the context above; ...

> +#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);
> +        pci_cleanup_msi(pdev);
> +        ret = iommu_remove_device(pdev);
> +        if ( pdev->domain )
> +            list_del(&pdev->domain_list);
> +        free_pdev(pseg, pdev);

... you unconditionally undo the if() part of the earlier conditional;
is there any guarantee that the other path can't ever be taken, now
and forever? If it can't be taken now (which I think is the case as
long as Dom0 wouldn't report the same device twice), then at least some
precaution wants taking. Maybe moving your addition into that if()
could be an option.

Furthermore I continue to wonder whether this ordering is indeed
preferable over doing software setup before hardware arrangements. This
would address the above issue as well as long as vpci_add_handlers() is
idempotent. And it would likely simplify error cleanup.

Jan