Re: [RFC PATCH 2/6] xen/public: arch-arm: reserve resources for virtio-pci

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.