[RFC,01/18] pc: create "PC" device class

Submitted by Eduardo Habkost on Oct. 3, 2012, 1:28 p.m.

Details

Message ID 1349270954-4657-2-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost Oct. 3, 2012, 1:28 p.m.
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(+)

Comments

Paolo Bonzini Oct. 3, 2012, 2:38 p.m.
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);
>  
>
Eduardo Habkost Oct. 3, 2012, 2:48 p.m.
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);
> >  
> > 
>
Paolo Bonzini Oct. 3, 2012, 2:50 p.m.
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
Anthony Liguori Oct. 4, 2012, 1:46 p.m.
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
Paolo Bonzini Oct. 4, 2012, 1:50 p.m.
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
Eduardo Habkost Oct. 4, 2012, 1:57 p.m.
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
Anthony Liguori Oct. 4, 2012, 2:28 p.m.
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
Anthony Liguori Oct. 4, 2012, 2:29 p.m.
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
Eduardo Habkost Oct. 4, 2012, 2:43 p.m.
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
Anthony Liguori Oct. 4, 2012, 3:10 p.m.
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
Eduardo Habkost Oct. 4, 2012, 3:57 p.m.
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
Andreas Färber Oct. 4, 2012, 4 p.m.
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
Eduardo Habkost Oct. 4, 2012, 4:03 p.m.
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.
Anthony Liguori Oct. 4, 2012, 5:42 p.m.
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
Anthony Liguori Oct. 4, 2012, 5:43 p.m.
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
Eduardo Habkost Oct. 4, 2012, 5:51 p.m.
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.

Patch hide | download patch | download mbox

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);