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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.10.2021 14:41, Bertrand Marquis wrote:
>> 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 ?

That would be okay and make sense when put next to MMCFG_BDF(), but
it would not address my comment: That still wouldn't be the public
interface. Otoh you only mimic hardware behavior, so perhaps the
splitting of the address isn't as relevant to put there as would be
GUEST_VPCI_ECAM_{BASE,SIZE}.

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

Maybe, but I'm not sure. This very much depends on what a domain
could in principle do with a partly set-up device. Plus let's
not forget that we're talking of only Dom0 here (for now at least,
i.e. not considering the dom0less case).

But I'd also like to further defer to Stefano.

> Same could also go for the free_pdev ?

I think it's wrong to free_pdev() here. We want to internally keep
record of the device, even if the device ends up unusable. The only
place where free_pdev() ought to be called is imo pci_remove_device().

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

I'm sorry, but I'm afraid I don't see what "the path" refers to
here. You can't mean the conditional in pci_add_device() selecting
between iommu_add_device() and iommu_enable_device(), as "remove"
can only mean "remove", never "disable".

Jan