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 <jbeul...@suse.com> 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
>
>
