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

[Igor Mammedov]

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 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

> +    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[] = {