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 15:10, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 11.10.2021 15:34, Bertrand Marquis wrote:
>>> 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 ?
> 
> Well, see my "Otoh ..." in the earlier reply. Keeping the two
> address splitting macros out of there is okay.

Ok.

> 
>>>>>> --- 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)
> 
> Well, that anomaly is what I did point out in my review remarks to
> Rahul. We shouldn't remove an entry from the list if we didn't add
> one. But quite likely, if we don't free the pdev, we shouldn't be
> removing the list entry in either case.

This problem will not exist anymore when I will move the code up but I will add to adapt the error case in iommu to also remove the vpci handlers.
To be coherent I will do the same in the pci_remove_device code.

I will do all those in the v6 of the serie.

Thanks a lot for the answers.

Cheers
Bertrand

> 
> Jan
>