Re: [Qemu-devel] [PATCH 15/31] target-i386: use memory API to implement SMRAM

[Paolo Bonzini]

On 31/05/2015 20:09, Michael S. Tsirkin wrote:
> On Mon, May 11, 2015 at 03:49:01PM +0200, Paolo Bonzini wrote:
>> Remove cpu_smm_register and cpu_smm_update.  Instead, each CPU
>> address space gets an extra region which is an alias of
>> /machine/smram.  This extra region is enabled or disabled
>> as the CPU enters/exits SMM.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  bsd-user/main.c           |  4 ----
>>  hw/i386/pc.c              | 21 ---------------------
>>  hw/pci-host/pam.c         | 18 ++----------------
>>  hw/pci-host/piix.c        | 25 +++++--------------------
>>  hw/pci-host/q35.c         | 19 +++----------------
>>  include/hw/i386/pc.h      |  1 -
>>  include/hw/pci-host/pam.h |  5 +----
>>  include/hw/pci-host/q35.h |  1 -
>>  linux-user/main.c         |  4 ----
>>  target-i386/cpu-qom.h     |  4 +++-
>>  target-i386/cpu.c         | 33 +++++++++++++++++++++++++++++++--
>>  target-i386/cpu.h         |  3 ++-
>>  target-i386/machine.c     |  3 +++
>>  target-i386/smm_helper.c  | 12 ++++++++++--
>>  14 files changed, 60 insertions(+), 93 deletions(-)
>>
>> diff --git a/bsd-user/main.c b/bsd-user/main.c
>> index 5bfaf5c..ba0b998 100644
>> --- a/bsd-user/main.c
>> +++ b/bsd-user/main.c
>> @@ -108,10 +108,6 @@ void cpu_list_unlock(void)
>>  /***********************************************************/
>>  /* CPUX86 core interface */
>>  
>> -void cpu_smm_update(CPUX86State *env)
>> -{
>> -}
>> -
>>  uint64_t cpu_get_tsc(CPUX86State *env)
>>  {
>>      return cpu_get_real_ticks();
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index a8e6be1..a7ee9ef 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -163,27 +163,6 @@ uint64_t cpu_get_tsc(CPUX86State *env)
>>      return cpu_get_ticks();
>>  }
>>  
>> -/* SMM support */
>> -
>> -static cpu_set_smm_t smm_set;
>> -static void *smm_arg;
>> -
>> -void cpu_smm_register(cpu_set_smm_t callback, void *arg)
>> -{
>> -    assert(smm_set == NULL);
>> -    assert(smm_arg == NULL);
>> -    smm_set = callback;
>> -    smm_arg = arg;
>> -}
>> -
>> -void cpu_smm_update(CPUX86State *env)
>> -{
>> -    if (smm_set && smm_arg && CPU(x86_env_get_cpu(env)) == first_cpu) {
>> -        smm_set(!!(env->hflags & HF_SMM_MASK), smm_arg);
>> -    }
>> -}
>> -
>> -
>>  /* IRQ handling */
>>  int cpu_get_pic_interrupt(CPUX86State *env)
>>  {
>> diff --git a/hw/pci-host/pam.c b/hw/pci-host/pam.c
>> index 8272de3..99d7af9 100644
>> --- a/hw/pci-host/pam.c
>> +++ b/hw/pci-host/pam.c
>> @@ -31,26 +31,12 @@
>>  #include "sysemu/sysemu.h"
>>  #include "hw/pci-host/pam.h"
>>  
>> -void smram_update(MemoryRegion *smram_region, uint8_t smram,
>> -                  uint8_t smm_enabled)
>> +void smram_update(MemoryRegion *smram_region, uint8_t smram)
>>  {
>> -    bool smram_enabled;
>> -
>> -    smram_enabled = ((smm_enabled && (smram & SMRAM_G_SMRAME)) ||
>> -                        (smram & SMRAM_D_OPEN));
>> +    bool smram_enabled = (smram & SMRAM_D_OPEN);
>>      memory_region_set_enabled(smram_region, !smram_enabled);
>>  }
>>  
>> -void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
>> -                   MemoryRegion *smram_region)
>> -{
>> -    uint8_t smm_enabled = (smm != 0);
>> -    if (*host_smm_enabled != smm_enabled) {
>> -        *host_smm_enabled = smm_enabled;
>> -        smram_update(smram_region, smram, *host_smm_enabled);
>> -    }
>> -}
>> -
>>  void init_pam(DeviceState *dev, MemoryRegion *ram_memory,
>>                MemoryRegion *system_memory, MemoryRegion *pci_address_space,
>>                PAMMemoryRegion *mem, uint32_t start, uint32_t size)
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index a280d57..46c0278 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -106,7 +106,6 @@ struct PCII440FXState {
>>      PAMMemoryRegion pam_regions[13];
>>      MemoryRegion smram_region;
>>      MemoryRegion smram, low_smram;
>> -    uint8_t smm_enabled;
>>  };
>>  
>>  
>> @@ -139,23 +138,12 @@ static void 
>> i440fx_update_memory_mappings(PCII440FXState *d)
>>          pam_update(&d->pam_regions[i], i,
>>                     pd->config[I440FX_PAM + ((i + 1) / 2)]);
>>      }
>> -    smram_update(&d->smram_region, pd->config[I440FX_SMRAM], 
>> d->smm_enabled);
>> +    smram_update(&d->smram_region, pd->config[I440FX_SMRAM]);
>>      memory_region_set_enabled(&d->smram,
>>                                pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
>>      memory_region_transaction_commit();
>>  }
>>  
>> -static void i440fx_set_smm(int val, void *arg)
>> -{
>> -    PCII440FXState *d = arg;
>> -    PCIDevice *pd = PCI_DEVICE(d);
>> -
>> -    memory_region_transaction_begin();
>> -    smram_set_smm(&d->smm_enabled, val, pd->config[I440FX_SMRAM],
>> -                  &d->smram_region);
>> -    memory_region_transaction_commit();
>> -}
>> -
>>  
>>  static void i440fx_write_config(PCIDevice *dev,
>>                                  uint32_t address, uint32_t val, int len)
>> @@ -175,12 +163,13 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, 
>> int version_id)
>>      PCII440FXState *d = opaque;
>>      PCIDevice *pd = PCI_DEVICE(d);
>>      int ret, i;
>> +    uint8_t smm_enabled;
>>  
>>      ret = pci_device_load(pd, f);
>>      if (ret < 0)
>>          return ret;
>>      i440fx_update_memory_mappings(d);
>> -    qemu_get_8s(f, &d->smm_enabled);
>> +    qemu_get_8s(f, &smm_enabled);
>>  
>>      if (version_id == 2) {
>>          for (i = 0; i < PIIX_NUM_PIRQS; i++) {
>> @@ -208,7 +197,7 @@ static const VMStateDescription vmstate_i440fx = {
>>      .post_load = i440fx_post_load,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_PCI_DEVICE(parent_obj, PCII440FXState),
>> -        VMSTATE_UINT8(smm_enabled, PCII440FXState),
>> +        VMSTATE_UNUSED(1),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
> 
> Won't this cause problems for cross-version migration?
> Old qemu receiving this will think smm is enabled,
> unconditionally.

UNUSED is written as zeroes, so it will think SMM is _disabled_,
unconditionally.  Note that d->smram_region is backwards: it aliases to
VRAM, so it is enabled when SMRAM is closed and disabled when SMRAM is open.

This is correct for KVM, though not for TCG.  Backwards migration is not
supported officially upstream, and I think we can agree it is even less
supported for TCG.

Paolo

> 
>> @@ -300,11 +289,7 @@ static void i440fx_pcihost_realize(DeviceState *dev, 
>> Error **errp)
>>  
>>  static void i440fx_realize(PCIDevice *dev, Error **errp)
>>  {
>> -    PCII440FXState *d = I440FX_PCI_DEVICE(dev);
>> -
>>      dev->config[I440FX_SMRAM] = 0x02;
>> -
>> -    cpu_smm_register(&i440fx_set_smm, d);
>>  }
>>  
>>  PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>> @@ -360,7 +345,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
>>      memory_region_init(&f->smram, OBJECT(d), "smram", 1ull << 32);
>>      memory_region_set_enabled(&f->smram, true);
>>      memory_region_init_alias(&f->low_smram, OBJECT(d), "smram-low",
>> -                             f->system_memory, 0xa0000, 0x20000);
>> +                             f->ram_memory, 0xa0000, 0x20000);
>>      memory_region_set_enabled(&f->low_smram, true);
>>      memory_region_add_subregion(&f->smram, 0xa0000, &f->low_smram);
>>      object_property_add_alias(qdev_get_machine(), "smram",
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index de8326a..bd17a05 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -268,24 +268,12 @@ static void mch_update_smram(MCHPCIState *mch)
>>      PCIDevice *pd = PCI_DEVICE(mch);
>>  
>>      memory_region_transaction_begin();
>> -    smram_update(&mch->smram_region, pd->config[MCH_HOST_BRIDGE_SMRAM],
>> -                    mch->smm_enabled);
>> +    smram_update(&mch->smram_region, pd->config[MCH_HOST_BRIDGE_SMRAM]);
>>      memory_region_set_enabled(&mch->smram,
>>                                pd->config[MCH_HOST_BRIDGE_SMRAM] & 
>> SMRAM_G_SMRAME);
>>      memory_region_transaction_commit();
>>  }
>>  
>> -static void mch_set_smm(int smm, void *arg)
>> -{
>> -    MCHPCIState *mch = arg;
>> -    PCIDevice *pd = PCI_DEVICE(mch);
>> -
>> -    memory_region_transaction_begin();
>> -    smram_set_smm(&mch->smm_enabled, smm, pd->config[MCH_HOST_BRIDGE_SMRAM],
>> -                    &mch->smram_region);
>> -    memory_region_transaction_commit();
>> -}
>> -
>>  static void mch_write_config(PCIDevice *d,
>>                                uint32_t address, uint32_t val, int len)
>>  {
>> @@ -331,7 +319,7 @@ static const VMStateDescription vmstate_mch = {
>>      .post_load = mch_post_load,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_PCI_DEVICE(parent_obj, MCHPCIState),
>> -        VMSTATE_UINT8(smm_enabled, MCHPCIState),
>> +        VMSTATE_UNUSED(1),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>> @@ -402,7 +390,6 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>                             mch->pci_address_space);
>>  
>>      /* if *disabled* show SMRAM to all CPUs */
>> -    cpu_smm_register(&mch_set_smm, mch);
>>      memory_region_init_alias(&mch->smram_region, OBJECT(mch), 
>> "smram-region",
>>                               mch->pci_address_space, 0xa0000, 0x20000);
>>      memory_region_add_subregion_overlap(mch->system_memory, 0xa0000,
>> @@ -413,7 +400,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>>      memory_region_init(&mch->smram, OBJECT(mch), "smram", 1ull << 32);
>>      memory_region_set_enabled(&mch->smram, true);
>>      memory_region_init_alias(&mch->low_smram, OBJECT(mch), "smram-low",
>> -                             mch->system_memory, 0xa0000, 0x20000);
>> +                             mch->ram_memory, 0xa0000, 0x20000);
>>      memory_region_set_enabled(&mch->low_smram, true);
>>      memory_region_add_subregion(&mch->smram, 0xa0000, &mch->low_smram);
>>      object_property_add_const_link(qdev_get_machine(), "smram",
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 1b35168..c9a9f6e 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -211,7 +211,6 @@ void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus);
>>  void pc_pci_device_init(PCIBus *pci_bus);
>>  
>>  typedef void (*cpu_set_smm_t)(int smm, void *arg);
>> -void cpu_smm_register(cpu_set_smm_t callback, void *arg);
>>  
>>  void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
>>  
>> diff --git a/include/hw/pci-host/pam.h b/include/hw/pci-host/pam.h
>> index 4d03e4b..80dd605 100644
>> --- a/include/hw/pci-host/pam.h
>> +++ b/include/hw/pci-host/pam.h
>> @@ -86,10 +86,7 @@ typedef struct PAMMemoryRegion {
>>      unsigned current;
>>  } PAMMemoryRegion;
>>  
>> -void smram_update(MemoryRegion *smram_region, uint8_t smram,
>> -                  uint8_t smm_enabled);
>> -void smram_set_smm(uint8_t *host_smm_enabled, int smm, uint8_t smram,
>> -                   MemoryRegion *smram_region);
>> +void smram_update(MemoryRegion *smram_region, uint8_t smram);
>>  void init_pam(DeviceState *dev, MemoryRegion *ram, MemoryRegion *system,
>>                MemoryRegion *pci, PAMMemoryRegion *mem, uint32_t start, 
>> uint32_t size);
>>  void pam_update(PAMMemoryRegion *mem, int idx, uint8_t val);
>> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
>> index 4c9eacc..17adeaa 100644
>> --- a/include/hw/pci-host/q35.h
>> +++ b/include/hw/pci-host/q35.h
>> @@ -55,7 +55,6 @@ typedef struct MCHPCIState {
>>      MemoryRegion smram_region;
>>      MemoryRegion smram, low_smram;
>>      PcPciInfo pci_info;
>> -    uint8_t smm_enabled;
>>      ram_addr_t below_4g_mem_size;
>>      ram_addr_t above_4g_mem_size;
>>      uint64_t pci_hole64_size;
>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index 3f32db0..6989b82 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -215,10 +215,6 @@ void cpu_list_unlock(void)
>>  /***********************************************************/
>>  /* CPUX86 core interface */
>>  
>> -void cpu_smm_update(CPUX86State *env)
>> -{
>> -}
>> -
>>  uint64_t cpu_get_tsc(CPUX86State *env)
>>  {
>>      return cpu_get_real_ticks();
>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>> index 39cd878..7a4fddd 100644
>> --- a/target-i386/cpu-qom.h
>> +++ b/target-i386/cpu-qom.h
>> @@ -23,6 +23,7 @@
>>  #include "qom/cpu.h"
>>  #include "cpu.h"
>>  #include "qapi/error.h"
>> +#include "qemu/notify.h"
>>  
>>  #ifdef TARGET_X86_64
>>  #define TYPE_X86_CPU "x86_64-cpu"
>> @@ -111,7 +112,8 @@ typedef struct X86CPU {
>>      /* in order to simplify APIC support, we leave this pointer to the
>>         user */
>>      struct DeviceState *apic_state;
>> -    struct MemoryRegion *cpu_as_root;
>> +    struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
>> +    Notifier machine_done;
>>  } X86CPU;
>>  
>>  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 97c4804..7b6f9e4 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2751,6 +2751,21 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error 
>> **errp)
>>      object_property_set_bool(OBJECT(cpu->apic_state), true, "realized",
>>                               errp);
>>  }
>> +
>> +static void x86_cpu_machine_done(Notifier *n, void *unused)
>> +{
>> +    X86CPU *cpu = container_of(n, X86CPU, machine_done);
>> +    MemoryRegion *smram =
>> +        (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
>> +
>> +    if (smram) {
>> +        cpu->smram = g_new(MemoryRegion, 1);
>> +        memory_region_init_alias(cpu->smram, OBJECT(cpu), "smram",
>> +                                 smram, 0, 1ull << 32);
>> +        memory_region_set_enabled(cpu->smram, false);
>> +        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, 
>> cpu->smram, 1);
>> +    }
>> +}
>>  #else
>>  static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>>  {
>> @@ -2815,12 +2830,26 @@ static void x86_cpu_realizefn(DeviceState *dev, 
>> Error **errp)
>>  
>>  #ifndef CONFIG_USER_ONLY
>>      if (tcg_enabled()) {
>> +        cpu->cpu_as_mem = g_new(MemoryRegion, 1);
>>          cpu->cpu_as_root = g_new(MemoryRegion, 1);
>>          cs->as = g_new(AddressSpace, 1);
>> -        memory_region_init_alias(cpu->cpu_as_root, OBJECT(cpu), "memory",
>> -                                 get_system_memory(), 0, ~0ull);
>> +
>> +        /* Outer container... */
>> +        memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
>>          memory_region_set_enabled(cpu->cpu_as_root, true);
>> +
>> +        /* ... with two regions inside: normal system memory with low
>> +         * priority, and...
>> +         */
>> +        memory_region_init_alias(cpu->cpu_as_mem, OBJECT(cpu), "memory",
>> +                                 get_system_memory(), 0, ~0ull);
>> +        memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, 
>> cpu->cpu_as_mem, 0);
>> +        memory_region_set_enabled(cpu->cpu_as_mem, true);
>>          address_space_init(cs->as, cpu->cpu_as_root, "CPU");
>> +
>> +        /* ... SMRAM with higher priority, linked from /machine/smram.  */
>> +        cpu->machine_done.notify = x86_cpu_machine_done;
>> +        qemu_add_machine_init_done_notifier(&cpu->machine_done);
>>      }
>>  #endif
>>  
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 4510ae7..df6e885 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -1157,7 +1157,6 @@ void cpu_x86_update_cr3(CPUX86State *env, target_ulong 
>> new_cr3);
>>  void cpu_x86_update_cr4(CPUX86State *env, uint32_t new_cr4);
>>  
>>  /* hw/pc.c */
>> -void cpu_smm_update(CPUX86State *env);
>>  uint64_t cpu_get_tsc(CPUX86State *env);
>>  
>>  #define TARGET_PAGE_BITS 12
>> @@ -1323,7 +1322,9 @@ void cpu_vmexit(CPUX86State *nenv, uint32_t exit_code, 
>> uint64_t exit_info_1);
>>  /* seg_helper.c */
>>  void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw);
>>  
>> +/* smm_helper.c */
>>  void do_smm_enter(X86CPU *cpu);
>> +void cpu_smm_update(X86CPU *cpu);
>>  
>>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>>  
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index cd1ddd2..69d86cb 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -372,6 +372,9 @@ static int cpu_post_load(void *opaque, int version_id)
>>      }
>>      tlb_flush(cs, 1);
>>  
>> +    if (tcg_enabled()) {
>> +        cpu_smm_update(cpu);
>> +    }
>>      return 0;
>>  }
>>  
>> diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c
>> index 5617a14..762f4e5 100644
>> --- a/target-i386/smm_helper.c
>> +++ b/target-i386/smm_helper.c
>> @@ -40,6 +40,14 @@ void helper_rsm(CPUX86State *env)
>>  #define SMM_REVISION_ID 0x00020000
>>  #endif
>>  
>> +void cpu_smm_update(X86CPU *cpu)
>> +{
>> +    CPUX86State *env = &cpu->env;
>> +    bool smm_enabled = (env->hflags & HF_SMM_MASK);
>> +
>> +    memory_region_set_enabled(cpu->smram, smm_enabled);
>> +}
>> +
>>  void do_smm_enter(X86CPU *cpu)
>>  {
>>      CPUX86State *env = &cpu->env;
>> @@ -57,7 +65,7 @@ void do_smm_enter(X86CPU *cpu)
>>      } else {
>>          env->hflags2 |= HF2_NMI_MASK;
>>      }
>> -    cpu_smm_update(env);
>> +    cpu_smm_update(cpu);
>>  
>>      sm_state = env->smbase + 0x8000;
>>  
>> @@ -317,7 +325,7 @@ void helper_rsm(CPUX86State *env)
>>      }
>>      env->hflags2 &= ~HF2_SMM_INSIDE_NMI_MASK;
>>      env->hflags &= ~HF_SMM_MASK;
>> -    cpu_smm_update(env);
>> +    cpu_smm_update(cpu);
>>  
>>      qemu_log_mask(CPU_LOG_INT, "SMM: after RSM\n");
>>      log_cpu_state_mask(CPU_LOG_INT, CPU(cpu), CPU_DUMP_CCOP);
>> -- 
>> 1.8.3.1
>>
> 
>