On 12/02/16 12:54, Igor Mammedov wrote:
> 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 names were supposed to follow virtio 1.0, which calls these things
"device features" and "driver features".
Not a big deal anyway, I can update the file names as you suggest.
>> 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
Good idea, I will do that.
Thanks!
Laszlo
>
>> + 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[] = {
>
