Hi Jan,
> On 18 Oct 2021, at 08:47, Jan Beulich <[email protected]> wrote:
>
> On 15.10.2021 18:51, Bertrand Marquis wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,77 @@
>> +/*
>> + * 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 <xen/vpci.h>
>> +
>> +#include <asm/mmio.h>
>> +
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> + register_t *r, void *p)
>> +{
>> + pci_sbdf_t sbdf;
>> + /* data is needed to prevent a pointer cast on 32bit */
>> + unsigned long data;
>> +
>> + /* We ignore segment part and always handle segment 0 */
>> + sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
>> +
>> + if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>> + 1U << info->dabt.size, &data) )
>> + {
>
> Here it is quite clear that the SBDF you pass into vpci_ecam_read() is
> the virtual one. The function then calls vpci_read(), which in turn
> will call vpci_read_hw() in a number of situations (first and foremost
> whenever pci_get_pdev_by_domain() returns NULL). That function as well
> as pci_get_pdev_by_domain() use the passed in SBDF as if it was a
> physical one; I'm unable to spot any translation. Yet I do recall
> seeing assignment of a virtual device and function number somewhere
> (perhaps another of the related series), so the model also doesn't
> look to assume 1:1 mapping of SBDF.
This question was answered by Oleksandr I think.
>
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>> if ( !pdev->domain )
>> {
>> pdev->domain = hardware_domain;
>> +#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);
>> + pdev->domain = NULL;
>> + goto out;
>> + }
>> +#endif
>> ret = iommu_add_device(pdev);
>> if ( ret )
>> {
>
> Upon failure, vpci_add_handlers() will itself call vpci_remove_handlers().
> What about iommu_add_device() failure? The device will have ->domain
> zapped, but all vPCI handlers still in place. This aspect of insufficient
> error cleanup was pointed out already in review of v1.
Yes a call to vpci_remove_device should be made on the error path out if
iommu_add_device is failing. This should also be done in fact in
pci_remove_device before cleanup the msi.
We will push a patch with a proposal for a fix for this.
>
> Furthermore already in v1 I questioned why this would be Arm-specific: On
> x86 this code path is going to be taken for all devices Xen wasn't able
> to discover at boot (anything on segments we wouldn't consider config
> space access safe on without reassurance by Dom0 plus SR-IOV VFs, at the
> very least). Hence it is my understanding that something along these
> lines is actually also needed for PVH Dom0. I've just checked, and
> according to my mailbox that comment was actually left unresponded to.
>
> Roger, am I missing anything here as to PVH Dom0 getting handlers put in
> place?
>From Roger answer I understood that it will be needed (in the future).
When and if this is needed, the ifdef CONFIG_ARM can be removed
but this would change x86 code behaviour so I do not think it would
have been right to do that in this serie.
>
> Of course as soon as the CONFIG_ARM conditionals were dropped, the
> __hwdom_init issue would become an "active" one.
We will push a proposal for a fix for that.
If I understand Roger right, vpci_add_handler will also be needed in runtime
on x86 in the future so maybe it would even be right to remove the flag
altogether ?
Regards
Bertrand
>
> Jan
>
