[Qemu-devel] [PULL 0/5] X86 patch queue, 2015-05-29

[Qemu-devel] [PULL 0/5] X86 patch queue, 2015-05-29

[Eduardo Habkost]

The following changes since commit 2cc3bdbe2d3908f7a813d1c2d774cc2bf07746cd:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-block-2015-05-29' into staging (2015-05-29 15:32:15 +0100)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to d032544e08ed07c66afd45ca8a8420ca202fab75:

  arch_init: Drop target-x86_64.conf (2015-05-29 14:46:32 -0300)

----------------------------------------------------------------
X86 patch queue, 2015-05-29

----------------------------------------------------------------

Andreas Färber (1):
  pc: Ensure non-zero CPU ref count after attaching to ICC bus

Chen Fan (1):
  apic: map APIC's MMIO region at each CPU's address space

Eduardo Habkost (1):
  target-i386: Register QOM properties for feature flags

Ikey Doherty (1):
  arch_init: Drop target-x86_64.conf

Zhu Guihua (1):
  apic: convert ->busdev.qdev casts to C casts

 Makefile                             |   7 +-
 arch_init.c                          |   1 -
 exec.c                               |   5 ++
 hw/i386/pc.c                         |  18 ++---
 hw/intc/apic.c                       |   9 ++-
 hw/intc/apic_common.c                |  14 ++--
 include/exec/memory.h                |   5 ++
 sysconfigs/target/target-x86_64.conf |   0
 target-i386/cpu.c                    | 124 +++++++++++++++++++++++++++++++++++
 9 files changed, 158 insertions(+), 25 deletions(-)
 delete mode 100644 sysconfigs/target/target-x86_64.conf

-- 
2.1.0

[Qemu-devel] [PULL 1/5] pc: Ensure non-zero CPU ref count after attaching to ICC bus

[Eduardo Habkost]

From: Andreas Färber <afaerber@suse.de>

Setting the parent bus of a device increases its ref count, which we
ultimately want to level out. However it is only safe to do so after the
last reference to the device in local code, as qom-set or similar operations
might decrease the ref count.

Therefore move the object_unref() from pc_new_cpu() into its callers.

The APIC operations on the last CPU in pc_cpus_init() are still potentially
insecure, but that is beyond the scope of this code movement.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 769eb25..45028ea 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1006,7 +1006,6 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
     }
 
     qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
-    object_unref(OBJECT(cpu));
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
@@ -1025,7 +1024,9 @@ static const char *current_cpu_model;
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     DeviceState *icc_bridge;
+    X86CPU *cpu;
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
+    Error *local_err = NULL;
 
     if (id < 0) {
         error_setg(errp, "Invalid CPU id: %" PRIi64, id);
@@ -1053,7 +1054,12 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 
     icc_bridge = DEVICE(object_resolve_path_type("icc-bridge",
                                                  TYPE_ICC_BRIDGE, NULL));
-    pc_new_cpu(current_cpu_model, apic_id, icc_bridge, errp);
+    cpu = pc_new_cpu(current_cpu_model, apic_id, icc_bridge, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    object_unref(OBJECT(cpu));
 }
 
 void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
@@ -1087,6 +1093,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
             error_report_err(error);
             exit(1);
         }
+        object_unref(OBJECT(cpu));
     }
 
     /* map APIC MMIO area if CPU has APIC */
-- 
2.1.0

[Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

Replace mapping APIC at global system address space with
mapping it at per-CPU address spaces.

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c                |  5 +++++
 hw/i386/pc.c          |  7 -------
 hw/intc/apic_common.c | 14 ++++++++------
 include/exec/memory.h |  5 +++++
 target-i386/cpu.c     |  2 ++
 5 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/exec.c b/exec.c
index e19ab22..71c02ed 100644
--- a/exec.c
+++ b/exec.c
@@ -2712,6 +2712,11 @@ void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
     cpu_notify_map_clients();
 }
 
+MemoryRegion *address_space_root_memory_region(AddressSpace *as)
+{
+    return as->root;
+}
+
 void *cpu_physical_memory_map(hwaddr addr,
                               hwaddr *plen,
                               int is_write)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 45028ea..185e748 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1096,13 +1096,6 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
         object_unref(OBJECT(cpu));
     }
 
-    /* map APIC MMIO area if CPU has APIC */
-    if (cpu && cpu->apic_state) {
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
-                                APIC_DEFAULT_ADDRESS, 0x1000);
-    }
-
     /* tell smbios about cpuid version and features */
     smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d595d63..f251787 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -296,7 +296,8 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     APICCommonClass *info;
     static DeviceState *vapic;
     static int apic_no;
-    static bool mmio_registered;
+    CPUState *cpu = CPU(s->cpu);
+    MemoryRegion *root;
 
     if (apic_no >= MAX_APICS) {
         error_setg(errp, "%s initialization failed.",
@@ -307,11 +308,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
 
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
-    if (!mmio_registered) {
-        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
-        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
-        mmio_registered = true;
-    }
+
+    root = address_space_root_memory_region(cpu->as);
+    memory_region_add_subregion_overlap(root,
+                                        s->apicbase & MSR_IA32_APICBASE_BASE,
+                                        &s->io_memory,
+                                        0x1000);
 
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
diff --git a/include/exec/memory.h b/include/exec/memory.h
index b61c84f..a16650f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1295,6 +1295,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
 void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                          int is_write, hwaddr access_len);
 
+/* address_space_root_memory_region: get root memory region
+ *
+ * @as: #AddressSpace to be accessed
+ */
+MemoryRegion *address_space_root_memory_region(AddressSpace *as);
 
 #endif
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3305e09..f83e526 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2740,6 +2740,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
+    cpu_set_apic_base(cpu->apic_state,
+                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
 }
 
 static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
2.1.0

[Qemu-devel] [PULL 3/5] apic: convert ->busdev.qdev casts to C casts

[Eduardo Habkost]

From: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Use C casts to avoid accessing ICCDevice's qdev field
directly.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Acked-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/intc/apic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..77b639c 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -370,13 +370,14 @@ static int apic_irq_pending(APICCommonState *s)
 static void apic_update_irq(APICCommonState *s)
 {
     CPUState *cpu;
+    DeviceState *dev = (DeviceState *)s;
 
     cpu = CPU(s->cpu);
     if (!qemu_cpu_is_self(cpu)) {
         cpu_interrupt(cpu, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(cpu, CPU_INTERRUPT_HARD);
-    } else if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    } else if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
@@ -549,10 +550,12 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
 
 static bool apic_check_pic(APICCommonState *s)
 {
-    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    DeviceState *dev = (DeviceState *)s;
+
+    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    apic_deliver_pic_intr(dev, 1);
     return true;
 }
 
-- 
2.1.0

[Qemu-devel] [PULL 4/5] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

This uses the feature name arrays to register QOM properties for feature
flags. This simply adds properties that can be configured using -global,
but doesn't change x86_cpu_parse_featurestr() to use them yet.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f83e526..e38943e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2843,12 +2843,126 @@ out:
     }
 }
 
+typedef struct BitProperty {
+    uint32_t *ptr;
+    uint32_t mask;
+} BitProperty;
+
+static void x86_cpu_get_bit_prop(Object *obj,
+                                 struct Visitor *v,
+                                 void *opaque,
+                                 const char *name,
+                                 Error **errp)
+{
+    BitProperty *fp = opaque;
+    bool value = (*fp->ptr & fp->mask) == fp->mask;
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_cpu_set_bit_prop(Object *obj,
+                                 struct Visitor *v,
+                                 void *opaque,
+                                 const char *name,
+                                 Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    BitProperty *fp = opaque;
+    Error *local_err = NULL;
+    bool value;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_bool(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (value) {
+        *fp->ptr |= fp->mask;
+    } else {
+        *fp->ptr &= ~fp->mask;
+    }
+}
+
+static void x86_cpu_release_bit_prop(Object *obj, const char *name,
+                                     void *opaque)
+{
+    BitProperty *prop = opaque;
+    g_free(prop);
+}
+
+/* Register a boolean property to get/set a single bit in a uint32_t field.
+ *
+ * The same property name can be registered multiple times to make it affect
+ * multiple bits in the same FeatureWord. In that case, the getter will return
+ * true only if all bits are set.
+ */
+static void x86_cpu_register_bit_prop(X86CPU *cpu,
+                                      const char *prop_name,
+                                      uint32_t *field,
+                                      int bitnr)
+{
+    BitProperty *fp;
+    ObjectProperty *op;
+    uint32_t mask = (1UL << bitnr);
+
+    op = object_property_find(OBJECT(cpu), prop_name, NULL);
+    if (op) {
+        fp = op->opaque;
+        assert(fp->ptr == field);
+        fp->mask |= mask;
+    } else {
+        fp = g_new0(BitProperty, 1);
+        fp->ptr = field;
+        fp->mask = mask;
+        object_property_add(OBJECT(cpu), prop_name, "bool",
+                            x86_cpu_get_bit_prop,
+                            x86_cpu_set_bit_prop,
+                            x86_cpu_release_bit_prop, fp, &error_abort);
+    }
+}
+
+static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
+                                               FeatureWord w,
+                                               int bitnr)
+{
+    Object *obj = OBJECT(cpu);
+    int i;
+    char **names;
+    FeatureWordInfo *fi = &feature_word_info[w];
+
+    if (!fi->feat_names) {
+        return;
+    }
+    if (!fi->feat_names[bitnr]) {
+        return;
+    }
+
+    names = g_strsplit(fi->feat_names[bitnr], "|", 0);
+
+    feat2prop(names[0]);
+    x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr);
+
+    for (i = 1; names[i]; i++) {
+        feat2prop(names[i]);
+        object_property_add_alias(obj, names[i], obj, g_strdup(names[0]),
+                                  &error_abort);
+    }
+
+    g_strfreev(names);
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
     X86CPU *cpu = X86_CPU(obj);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
+    FeatureWord w;
     static int inited;
 
     cs->env_ptr = env;
@@ -2889,6 +3003,14 @@ static void x86_cpu_initfn(Object *obj)
     cpu->apic_id = -1;
 #endif
 
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        int bitnr;
+
+        for (bitnr = 0; bitnr < 32; bitnr++) {
+            x86_cpu_register_feature_bit_props(cpu, w, bitnr);
+        }
+    }
+
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
     /* init various static tables used in TCG mode */
-- 
2.1.0

[Qemu-devel] [PULL 5/5] arch_init: Drop target-x86_64.conf

[Eduardo Habkost]

From: Ikey Doherty <michael.i.doherty@intel.com>

The target-x86_64.conf sysconfig file has been empty and essentially ignored
now for several years. This change removes the unused file to enable moving
towards a stateless configuration.

Signed-off-by: Ikey Doherty <michael.i.doherty@intel.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 Makefile                             | 7 +------
 arch_init.c                          | 1 -
 sysconfigs/target/target-x86_64.conf | 0
 3 files changed, 1 insertion(+), 7 deletions(-)
 delete mode 100644 sysconfigs/target/target-x86_64.conf

diff --git a/Makefile b/Makefile
index d945804..2d52536 100644
--- a/Makefile
+++ b/Makefile
@@ -389,13 +389,8 @@ ifneq (,$(findstring qemu-ga,$(TOOLS)))
 endif
 endif
 
-install-confdir:
-	$(INSTALL_DIR) "$(DESTDIR)$(qemu_confdir)"
 
-install-sysconfig: install-datadir install-confdir
-	$(INSTALL_DATA) $(SRC_PATH)/sysconfigs/target/target-x86_64.conf "$(DESTDIR)$(qemu_confdir)"
-
-install: all $(if $(BUILD_DOCS),install-doc) install-sysconfig \
+install: all $(if $(BUILD_DOCS),install-doc) \
 install-datadir install-localstatedir
 ifneq ($(TOOLS),)
 	$(call install-prog,$(TOOLS),$(DESTDIR)$(bindir))
diff --git a/arch_init.c b/arch_init.c
index 23d3feb..b5d90a4 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -136,7 +136,6 @@ static struct defconfig_file {
     bool userconfig;
 } default_config_files[] = {
     { CONFIG_QEMU_CONFDIR "/qemu.conf",                   true },
-    { CONFIG_QEMU_CONFDIR "/target-" TARGET_NAME ".conf", true },
     { NULL }, /* end of list */
 };
 
diff --git a/sysconfigs/target/target-x86_64.conf b/sysconfigs/target/target-x86_64.conf
deleted file mode 100644
index e69de29..0000000
-- 
2.1.0

Re: [Qemu-devel] [PULL 0/5] X86 patch queue, 2015-05-29

[Peter Maydell]

On 29 May 2015 at 19:04, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The following changes since commit 2cc3bdbe2d3908f7a813d1c2d774cc2bf07746cd:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-block-2015-05-29' into staging (2015-05-29 15:32:15 +0100)
>
> are available in the git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/x86-pull-request
>
> for you to fetch changes up to d032544e08ed07c66afd45ca8a8420ca202fab75:
>
>   arch_init: Drop target-x86_64.conf (2015-05-29 14:46:32 -0300)
>
> ----------------------------------------------------------------
> X86 patch queue, 2015-05-29

Hi. I'm afraid this patchset provokes a lot of warnings from
clang's undefined-behaviour sanitizer when we run make check:

/home/petmay01/linaro/qemu-for-merges/hw/intc/apic_common.c:314:55:
runtime error: left shift of 1048575 by 12 places cannot be
represented in type 'int'

This is because this:
target-i386/cpu.h:#define MSR_IA32_APICBASE_BASE (0xfffff<<12)

is shifting a 1 into the sign bit of a signed integer. You need
to write 0xfffffU to force an unsigned shift here.

(The undef sanitizer is one of those things I don't really
expect submaintainers to run but which I have enabled for
my build process. At the moment we're almost clean for a
make check run, so I'm keen to avoid introducing new warnings.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Paolo Bonzini]

On 29/05/2015 20:04, Eduardo Habkost wrote:
>      static int apic_no;
> -    static bool mmio_registered;
> +    CPUState *cpu = CPU(s->cpu);
> +    MemoryRegion *root;
>  
>      if (apic_no >= MAX_APICS) {
>          error_setg(errp, "%s initialization failed.",
> @@ -307,11 +308,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>  
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
> -    if (!mmio_registered) {
> -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> -        mmio_registered = true;
> -    }
> +
> +    root = address_space_root_memory_region(cpu->as);

I think just using cpu->as->root is okay.

> +    memory_region_add_subregion_overlap(root,
> +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
> +                                        &s->io_memory,
> +                                        0x1000);

I think this patch is incorrect, because you do not install a separate
address space for each CPU.  Also, the CPU address space is only used
with TCG so it should be guarded by "if (tcg_enabled())".

Paolo

>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index b61c84f..a16650f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1295,6 +1295,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len);
>  
> +/* address_space_root_memory_region: get root memory region
> + *
> + * @as: #AddressSpace to be accessed
> + */
> +MemoryRegion *address_space_root_memory_region(AddressSpace *as);
>  
>  #endif
>  
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3305e09..f83e526 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2740,6 +2740,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      /* TODO: convert to link<> */
>      apic = APIC_COMMON(cpu->apic_state);
>      apic->cpu = cpu;
> +    cpu_set_apic_base(cpu->apic_state,
> +                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
>  }
>  

Re: [Qemu-devel] [PULL 0/5] X86 patch queue, 2015-05-29

[Eduardo Habkost]

On Fri, May 29, 2015 at 07:57:16PM +0100, Peter Maydell wrote:
> On 29 May 2015 at 19:04, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > The following changes since commit 2cc3bdbe2d3908f7a813d1c2d774cc2bf07746cd:
> >
> >   Merge remote-tracking branch 'remotes/armbru/tags/pull-block-2015-05-29' into staging (2015-05-29 15:32:15 +0100)
> >
> > are available in the git repository at:
> >
> >   git://github.com/ehabkost/qemu.git tags/x86-pull-request
> >
> > for you to fetch changes up to d032544e08ed07c66afd45ca8a8420ca202fab75:
> >
> >   arch_init: Drop target-x86_64.conf (2015-05-29 14:46:32 -0300)
> >
> > ----------------------------------------------------------------
> > X86 patch queue, 2015-05-29
> 
> Hi. I'm afraid this patchset provokes a lot of warnings from
> clang's undefined-behaviour sanitizer when we run make check:
> 
> /home/petmay01/linaro/qemu-for-merges/hw/intc/apic_common.c:314:55:
> runtime error: left shift of 1048575 by 12 places cannot be
> represented in type 'int'
> 
> This is because this:
> target-i386/cpu.h:#define MSR_IA32_APICBASE_BASE (0xfffff<<12)
> 
> is shifting a 1 into the sign bit of a signed integer. You need
> to write 0xfffffU to force an unsigned shift here.
> 
> (The undef sanitizer is one of those things I don't really
> expect submaintainers to run but which I have enabled for
> my build process. At the moment we're almost clean for a
> make check run, so I'm keen to avoid introducing new warnings.)

I will submit a fix and send a new pull request later. Thanks!

(I will also add a new build configuration to my setup using clang and
-fsanitize=undefined)

-- 
Eduardo

Re: [Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Eduardo Habkost]

On Fri, May 29, 2015 at 09:27:19PM +0200, Paolo Bonzini wrote:
> On 29/05/2015 20:04, Eduardo Habkost wrote:
> >      static int apic_no;
> > -    static bool mmio_registered;
> > +    CPUState *cpu = CPU(s->cpu);
> > +    MemoryRegion *root;
> >  
> >      if (apic_no >= MAX_APICS) {
> >          error_setg(errp, "%s initialization failed.",
> > @@ -307,11 +308,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
> >  
> >      info = APIC_COMMON_GET_CLASS(s);
> >      info->realize(dev, errp);
> > -    if (!mmio_registered) {
> > -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> > -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> > -        mmio_registered = true;
> > -    }
> > +
> > +    root = address_space_root_memory_region(cpu->as);
> 
> I think just using cpu->as->root is okay.
> 
> > +    memory_region_add_subregion_overlap(root,
> > +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
> > +                                        &s->io_memory,
> > +                                        0x1000);
> 
> I think this patch is incorrect, because you do not install a separate
> address space for each CPU.  Also, the CPU address space is only used
> with TCG so it should be guarded by "if (tcg_enabled())".

I am removing this from the queue. I guess I should have waited for some
feedback from the memory API maintainer before committing.

-- 
Eduardo

Re: [Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Igor Mammedov]

On Fri, 29 May 2015 21:27:19 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 29/05/2015 20:04, Eduardo Habkost wrote:
> >      static int apic_no;
> > -    static bool mmio_registered;
> > +    CPUState *cpu = CPU(s->cpu);
> > +    MemoryRegion *root;
> >  
> >      if (apic_no >= MAX_APICS) {
> >          error_setg(errp, "%s initialization failed.",
> > @@ -307,11 +308,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
> >  
> >      info = APIC_COMMON_GET_CLASS(s);
> >      info->realize(dev, errp);
> > -    if (!mmio_registered) {
> > -        ICCBus *b = ICC_BUS(qdev_get_parent_bus(dev));
> > -        memory_region_add_subregion(b->apic_address_space, 0, &s->io_memory);
> > -        mmio_registered = true;
> > -    }
> > +
> > +    root = address_space_root_memory_region(cpu->as);
> 
> I think just using cpu->as->root is okay.
> 
> > +    memory_region_add_subregion_overlap(root,
> > +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
> > +                                        &s->io_memory,
> > +                                        0x1000);
> 
> I think this patch is incorrect, because you do not install a separate
> address space for each CPU.  Also, the CPU address space is only used
> with TCG so it should be guarded by "if (tcg_enabled())".
Don't we need it be mapped on for KVM for MSI to work
when using kvm-apic?

kvm_apic_io_ops->write =  kvm_apic_mem_write->kvm_irqchip_send_msi()


> 
> Paolo
> 
> >      /* Note: We need at least 1M to map the VAPIC option ROM */
> >      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index b61c84f..a16650f 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1295,6 +1295,11 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
> >  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
> >                           int is_write, hwaddr access_len);
> >  
> > +/* address_space_root_memory_region: get root memory region
> > + *
> > + * @as: #AddressSpace to be accessed
> > + */
> > +MemoryRegion *address_space_root_memory_region(AddressSpace *as);
> >  
> >  #endif
> >  
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3305e09..f83e526 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2740,6 +2740,8 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
> >      /* TODO: convert to link<> */
> >      apic = APIC_COMMON(cpu->apic_state);
> >      apic->cpu = cpu;
> > +    cpu_set_apic_base(cpu->apic_state,
> > +                      APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
> >  }
> >  
> 

Re: [Qemu-devel] [PULL 2/5] apic: map APIC's MMIO region at each CPU's address space

[Paolo Bonzini]

On 03/06/2015 10:29, Igor Mammedov wrote:
>>> > > +    root = address_space_root_memory_region(cpu->as);
>> > 
>> > I think just using cpu->as->root is okay.
>> > 
>>> > > +    memory_region_add_subregion_overlap(root,
>>> > > +                                        s->apicbase & MSR_IA32_APICBASE_BASE,
>>> > > +                                        &s->io_memory,
>>> > > +                                        0x1000);
>> > 
>> > I think this patch is incorrect, because you do not install a separate
>> > address space for each CPU.  Also, the CPU address space is only used
>> > with TCG so it should be guarded by "if (tcg_enabled())".
> Don't we need it be mapped on for KVM for MSI to work
> when using kvm-apic?
> 
> kvm_apic_io_ops->write =  kvm_apic_mem_write->kvm_irqchip_send_msi()

Yes, and the reason this patch worked is simply that by default cpu->as
is &address_space_memory.  The patch was just registering the same
region once per VCPU instead of 1, in the same place as before.

Paolo

[Qemu-devel] [PULL 3/5] apic: convert ->busdev.qdev casts to C casts

[Eduardo Habkost]

From: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Use C casts to avoid accessing ICCDevice's qdev field
directly.

Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Acked-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/intc/apic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 0f97b47..77b639c 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -370,13 +370,14 @@ static int apic_irq_pending(APICCommonState *s)
 static void apic_update_irq(APICCommonState *s)
 {
     CPUState *cpu;
+    DeviceState *dev = (DeviceState *)s;
 
     cpu = CPU(s->cpu);
     if (!qemu_cpu_is_self(cpu)) {
         cpu_interrupt(cpu, CPU_INTERRUPT_POLL);
     } else if (apic_irq_pending(s) > 0) {
         cpu_interrupt(cpu, CPU_INTERRUPT_HARD);
-    } else if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    } else if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
         cpu_reset_interrupt(cpu, CPU_INTERRUPT_HARD);
     }
 }
@@ -549,10 +550,12 @@ static void apic_deliver(DeviceState *dev, uint8_t dest, uint8_t dest_mode,
 
 static bool apic_check_pic(APICCommonState *s)
 {
-    if (!apic_accept_pic_intr(&s->busdev.qdev) || !pic_get_output(isa_pic)) {
+    DeviceState *dev = (DeviceState *)s;
+
+    if (!apic_accept_pic_intr(dev) || !pic_get_output(isa_pic)) {
         return false;
     }
-    apic_deliver_pic_intr(&s->busdev.qdev, 1);
+    apic_deliver_pic_intr(dev, 1);
     return true;
 }
 
-- 
2.1.0