On 06/09/17 02:19, Michael S. Tsirkin wrote: > On Fri, Jun 09, 2017 at 01:01:54AM +0200, Laszlo Ersek wrote: >> On 06/08/17 21:55, Michael S. Tsirkin wrote: >>> On Thu, Jun 08, 2017 at 09:48:53PM +0200, Gerd Hoffmann wrote: >>>> Hi, >>>> >>>>> I really dislike negotiation being re-invented for each device. Do >>>>> we >>>>> need these tricks? Can we just do fw cfg with standard discovery? >>>>> This ties in with my proposal to generalize smi features to >>>>> generic ones. >>>> >>>> Device properties should be part of the device. >>>> We should have done this with the smi too. >>> >>> What is part of the device and what isn't? It's all part >>> of QEMU in the end. Adding probing for multiple devices >>> will just add to number of exits and slow down guest boot. >>> >>> We do want to stick to emulating real devices if we can, no argument >>> here - but this stuff is PV anyway - what do we gain by spreading it >>> out? >>> >>>> A more standard way to handle this would be to add a vendor-specific >>>> pci capability and place the register there. Not sure we have room for >>>> that in the pci config space though. >>>> >>>> cheers, >>>> Gerd >>> >>> We don't have room anywhere in PCI config space. Laszlo makes argument >>> why it's safe for this device based on spec but it's anyone's guess >>> whether current and future software will follow spec. In short, going >>> anywhere near the emulated device has a potential to break some drivers. >> >> I'm fine using any QEMU facility that lets independent firmware modules >> perform their feature detections / negotiations / lockdowns in arbitrary >> order between each other. (Hopefully without extreme parsing requirements.) > > How about adding etc/mch/features etc copying the smi stuff? Is this > simple enough? We can worry about removing code duplication later.
I'm having a hard time mapping the negotiation used for SMI to this TSEG-size case. (Just to be sure I've now re-read the commit message on 50de920b372b ("hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg", 2017-01-26).) For SMI, there is only one way to trigger it. And the broadcast feature, if negotiated, modifies the behavior of SMI. It does not change how the SMI is triggered by the guest. The TSEG size selection differs. There are already three options for the guest (bit patterns 00 -> 1MB, 01 -> 2MB, 10 -> 8MB) to write to ESMRAMC.TSEG_SZ. Let's say the guest wants more: - First, beyond learning whether "more is possible", it has to know exactly "how much". So a single bit in some "host-features" bitmask is not good enough for that. - Second, assuming an increased TSEG size is available (with a queriable size), the bit patterns 00, 01, 10 should still not change their behavior / meaning. For example, I don't see how ESMRAMC.TSEG_SZ == 00 (implying 1MB TSEG) could coexist with the guest actually negotiating 16MB via another communication channel. Whatever code looked at ESMRAMC.TSEG_SZ would rightfully think there was a 1MB TSEG. So I think such a side-channel negotiation would actually violate specified behavior from the Q35 spec, rather than just fill in reserved/unspecified bits. - The third doubt I have is security / lockdown. In the SMI case, we defined our own lockdown scheme, and it did not overlap or conflict with anything. For SMRAM however, the SMRAM.D_LCK bit already locks down a whole bunch of things, including the ESMRAMC.TSEG_SZ field. Should SMRAM.D_LCK also lock down the "other" negotiation channel? If that's our choice, then we diverge at once from the negotiation pattern seen with SMI. Or else, what if we lock down the "other" channel (selecting 16MB for example), but don't lock down D_LCK? Then ESMRAMC.TSEG_SZ can still be modified, and that should actually affect the TSEG size (because D_LCK is not set). ... I think the negotiation pattern we used for etc/smi/* doesn't apply here; the Q35 spec already defines too much of a selection / lockdown dance for that. We could perhaps figure out the interactions, but (a) it's a security thing so there isn't much room for mistakes, and (b) we wouldn't be reusing the pattern seen with the SMI feature negotiation anyway. ESMRAMC.TSEG_SZ == 11 is the simplest (non-conflicting) extension to the Q35 spec, IMO. That's the core thing. How we pass the information to the guest that (a) ESMRAMC.TSEG_SZ == 11 is available, and (b) what size it actually means is a secondary question. For this, my original suggestion was a new fw_cfg file indeed: https://lists.01.org/pipermail/edk2-devel/2017-May/010404.html but Gerd seemed to prefer PCI config space. I was fine either way. (Please note that a malicious guest OS cannot fake an fw_cfg file on old QEMU, but it can fill in unimplemented registers in PCI config space. This is why using fw_cfg for querying the extended TSEG availability and size would be a "read only" operation in the guest, and why using PCI config space instead requires a *write* operation first. The latter has nothing to do with feature negotiation; the write occurs only to suppress earlier tampering from a malicious guest OS, pre-reboot.) Summary: (1) I don't think the pattern that we used for broadcast SMI applies here; we already have a spec-mandated selection and lockdown protocol involving ESMRAMC.TSEG_SZ and SMRAM.D_LCK. I don't think we can, or should try to, diverge from this protocol. In fact we're lucky to have a reserved bitmask value (11) in ESMRAMC.TSEG_SZ. (2) How exactly we inform the guest about the TSEG size that (ESMRAMC.TSEG_SZ == 11) corresponds to is a secondary question. We can make it a constant (and pray, like Paolo mentions), or we can make it cmdline-configurable and expose it via PCI config space, or we can make it cmdline-configurable and expose it via a new fw_cfg file. In OVMF I can easily accommodate either of these three options. Thanks Laszlo