Re: [PATCH 4/6] qemu: assign PCI address to device pvpanic-pci

On 2/9/23 17:33, Andrea Bolognani wrote: > On Wed, Feb 08, 2023 at 12:49:03PM +0100, Kristina Hanicova wrote: >> +++ b/src/qemu/qemu_domain_address.c >> @@ -1062,10 +1062,24 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev, >> } >> break; >> >> + case VIR_DOMAIN_DEVICE_PANIC: >> + switch ((virDomainPanicModel) dev->data.panic->model) { >> + case VIR_DOMAIN_PANIC_MODEL_PVPANIC: >> + return pciFlags; > > So, this is correct, because pvpanic-pci is indeed a conventional PCI > device (as opposed to a PCI Express device). > > However, when used with a PCIe-based machine such as aarch64/virt, > these flags result in the device being plugged into a > pcie-pci-bridge, which in turn is plugged into a pcie-root-port, > itself plugged into pcie.0. > > While this seems to work fine, it's also kind of a waste of PCI > controllers. For a "system" device such as pvpanic-pci, I think > plugging directly into pcie.0 might be more appropriate. This is > particularly true of x86/q35, where with the current implementation > switching from ISA pvpanic to pvpanic-pci would result in adding two > extra PCI controllers. > > So I think this would be a good fit for the > VIR_PCI_CONNECT_INTEGRATED flag that I've added a while ago for use > with virtio-iommu. While this would technically result in libvirt > being more restrictive than QEMU in what PCI addresses it accepts for > pvpanic-pci, I don't think this would limit users in practice, and in > the default case (libvirt automatically picking the address) the > resulting configuration would be preferable. > > We can always decide to relax this restriction later down the line, > if it turns out to really be a limiting factor. > > Eric, what do you think about this idea? I do agree. If we can avoid putting that device downstream to a pcie-to-pci bridge that's much better. Putting it onto pcie.0 looks more appropriate to me indeed.

> (There's one caveat: VIR_PCI_CONNECT_INTEGRATED doesn't seem to work > with pciFlags at the moment O:-) It works fine with pcieFlags and > virtioFlags. I'll try to figure out why that's the case.)