On Tue, 21 Nov 2023, Stewart Hildebrand wrote:
> On 11/17/23 03:11, Oleksandr Tyshchenko wrote:
> >
> >
> > On 17.11.23 05:31, Stewart Hildebrand wrote:
> >
> > Hello Stewart
> >
> > [answering only for virtio-pci bits as for vPCI I am only familiar with
> > code responsible for trapping config space accesses]
> >
> > [snip]
> >
> >>>
> >>>
> >>> Let me start by saying that if we can get away with it, I think that a
> >>> single PCI Root Complex in Xen would be best because it requires less
> >>> complexity. Why emulate 2/3 PCI Root Complexes if we can emulate only
> >>> one?
> >>>
> >>> Stewart, you are deep into vPCI, what's your thinking?
> >>
> >> First allow me explain the moving pieces in a bit more detail (skip ahead
> >> to "Back to the question: " if you don't want to be bored with the
> >> details). I played around with this series, and I passed through a PCI
> >> device (with vPCI) and enabled virtio-pci:
> >>
> >> virtio = [
> >> "type=virtio,device,transport=pci,bdf=0000:00:00.0,backend_type=qemu" ]
> >> device_model_args = [ "-device", "virtio-serial-pci" ]
> >> pci = [ "01:00.0" ]
> >>
> >> Indeed we get two root complexes (2 ECAM ranges, 2 sets of interrupts,
> >> etc.) from the domU point of view:
> >>
> >> pcie@10000000 {
> >> compatible = "pci-host-ecam-generic";
> >> device_type = "pci";
> >> reg = <0x00 0x10000000 0x00 0x10000000>;
> >> bus-range = <0x00 0xff>;
> >> #address-cells = <0x03>;
> >> #size-cells = <0x02>;
> >> status = "okay";
> >> ranges = <0x2000000 0x00 0x23000000 0x00 0x23000000 0x00
> >> 0x10000000 0x42000000 0x01 0x00 0x01 0x00 0x01 0x00>;
> >> #interrupt-cells = <0x01>;
> >> interrupt-map = <0x00 0x00 0x00 0x01 0xfde8 0x00 0x74 0x04>;
> >> interrupt-map-mask = <0x00 0x00 0x00 0x07>;
> >
> >
> > I am wondering how you got interrupt-map here? AFAIR upstream toolstack
> > doesn't add that property for vpci dt node.
>
> You are correct. I'm airing my dirty laundry here: this is a hack to get a
> legacy interrupt passed through on a ZCU102 board (Zynq UltraScale+), which
> doesn't have GICv3/ITS. In a system with a GICv3/ITS, we would have an
> msi-map property (this is also not yet upstream, but is in a couple of
> downstreams).
>
> >
> >> };
> >>
> >> pcie@33000000 {
> >> compatible = "pci-host-ecam-generic";
> >> device_type = "pci";
> >> reg = <0x00 0x33000000 0x00 0x200000>;
> >> bus-range = <0x00 0x01>;
> >> #address-cells = <0x03>;
> >> #size-cells = <0x02>;
> >> status = "okay";
> >> ranges = <0x2000000 0x00 0x34000000 0x00 0x34000000 0x00 0x800000
> >> 0x42000000 0x00 0x3a000000 0x00 0x3a000000 0x00 0x800000>;
> >> dma-coherent;
> >> #interrupt-cells = <0x01>;
> >> interrupt-map = <0x00 0x00 0x00 0x01 0xfde8 0x00 0x0c 0x04 0x00
> >> 0x00 0x00 0x02 0xfde8 0x00 0x0d 0x04 0x00 0x00 0x00 0x03 0xfde8 0x00 0x0e
> >> 0x04 0x00 0x00 0x00 0x04 0xfde8 0x00 0x0f 0x04 0x800 0x00 0x00 0x01 0xfde8
> >> 0x00 0x0d 0x04 0x800 0x00 0x00 0x02 0xfde8 0x00 0x0e 0x04 0x800 0x00 0x00
> >> 0x03 0xfde8 0x00 0x0f 0x04 0x800 0x00 0x00 0x04 0xfde8 0x00 0x0c 0x04
> >> 0x1000 0x00 0x00 0x01 0xfde8 0x00 0x0e 0x04 0x1000 0x00 0x00 0x02 0xfde8
> >> 0x00 0x0f 0x04 0x1000 0x00 0x00 0x03 0xfde8 0x00 0x0c 0x04 0x1000 0x00
> >> 0x00 0x04 0xfde8 0x00 0x0d 0x04 0x1800 0x00 0x00 0x01 0xfde8 0x00 0x0f
> >> 0x04 0x1800 0x00 0x00 0x02 0xfde8 0x00 0x0c 0x04 0x1800 0x00 0x00 0x03
> >> 0xfde8 0x00 0x0d 0x04 0x1800 0x00 0x00 0x04 0xfde8 0x00 0x0e 0x04>;
> >> interrupt-map-mask = <0x1800 0x00 0x00 0x07>;
> >
> >
> > that is correct dump.
> >
> > BTW, if you added "grant_usage=1" (it is disabled by default for dom0)
> > to virtio configuration you would get iommu-map property here as well
> > [1]. This is another point to think about when considering combined
> > approach (single PCI Host bridge node -> single virtual root complex), I
> > guess usual PCI device doesn't want grant based DMA addresses, correct?
> > If so, it shouldn't be specified in the property.
>
> Right, a usual PCI device doesn't want/need grant based DMA addresses. The
> iommu-map property [2] is flexible enough to make it apply only to certain
> vBDFs or ranges of vBDFs. Actually, it looks like ("libxl/arm: Reuse generic
> PCI-IOMMU bindings for virtio-pci devices") already has the logic for doing
> exactly this. So it should be fine to have the iommu-map property in the
> single root complex and still do regular PCI passthrough.
>
> Presumably, we would also want msi-map [3] to apply to some vBDFs, not
> others. msi-map has the same flexible BDF matching as iommu-map (these two
> bindings actually share the same parsing function), so we should be able to
> use a similar strategy here if needed.
>
> We would also want interrupt-map [4] to apply to some vBDFs, not others. The
> BDF matching with interrupt-map behaves slightly differently, but I believe
> it is still flexible enough to achieve this. In this case, the function
> create_virtio_pci_irq_map(), added in ("libxl/arm: Add basic virtio-pci
> support"), would need some re-thinking.
>
> In summary, with a single virtual root complex, the toolstack needs to know
> which vBDFs are virtio-pci, and which vBDFs are passthrough, so it can create
> the [iommu,msi,interrupt]-map properties accordingly.
That should be fine given that the toolstack also knows all the PCI
Passthrough devices too.
