Hi, > > And, yes, the logic to match entries in the fw_cfg file with the correct > > device using vendor and device id looks somewhat fragile to me too. > > > > Existing code in qemu+firmware (for example bootorder) uses the location > > in the physical device tree to identify devices, like this: > > > > /pci@i0cf8/pci-bridge@3/*@0/*@0/*@0,0 > > ^^^^^^^^^ pcie root bus > > ^^^^^^^^^^^^ pcie root port @ slot 3 > > ^^^ virtio-scsi-pci @ slot 0 > > ^^^ scsi controller bus #0 > > ^^^^^ scsi device target 0, lun 0 > > Good point but the problem is CheckDevice()'s own signature, which is > fixed by UEFI PI spec (only passes > VendorId/DeviceId/RevisionId/SubsystemVendorId/SubsystemDeviceId). Even > though the path exists internally, the standard protocol interface > doesn't pass it to the callback.
Hmm, yes. Seems to be designed to apply quirks to device classes, not individual devices. Also note that OVMF already has an incompatible pci device driver and there can be only one instance, so the code must be merged into the existing driver instead of adding a second. > Therefore, we prepare the blob entries > in the same order PciBusDxe discovers devices, so matching by VID:DID > inherently works. Question is whenever we want have that edk2 limitation and the knowledge about edk2 internals (pci scan order) encoded in the qemu <-> firmware protocol. I think it makes sense to (additionally) pass the complete device path even if the current edk2 implementation doesn't use it, so we have the option to improve things later on without having to change the qemu <-> firmware protocolS for that. > > I can see that allowing fixed and non-fixed bars mix is much harder to > > handle. Do we need to ask the user to manually set that though? I'd > > prefer pci devices propagating automatically to the parent bus that they > > have fixed bars and additional constrains apply. > > I looked at this again, and technically nothing actually needs the flag > to exist. The real reason I kept it is closer to a usability one; it's > meant to be a visible signal in the launch script itself, so anyone > reading or writing the qemu command line sees up front that every device > under that root port is expected to have pci-bars= configured, rather > than that requirement only surfacing as a runtime error if something's > missing. I'm not sure how much of a usability win that actually is, if you forget to set the flag you still get a runtime error. In general I like things which can be done automatically actually happen automatically as this simplifies things for the user in most cases. > > Also: if the main use case for this is to map vfio devices with guest > > physical address == host physical address, is there a need to specify > > this manually at all? Shouldn't we have a 'vfio-pci-fixed' device which > > handles this automatically? > > VFIO GPA == HPA is the primary motivation, but I don't think fixed-bar > should be tied to VFIO or automatically derive guest addresses from the > host. Why not? It is a great usability improvement IMHO. > For the VFIO use case, the admin can choose to specify the host > BAR addresses as the fixed-bar configuration to get GPA == HPA, but the > mechanism itself doesn't assume or enforce that -- the desired guest > layout isn't always just a copy of the host's, so having fixed-bar > auto-derive it on its own would be incorrect in some cases, not just > less general. You still can have fixed-bar-<nr>=<addr> properties to override the auto-discovered address for some or all pci bars. > The mechanism remains a generic way to explicitly specify > PCI BAR addresses. Yes, the code which creates the fw_cfg files is generic and it makes sense to have that in the core pci code, so it can be used for every pci device. Nevertheless I'd tend to only expose the properties for devices where an actual use case exists. Which is obviously vfio-pci(-fixed). Also pci-testdev for development / testing / CI. I can't see much beyond that though. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#122146): https://edk2.groups.io/g/devel/message/122146 Mute This Topic: https://groups.io/mt/120952983/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
