Re: [PATCH v6 4/5] [FUTURE] xen/arm: enable vPCI for domUs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 5 Dec 2023, Stewart Hildebrand wrote:
> On 12/5/23 12:09, Roger Pau Monné wrote:
> > On Tue, Dec 05, 2023 at 11:27:03AM -0500, Stewart Hildebrand wrote:
> >> On 12/5/23 06:08, Roger Pau Monné wrote:
> >>> On Mon, Dec 04, 2023 at 02:07:51PM -0800, Stefano Stabellini wrote:
> >>>> On Mon, 4 Dec 2023, Roger Pau Monné wrote:
> >>>>> On Fri, Dec 01, 2023 at 06:56:32PM -0800, Stefano Stabellini wrote:
> >>>>>> On Fri, 1 Dec 2023, Roger Pau Monné wrote:
> >>>>>>> On Mon, Nov 13, 2023 at 05:21:13PM -0500, Stewart Hildebrand wrote:
> >>>>>>>> @@ -1618,6 +1630,14 @@ int iommu_do_pci_domctl(
> >>>>>>>>          bus = PCI_BUS(machine_sbdf);
> >>>>>>>>          devfn = PCI_DEVFN(machine_sbdf);
> >>>>>>>>  
> >>>>>>>> +        if ( needs_vpci(d) && !has_vpci(d) )
> >>>>>>>> +        {
> >>>>>>>> +            printk(XENLOG_G_WARNING "Cannot assign %pp to %pd: vPCI 
> >>>>>>>> support not enabled\n",
> >>>>>>>> +                   &PCI_SBDF(seg, bus, devfn), d);
> >>>>>>>> +            ret = -EPERM;
> >>>>>>>> +            break;
> >>>>>>>
> >>>>>>> I think this is likely too restrictive going forward.  The current
> >>>>>>> approach is indeed to enable vPCI on a per-domain basis because that's
> >>>>>>> how PVH dom0 uses it, due to being unable to use ioreq servers.
> >>>>>>>
> >>>>>>> If we start to expose vPCI suport to guests the interface should be on
> >>>>>>> a per-device basis, so that vPCI could be enabled for some devices,
> >>>>>>> while others could still be handled by ioreq servers.
> >>>>>>>
> >>>>>>> We might want to add a new flag to xen_domctl_assign_device (used by
> >>>>>>> XEN_DOMCTL_assign_device) in order to signal whether the device will
> >>>>>>> use vPCI.
> >>>>>>
> >>>>>> Actually I don't think this is a good idea. I am all for flexibility 
> >>>>>> but
> >>>>>> supporting multiple different configurations comes at an extra cost for
> >>>>>> both maintainers and contributors. I think we should try to reduce the
> >>>>>> amount of configurations we support rather than increasing them
> >>>>>> (especially on x86 where we have PV, PVH, HVM).
> >>>>>
> >>>>> I think it's perfectly fine to initially require a domain to have all
> >>>>> its devices either passed through using vPCI or ireqs, but the
> >>>>> interface IMO should allow for such differentiation in the future.
> >>>>> That's why I think introducing a domain wide vPCI flag might not be
> >>>>> the best option going forward.
> >>>>>
> >>>>> It would be perfectly fine for XEN_DOMCTL_assign_device to set a
> >>>>> domain wide vPCI flag, iow:
> >>>>>
> >>>>> if ( HYPERCALL_VPCI_FLAG_SET && !has_vpci(d) )
> >>>>> {
> >>>>>     if ( has_arch_pdevs(d) )
> >>>>>     {
> >>>>>         printk("All passthrough devices must use the same backend\n");
> >>>>>         return -EINVAL;
> >>>>>     }
> >>>>>
> >>>>>     /* Set vPCI domain flag */
> >>>>> }
> >>>>
> >>>> That would be fine by me, but maybe we can avoid this change too. I was
> >>>> imagining that vPCI would be enabled at domain creation, not at runtime.
> >>>> And that vPCI would be enabled by default for all PVH guests (once we
> >>>> are past the initial experimental phase.)
> >>>
> >>> Then we don't even need a new CDF flag, and just enable vPCI when
> >>> IOMMU is enabled?  IOW: we can key the enabling of vPCI to
> >>> XEN_DOMCTL_CDF_iommu for specific domain types?
> >>
> >> There are many Arm based platforms that need to use iommu but don't have 
> >> (or don't use) PCI, so we'd still like to have a separate vPCI flag.
> > 
> > OK, read below though - if we switch to vPCI being a descendant of
> > IOREQ (so that the PCI config space decoding is done by IOREQ) we
> > could hotplug vPCI managed devices at runtime without requiring any
> > prior initialization at domain create, since the traps to the PCI
> > config space would be setup by IOREQ.
> > 
> > We might need a PCI flag in order to signal whether the domain is
> > intended to use PCI devices or not, and so whether IOREQ needs to
> > setup PCI config space traps (either fully emulated or passthrough
> > devices).  But that would be arch-specific AFAICT, as on x86 we
> > always trap accesses to the PCI IO ports.
> 
> On Arm, the toolstack (or dom0less creation code) needs to construct a 
> {v,ioreq}PCI root complex node in the device tree before guest boot. 
> Attempting to hot plug such a device tree node at runtime sounds like a goal 
> for the (far) future. On Arm, we don't trap the {v,ioreq}PCI config space by 
> default, so, yes, we for sure would need such a {v,ioreq}PCI flag for setting 
> up the {v,ioreq}PCI mmio handlers if we go this route.

Yes and also dynamic configuration and hotplug are actually detrimental
in safety configurations where you need as much as possible, ideally
everything, to be specified at build time.