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.
