| Message ID | 27af0665bdcccc032cf477190cad35dd754fc4fb.1439877857.git.zhugh.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 19, 2015 at 05:36:29PM +0800, Zhu Guihua wrote: > 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. Can you improve the commit message by explaining not just what is being done, but why this is needed? > > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com> > --- > hw/i386/pc.c | 7 ------- > hw/intc/apic_common.c | 6 ------ > target-i386/cpu.c | 21 +++++++++++++++++++++ > 3 files changed, 21 insertions(+), 13 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 9f2924e..8b7dbe5 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1158,13 +1158,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 0032b97..c0b32eb 100644 > --- a/hw/intc/apic_common.c > +++ b/hw/intc/apic_common.c > @@ -296,7 +296,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp) > APICCommonClass *info; > static DeviceState *vapic; > static int apic_no; > - static bool mmio_registered; > > if (apic_no >= MAX_APICS) { > error_setg(errp, "%s initialization failed.", > @@ -307,11 +306,6 @@ 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; > - } > > /* Note: We need at least 1M to map the VAPIC option ROM */ > if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK && > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index cfb8aa7..8eed88c 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2745,6 +2745,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > /* TODO: convert to link<> */ > apic = APIC_COMMON(cpu->apic_state); > apic->cpu = cpu; > + apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE; > } > > static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp) > @@ -2789,8 +2790,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > X86CPU *cpu = X86_CPU(dev); > X86CPUClass *xcc = X86_CPU_GET_CLASS(dev); > CPUX86State *env = &cpu->env; > + APICCommonState *apic; > Error *local_err = NULL; > static bool ht_warned; > + static bool apic_mmio_map_once; > > if (cpu->apic_id < 0) { > error_setg(errp, "apic-id property was not initialized properly"); > @@ -2877,6 +2880,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > if (local_err != NULL) { > goto out; > } > + > + /* map APIC MMIO area */ > + apic = APIC_COMMON(cpu->apic_state); > + if (tcg_enabled()) { > + memory_region_add_subregion_overlap(cpu->cpu_as_root, > + apic->apicbase & > + MSR_IA32_APICBASE_BASE, > + &apic->io_memory, > + 0x1000); Why exactly is this necessary? If this is necessary, why don't we need to do this for non-TCG accelerators? > + } else if (!apic_mmio_map_once) { > + memory_region_add_subregion_overlap(get_system_memory(), > + apic->apicbase & > + MSR_IA32_APICBASE_BASE, > + &apic->io_memory, > + 0x1000); > + apic_mmio_map_once = true; > + } I see that you are doing two things at the same time: 1) Moving the memory region registration to x86_cpu_realizefn(); 2) Adding a special case for TCG that uses cpu->cpu_as_root. Doing this in two separate patches seems more appropriate.
On 21/08/2015 15:54, Eduardo Habkost wrote: > > + if (tcg_enabled()) { > > + memory_region_add_subregion_overlap(cpu->cpu_as_root, > > + apic->apicbase & > > + MSR_IA32_APICBASE_BASE, > > + &apic->io_memory, > > + 0x1000); > > Why exactly is this necessary? If this is necessary, why don't we need > to do this for non-TCG accelerators? At least KVM and qtest do not support per-CPU address spaces. I'm not sure about Xen, it looks like it could but it would be slower. Paolo
On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 21/08/2015 15:54, Eduardo Habkost wrote: >> > + if (tcg_enabled()) { >> > + memory_region_add_subregion_overlap(cpu->cpu_as_root, >> > + apic->apicbase & >> > + MSR_IA32_APICBASE_BASE, >> > + &apic->io_memory, >> > + 0x1000); >> >> Why exactly is this necessary? If this is necessary, why don't we need >> to do this for non-TCG accelerators? > > At least KVM and qtest do not support per-CPU address spaces. Right, but given this restriction why can't we also do whatever we need to work without the per-CPU address spaces with TCG? thanks -- PMM
On Mon, Aug 24, 2015 at 03:56:56PM +0100, Peter Maydell wrote: > On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > On 21/08/2015 15:54, Eduardo Habkost wrote: > >> > + if (tcg_enabled()) { > >> > + memory_region_add_subregion_overlap(cpu->cpu_as_root, > >> > + apic->apicbase & > >> > + MSR_IA32_APICBASE_BASE, > >> > + &apic->io_memory, > >> > + 0x1000); > >> > >> Why exactly is this necessary? If this is necessary, why don't we need > >> to do this for non-TCG accelerators? > > > > At least KVM and qtest do not support per-CPU address spaces. > > Right, but given this restriction why can't we also do whatever > we need to work without the per-CPU address spaces with TCG? Yeah, that was my question. I know why we can't use cpu->cpu_as_root in KVM, but I don't understand why we need to use cpu->cpu_as_root with TCG. If that's really an actual per-accelerator requirement we can't avoid, I would prefer to implement it as an AccelClass method instead of a tcg_enabled() check.
(sorry for top posting) Because the emulation quality is indeed a bit better with the per-CPU address spaces; you could move each APIC's base address independent of the others. However, this is not a feature that is actually used by anything in practice, so I doubt anyone cares about TCG implementing it correctly. Paolo -----Original Message----- From: Peter Maydell [peter.maydell@linaro.org] Received: lunedì, 24 ago 2015, 16:57 To: Paolo Bonzini [pbonzini@redhat.com] CC: Eduardo Habkost [ehabkost@redhat.com]; Zhu Guihua [zhugh.fnst@cn.fujitsu.com]; ChenFan [chen.fan.fnst@cn.fujitsu.com]; Igor Mammedov [imammedo@redhat.com]; izumi.taku@jp.fujitsu.com, QEMU Developers [qemu-devel@nongnu.org]; Andreas Färber [afaerber@suse.de] Subject: Re: [Qemu-devel] [RESEND PATCH v9 1/4] apic: map APIC's MMIO region at each CPU's address space On 24 August 2015 at 01:55, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 21/08/2015 15:54, Eduardo Habkost wrote: >> > + if (tcg_enabled()) { >> > + memory_region_add_subregion_overlap(cpu->cpu_as_root, >> > + apic->apicbase & >> > + MSR_IA32_APICBASE_BASE, >> > + &apic->io_memory, >> > + 0x1000); >> >> Why exactly is this necessary? If this is necessary, why don't we need >> to do this for non-TCG accelerators? > > At least KVM and qtest do not support per-CPU address spaces. Right, but given this restriction why can't we also do whatever we need to work without the per-CPU address spaces with TCG? thanks -- PMM
On Wed, Aug 26, 2015 at 11:27:08AM -0400, Paolo Bonzini wrote: [...] > > >> > + if (tcg_enabled()) { > > >> > + memory_region_add_subregion_overlap(cpu->cpu_as_root, > > >> > + apic->apicbase & > > >> > + MSR_IA32_APICBASE_BASE, > > >> > + &apic->io_memory, > > >> > + 0x1000); > > >> > > >> Why exactly is this necessary? If this is necessary, why don't we need > > >> to do this for non-TCG accelerators? > > > > > > At least KVM and qtest do not support per-CPU address spaces. > > > > Right, but given this restriction why can't we also do whatever > > we need to work without the per-CPU address spaces with TCG? > > > > Because the emulation quality is indeed a bit better with the per-CPU > address spaces; you could move each APIC's base address independent of > the others. However, this is not a feature that is actually used by > anything in practice, so I doubt anyone cares about TCG implementing > it correctly. Do we need additional changes in TCG to implement it correctly, or is this going to work out of the box as soon as we apply this series? If it's the latter, the patch makes sense to me (but please add a comment to the code explaining why). If it's the former, I don't see the point of making the code more complex before that feature is actually implemented by TCG. Also, we could make the logic simpler if we just check if cpu->cpu_as_root is set, e.g.: /* Use per-CPU address space if available (TCG supports it, KVM * doesn't). This allows the APIC base address of each CPU * to be moved independently. */ memory_region_add_subregion_overlap(cpu->cpu_as_root ?: get_system_memory(), apic->apicbase & MSR_IA32_APICBASE_BASE, &apic->io_memory, 0x1000);
On 08/26/2015 11:49 PM, Eduardo Habkost wrote: > On Wed, Aug 26, 2015 at 11:27:08AM -0400, Paolo Bonzini wrote: > [...] >>>>>> + if (tcg_enabled()) { >>>>>> + memory_region_add_subregion_overlap(cpu->cpu_as_root, >>>>>> + apic->apicbase & >>>>>> + MSR_IA32_APICBASE_BASE, >>>>>> + &apic->io_memory, >>>>>> + 0x1000); >>>>> Why exactly is this necessary? If this is necessary, why don't we need >>>>> to do this for non-TCG accelerators? >>>> At least KVM and qtest do not support per-CPU address spaces. >>> Right, but given this restriction why can't we also do whatever >>> we need to work without the per-CPU address spaces with TCG? >>> >> Because the emulation quality is indeed a bit better with the per-CPU >> address spaces; you could move each APIC's base address independent of >> the others. However, this is not a feature that is actually used by >> anything in practice, so I doubt anyone cares about TCG implementing >> it correctly. > Do we need additional changes in TCG to implement it correctly, or is > this going to work out of the box as soon as we apply this series? > > If it's the latter, the patch makes sense to me (but please add a > comment to the code explaining why). If it's the former, I don't see the > point of making the code more complex before that feature is actually > implemented by TCG. > > Also, we could make the logic simpler if we just check if > cpu->cpu_as_root is set, e.g.: > > /* Use per-CPU address space if available (TCG supports it, KVM > * doesn't). This allows the APIC base address of each CPU > * to be moved independently. > */ > memory_region_add_subregion_overlap(cpu->cpu_as_root ?: > get_system_memory(), > apic->apicbase & > MSR_IA32_APICBASE_BASE, > &apic->io_memory, > 0x1000); Yeah, the logic is better. I will take this, thanks. And, comments will be added in next version. Thanks, Zhu
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 9f2924e..8b7dbe5 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1158,13 +1158,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 0032b97..c0b32eb 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -296,7 +296,6 @@ static void apic_common_realize(DeviceState *dev, Error **errp) APICCommonClass *info; static DeviceState *vapic; static int apic_no; - static bool mmio_registered; if (apic_no >= MAX_APICS) { error_setg(errp, "%s initialization failed.", @@ -307,11 +306,6 @@ 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; - } /* Note: We need at least 1M to map the VAPIC option ROM */ if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK && diff --git a/target-i386/cpu.c b/target-i386/cpu.c index cfb8aa7..8eed88c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2745,6 +2745,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) /* TODO: convert to link<> */ apic = APIC_COMMON(cpu->apic_state); apic->cpu = cpu; + apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE; } static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp) @@ -2789,8 +2790,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) X86CPU *cpu = X86_CPU(dev); X86CPUClass *xcc = X86_CPU_GET_CLASS(dev); CPUX86State *env = &cpu->env; + APICCommonState *apic; Error *local_err = NULL; static bool ht_warned; + static bool apic_mmio_map_once; if (cpu->apic_id < 0) { error_setg(errp, "apic-id property was not initialized properly"); @@ -2877,6 +2880,24 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) if (local_err != NULL) { goto out; } + + /* map APIC MMIO area */ + apic = APIC_COMMON(cpu->apic_state); + if (tcg_enabled()) { + memory_region_add_subregion_overlap(cpu->cpu_as_root, + apic->apicbase & + MSR_IA32_APICBASE_BASE, + &apic->io_memory, + 0x1000); + } else if (!apic_mmio_map_once) { + memory_region_add_subregion_overlap(get_system_memory(), + apic->apicbase & + MSR_IA32_APICBASE_BASE, + &apic->io_memory, + 0x1000); + apic_mmio_map_once = true; + } + cpu_reset(cs); xcc->parent_realize(dev, &local_err);