| Message ID | 1349270954-4657-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 03/10/2012 15:28, Eduardo Habkost ha scritto: > We can make it a child of a generic "machine" class later, but right now > a "PC" class is needed to allow global-properties to control some > details of CPU creation on the PC code. Does it need to be a Device, or can it be a normal Object (or for clarity a derivative of TYPE_CONTAINER)? Paolo > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/pc.c | 18 ++++++++++++++++++ > hw/pc.h | 6 ++++++ > 2 files changed, 24 insertions(+) > > diff --git a/hw/pc.c b/hw/pc.c > index 7e7e0e2..9b68282 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > } > } > > +typedef struct PC { > + DeviceState parent_obj; > +} PC; > + > +static const TypeInfo pc_type_info = { > + .name = TYPE_PC_MACHINE, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(PC), > + .class_size = sizeof(DeviceClass), > +}; > + > +static void pc_register_type(void) > +{ > + type_register_static(&pc_type_info); > +} > + > +type_init(pc_register_type); > + > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > { > int index = le32_to_cpu(e820_table.count); > diff --git a/hw/pc.h b/hw/pc.h > index e4db071..77e898f 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); > /* pc.c */ > extern int fd_bootchk; > > +#define TYPE_PC_MACHINE "PC" > +#define PC(obj) \ > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) > +struct PC; > +typedef struct PC PC; > + > void pc_register_ferr_irq(qemu_irq irq); > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > >
On Wed, Oct 03, 2012 at 04:38:10PM +0200, Paolo Bonzini wrote: > Il 03/10/2012 15:28, Eduardo Habkost ha scritto: > > We can make it a child of a generic "machine" class later, but right now > > a "PC" class is needed to allow global-properties to control some > > details of CPU creation on the PC code. > > Does it need to be a Device, or can it be a normal Object (or for > clarity a derivative of TYPE_CONTAINER)? It needs to be a Device because that's the only way to use global properties, today. I have some experimental patches that allow normal objects to get global properties set (as long as an explicit call to a object_set_global_props() function is made). Would that be preferrable? > > Paolo > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/pc.c | 18 ++++++++++++++++++ > > hw/pc.h | 6 ++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/hw/pc.c b/hw/pc.c > > index 7e7e0e2..9b68282 100644 > > --- a/hw/pc.c > > +++ b/hw/pc.c > > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > > } > > } > > > > +typedef struct PC { > > + DeviceState parent_obj; > > +} PC; > > + > > +static const TypeInfo pc_type_info = { > > + .name = TYPE_PC_MACHINE, > > + .parent = TYPE_DEVICE, > > + .instance_size = sizeof(PC), > > + .class_size = sizeof(DeviceClass), > > +}; > > + > > +static void pc_register_type(void) > > +{ > > + type_register_static(&pc_type_info); > > +} > > + > > +type_init(pc_register_type); > > + > > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > > { > > int index = le32_to_cpu(e820_table.count); > > diff --git a/hw/pc.h b/hw/pc.h > > index e4db071..77e898f 100644 > > --- a/hw/pc.h > > +++ b/hw/pc.h > > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); > > /* pc.c */ > > extern int fd_bootchk; > > > > +#define TYPE_PC_MACHINE "PC" > > +#define PC(obj) \ > > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) > > +struct PC; > > +typedef struct PC PC; > > + > > void pc_register_ferr_irq(qemu_irq irq); > > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > > > > >
Il 03/10/2012 16:48, Eduardo Habkost ha scritto: >> > >> > Does it need to be a Device, or can it be a normal Object (or for >> > clarity a derivative of TYPE_CONTAINER)? > It needs to be a Device because that's the only way to use global > properties, today. > > I have some experimental patches that allow normal objects to get global > properties set (as long as an explicit call to a > object_set_global_props() function is made). Would that be preferrable? That's nice, though there would be the question of when to call the function... I suppose instance_init would do. Paolo
Eduardo Habkost <ehabkost@redhat.com> writes: > We can make it a child of a generic "machine" class later, but right now > a "PC" class is needed to allow global-properties to control some > details of CPU creation on the PC code. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/pc.c | 18 ++++++++++++++++++ > hw/pc.h | 6 ++++++ > 2 files changed, 24 insertions(+) > > diff --git a/hw/pc.c b/hw/pc.c > index 7e7e0e2..9b68282 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > } > } > > +typedef struct PC { > + DeviceState parent_obj; > +} PC; So the general problem with this approach is that it strays from modeling hardware. I guess I'm confused why we're not just adding an apic_id property to the CPU objects and setting that via the normal QOM accessors. Wouldn't that solve the problem? Regards, Anthony Liguori > + > +static const TypeInfo pc_type_info = { > + .name = TYPE_PC_MACHINE, > + .parent = TYPE_DEVICE, > + .instance_size = sizeof(PC), > + .class_size = sizeof(DeviceClass), > +}; > + > +static void pc_register_type(void) > +{ > + type_register_static(&pc_type_info); > +} > + > +type_init(pc_register_type); > + > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > { > int index = le32_to_cpu(e820_table.count); > diff --git a/hw/pc.h b/hw/pc.h > index e4db071..77e898f 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); > /* pc.c */ > extern int fd_bootchk; > > +#define TYPE_PC_MACHINE "PC" > +#define PC(obj) \ > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) > +struct PC; > +typedef struct PC PC; > + > void pc_register_ferr_irq(qemu_irq irq); > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > > -- > 1.7.11.4
Il 04/10/2012 15:46, Anthony Liguori ha scritto: >> > +typedef struct PC { >> > + DeviceState parent_obj; >> > +} PC; > So the general problem with this approach is that it strays from > modeling hardware. It doesn't really; it's a motherboard object, there's no reason why /machine shouldn't be a Device itself, with a few objects (CPUs, the i440FX, the IOAPIC, and of course the peripherals) hanging off it. Paolo
On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > We can make it a child of a generic "machine" class later, but right now > > a "PC" class is needed to allow global-properties to control some > > details of CPU creation on the PC code. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/pc.c | 18 ++++++++++++++++++ > > hw/pc.h | 6 ++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/hw/pc.c b/hw/pc.c > > index 7e7e0e2..9b68282 100644 > > --- a/hw/pc.c > > +++ b/hw/pc.c > > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > > } > > } > > > > +typedef struct PC { > > + DeviceState parent_obj; > > +} PC; > > So the general problem with this approach is that it strays from > modeling hardware. True, it's not modelling hardware. It's controlling the behavior of the QEMU code that set APIC IDs, because we need to keep the old behavior on old machine-types. > > I guess I'm confused why we're not just adding an apic_id property to > the CPU objects and setting that via the normal QOM accessors. > > Wouldn't that solve the problem? > It wouldn't solve the problem (although it can make the code look better). The problem is not setting the APIC ID, is controlling the code that generates the APIC IDs. I don't care too much where that code would live (it could be inside cpu.c or helper.c), but it still needs a flag where old machine-types tell it "please keep the old behavior for compatibility". > Regards, > > Anthony Liguori > > > + > > +static const TypeInfo pc_type_info = { > > + .name = TYPE_PC_MACHINE, > > + .parent = TYPE_DEVICE, > > + .instance_size = sizeof(PC), > > + .class_size = sizeof(DeviceClass), > > +}; > > + > > +static void pc_register_type(void) > > +{ > > + type_register_static(&pc_type_info); > > +} > > + > > +type_init(pc_register_type); > > + > > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > > { > > int index = le32_to_cpu(e820_table.count); > > diff --git a/hw/pc.h b/hw/pc.h > > index e4db071..77e898f 100644 > > --- a/hw/pc.h > > +++ b/hw/pc.h > > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); > > /* pc.c */ > > extern int fd_bootchk; > > > > +#define TYPE_PC_MACHINE "PC" > > +#define PC(obj) \ > > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) > > +struct PC; > > +typedef struct PC PC; > > + > > void pc_register_ferr_irq(qemu_irq irq); > > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > > > > -- > > 1.7.11.4
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 04/10/2012 15:46, Anthony Liguori ha scritto: >>> > +typedef struct PC { >>> > + DeviceState parent_obj; >>> > +} PC; >> So the general problem with this approach is that it strays from >> modeling hardware. > > It doesn't really; it's a motherboard object, there's no reason why > /machine shouldn't be a Device itself, with a few objects (CPUs, the > i440FX, the IOAPIC, and of course the peripherals) hanging off it. Okay, but modeling a motherboard is different than creating a "PC" object and throwing in the kitchen skink. And I'm not sure that going top-down is the best strategy. I think going bottom up makes more sense (starting with modeling Super IO chip). Regards, Anthony Liguori > > Paolo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > We can make it a child of a generic "machine" class later, but right now >> > a "PC" class is needed to allow global-properties to control some >> > details of CPU creation on the PC code. >> > >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> > --- >> > hw/pc.c | 18 ++++++++++++++++++ >> > hw/pc.h | 6 ++++++ >> > 2 files changed, 24 insertions(+) >> > >> > diff --git a/hw/pc.c b/hw/pc.c >> > index 7e7e0e2..9b68282 100644 >> > --- a/hw/pc.c >> > +++ b/hw/pc.c >> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) >> > } >> > } >> > >> > +typedef struct PC { >> > + DeviceState parent_obj; >> > +} PC; >> >> So the general problem with this approach is that it strays from >> modeling hardware. > > True, it's not modelling hardware. It's controlling the behavior of the > QEMU code that set APIC IDs, because we need to keep the old behavior on > old machine-types. > >> >> I guess I'm confused why we're not just adding an apic_id property to >> the CPU objects and setting that via the normal QOM accessors. >> >> Wouldn't that solve the problem? >> > > It wouldn't solve the problem (although it can make the code look > better). > > The problem is not setting the APIC ID, is controlling the code that > generates the APIC IDs. I don't care too much where that code would live > (it could be inside cpu.c or helper.c), but it still needs a flag where > old machine-types tell it "please keep the old behavior for > compatibility". Can you just add a flag to pc_init1 and set the apic_id property according to that flag? Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls pc_init1 with the appropriate flag value. Regards, Anthony Liguori > > >> Regards, >> >> Anthony Liguori >> >> > + >> > +static const TypeInfo pc_type_info = { >> > + .name = TYPE_PC_MACHINE, >> > + .parent = TYPE_DEVICE, >> > + .instance_size = sizeof(PC), >> > + .class_size = sizeof(DeviceClass), >> > +}; >> > + >> > +static void pc_register_type(void) >> > +{ >> > + type_register_static(&pc_type_info); >> > +} >> > + >> > +type_init(pc_register_type); >> > + >> > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) >> > { >> > int index = le32_to_cpu(e820_table.count); >> > diff --git a/hw/pc.h b/hw/pc.h >> > index e4db071..77e898f 100644 >> > --- a/hw/pc.h >> > +++ b/hw/pc.h >> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); >> > /* pc.c */ >> > extern int fd_bootchk; >> > >> > +#define TYPE_PC_MACHINE "PC" >> > +#define PC(obj) \ >> > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) >> > +struct PC; >> > +typedef struct PC PC; >> > + >> > void pc_register_ferr_irq(qemu_irq irq); >> > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); >> > >> > -- >> > 1.7.11.4 > > -- > Eduardo
On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 04/10/2012 15:46, Anthony Liguori ha scritto: > >>> > +typedef struct PC { > >>> > + DeviceState parent_obj; > >>> > +} PC; > >> So the general problem with this approach is that it strays from > >> modeling hardware. > > > > It doesn't really; it's a motherboard object, there's no reason why > > /machine shouldn't be a Device itself, with a few objects (CPUs, the > > i440FX, the IOAPIC, and of course the peripherals) hanging off it. > > Okay, but modeling a motherboard is different than creating a "PC" > object and throwing in the kitchen skink. > > And I'm not sure that going top-down is the best strategy. I think > going bottom up makes more sense (starting with modeling Super IO chip). > So, would you be OK with this implementation if the class were named "Motherboard", "set-of-CPU-sockets", or something like that? (The change on the APIC ID generation logic is not something that affects only individual CPUs or APICs, but the fw_cfg NUMA & CPU hotplug code as well, that's why I don't think "contiguous_apic_ids" should be a property of CPU or APIC objects). > Regards, > > Anthony Liguori > > > > > Paolo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto: >> >>> > +typedef struct PC { >> >>> > + DeviceState parent_obj; >> >>> > +} PC; >> >> So the general problem with this approach is that it strays from >> >> modeling hardware. >> > >> > It doesn't really; it's a motherboard object, there's no reason why >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it. >> >> Okay, but modeling a motherboard is different than creating a "PC" >> object and throwing in the kitchen skink. >> >> And I'm not sure that going top-down is the best strategy. I think >> going bottom up makes more sense (starting with modeling Super IO chip). >> > > So, would you be OK with this implementation if the class were named > "Motherboard", "set-of-CPU-sockets", or something like that? I would, but you're mixing up modeling with bug fixing. There's a very easy way to achieve your goal without dramatic remodeling. Just assign APIC ids during CPU creation and make contiguous_apic_ids a parameter of pc_init1. You don't need to worry about CPU hotplug. It doesn't exist in qemu.git and is broken in qemu-kvm.git. Regards, Anthony Liguori > > (The change on the APIC ID generation logic is not something that > affects only individual CPUs or APICs, but the fw_cfg NUMA & CPU hotplug > code as well, that's why I don't think "contiguous_apic_ids" should be a > property of CPU or APIC objects). > > >> Regards, >> >> Anthony Liguori >> >> > >> > Paolo > > -- > Eduardo
On Thu, Oct 04, 2012 at 09:29:57AM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote: > >> Eduardo Habkost <ehabkost@redhat.com> writes: > >> > >> > We can make it a child of a generic "machine" class later, but right now > >> > a "PC" class is needed to allow global-properties to control some > >> > details of CPU creation on the PC code. > >> > > >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> > --- > >> > hw/pc.c | 18 ++++++++++++++++++ > >> > hw/pc.h | 6 ++++++ > >> > 2 files changed, 24 insertions(+) > >> > > >> > diff --git a/hw/pc.c b/hw/pc.c > >> > index 7e7e0e2..9b68282 100644 > >> > --- a/hw/pc.c > >> > +++ b/hw/pc.c > >> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) > >> > } > >> > } > >> > > >> > +typedef struct PC { > >> > + DeviceState parent_obj; > >> > +} PC; > >> > >> So the general problem with this approach is that it strays from > >> modeling hardware. > > > > True, it's not modelling hardware. It's controlling the behavior of the > > QEMU code that set APIC IDs, because we need to keep the old behavior on > > old machine-types. > > > >> > >> I guess I'm confused why we're not just adding an apic_id property to > >> the CPU objects and setting that via the normal QOM accessors. > >> > >> Wouldn't that solve the problem? > >> > > > > It wouldn't solve the problem (although it can make the code look > > better). > > > > The problem is not setting the APIC ID, is controlling the code that > > generates the APIC IDs. I don't care too much where that code would live > > (it could be inside cpu.c or helper.c), but it still needs a flag where > > old machine-types tell it "please keep the old behavior for > > compatibility". > > Can you just add a flag to pc_init1 and set the apic_id property > according to that flag? > > Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls > pc_init1 with the appropriate flag value. I wish I was told this 6 months ago! I thought we wanted to avoid that and wanted to start using global properties instead of making the list of pc_init1() parameters grow. if that's acceptable then, yes, we could fix the bug without having to deal with class and object modelling. > > Regards, > > Anthony Liguori > > > > > > >> Regards, > >> > >> Anthony Liguori > >> > >> > + > >> > +static const TypeInfo pc_type_info = { > >> > + .name = TYPE_PC_MACHINE, > >> > + .parent = TYPE_DEVICE, > >> > + .instance_size = sizeof(PC), > >> > + .class_size = sizeof(DeviceClass), > >> > +}; > >> > + > >> > +static void pc_register_type(void) > >> > +{ > >> > + type_register_static(&pc_type_info); > >> > +} > >> > + > >> > +type_init(pc_register_type); > >> > + > >> > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) > >> > { > >> > int index = le32_to_cpu(e820_table.count); > >> > diff --git a/hw/pc.h b/hw/pc.h > >> > index e4db071..77e898f 100644 > >> > --- a/hw/pc.h > >> > +++ b/hw/pc.h > >> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); > >> > /* pc.c */ > >> > extern int fd_bootchk; > >> > > >> > +#define TYPE_PC_MACHINE "PC" > >> > +#define PC(obj) \ > >> > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) > >> > +struct PC; > >> > +typedef struct PC PC; > >> > + > >> > void pc_register_ferr_irq(qemu_irq irq); > >> > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); > >> > > >> > -- > >> > 1.7.11.4 > > > > -- > > Eduardo
Am 04.10.2012 15:50, schrieb Paolo Bonzini: > Il 04/10/2012 15:46, Anthony Liguori ha scritto: >>>> +typedef struct PC { >>>> + DeviceState parent_obj; >>>> +} PC; >> So the general problem with this approach is that it strays from >> modeling hardware. > > It doesn't really; it's a motherboard object, there's no reason why > /machine shouldn't be a Device itself, with a few objects (CPUs, the > i440FX, the IOAPIC, and of course the peripherals) hanging off it. On the other hand, why should it? We'd have QEMUMachine::reset vs. DeviceState::reset then and I don't see an immanent need for gpio / irqs on the mainboard either (on evaluation boards there are, but they are usually properties of the SoC modelling-wise). If we want to bundle objects, we have a dedicated Container type; otherwise we could just derive from TYPE_OBJECT directly. In the end it just goes back to our attempt to generalize the properties mechanisms and/or to a lack of APIs to do exactly what we want... ;) Andreas
On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto: > >> >>> > +typedef struct PC { > >> >>> > + DeviceState parent_obj; > >> >>> > +} PC; > >> >> So the general problem with this approach is that it strays from > >> >> modeling hardware. > >> > > >> > It doesn't really; it's a motherboard object, there's no reason why > >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the > >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it. > >> > >> Okay, but modeling a motherboard is different than creating a "PC" > >> object and throwing in the kitchen skink. > >> > >> And I'm not sure that going top-down is the best strategy. I think > >> going bottom up makes more sense (starting with modeling Super IO chip). > >> > > > > So, would you be OK with this implementation if the class were named > > "Motherboard", "set-of-CPU-sockets", or something like that? > > I would, but you're mixing up modeling with bug fixing. > > There's a very easy way to achieve your goal without dramatic > remodeling. > > Just assign APIC ids during CPU creation and make contiguous_apic_ids a > parameter of pc_init1. > > You don't need to worry about CPU hotplug. It doesn't exist in qemu.git > and is broken in qemu-kvm.git. With or without CPU hotplug, the max_cpus variable already exists, and I want to avoid breaking code that's already using it, and adding Yet Another problem to be fixed by whoever is going to make CPU hotplug work.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote: >> >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> >> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto: >> >> >>> > +typedef struct PC { >> >> >>> > + DeviceState parent_obj; >> >> >>> > +} PC; >> >> >> So the general problem with this approach is that it strays from >> >> >> modeling hardware. >> >> > >> >> > It doesn't really; it's a motherboard object, there's no reason why >> >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the >> >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it. >> >> >> >> Okay, but modeling a motherboard is different than creating a "PC" >> >> object and throwing in the kitchen skink. >> >> >> >> And I'm not sure that going top-down is the best strategy. I think >> >> going bottom up makes more sense (starting with modeling Super IO chip). >> >> >> > >> > So, would you be OK with this implementation if the class were named >> > "Motherboard", "set-of-CPU-sockets", or something like that? >> >> I would, but you're mixing up modeling with bug fixing. >> >> There's a very easy way to achieve your goal without dramatic >> remodeling. >> >> Just assign APIC ids during CPU creation and make contiguous_apic_ids a >> parameter of pc_init1. >> >> You don't need to worry about CPU hotplug. It doesn't exist in qemu.git >> and is broken in qemu-kvm.git. > > With or without CPU hotplug, the max_cpus variable already exists, and I > want to avoid breaking code that's already using it, and adding Yet > Another problem to be fixed by whoever is going to make CPU hotplug > work. Sorry, what does max_cpus have to do with apic ids?? Regards, Anthony Liguori > > -- > Eduardo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Oct 04, 2012 at 09:29:57AM -0500, Anthony Liguori wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Thu, Oct 04, 2012 at 08:46:46AM -0500, Anthony Liguori wrote: >> >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> >> >> > We can make it a child of a generic "machine" class later, but right now >> >> > a "PC" class is needed to allow global-properties to control some >> >> > details of CPU creation on the PC code. >> >> > >> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> >> > --- >> >> > hw/pc.c | 18 ++++++++++++++++++ >> >> > hw/pc.h | 6 ++++++ >> >> > 2 files changed, 24 insertions(+) >> >> > >> >> > diff --git a/hw/pc.c b/hw/pc.c >> >> > index 7e7e0e2..9b68282 100644 >> >> > --- a/hw/pc.c >> >> > +++ b/hw/pc.c >> >> > @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) >> >> > } >> >> > } >> >> > >> >> > +typedef struct PC { >> >> > + DeviceState parent_obj; >> >> > +} PC; >> >> >> >> So the general problem with this approach is that it strays from >> >> modeling hardware. >> > >> > True, it's not modelling hardware. It's controlling the behavior of the >> > QEMU code that set APIC IDs, because we need to keep the old behavior on >> > old machine-types. >> > >> >> >> >> I guess I'm confused why we're not just adding an apic_id property to >> >> the CPU objects and setting that via the normal QOM accessors. >> >> >> >> Wouldn't that solve the problem? >> >> >> > >> > It wouldn't solve the problem (although it can make the code look >> > better). >> > >> > The problem is not setting the APIC ID, is controlling the code that >> > generates the APIC IDs. I don't care too much where that code would live >> > (it could be inside cpu.c or helper.c), but it still needs a flag where >> > old machine-types tell it "please keep the old behavior for >> > compatibility". >> >> Can you just add a flag to pc_init1 and set the apic_id property >> according to that flag? >> >> Then you simply add a pc_init_post_1_3 and pc_init_pre_1_3 that calls >> pc_init1 with the appropriate flag value. > > I wish I was told this 6 months ago! I thought we wanted to avoid that > and wanted to start using global properties instead of making the list > of pc_init1() parameters grow. > > if that's acceptable then, yes, we could fix the bug without having to > deal with class and object modelling. Yes, this is absolutely acceptable. This is how we handle this kind of stuff today (like for kvmclock). Regards, Anthony Liguori > > >> >> Regards, >> >> Anthony Liguori >> >> > >> > >> >> Regards, >> >> >> >> Anthony Liguori >> >> >> >> > + >> >> > +static const TypeInfo pc_type_info = { >> >> > + .name = TYPE_PC_MACHINE, >> >> > + .parent = TYPE_DEVICE, >> >> > + .instance_size = sizeof(PC), >> >> > + .class_size = sizeof(DeviceClass), >> >> > +}; >> >> > + >> >> > +static void pc_register_type(void) >> >> > +{ >> >> > + type_register_static(&pc_type_info); >> >> > +} >> >> > + >> >> > +type_init(pc_register_type); >> >> > + >> >> > int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) >> >> > { >> >> > int index = le32_to_cpu(e820_table.count); >> >> > diff --git a/hw/pc.h b/hw/pc.h >> >> > index e4db071..77e898f 100644 >> >> > --- a/hw/pc.h >> >> > +++ b/hw/pc.h >> >> > @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); >> >> > /* pc.c */ >> >> > extern int fd_bootchk; >> >> > >> >> > +#define TYPE_PC_MACHINE "PC" >> >> > +#define PC(obj) \ >> >> > + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) >> >> > +struct PC; >> >> > +typedef struct PC PC; >> >> > + >> >> > void pc_register_ferr_irq(qemu_irq irq); >> >> > void pc_acpi_smi_interrupt(void *opaque, int irq, int level); >> >> > >> >> > -- >> >> > 1.7.11.4 >> > >> > -- >> > Eduardo > > -- > Eduardo
On Thu, Oct 04, 2012 at 12:42:23PM -0500, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Thu, Oct 04, 2012 at 10:10:16AM -0500, Anthony Liguori wrote: > >> Eduardo Habkost <ehabkost@redhat.com> writes: > >> > >> > On Thu, Oct 04, 2012 at 09:28:13AM -0500, Anthony Liguori wrote: > >> >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> >> > >> >> > Il 04/10/2012 15:46, Anthony Liguori ha scritto: > >> >> >>> > +typedef struct PC { > >> >> >>> > + DeviceState parent_obj; > >> >> >>> > +} PC; > >> >> >> So the general problem with this approach is that it strays from > >> >> >> modeling hardware. > >> >> > > >> >> > It doesn't really; it's a motherboard object, there's no reason why > >> >> > /machine shouldn't be a Device itself, with a few objects (CPUs, the > >> >> > i440FX, the IOAPIC, and of course the peripherals) hanging off it. > >> >> > >> >> Okay, but modeling a motherboard is different than creating a "PC" > >> >> object and throwing in the kitchen skink. > >> >> > >> >> And I'm not sure that going top-down is the best strategy. I think > >> >> going bottom up makes more sense (starting with modeling Super IO chip). > >> >> > >> > > >> > So, would you be OK with this implementation if the class were named > >> > "Motherboard", "set-of-CPU-sockets", or something like that? > >> > >> I would, but you're mixing up modeling with bug fixing. > >> > >> There's a very easy way to achieve your goal without dramatic > >> remodeling. > >> > >> Just assign APIC ids during CPU creation and make contiguous_apic_ids a > >> parameter of pc_init1. > >> > >> You don't need to worry about CPU hotplug. It doesn't exist in qemu.git > >> and is broken in qemu-kvm.git. > > > > With or without CPU hotplug, the max_cpus variable already exists, and I > > want to avoid breaking code that's already using it, and adding Yet > > Another problem to be fixed by whoever is going to make CPU hotplug > > work. > > Sorry, what does max_cpus have to do with apic ids?? See patch 15/18.
diff --git a/hw/pc.c b/hw/pc.c index 7e7e0e2..9b68282 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -550,6 +550,24 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val) } } +typedef struct PC { + DeviceState parent_obj; +} PC; + +static const TypeInfo pc_type_info = { + .name = TYPE_PC_MACHINE, + .parent = TYPE_DEVICE, + .instance_size = sizeof(PC), + .class_size = sizeof(DeviceClass), +}; + +static void pc_register_type(void) +{ + type_register_static(&pc_type_info); +} + +type_init(pc_register_type); + int e820_add_entry(uint64_t address, uint64_t length, uint32_t type) { int index = le32_to_cpu(e820_table.count); diff --git a/hw/pc.h b/hw/pc.h index e4db071..77e898f 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -102,6 +102,12 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out); /* pc.c */ extern int fd_bootchk; +#define TYPE_PC_MACHINE "PC" +#define PC(obj) \ + OBJECT_CHECK(PC, (obj), TYPE_PC_MACHINE) +struct PC; +typedef struct PC PC; + void pc_register_ferr_irq(qemu_irq irq); void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
We can make it a child of a generic "machine" class later, but right now a "PC" class is needed to allow global-properties to control some details of CPU creation on the PC code. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/pc.c | 18 ++++++++++++++++++ hw/pc.h | 6 ++++++ 2 files changed, 24 insertions(+)