Re: [Qemu-devel] [PATCH v4 5/7] hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg

[Laszlo Ersek]

On 12/02/16 12:54, Igor Mammedov wrote:
> On Thu,  1 Dec 2016 18:06:22 +0100
> Laszlo Ersek <address@hidden> 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" <address@hidden>
>> Cc: Gerd Hoffmann <address@hidden>
>> Cc: Igor Mammedov <address@hidden>
>> Cc: Paolo Bonzini <address@hidden>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  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[] = {
>