On 04/29/19 18:01, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 4/23/19 9:02 PM, Laszlo Ersek wrote:
>> On 04/22/19 21:50, Philippe Mathieu-Daudé wrote:
>>> Implement fw_cfg_arch_key_name(), which returns the name of a
>>> ppc-specific key.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>>> ---
>>> hw/ppc/Makefile.objs | 2 +-
>>> hw/ppc/fw_cfg.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 46 insertions(+), 1 deletion(-)
>>> create mode 100644 hw/ppc/fw_cfg.c
>>>
>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>>> index 1111b218a04..ae940981553 100644
>>> --- a/hw/ppc/Makefile.objs
>>> +++ b/hw/ppc/Makefile.objs
>>> @@ -1,5 +1,5 @@
>>> # shared objects
>>> -obj-y += ppc.o ppc_booke.o fdt.o
>>> +obj-y += ppc.o ppc_booke.o fdt.o fw_cfg.o
>>> # IBM pSeries (sPAPR)
>>> obj-$(CONFIG_PSERIES) += spapr.o spapr_caps.o spapr_vio.o spapr_events.o
>>> obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>> diff --git a/hw/ppc/fw_cfg.c b/hw/ppc/fw_cfg.c
>>> new file mode 100644
>>> index 00000000000..a88b5c4bde2
>>> --- /dev/null
>>> +++ b/hw/ppc/fw_cfg.c
>>> @@ -0,0 +1,45 @@
>>> +/*
>>> + * fw_cfg helpers (PPC specific)
>>> + *
>>> + * Copyright (c) 2019 Red Hat, Inc.
>>> + *
>>> + * Author:
>>> + * Philippe Mathieu-Daudé <phi...@redhat.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/ppc/ppc.h"
>>> +#include "hw/nvram/fw_cfg.h"
>>> +
>>> +const char *fw_cfg_arch_key_name(uint16_t key)
>>> +{
>>> + static const struct {
>>> + uint16_t key;
>>> + const char *name;
>>> + } fw_cfg_arch_wellknown_keys[] = {
>>> + {FW_CFG_PPC_WIDTH, "width"},
>>> + {FW_CFG_PPC_HEIGHT, "height"},
>>> + {FW_CFG_PPC_DEPTH, "depth"},
>>> + {FW_CFG_PPC_TBFREQ, "tbfreq"},
>>> + {FW_CFG_PPC_CLOCKFREQ, "clockfreq"},
>>> + {FW_CFG_PPC_IS_KVM, "is_kvm"},
>>> + {FW_CFG_PPC_KVM_HC, "kvm_hc"},
>>> + {FW_CFG_PPC_KVM_PID, "pid"},
>>> + {FW_CFG_PPC_NVRAM_ADDR, "nvram_addr"},
>>> + {FW_CFG_PPC_BUSFREQ, "busfreq"},
>>> + {FW_CFG_PPC_NVRAM_FLAT, "nvram_flat"},
>>> + {FW_CFG_PPC_VIACONFIG, "viaconfig"},
>>> + };
>>> +
>>> + for (size_t i = 0; i < ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
>>> + if (fw_cfg_arch_wellknown_keys[i].key == key) {
>>> + return fw_cfg_arch_wellknown_keys[i].name;
>>> + }
>>> + }
>>> + return NULL;
>>> +}
>>>
>>
>> (1) Have you considered extracting the struct type, and the linear
>> search, to code that's shared between the arches?
>>
>> It might suffice to make only the "fw_cfg_arch_wellknown_keys" array
>> target-specific.
>
> Yes, I tried different ways:
>
> 1/ Declare as extern
>
> If we declare the array as 'extern const', we can no more use the
> ARRAY_SIZE() macro, so we have to use an 'extern const size_t' too.
> (No need to use a getter() since the array is *const*).
>
> I personally try to avoid extern variables when possible, I find them
> bug prone.
>
> 2/ Add a macro in the header, use it in each source
>
> The macro is ugly to read, the result looked worse to me.
>
> 3/ I don't expect new keys to be added often, and adding them will be
> trivial 1-line patch each key.
>
> I might be unaware of better ways to deduplicate this, so if you have
> suggestions I'm happy to learn :)
In the loop condition, you could replace ARRAY_SIZE with a terminator
element check, and you could terminate the arrays with an
{ FW_CFG_INVALID, NULL }
element. Then the loop could be extracted, and you wouldn't need further
size_t globals, for replacing ARRAY_SIZE.
But, again, it's not that important.
Thanks,
Laszlo
>> (It's not complex code so I don't mind if you opt for the code duplication.)
>>
>> (2) PPC highlights my question#2 from under "v3 3/7". Namely, we
>> extracted the x86 constants into "hw/i386/fw_cfg.h". But the PPC
>> constants already exist in "include/hw/ppc/ppc.h". (My point being the
>> difference in the "include/" pathname prefix.) Should we be consistent?
>
> I'd like to be consistent :)
>
> So far only machines set fw_cfg keys.
>
> I don't see arch-specific devices accessing arch-specific fw_cfg keys,
> so we might move arch-specific key definitions in hw/$ARCH/fw_cfg.h (not
> include/hw/$ARCH/fw_cfg.h).
>
>> If you decide to stick with this variant:
>>
>> Reviewed-by: Laszlo Ersek <ler...@redhat.com>
>
> Thanks!
>
>>
>> Thanks
>> Laszlo
>>
