Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: virt: Add cpu-topology DT node.

[Philippe Mathieu-Daudé]

On 6/25/19 12:41 PM, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 12:36:35PM +0200, Philippe Mathieu-Daudé wrote:
>> On 6/25/19 1:24 AM, Alistair Francis wrote:
>>> On Mon, Jun 24, 2019 at 3:57 PM Atish Patra <address@hidden> wrote:
>>>>
>>>> Currently, there is no cpu topology defined in RISC-V.
>>>> Define a device tree node that clearly describes the
>>>> entire topology. This saves the trouble of scanning individual
>>>> cache to figure out the topology.
>>>>
>>>> Here is the linux kernel patch series that enables topology
>>>> for RISC-V.
>>>>
>>>> http://lists.infradead.org/pipermail/linux-riscv/2019-June/005072.html
>>>>
>>>> CPU topology after applying this patch in QEMU & above series in kernel
>>>>
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
>>>> 2
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/physical_package_id
>>>> 0
>>>> / # cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
>>>> 0-7
>>>>
>>>> Signed-off-by: Atish Patra <address@hidden>
>>>> ---
>>>>  hw/riscv/virt.c | 21 +++++++++++++++++++--
>>>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>> index 84d94d0c42d8..da0b8aa18747 100644
>>>> --- a/hw/riscv/virt.c
>>>> +++ b/hw/riscv/virt.c
>>>> @@ -203,9 +203,12 @@ static void *create_fdt(RISCVVirtState *s, const 
>>>> struct MemmapEntry *memmap,
>>>>          qemu_fdt_setprop_string(fdt, nodename, "status", "okay");
>>>>          qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
>>>>          qemu_fdt_setprop_string(fdt, nodename, "device_type", "cpu");
>>>> +        qemu_fdt_setprop_cell(fdt, nodename, "phandle", cpu_phandle);
>>>> +        qemu_fdt_setprop_cell(fdt, nodename, "linux,phandle", 
>>>> cpu_phandle);
>>>> +        int intc_phandle = phandle++;
>>>
>>> Don't declare variables in the middle of code. The variable must be
>>> declared at the start of a block.
>>
>> I guess this has been relaxed since we allow GNU C99:
> 
> Even though we allow GNU C99 I think it is undesirable to declare variables
> in the middle of methods. This is especially true when combined with "goto"
> as you end up with undefined / uninitialized vairable contents at the jump
> target, if we've jumped over the variable declaration.
> 
> We can't enforce location of variable declarations, but I'd really
> recommend we keep them all at the start of code blocks.

In this case I find it desirable:

  for (int i = 0; ...) {
    ...
  }

For the rest, I agree to keep them at the start of code block.

Regards,

Phil.