[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 15/31] target-i386: use memory API to implement
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH 15/31] target-i386: use memory API to implement SMRAM |
|
Date: |
Mon, 1 Jun 2015 10:10:45 +0200 |
On Mon, Jun 01, 2015 at 09:30:38AM +0200, Paolo Bonzini wrote:
>
>
> 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
Generally backwards migration is nice to have to test cross-version
migration properly by doing ping-pong.
Looks like we only need to set smm_enabled correctly before save,
and it'll work cleanly.
No?
> >
> >> @@ -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
> >>
> >
> >