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

login
register
mail settings
Submitter Eduardo Habkost
Date Oct. 3, 2012, 1:28 p.m.
Message ID <1349270954-4657-2-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/188773/
State New
Headers show

Comments

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(+)
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

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