Re: [Qemu-devel] [PATCH 1/1] Introduce a new bus "ICC" to connect APIC

[liu ping fan]

On Wed, Oct 19, 2011 at 03:42:27PM +0200, Jan Kiszka wrote:
> On 2011-10-19 15:33, Jan Kiszka wrote:
> > On 2011-10-19 14:54, Anthony Liguori wrote:
> >> On 10/19/2011 05:53 AM, Jan Kiszka wrote:
> >>> On 2011-10-19 03:55, address@hidden wrote:
> >>>> From: Liu Ping Fan<address@hidden>
> >>>>
> >>>> Introduce a new structure CPUS as the controller of ICC (INTERRUPT
> >>>> CONTROLLER COMMUNICATIONS), and new bus "ICC" to hold APIC,instead
> >>>> of sysbus. So we can support APIC hot-plug feature.
> >>>>
> >>>> Signed-off-by: liu ping fan<address@hidden>
> >>>> ---
> >>>>   Makefile.target |    1 +
> >>>>   hw/apic.c       |   25 +++++++++++----
> >>>>   hw/apic.h       |    1 +
> >>>>   hw/icc_bus.c    |   91 
> >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>   hw/icc_bus.h    |   56 ++++++++++++++++++++++++++++++++++
> >>>>   hw/pc.c         |   11 ++++--
> >>>>   6 files changed, 174 insertions(+), 11 deletions(-)
> >>>>   create mode 100644 hw/icc_bus.c
> >>>>   create mode 100644 hw/icc_bus.h
> >>>>
> >>>> diff --git a/Makefile.target b/Makefile.target
> >>>> index 9011f28..5607c6d 100644
> >>>> --- a/Makefile.target
> >>>> +++ b/Makefile.target
> >>>> @@ -241,6 +241,7 @@ obj-i386-$(CONFIG_KVM) += kvmclock.o
> >>>>   obj-i386-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> >>>>   obj-i386-y += testdev.o
> >>>>   obj-i386-y += acpi.o acpi_piix4.o
> >>>> +obj-i386-y += icc_bus.o
> >>>>
> >>>>   obj-i386-y += pcspk.o i8254.o
> >>>>   obj-i386-$(CONFIG_KVM_PIT) += i8254-kvm.o
> >>>> diff --git a/hw/apic.c b/hw/apic.c
> >>>> index 69d6ac5..00d2297 100644
> >>>> --- a/hw/apic.c
> >>>> +++ b/hw/apic.c
> >>>> @@ -21,9 +21,10 @@
> >>>>   #include "ioapic.h"
> >>>>   #include "qemu-timer.h"
> >>>>   #include "host-utils.h"
> >>>> -#include "sysbus.h"
> >>>> +#include "icc_bus.h"
> >>>>   #include "trace.h"
> >>>>   #include "kvm.h"
> >>>> +#include "exec-memory.h"
> >>>
> >>> Mmh, don't your rather want memory.h?
> >>>
> >>>>
> >>>>   /* APIC Local Vector Table */
> >>>>   #define APIC_LVT_TIMER   0
> >>>> @@ -80,7 +81,7 @@
> >>>>   typedef struct APICState APICState;
> >>>>
> >>>>   struct APICState {
> >>>> -    SysBusDevice busdev;
> >>>> +    ICCBusDevice busdev;
> >>>>       MemoryRegion io_memory;
> >>>>       void *cpu_env;
> >>>>       uint32_t apicbase;
> >>>> @@ -1104,9 +1105,20 @@ static const MemoryRegionOps apic_io_ops = {
> >>>>       .endianness = DEVICE_NATIVE_ENDIAN,
> >>>>   };
> >>>>
> >>>> -static int apic_init1(SysBusDevice *dev)
> >>>> +/**/
> >>>
> >>> Empty comment.
> >>>
> >>>> +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base)
> >>>>   {
> >>>> -    APICState *s = FROM_SYSBUS(APICState, dev);
> >>>> +    APICState *s = DO_UPCAST(APICState, busdev.qdev, dev);
> >>>> +
> >>>> +    memory_region_add_subregion(get_system_memory(),
> >>>> +                                base,
> >>>> +&s->io_memory);
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static int apic_init1(ICCBusDevice *dev)
> >>>> +{
> >>>> +    APICState *s = DO_UPCAST(APICState, busdev, dev);
> >>>>       static int last_apic_idx;
> >>>>
> >>>>       if (last_apic_idx>= MAX_APICS) {
> >>>> @@ -1114,7 +1126,6 @@ static int apic_init1(SysBusDevice *dev)
> >>>>       }
> >>>>       memory_region_init_io(&s->io_memory,&apic_io_ops, s, "apic",
> >>>>                             MSI_ADDR_SIZE);
> >>>> -    sysbus_init_mmio_region(dev,&s->io_memory);
> >>>>
> >>>>       s->timer = qemu_new_timer_ns(vm_clock, apic_timer, s);
> >>>>       s->idx = last_apic_idx++;
> >>>> @@ -1122,7 +1133,7 @@ static int apic_init1(SysBusDevice *dev)
> >>>>       return 0;
> >>>>   }
> >>>>
> >>>> -static SysBusDeviceInfo apic_info = {
> >>>> +static ICCBusDeviceInfo apic_info = {
> >>>>       .init = apic_init1,
> >>>>       .qdev.name = "apic",
> >>>>       .qdev.size = sizeof(APICState),
> >>>> @@ -1138,7 +1149,7 @@ static SysBusDeviceInfo apic_info = {
> >>>>
> >>>>   static void apic_register_devices(void)
> >>>>   {
> >>>> -    sysbus_register_withprop(&apic_info);
> >>>> +    iccbus_register_devinfo(&apic_info);
> >>>>   }
> >>>>
> >>>>   device_init(apic_register_devices)
> >>>> diff --git a/hw/apic.h b/hw/apic.h
> >>>> index c857d52..e2c0af5 100644
> >>>> --- a/hw/apic.h
> >>>> +++ b/hw/apic.h
> >>>> @@ -20,6 +20,7 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
> >>>>   uint8_t cpu_get_apic_tpr(DeviceState *s);
> >>>>   void apic_init_reset(DeviceState *s);
> >>>>   void apic_sipi(DeviceState *s);
> >>>> +int apic_mmio_map(DeviceState *dev, target_phys_addr_t base);
> >>>>
> >>>>   /* pc.c */
> >>>>   int cpu_is_bsp(CPUState *env);
> >>>> diff --git a/hw/icc_bus.c b/hw/icc_bus.c
> >>>> new file mode 100644
> >>>> index 0000000..61a408e
> >>>> --- /dev/null
> >>>> +++ b/hw/icc_bus.c
> >>>> @@ -0,0 +1,91 @@
> >>>> +/* icc_bus.c
> >>>> + * emulate x86 ICC(INTERRUPT CONTROLLER COMMUNICATIONS) bus
> >>>
> >>> Copyright?
> >>>
> >>>> + *
> >>>> + * This library is free software; you can redistribute it and/or
> >>>> + * modify it under the terms of the GNU Lesser General Public
> >>>> + * License as published by the Free Software Foundation; either
> >>>> + * version 2 of the License, or (at your option) any later version.
> >>>> + *
> >>>> + * This library is distributed in the hope that it will be useful,
> >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >>>> + * Lesser General Public License for more details.
> >>>> + *
> >>>> + * You should have received a copy of the GNU Lesser General Public
> >>>> + * License along with this library; if not, 
> >>>> see<http://www.gnu.org/licenses/>
> >>>> + */
> >>>> +#include "icc_bus.h"
> >>>> +
> >>>> +static CPUS *dummy_cpus;
> >>>
> >>> Why "dummy"? Also, no statics please. The bus owner is the chipset
> >>> (440fx), so embedded it there. That also avoid that strange "cpus"
> >>> device below.
> >>
> >> That's an odd model IMHO.  The i440fx doesn't implement the ICC bus.  The 
> >> ICC
> >> bus is entirely independent of the northbridge.
> >>
> >> Maybe CPUSockets would be a better device name?
> >>
> > 
> > The problem is that it is even something non-existent in the absence of
> > an IOAPIC - which is an optional device for the i440fx/PIIX3, though we
> > only model this for ISA PCs, not for PCI machines (and make a lot of
> > mistakes in this area as I recently noticed). Still, we will need an ICC
> > bus on a ISAPC from now on. Also not correct.
> > 
> > In an ideal world there would be no dummy device CPUSockets, just an
> > optional unowned ICC bus. CPUs were not ICC bus devices, but could
> > attach to one if its there. But I think this cannot be modeled yet. So
> > we likely have to live with the proposed setup for now.
Yeah, what makes modeling hard is the "unowned" ICC bus in the ideal world.
But obeying to qdev model requirement, there must be a device parent for ICC
bus, so we come to the compromise --CPUSockets and maybe it could be a holder
for other orphan buses besides ICC, when we run into the similar corner.
> 
> Mmh, maybe we could at least provide two types of CPUs instead: a sysbus
> version for non-ICC setups and a ICC version if there is such a bus. The
> only variance should be in two qdev descriptions and two MMIO map helpers.
This makes the model more ideal, but at a cost of code maintenance. As you
said, we could remove the dummy CPUSockets device at some later point, what
about leaving it handled all at once at that time.
> 
> Still leaves us with the dummy CPUSockets device, but that could be
> removed at some later point. I presume empty vmsds create no traces in
> the binary migration format, correct?
Yeah, you are right. I made the mistake because not familiar with vmsd.

Thanks and regards,
ping fan
> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux