Re: [PATCH v2 3/3] [FUTURE] xen/arm: enable vPCI for domUs

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 07, 2023 at 01:09:40PM +0100, Julien Grall wrote:
> Hi,
> 
> On 07/07/2023 12:34, Roger Pau Monné wrote:
> > On Fri, Jul 07, 2023 at 12:16:46PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 07/07/2023 11:47, Roger Pau Monné wrote:
> > > > On Fri, Jul 07, 2023 at 11:33:14AM +0100, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 07/07/2023 11:06, Roger Pau Monné wrote:
> > > > > > On Fri, Jul 07, 2023 at 10:00:51AM +0100, Julien Grall wrote:
> > > > > > > On 07/07/2023 02:47, Stewart Hildebrand wrote:
> > > > > > > > Note that CONFIG_HAS_VPCI_GUEST_SUPPORT is not currently used 
> > > > > > > > in the upstream
> > > > > > > > code base. It will be used by the vPCI series [1]. This patch 
> > > > > > > > is intended to be
> > > > > > > > merged as part of the vPCI series.
> > > > > > > > 
> > > > > > > > v1->v2:
> > > > > > > > * new patch
> > > > > > > > ---
> > > > > > > >      xen/arch/arm/Kconfig              | 1 +
> > > > > > > >      xen/arch/arm/include/asm/domain.h | 2 +-
> > > > > > > >      2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > > > > > index 4e0cc421ad48..75dfa2f5a82d 100644
> > > > > > > > --- a/xen/arch/arm/Kconfig
> > > > > > > > +++ b/xen/arch/arm/Kconfig
> > > > > > > > @@ -195,6 +195,7 @@ config PCI_PASSTHROUGH
> > > > > > > >         depends on ARM_64
> > > > > > > >         select HAS_PCI
> > > > > > > >         select HAS_VPCI
> > > > > > > > +       select HAS_VPCI_GUEST_SUPPORT
> > > > > > > >         default n
> > > > > > > >         help
> > > > > > > >           This option enables PCI device passthrough
> > > > > > > > diff --git a/xen/arch/arm/include/asm/domain.h 
> > > > > > > > b/xen/arch/arm/include/asm/domain.h
> > > > > > > > index 1a13965a26b8..6e016b00bae1 100644
> > > > > > > > --- a/xen/arch/arm/include/asm/domain.h
> > > > > > > > +++ b/xen/arch/arm/include/asm/domain.h
> > > > > > > > @@ -298,7 +298,7 @@ static inline void arch_vcpu_block(struct 
> > > > > > > > vcpu *v) {}
> > > > > > > >      #define arch_vm_assist_valid_mask(d) (1UL << 
> > > > > > > > VMASST_TYPE_runstate_update_flag)
> > > > > > > > -#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && 
> > > > > > > > is_hardware_domain(d); })
> > > > > > > > +#define has_vpci(d)    ({ (void)(d); 
> > > > > > > > IS_ENABLED(CONFIG_HAS_VPCI); })
> > > > > > > 
> > > > > > > As I mentioned in the previous patch, wouldn't this enable vPCI
> > > > > > > unconditionally for all the domain? Shouldn't this be instead an 
> > > > > > > optional
> > > > > > > feature which would be selected by the toolstack?
> > > > > > 
> > > > > > I do think so, at least on x86 we signal whether vPCI should be
> > > > > > enabled for a domain using xen_arch_domainconfig at domain creation.
> > > > > > 
> > > > > > Ideally we would like to do this on a per-device basis for domUs, so
> > > > > > we should consider adding a new flag to xen_domctl_assign_device in
> > > > > > order to signal whether the assigned device should use vPCI.
> > > > > 
> > > > > I am a bit confused with this paragraph. If the device is not using 
> > > > > vPCI,
> > > > > how will it be exposed to the domain? Are you planning to support 
> > > > > both vPCI
> > > > > and PV PCI passthrough for a same domain?
> > > > 
> > > > You could have an external device model handling it using the ioreq
> > > > interface, like we currently do passthrough for HVM guests.
> > > 
> > > IMHO, if one decide to use QEMU for emulating the host bridge, then there 
> > > is
> > > limited point to also ask Xen to emulate the hostbridge for some other
> > > device. So what would be the use case where you would want to be a
> > > per-device basis decision?
> > 
> > You could also emulate the bridge in Xen and then have QEMU and
> > vPCI handle accesses to the PCI config space for different devices.
> > The ioreq interface already allows registering for config space
> > accesses on a per SBDF basis.
> > 
> > XenServer currently has a use-case where generic PCI device
> > passthrough is handled by QEMU, while some GPUs are passed through
> > using a custom emulator.  So some domains effectively end with a QEMU
> > instance and a custom emulator, I don't see why you couldn't
> > technically replace QEMU with vPCI in this scenario.
> > 
> > The PCI root complex might be emulated by QEMU, or ideally by Xen.
> > That shouldn't prevent other device models from handling accesses for
> > devices, as long as accesses to the ECAM region(s) are trapped and
> > decoded by Xen.  IOW: if we want bridges to be emulated by ioreq
> > servers we need to introduce an hypercall to register ECAM regions
> > with Xen so that it can decode accesses and forward them
> > appropriately.
> 
> Thanks for the clarification. Going back to the original discussion. Even
> with this setup, I think we still need to tell at domain creation whether
> vPCI will be used (think PCI hotplug).

Well, for PCI hotplug you will still need to execute a
XEN_DOMCTL_assign_device hypercall in order to assign the device, at
which point you could pass the vPCI flag.

What you likely want at domain create is whether the IOMMU should be
enabled or not, as we no longer allow late enabling of the IOMMU once
the domain has been created.

One question I have is whether Arm plans to allow exposing fully
emulated devices on the PCI config space, or that would be limited to
PCI device passthrough?

IOW: should an emulated PCI root complex be unconditionally exposed to
guests so that random ioreq servers can register for SBDF slots?

> After that, the device assignment hypercall could have a way to say whether
> the device will be emulated by vPCI. But I don't think this is necessary to
> have from day one as the ABI will be not stable (this is a DOMCTL).

Indeed, it's not a stable interface, but we might as well get
something sane if we have to plumb it through the tools.  Either if
it's a domain create flag or a device attach flag you will need some
plumbing to do at the toolstack level, at which point we might as well
use an interface that doesn't have arbitrary limits.

Thanks, Roger.