Re: [Qemu-devel] [PATCH 4/9] cpu/topology: add ARM support for smp machine properties

[Like Xu <like.xu@linux.intel.com>]

On 2019/4/2 12:45, Peter Maydell wrote:
> On Tue, 2 Apr 2019 at 09:46, Like Xu <like.xu@linux.intel.com> wrote:
>>
>> On 2019/4/2 7:38, Eduardo Habkost wrote:
>>> On Mon, Apr 01, 2019 at 10:56:30AM +0800, Like Xu wrote:
>>>> On 2019/3/29 17:27, Alex Bennée wrote:
>>> [...]
>>>>>> @@ -1713,6 +1717,9 @@ static void cortex_a9_initfn(Object *obj)
>>>>>>     #ifndef CONFIG_USER_ONLY
>>>>>>     static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>>>>>     {
>>>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>>>
>>>>> How expensive is qdev_get_machine? This could potentially be a
>>>>> performance issue if this register is read a lot.
>>>>
>>>> It may not bother us cause the MachineState object would be feteched from
>>>> qemu QOM at its first requested time and save it as static variable for
>>>> incoming requests.
>>>
>>> We already have a current_machine variable declared in
>>> include/hw/boards.h.  We normally avoid using it and use
>>> MACHINE(qdev_get_machine()) instead, but I never understood why.
>>>
>> Which one do you prefer to access smp machine properties?
> 
> My suggestion would be that we use qdev_get_machine(). I think
> it would be nice to make the remaining dozen or so uses of
> the global current_machine outside vl.c use qdev_get_machine()
> instead, and then make current_machine local to vl.c instead
> of global.
> 
> thanks
> -- PMM
> 
> 
It looks good to me and I am not sure if this refactoring should be 
added in next patch series or as a separate patch.