[Qemu-devel] [RFC PATCH 0/7] x86: SMRAM implementation for KVM

[Qemu-devel] [RFC PATCH 0/7] x86: SMRAM implementation for KVM

[Paolo Bonzini]

This is the final piece of x86 SMM implementation, tested with PIIX
(low SMRAM) and Q35 (high SMRAM).

There is a problem---it has an awful layering violation in patch 5,
and an only slightly better one in patch 6.

If anyone has ideas, please speak up.  Note that it is not possible
to call KVM_SET_USER_MEMORY_REGION every time you enter or leave SMM,
because that would only work for single-processor virtual machines.

Paolo

Andrew Jones (1):
  kvm-all: put kvm_mem_flags to more work

Paolo Bonzini (6):
  kvm-all: remove useless typedef
  kvm-all: move KVMState definitions to kvm_int.h
  kvm-all: add KVM address space
  memory: add kvm_mem_flags to MemoryRegion
  i386: disable the region in /machine/smram when SMRAM is open
  kvm-i386: register SMRAM regions with KVM_MEM_X86_SMRAM

 hw/pci-host/piix.c       |   6 +++
 hw/pci-host/q35.c        |  20 ++++++---
 include/exec/memory.h    |   4 ++
 include/sysemu/kvm_int.h |  73 +++++++++++++++++++++++++++++++
 kvm-all.c                | 111 ++++++++++++++++-------------------------------
 memory.c                 |  17 ++++++--
 target-i386/kvm.c        |  27 ++++++++++++
 7 files changed, 173 insertions(+), 85 deletions(-)
 create mode 100644 include/sysemu/kvm_int.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] kvm-all: put kvm_mem_flags to more work

[Paolo Bonzini]

From: Andrew Jones <drjones@redhat.com>

Currently kvm_mem_flags just translates bools to bits, let's
make it also determine the bools first. This avoids its parameter
list growing each time we add a flag.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 5ad4877..6e1a3f8 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -295,10 +295,14 @@ err:
  * dirty pages logging control
  */
 
-static int kvm_mem_flags(KVMState *s, bool log_dirty, bool readonly)
+static int kvm_mem_flags(MemoryRegion *mr)
 {
+    bool readonly = mr->readonly || memory_region_is_romd(mr);
     int flags = 0;
-    flags = log_dirty ? KVM_MEM_LOG_DIRTY_PAGES : 0;
+
+    if (memory_region_is_logging(mr)) {
+        flags |= KVM_MEM_LOG_DIRTY_PAGES;
+    }
     if (readonly && kvm_readonly_mem_allowed) {
         flags |= KVM_MEM_READONLY;
     }
@@ -313,7 +317,10 @@ static int kvm_slot_dirty_pages_log_change(KVMSlot *mem, bool log_dirty)
 
     old_flags = mem->flags;
 
-    flags = (mem->flags & ~mask) | kvm_mem_flags(s, log_dirty, false);
+    flags = mem->flags & ~mask;
+    if (log_dirty) {
+        flags |= KVM_MEM_LOG_DIRTY_PAGES;
+    }
     mem->flags = flags;
 
     /* If nothing changed effectively, no need to issue ioctl */
@@ -663,9 +670,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     KVMSlot *mem, old;
     int err;
     MemoryRegion *mr = section->mr;
-    bool log_dirty = memory_region_is_logging(mr);
     bool writeable = !mr->readonly && !mr->rom_device;
-    bool readonly_flag = mr->readonly || memory_region_is_romd(mr);
     hwaddr start_addr = section->offset_within_address_space;
     ram_addr_t size = int128_get64(section->size);
     void *ram = NULL;
@@ -709,7 +714,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             (ram - start_addr == mem->ram - mem->start_addr)) {
             /* The new slot fits into the existing one and comes with
              * identical parameters - update flags and done. */
-            kvm_slot_dirty_pages_log_change(mem, log_dirty);
+            kvm_slot_dirty_pages_log_change(mem, memory_region_is_logging(mr));
             return;
         }
 
@@ -742,7 +747,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = old.memory_size;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags = kvm_mem_flags(s, log_dirty, readonly_flag);
+            mem->flags = kvm_mem_flags(mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -763,7 +768,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = start_addr - old.start_addr;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags =  kvm_mem_flags(s, log_dirty, readonly_flag);
+            mem->flags =  kvm_mem_flags(mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -787,7 +792,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             size_delta = mem->start_addr - old.start_addr;
             mem->memory_size = old.memory_size - size_delta;
             mem->ram = old.ram + size_delta;
-            mem->flags = kvm_mem_flags(s, log_dirty, readonly_flag);
+            mem->flags = kvm_mem_flags(mr);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -809,7 +814,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     mem->memory_size = size;
     mem->start_addr = start_addr;
     mem->ram = ram;
-    mem->flags = kvm_mem_flags(s, log_dirty, readonly_flag);
+    mem->flags = kvm_mem_flags(mr);
 
     err = kvm_set_user_memory_region(s, mem);
     if (err) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/7] kvm-all: remove useless typedef

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 kvm-all.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 6e1a3f8..8205ea1 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -69,8 +69,6 @@ typedef struct KVMSlot
     int flags;
 } KVMSlot;
 
-typedef struct kvm_dirty_log KVMDirtyLog;
-
 struct KVMState
 {
     AccelState parent_obj;
@@ -425,7 +423,7 @@ static int kvm_physical_sync_dirty_bitmap(MemoryRegionSection *section)
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
-    KVMDirtyLog d = {};
+    struct kvm_dirty_log d = {};
     KVMSlot *mem;
     int ret = 0;
     hwaddr start_addr = section->offset_within_address_space;
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/7] kvm-all: move KVMState definitions to kvm_int.h

[Paolo Bonzini]

i386 code will have to look inside KVMState in order to modify the
address space used for KVM_SET_USER_MEM_REGION.  Create an internal
header so that KVMState is not exposed outside.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/sysemu/kvm_int.h | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
 kvm-all.c                | 58 +--------------------------------------
 2 files changed, 71 insertions(+), 57 deletions(-)
 create mode 100644 include/sysemu/kvm_int.h

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
new file mode 100644
index 0000000..d5f746f
--- /dev/null
+++ b/include/sysemu/kvm_int.h
@@ -0,0 +1,70 @@
+/*
+ * Internal definitions for a target's KVM support
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_KVM_INT_H
+#define QEMU_KVM_INT_H
+
+#include "sysemu/sysemu.h"
+#include "sysemu/accel.h"
+#include <sysemu/kvm.h>
+
+#define KVM_MSI_HASHTAB_SIZE    256
+
+typedef struct KVMSlot
+{
+    hwaddr start_addr;
+    ram_addr_t memory_size;
+    void *ram;
+    int slot;
+    int flags;
+} KVMSlot;
+
+struct KVMState
+{
+    AccelState parent_obj;
+
+    KVMSlot *slots;
+    int nr_slots;
+    int fd;
+    int vmfd;
+    int coalesced_mmio;
+    struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
+    bool coalesced_flush_in_progress;
+    int broken_set_mem_region;
+    int migration_log;
+    int vcpu_events;
+    int robust_singlestep;
+    int debugregs;
+#ifdef KVM_CAP_SET_GUEST_DEBUG
+    struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
+#endif
+    int pit_state2;
+    int xsave, xcrs;
+    int many_ioeventfds;
+    int intx_set_mask;
+    /* The man page (and posix) say ioctl numbers are signed int, but
+     * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
+     * unsigned, and treating them as signed here can break things */
+    unsigned irq_set_ioctl;
+    unsigned int sigmask_len;
+#ifdef KVM_CAP_IRQ_ROUTING
+    struct kvm_irq_routing *irq_routes;
+    int nr_allocated_irq_routes;
+    uint32_t *used_gsi_bitmap;
+    unsigned int gsi_count;
+    QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
+    bool direct_msi;
+#endif
+};
+
+#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
+
+#define KVM_STATE(obj) \
+    OBJECT_CHECK(KVMState, (obj), TYPE_KVM_ACCEL)
+
+#endif
diff --git a/kvm-all.c b/kvm-all.c
index 8205ea1..215ed33 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -24,13 +24,11 @@
 #include "qemu/atomic.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
-#include "sysemu/sysemu.h"
-#include "sysemu/accel.h"
 #include "hw/hw.h"
 #include "hw/pci/msi.h"
 #include "hw/s390x/adapter.h"
 #include "exec/gdbstub.h"
-#include "sysemu/kvm.h"
+#include "sysemu/kvm_int.h"
 #include "qemu/bswap.h"
 #include "exec/memory.h"
 #include "exec/ram_addr.h"
@@ -58,60 +56,6 @@
     do { } while (0)
 #endif
 
-#define KVM_MSI_HASHTAB_SIZE    256
-
-typedef struct KVMSlot
-{
-    hwaddr start_addr;
-    ram_addr_t memory_size;
-    void *ram;
-    int slot;
-    int flags;
-} KVMSlot;
-
-struct KVMState
-{
-    AccelState parent_obj;
-
-    KVMSlot *slots;
-    int nr_slots;
-    int fd;
-    int vmfd;
-    int coalesced_mmio;
-    struct kvm_coalesced_mmio_ring *coalesced_mmio_ring;
-    bool coalesced_flush_in_progress;
-    int broken_set_mem_region;
-    int migration_log;
-    int vcpu_events;
-    int robust_singlestep;
-    int debugregs;
-#ifdef KVM_CAP_SET_GUEST_DEBUG
-    struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
-#endif
-    int pit_state2;
-    int xsave, xcrs;
-    int many_ioeventfds;
-    int intx_set_mask;
-    /* The man page (and posix) say ioctl numbers are signed int, but
-     * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
-     * unsigned, and treating them as signed here can break things */
-    unsigned irq_set_ioctl;
-    unsigned int sigmask_len;
-#ifdef KVM_CAP_IRQ_ROUTING
-    struct kvm_irq_routing *irq_routes;
-    int nr_allocated_irq_routes;
-    uint32_t *used_gsi_bitmap;
-    unsigned int gsi_count;
-    QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
-    bool direct_msi;
-#endif
-};
-
-#define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
-
-#define KVM_STATE(obj) \
-    OBJECT_CHECK(KVMState, (obj), TYPE_KVM_ACCEL)
-
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupts_allowed;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/7] kvm-all: add KVM address space

[Paolo Bonzini]

Until now, KVM_SET_USER_MEMORY_REGION has been working on
address_space_memory.  However, KVM's memory slots are the
CPU view of the memory, which does not exactly match
address_space_memory.

Let the architecture-specific code build the CPU view of the
memory by combining address_space_memory and other MemoryRegions.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/sysemu/kvm_int.h |  3 +++
 kvm-all.c                | 21 ++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index d5f746f..5e4090d 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -60,6 +60,9 @@ struct KVMState
     QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
     bool direct_msi;
 #endif
+
+    AddressSpace kvm_as;
+    MemoryRegion kvm_as_root, kvm_as_mem;
 };
 
 #define TYPE_KVM_ACCEL ACCEL_CLASS_NAME("kvm")
diff --git a/kvm-all.c b/kvm-all.c
index 215ed33..c70436e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1401,6 +1401,7 @@ static int kvm_init(MachineState *ms)
     const char *kvm_type;
 
     s = KVM_STATE(ms->accelerator);
+    kvm_state = s;
 
     /*
      * On systems where the kernel can support different base page
@@ -1578,9 +1579,21 @@ static int kvm_init(MachineState *ms)
     kvm_vm_attributes_allowed =
         (kvm_check_extension(s, KVM_CAP_VM_ATTRIBUTES) > 0);
 
+    /* An outer container, with normal system memory inside.
+     * kvm_arch_init can add more.
+     */
+    memory_region_init(&s->kvm_as_root, OBJECT(s), "mem-container", ~0ull);
+    memory_region_set_enabled(&s->kvm_as_root, true);
+    memory_region_init_alias(&s->kvm_as_mem, OBJECT(s), "memory",
+                             get_system_memory(), 0, ~0ull);
+    memory_region_set_enabled(&s->kvm_as_mem, true);
+    memory_region_add_subregion_overlap(&s->kvm_as_root, 0, &s->kvm_as_mem, 0);
+
+    address_space_init(&s->kvm_as, &s->kvm_as_root, "KVM");
+
     ret = kvm_arch_init(ms, s);
     if (ret < 0) {
-        goto err;
+        goto err_as_destroy;
     }
 
     ret = kvm_irqchip_create(ms, s);
@@ -1588,8 +1601,7 @@ static int kvm_init(MachineState *ms)
         goto err;
     }
 
-    kvm_state = s;
-    memory_listener_register(&kvm_memory_listener, &address_space_memory);
+    memory_listener_register(&kvm_memory_listener, &s->kvm_as);
     memory_listener_register(&kvm_io_listener, &address_space_io);
 
     s->many_ioeventfds = kvm_check_many_ioeventfds();
@@ -1598,6 +1610,8 @@ static int kvm_init(MachineState *ms)
 
     return 0;
 
+err_as_destroy:
+    address_space_destroy(&s->kvm_as);
 err:
     assert(ret < 0);
     if (s->vmfd >= 0) {
@@ -1607,6 +1621,7 @@ err:
         close(s->fd);
     }
     g_free(s->slots);
+    kvm_state = NULL;
 
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/7] memory: add kvm_mem_flags to MemoryRegion

[Paolo Bonzini]

This patch is ugly; it adds a KVM-specific flag to memory regions that
is used to distinguish SMRAM regions from others.  This of course is a
layering violation, but I have no other good ideas about how to avoid it.

If you let KVM use address_space_memory as it did until now, and add
separate calls to KVM_SET_USER_MEMORY_REGION for SMRAM regions, it's
much harder to cover the case when a RAM or ROM BAR is placed to cover
parts of SMRAM.

Hence, this series uses instead one address space only, including
all of system memory, PCI BARs and SMRAM.  This way, hiding the BARs
is done automatically by the memory core.  This, however, has an
important issue which requires this kind of layering violation.

The issue is that when you close SMRAM there is no change in the memory
map as far as the memory API is concerned.  The only change is in the
flags we pass to KVM, but these are not visible to the memory API.  So
KVM needs to convince the memory API to see a region_del/region_add
sequence from the listener; it needs to convince flatrange_equal to
return false for the "open SMRAM" and "closed SMRAM" cases.  This is
what the kvm_mem_flags do.

Besides the layering violation, there is also the question of how to
handle flags from subregions.  The current code just ORs the parent
region's flags into all subregions.

This being ugly, and hopefully temporary until someone shows me the
light, I haven't yet added nice accessors for the field.  The final
patch accesses the field blindly from target-i386/kvm.c.

One alternative would be to point two different RAM regions to the same
guest memory, using memory_region_init_ram_ptr.  That works, but it's
even more gross.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/memory.h |  4 ++++
 kvm-all.c             | 13 +++++++------
 memory.c              | 17 +++++++++++++----
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index b61c84f..cccba59 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -192,6 +192,8 @@ struct MemoryRegion {
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
     NotifierList iommu_notify;
+
+    int kvm_mem_flags;
 };
 
 /**
@@ -264,6 +266,8 @@ struct MemoryRegionSection {
     Int128 size;
     hwaddr offset_within_address_space;
     bool readonly;
+
+    int kvm_mem_flags;
 };
 
 /**
diff --git a/kvm-all.c b/kvm-all.c
index c70436e..ccb7dd3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -237,10 +237,11 @@ err:
  * dirty pages logging control
  */
 
-static int kvm_mem_flags(MemoryRegion *mr)
+static int kvm_mem_flags(MemoryRegionSection *section)
 {
+    MemoryRegion *mr = section->mr;
     bool readonly = mr->readonly || memory_region_is_romd(mr);
-    int flags = 0;
+    int flags = section->kvm_mem_flags;
 
     if (memory_region_is_logging(mr)) {
         flags |= KVM_MEM_LOG_DIRTY_PAGES;
@@ -689,7 +690,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = old.memory_size;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags = kvm_mem_flags(mr);
+            mem->flags = kvm_mem_flags(section);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -710,7 +711,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             mem->memory_size = start_addr - old.start_addr;
             mem->start_addr = old.start_addr;
             mem->ram = old.ram;
-            mem->flags =  kvm_mem_flags(mr);
+            mem->flags =  kvm_mem_flags(section);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -734,7 +735,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
             size_delta = mem->start_addr - old.start_addr;
             mem->memory_size = old.memory_size - size_delta;
             mem->ram = old.ram + size_delta;
-            mem->flags = kvm_mem_flags(mr);
+            mem->flags = kvm_mem_flags(section);
 
             err = kvm_set_user_memory_region(s, mem);
             if (err) {
@@ -756,7 +757,7 @@ static void kvm_set_phys_mem(MemoryRegionSection *section, bool add)
     mem->memory_size = size;
     mem->start_addr = start_addr;
     mem->ram = ram;
-    mem->flags = kvm_mem_flags(mr);
+    mem->flags = kvm_mem_flags(section);
 
     err = kvm_set_user_memory_region(s, mem);
     if (err) {
diff --git a/memory.c b/memory.c
index 03c536b..cf810f1 100644
--- a/memory.c
+++ b/memory.c
@@ -160,6 +160,7 @@ static bool memory_listener_match(MemoryListener *listener,
         .size = (fr)->addr.size,                                        \
         .offset_within_address_space = int128_get64((fr)->addr.start),  \
         .readonly = (fr)->readonly,                                     \
+        .kvm_mem_flags = (fr)->kvm_mem_flags,                           \
               }))
 
 struct CoalescedMemoryRange {
@@ -219,6 +220,7 @@ struct FlatRange {
     MemoryRegion *mr;
     hwaddr offset_in_region;
     AddrRange addr;
+    int kvm_mem_flags;
     uint8_t dirty_log_mask;
     bool romd_mode;
     bool readonly;
@@ -246,6 +248,7 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b)
         && addrrange_equal(a->addr, b->addr)
         && a->offset_in_region == b->offset_in_region
         && a->romd_mode == b->romd_mode
+        && a->kvm_mem_flags == b->kvm_mem_flags
         && a->readonly == b->readonly;
 }
 
@@ -306,7 +309,8 @@ static bool can_merge(FlatRange *r1, FlatRange *r2)
                      int128_make64(r2->offset_in_region))
         && r1->dirty_log_mask == r2->dirty_log_mask
         && r1->romd_mode == r2->romd_mode
-        && r1->readonly == r2->readonly;
+        && r1->readonly == r2->readonly
+        && r1->kvm_mem_flags == r2->kvm_mem_flags;
 }
 
 /* Attempt to simplify a view by merging adjacent ranges */
@@ -542,6 +546,7 @@ static void render_memory_region(FlatView *view,
                                  MemoryRegion *mr,
                                  Int128 base,
                                  AddrRange clip,
+                                 int kvm_mem_flags,
                                  bool readonly)
 {
     MemoryRegion *subregion;
@@ -556,6 +561,7 @@ static void render_memory_region(FlatView *view,
         return;
     }
 
+    kvm_mem_flags |= mr->kvm_mem_flags;
     int128_addto(&base, int128_make64(mr->addr));
     readonly |= mr->readonly;
 
@@ -570,13 +576,15 @@ static void render_memory_region(FlatView *view,
     if (mr->alias) {
         int128_subfrom(&base, int128_make64(mr->alias->addr));
         int128_subfrom(&base, int128_make64(mr->alias_offset));
-        render_memory_region(view, mr->alias, base, clip, readonly);
+        render_memory_region(view, mr->alias, base, clip,
+			     kvm_mem_flags, readonly);
         return;
     }
 
     /* Render subregions in priority order. */
     QTAILQ_FOREACH(subregion, &mr->subregions, subregions_link) {
-        render_memory_region(view, subregion, base, clip, readonly);
+        render_memory_region(view, subregion, base, clip, 
+                             kvm_mem_flags, readonly);
     }
 
     if (!mr->terminates) {
@@ -591,6 +599,7 @@ static void render_memory_region(FlatView *view,
     fr.dirty_log_mask = mr->dirty_log_mask;
     fr.romd_mode = mr->romd_mode;
     fr.readonly = readonly;
+    fr.kvm_mem_flags = kvm_mem_flags;
 
     /* Render the region itself into any gaps left by the current view. */
     for (i = 0; i < view->nr && int128_nz(remain); ++i) {
@@ -632,7 +641,7 @@ static FlatView *generate_memory_topology(MemoryRegion *mr)
 
     if (mr) {
         render_memory_region(view, mr, int128_zero(),
-                             addrrange_make(int128_zero(), int128_2_64()), false);
+                             addrrange_make(int128_zero(), int128_2_64()), 0, false);
     }
     flatview_simplify(view);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/7] i386: disable the region in /machine/smram when SMRAM is open

[Paolo Bonzini]

This patch provides some help from the chipset in handling SMRAM.

SMRAM regions can overlap with e.g. PCI BARs.  When this happens, PCI
BARs will be hidden behind SMRAM and will cause a userspace MMIO exit.
This can be achieved easily with KVM just by giving a higher priority
to the SMRAM region.  The SMRAM region is changed to a KVM_MEM_X86_SMRAM
memory slot and causes a MMIO exit when outside SMM.

Unfortunately, when SMRAM is open, the SMRAM region in system memory
would be treated like a PCI BAR and would be hidden behind SMRAM.  In
order to avoid this, disable the region in /machine/smram when SMRAM
is open.  Then the KVM address space will include the SMRAM region
through system memory (thus without the flag) rather than through
/machine/smram.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-host/piix.c |  6 ++++++
 hw/pci-host/q35.c  | 20 +++++++++++++-------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 3f23851..ee8a680 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -142,6 +142,12 @@ static void i440fx_update_memory_mappings(PCII440FXState *d)
                               !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
     memory_region_set_enabled(&d->smram,
                               pd->config[I440FX_SMRAM] & SMRAM_G_SMRAME);
+
+    /* KVM requires disabling the region in /machine/smram when SMRAM
+     * is open.
+     */
+    memory_region_set_enabled(&d->low_smram,
+                              !(pd->config[I440FX_SMRAM] & SMRAM_D_OPEN));
     memory_region_transaction_commit();
 }
 
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 60a9f2c..3a016fa 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -305,18 +305,24 @@ static void mch_update_smram(MCHPCIState *mch)
         memory_region_set_enabled(&mch->smram_region, h_smrame);
         /* Show high SMRAM if H_SMRAME = 1 */
         memory_region_set_enabled(&mch->open_high_smram, h_smrame);
+
+        /* KVM requires disabling the region in /machine/smram when SMRAM
+         * is open.
+         */
+        memory_region_set_enabled(&mch->low_smram, false);
+        memory_region_set_enabled(&mch->high_smram, false);
     } else {
         /* Hide high SMRAM and low SMRAM */
         memory_region_set_enabled(&mch->smram_region, true);
         memory_region_set_enabled(&mch->open_high_smram, false);
-    }
 
-    if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_G_SMRAME) {
-        memory_region_set_enabled(&mch->low_smram, !h_smrame);
-        memory_region_set_enabled(&mch->high_smram, h_smrame);
-    } else {
-        memory_region_set_enabled(&mch->low_smram, false);
-        memory_region_set_enabled(&mch->high_smram, false);
+        if (pd->config[MCH_HOST_BRIDGE_SMRAM] & SMRAM_G_SMRAME) {
+            memory_region_set_enabled(&mch->low_smram, !h_smrame);
+            memory_region_set_enabled(&mch->high_smram, h_smrame);
+        } else {
+            memory_region_set_enabled(&mch->low_smram, false);
+            memory_region_set_enabled(&mch->high_smram, false);
+        }
     }
 
     if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/7] kvm-i386: register SMRAM regions with KVM_MEM_X86_SMRAM

[Paolo Bonzini]

This patch adds SMRAM regions to the KVM CPU address space.  The
/machine/smram container has KVM_MEM_X86_SMRAM set in the
memory flags, and this flag is propagated to the memory slot.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 09b4fc7..224f8db 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -87,6 +87,8 @@ static bool has_msr_xss;
 static bool has_msr_architectural_pmu;
 static uint32_t num_architectural_pmu_counters;
 
+static Notifier smram_machine_done;
+
 bool kvm_allows_irq0_override(void)
 {
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
@@ -846,6 +848,25 @@ static int kvm_get_supported_msrs(KVMState *s)
     return ret;
 }
 
+static void smram_notify(Notifier *n, void *unused)
+{
+    MemoryRegion *smram =
+       (MemoryRegion *) object_resolve_path("/machine/smram", NULL);
+
+    if (!smram) {
+        return;
+    }
+
+    /* Add the SMRAM regions to the KVM address space, so that they will
+     * be considered when adding memory slots.
+     *
+     * This requires that the chipsets *disables* regions in /machine/smram
+     * whenever SMRAM is opened.
+     */
+    memory_region_add_subregion_overlap(&kvm_state->kvm_as_root, 0, smram, 10);
+    smram->kvm_mem_flags |= KVM_MEM_X86_SMRAM;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     uint64_t identity_base = 0xfffbc000;
@@ -904,6 +925,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
             return ret;
         }
     }
+
+    if (kvm_check_extension(s, KVM_CAP_X86_SMM)) {
+        smram_machine_done.notify = smram_notify;
+        qemu_add_machine_init_done_notifier(&smram_machine_done);
+    }
+
     return 0;
 }
 
-- 
1.8.3.1