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

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

Hi Jan,

> On 11 Oct 2021, at 14:09, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 11.10.2021 14:41, Bertrand Marquis wrote:
>>> On 7 Oct 2021, at 14:43, Jan Beulich <jbeulich@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 itMMCFG_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}.

Ok now I get what you wanted.

You would actually like both MMCFG_BDF and MMCFG_REG_OFFSET to
be moved to arch-arm.h.

Then I am not quite sure to follow why.
Those are not macros coming out of a way we have to define this but from
how it works in standard PCI.
The base and size are needed to know where the PCI bus will be.

So why should those be needed in public headers ?

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

Ok, I must admit I do not really see at that stage why doing an MSI cleanup
could be needed so I will wait for Stefano to know if I need to keep this when
moving the block up (at the end it is theoretical right now as this is empty).

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

Ok.

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

I will try to explain: when we just enable we do not add an entry in the list but
we still remove an entry from the list every time (as the condition becomes
always true because pdev->domain is at the end always set)

Regards
Bertrand

> 
> Jan
>