On Thu, 1 Dec 2016 18:06:22 +0100 Laszlo Ersek <ler...@redhat.com> wrote:
> Introduce the following fw_cfg files: > > - "etc/smi/host-features": a little endian uint64_t feature bitmap, > presenting the features known by the host to the guest. Read-only for > the guest. 'host' here is a little bit confusing, I'd suggest 'supported-features' instead. > The content of this file is calculated by QEMU at startup (the > calculation will be added later). The same machine type is supposed to > expose the same SMI features regardless of QEMU version, hence this file > is not migrated. > > - "etc/smi/guest-features": a little endian uint64_t feature bitmap, > representing the features the guest would like to request. Read-write > for the guest. and to match above 'requested-features' > The guest can freely (re)write this file, it has no direct consequence. > Initial value is zero. A nonzero value causes the SMI-related fw_cfg > files and fields that are under guest influence to be migrated. > > - "etc/smi/features-ok": contains a uint8_t value, and it is read-only for > the guest. When the guest selects the associated fw_cfg key, the guest > features are validated against the host features. In case of error, the > negotiation doesn't proceed, and the features-ok file remains zero. In > case of success, the features-ok file becomes (uint8_t)1, and the > negotiated features are locked down internally (to which no further > changes are possible until reset). > > The initial value is zero. A nonzero value causes the SMI-related > fw_cfg files and fields that are under guest influence to be migrated. > > The C-language fields backing the host-features and guest-features files > are uint8_t arrays. This is because they carry guest-side representation > (our choice is little endian), while VMSTATE_UINT64() assumes / implies > host-side endianness for any uint64_t fields. If we migrate a guest > between hosts with different endiannesses (which is possible with TCG), > then the host-side value is preserved, and the host-side representation is > translated. This would be visible to the guest through fw_cfg, unless we > used plain byte arrays. So we do. > > Cc: "Michael S. Tsirkin" <m...@redhat.com> > Cc: Gerd Hoffmann <kra...@redhat.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > include/hw/i386/ich9.h | 12 +++++++- > hw/i386/pc_q35.c | 2 +- > hw/isa/lpc_ich9.c | 83 > +++++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 94 insertions(+), 3 deletions(-) > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h > index 5fd7e97d2347..33142eb37252 100644 > --- a/include/hw/i386/ich9.h > +++ b/include/hw/i386/ich9.h > @@ -15,11 +15,12 @@ > #include "hw/pci/pci_bus.h" > > void ich9_lpc_set_irq(void *opaque, int irq_num, int level); > int ich9_lpc_map_irq(PCIDevice *pci_dev, int intx); > PCIINTxRoute ich9_route_intx_pin_to_irq(void *opaque, int pirq_pin); > -void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled); > +void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool smm_enabled, > + uint64_t smi_host_features); > I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base); > > void ich9_generate_smi(void); > void ich9_generate_nmi(void); > > @@ -62,10 +63,19 @@ typedef struct ICH9LPCState { > * register contents and IO memory region > */ > uint8_t rst_cnt; > MemoryRegion rst_cnt_mem; > > + /* SMI feature negotiation via fw_cfg */ > + uint8_t smi_host_features[8]; /* guest-visible, read-only, little > + * endian uint64_t */ > + uint8_t smi_guest_features[8]; /* guest-visible, read-write, little > + * endian uint64_t */ how about adding _le suffix to 2 above fields? that way it would be easier to read usage places without chance to misinterpret content > + uint8_t smi_features_ok; /* guest-visible, read-only; selecting > it > + * triggers feature lockdown */ > + uint64_t smi_negotiated_features; /* guest-invisible, host endian */ > + > /* isa bus */ > ISABus *isa_bus; > MemoryRegion rcrb_mem; /* root complex register block */ > Notifier machine_ready; > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 7fa40e7cbe0e..eb0953bb6b16 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -228,11 +228,11 @@ static void pc_q35_init(MachineState *machine) > /* init basic PC hardware */ > pc_basic_device_init(isa_bus, pcms->gsi, &rtc_state, !mc->no_floppy, > (pcms->vmport != ON_OFF_AUTO_ON), 0xff0104); > > /* connect pm stuff to lpc */ > - ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms)); > + ich9_lpc_pm_init(lpc, pc_machine_is_smm_enabled(pcms), 0); > > /* ahci and SATA device, for q35 1 ahci controller is built-in */ > ahci = pci_create_simple_multifunction(host_bus, > PCI_DEVFN(ICH9_SATA1_DEV, > ICH9_SATA1_FUNC), > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > index 10d1ee8b9310..ee1fa553bfee 100644 > --- a/hw/isa/lpc_ich9.c > +++ b/hw/isa/lpc_ich9.c > @@ -46,10 +46,12 @@ > #include "hw/acpi/ich9.h" > #include "hw/pci/pci_bus.h" > #include "exec/address-spaces.h" > #include "sysemu/sysemu.h" > #include "qom/cpu.h" > +#include "hw/nvram/fw_cfg.h" > +#include "qemu/cutils.h" > > > /*****************************************************************************/ > /* ICH9 LPC PCI to ISA bridge */ > > static void ich9_lpc_reset(DeviceState *qdev); > @@ -358,17 +360,68 @@ static void ich9_set_sci(void *opaque, int irq_num, int > level) > } else { > ich9_lpc_update_pic(lpc, irq); > } > } > > -void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled) > +static void smi_features_ok_callback(void *opaque) > +{ > + ICH9LPCState *lpc = opaque; > + uint64_t host_features, guest_features; > + > + if (lpc->smi_features_ok) { > + /* negotiation already complete, features locked */ > + return; > + } > + > + memcpy(&host_features, lpc->smi_host_features, sizeof host_features); > + memcpy(&guest_features, lpc->smi_guest_features, sizeof guest_features); > + le64_to_cpus(&host_features); > + le64_to_cpus(&guest_features); > + > + if (guest_features & ~host_features) { > + /* guest requests invalid features, leave @features_ok at zero */ > + return; > + } > + > + /* valid feature subset requested, lock it down, report success */ > + lpc->smi_negotiated_features = guest_features; > + lpc->smi_features_ok = 1; > +} > + > +void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled, > + uint64_t smi_host_features) > { > ICH9LPCState *lpc = ICH9_LPC_DEVICE(lpc_pci); > qemu_irq sci_irq; > + FWCfgState *fw_cfg = fw_cfg_find(); > > sci_irq = qemu_allocate_irq(ich9_set_sci, lpc, 0); > ich9_pm_init(lpc_pci, &lpc->pm, smm_enabled, sci_irq); > + > + if (smi_host_features && fw_cfg) { > + cpu_to_le64s(&smi_host_features); > + memcpy(lpc->smi_host_features, &smi_host_features, > + sizeof smi_host_features); > + fw_cfg_add_file(fw_cfg, "etc/smi/host-features", > + lpc->smi_host_features, > + sizeof lpc->smi_host_features); > + > + /* The other two guest-visible fields are cleared on device reset, we > + * just link them into fw_cfg here. > + */ > + fw_cfg_add_file_callback(fw_cfg, "etc/smi/guest-features", > + NULL, NULL, > + lpc->smi_guest_features, > + sizeof lpc->smi_guest_features, > + false); > + fw_cfg_add_file_callback(fw_cfg, "etc/smi/features-ok", > + smi_features_ok_callback, lpc, > + &lpc->smi_features_ok, > + sizeof lpc->smi_features_ok, > + true); > + } > + > ich9_lpc_reset(&lpc->d.qdev); > } > > /* APM */ > > @@ -505,10 +558,14 @@ static void ich9_lpc_reset(DeviceState *qdev) > ich9_lpc_pmbase_sci_update(lpc); > ich9_lpc_rcba_update(lpc, rcba_old); > > lpc->sci_level = 0; > lpc->rst_cnt = 0; > + > + memset(lpc->smi_guest_features, 0, sizeof lpc->smi_guest_features); > + lpc->smi_features_ok = 0; > + lpc->smi_negotiated_features = 0; > } > > /* root complex register block is mapped into memory space */ > static const MemoryRegionOps rcrb_mmio_ops = { > .read = ich9_cc_read, > @@ -666,10 +723,33 @@ static const VMStateDescription vmstate_ich9_rst_cnt = { > VMSTATE_UINT8(rst_cnt, ICH9LPCState), > VMSTATE_END_OF_LIST() > } > }; > > +static bool ich9_smi_feat_needed(void *opaque) > +{ > + ICH9LPCState *lpc = opaque; > + > + return !buffer_is_zero(lpc->smi_guest_features, > + sizeof lpc->smi_guest_features) || > + lpc->smi_features_ok; > +} > + > +static const VMStateDescription vmstate_ich9_smi_feat = { > + .name = "ICH9LPC/smi_feat", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = ich9_smi_feat_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT8_ARRAY(smi_guest_features, ICH9LPCState, > + sizeof(uint64_t)), > + VMSTATE_UINT8(smi_features_ok, ICH9LPCState), > + VMSTATE_UINT64(smi_negotiated_features, ICH9LPCState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_ich9_lpc = { > .name = "ICH9LPC", > .version_id = 1, > .minimum_version_id = 1, > .post_load = ich9_lpc_post_load, > @@ -681,10 +761,11 @@ static const VMStateDescription vmstate_ich9_lpc = { > VMSTATE_UINT32(sci_level, ICH9LPCState), > VMSTATE_END_OF_LIST() > }, > .subsections = (const VMStateDescription*[]) { > &vmstate_ich9_rst_cnt, > + &vmstate_ich9_smi_feat, > NULL > } > }; > > static Property ich9_lpc_properties[] = {