Implement fw_cfg_arch_key_name(), which returns the name of a
ppc-specific key.

Signed-off-by: Philippe Mathieu-Daudé <philmd@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é <philmd@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;
+}
-- 
2.20.1

On Mon, Apr 22, 2019 at 09:50:18PM +0200, 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é <philmd@redhat.com>

What ppc machines actually use fw_cfg?  I know pseries and powernv
don't.

> ---
>  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é <philmd@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;
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Hi David,

On 4/23/19 3:20 AM, David Gibson wrote:
> On Mon, Apr 22, 2019 at 09:50:18PM +0200, 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é <philmd@redhat.com>
> 
> What ppc machines actually use fw_cfg?  I know pseries and powernv
> don't.

I see:

- 40p
- g3beige (newworld)
- mac99 (oldworld)

I guess it is because they use OpenBIOS.

>> ---
>>  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é <philmd@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;
>> +}
> 

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é <philmd@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é <philmd@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.

(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?

If you decide to stick with this variant:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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é <philmd@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é <philmd@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 :)

> (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 <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 

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é <philmd@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é <philmd@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 <lersek@redhat.com>
> 
> Thanks!
> 
>>
>> Thanks
>> Laszlo
>>

On 4/30/19 11:41 AM, Laszlo Ersek wrote:
> 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é <philmd@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é <philmd@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.

Clever, I forgot this way :>

> 
> 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 <lersek@redhat.com>
>>
>> Thanks!
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>