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 <zhugh.f...@cn.fujitsu.com>
>>> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>>> ---
>>> 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 <mar...@redhat.com>
>>
>> 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)
