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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

As Rahul is on leave, I will answer you and make the changes needed.

> On 7 Oct 2021, at 14:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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}?

I will move that in the next version to xen/pci.h and rename it 
MMCFG_REG_OFFSET.
Would that be ok ?

> 
>> +/* 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).

I will remove the check for len > 8 as dabt.size cannot have a value
greater than 3.

But I will have to introduce a check for len > 4 on 32 bit systems (see after).

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

Unaligned IO access could be synthesise also on arm to but I would
rather not make such a change now without testing it (and there is
also a question of it making sense).

So if it is ok with you I will keep that check and discuss it with Rahul
when he is back. I will add a comment in the code to make that clear.

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

Right I will remove it.

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

I will add the following comment just before:
/* We ignore segment part and always handle segment 0 */

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

Dabt.size could be 3 even on ARM32 but we should not allow 64bit
access on mmio regions on arm32.

So I will surround this code with ifdef CONFIG_ARM_64 and add a test
for len > 4 to prevent this case on 32bit.

To be completely right we should disable this also for 32bit guests but
this change would be a bit more invasive.

> 
>> +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?

We can and I will remove the data variable.

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

I agree with you so I will move this code block before iommu part.

But digging deeper into this, I would have 2 questions:

- msi_cleanup was done there after a request from Stefano, but is not
done in case or iommu error, is there an issue to solve here ?
Same could also go for the free_pdev ?

- cleanup code was exactly the same as pci_remove_device code.
Should the question about the path also be checked there ?

Regards
Bertrand


> 
> Jan
> 
>