Re: [Qemu-devel] [RESEND PATCH v3] i386: keep cpu_model field in MachineState uptodate

[Andreas Färber]

Am 18.10.2015 um 14:20 schrieb Michael S. Tsirkin:
> On Sun, Oct 18, 2015 at 02:18:31PM +0300, Marcel Apfelbaum wrote:
>> On 10/15/2015 06:12 AM, Zhu Guihua wrote:
>>> Update cpu_model in MachineState for i386, so that the field can be used
>>> for cpu hotplug, instead of using a static variable.
>>>
>>> This patch is rebased on the latest master.
>>>
>>> Signed-off-by: Zhu Guihua <address@hidden>
>>> Reviewed-by: Eduardo Habkost <address@hidden>
>>> ---
>>> v3:
>>>  -use PCMachineState in pc_cpus_init() instead MachineState
>>>
>>> v2:
>>>  -transfer MachineState from all pc_cpus_init() callers
>>> ---
>>>  hw/i386/pc.c         | 17 ++++++++---------
>>>  hw/i386/pc_piix.c    |  2 +-
>>>  hw/i386/pc_q35.c     |  2 +-
>>>  include/hw/i386/pc.h |  2 +-
>>>  4 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 682867a..208f553 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -1077,11 +1077,10 @@ out:
>>>      return cpu;
>>>  }
>>>
>>> -static const char *current_cpu_model;
>>> -
>>>  void pc_hot_add_cpu(const int64_t id, Error **errp)
>>>  {
>>>      X86CPU *cpu;
>>> +    MachineState *machine = MACHINE(qdev_get_machine());
>>>      int64_t apic_id = x86_cpu_apic_id_from_index(id);
>>>      Error *local_err = NULL;
>>>
>>> @@ -1109,7 +1108,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>>>          return;
>>>      }
>>>
>>> -    cpu = pc_new_cpu(current_cpu_model, apic_id, &local_err);
>>> +    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);t
>>
>> Hi,
>>
>> I am not going to "stop" this patch and I do agree with what is trying to do.
>> What I still don't get is if we are "allowed" to directly access QOM 
>> object's private
>> fields outside the implementation C file.
>>
>> This is why we have some wrappers in include/hw/boards.h when we access 
>> machine's fields.
>>
>> Just wanted to raise the question, other than that (for what is worth):
>> Reviewed-by: Marcel Apfelbaum <address@hidden>
>>
>> Thanks,
>> Marcel
>>
> 
> 
> Andreas, could you ack/nack this patch pls?

I won't nack it, as putting it into the QOM state now is a good idea.
But I would rather put this into PCMachineState, as current_cpu_model
was PC-only and I'd prefer not to encourage more uses of the old API.

Marcel, since this is machine/PC and not core QOM/device, I don't see
accessing the field as a problem. An inline wrapper function would make
sense when many callers need to access it, which I do not envision
beyond this patch.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)