diff mbox

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

Message ID 1432922664-15129-3-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost May 29, 2015, 6:04 p.m. UTC 
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(-)

Comments

Paolo Bonzini May 29, 2015, 7:27 p.m. UTC | #1 
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);
>  }
>
Eduardo Habkost June 1, 2015, 8:01 p.m. UTC | #2 
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.
Igor Mammedov June 3, 2015, 8:29 a.m. UTC | #3 
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);
> >  }
> >  
>
Paolo Bonzini June 3, 2015, 8:30 a.m. UTC | #4 
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
diff mbox

Patch

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)