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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 11 Oct 2021, Jan Beulich wrote:
> On 11.10.2021 15:34, Bertrand Marquis wrote:
> >> On 11 Oct 2021, at 14:09, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> 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 
> >>> 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.
> 
> >>>>> --- 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).

I know that MSIs are not supported yet on ARM (pci_cleanup_msi does
nothing). But I wanted to make sure that the pci_cleanup_msi() calls are
present anywhere necessary, especially on the error paths. So that once
we add MSI support, we don't need to search through the code to find all
the error paths missing a pci_cleanup_msi() call.

To answer your first question: you are right, we are also missing a
pci_cleanup_msi() call in the case of IOMMU error. So it might be better
to move the call to pci_cleanup_msi() under the "out" label so that we
can do it once for both cases.

To answer your second point about whether it is necessary at all: if
MSIs and MSI-Xs cannot be already setup at this point at all (not even
the enable bit), then we don't need any call to pci_cleanup_msi() in
pci_add_device.