Re: [PATCH 1/5] hw/isa/vt82c686: Replace magic numbers by definitions

[BALATON Zoltan]

On Thu, 24 Jun 2021, Philippe Mathieu-Daudé wrote:
> Replace magic values of the Power Management / SMBus function (#4)
> by definitions from the datasheet. The result is less compact, and
> we can follow what the code does without having to recur to the
> datasheet.

I'm not sure this is an improvement. With the values it's clear what is 
done but I can't follow how these magic constants are defined or what they 
do so no idea if this is still correct. I think if you want to review a 
device model then you should be familiar with the device or consult the 
data sheet. Otherwise you can't spot problems in the definition of these 
constants either. I'm not a fan of hiding things behind cryptic macros 
when you could just write it in a straightforward way that could be 
understood more clearly.

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/isa/vt82c686.c | 50 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f57f3e70679..4ddcf2d398c 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -14,6 +14,7 @@
>  */
>
> #include "qemu/osdep.h"
> +#include "hw/registerfields.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> @@ -38,6 +39,16 @@
> #define TYPE_VIA_PM "via-pm"
> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>
> +REG8(PM_GEN_CFG0,                   0x40)
> +REG8(PM_GEN_CFG1,                   0x41)
> +FIELD(PM_GEN_CFG1, ACPI_IO_ENABLE,  7, 1)
> +REG32(PM_IO_BASE,                   0x48)
> +FIELD(PM_IO_BASE, ADDR,             7, 9)
> +REG32(SMBUS_IO_BASE,                0x90)
> +FIELD(SMBUS_IO_BASE, ADDR,          4, 12)
> +REG8(SMBUS_HOST_CONFIG,             0xd2)
> +FIELD(SMBUS_HOST_CONFIG, ENABLE,    0, 1)
> +
> struct ViaPMState {
>     PCIDevice dev;
>     MemoryRegion io;
> @@ -48,21 +59,24 @@ struct ViaPMState {
>
> static void pm_io_space_update(ViaPMState *s)
> {
> -    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
> +    uint32_t pmbase = pci_get_long(s->dev.config + A_PM_IO_BASE);
>
>     memory_region_transaction_begin();
> -    memory_region_set_address(&s->io, pmbase);
> -    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));
> +    memory_region_set_address(&s->io, pmbase & R_PM_IO_BASE_ADDR_MASK);
> +    memory_region_set_enabled(&s->io, FIELD_EX32(s->dev.config[A_PM_GEN_CFG1],
> +                                      PM_GEN_CFG1, ACPI_IO_ENABLE));
>     memory_region_transaction_commit();
> }
>
> static void smb_io_space_update(ViaPMState *s)
> {
> -    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
> +    uint32_t smbase = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
>
>     memory_region_transaction_begin();
> -    memory_region_set_address(&s->smb.io, smbase);
> -    memory_region_set_enabled(&s->smb.io, s->dev.config[0xd2] & BIT(0));
> +    memory_region_set_address(&s->smb.io, smbase & R_SMBUS_IO_BASE_ADDR_MASK);
> +    memory_region_set_enabled(&s->smb.io,
> +                              FIELD_EX32(s->dev.config[A_SMBUS_HOST_CONFIG],
> +                                         SMBUS_HOST_CONFIG, ENABLE));
>     memory_region_transaction_commit();
> }
>
> @@ -98,19 +112,21 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, 
> uint32_t val, int len)
>
>     trace_via_pm_write(addr, val, len);
>     pci_default_write_config(d, addr, val, len);
> -    if (ranges_overlap(addr, len, 0x48, 4)) {
> -        uint32_t v = pci_get_long(s->dev.config + 0x48);
> -        pci_set_long(s->dev.config + 0x48, (v & 0xff80UL) | 1);
> +    if (ranges_overlap(addr, len, A_PM_IO_BASE, 4)) {
> +        uint32_t v = pci_get_long(s->dev.config + A_PM_IO_BASE);
> +        pci_set_long(s->dev.config + A_PM_IO_BASE,
> +                     (v & R_PM_IO_BASE_ADDR_MASK) | 1);
>     }
> -    if (range_covers_byte(addr, len, 0x41)) {
> +    if (range_covers_byte(addr, len, A_PM_GEN_CFG1)) {
>         pm_io_space_update(s);
>     }
> -    if (ranges_overlap(addr, len, 0x90, 4)) {
> -        uint32_t v = pci_get_long(s->dev.config + 0x90);
> -        pci_set_long(s->dev.config + 0x90, (v & 0xfff0UL) | 1);
> +    if (ranges_overlap(addr, len, A_SMBUS_IO_BASE, 4)) {
> +        uint32_t v = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
> +        pci_set_long(s->dev.config + A_SMBUS_IO_BASE,
> +                     (v & R_SMBUS_IO_BASE_ADDR_MASK) | 1);
>     }
> -    if (range_covers_byte(addr, len, 0xd2)) {
> -        s->dev.config[0xd2] &= 0xf;
> +    if (range_covers_byte(addr, len, A_SMBUS_HOST_CONFIG)) {
> +        s->dev.config[A_SMBUS_HOST_CONFIG] &= 0xf;
>         smb_io_space_update(s);
>     }
> }
> @@ -176,9 +192,9 @@ static void via_pm_reset(DeviceState *d)
>     memset(s->dev.config + PCI_CONFIG_HEADER_SIZE, 0,
>            PCI_CONFIG_SPACE_SIZE - PCI_CONFIG_HEADER_SIZE);
>     /* Power Management IO base */
> -    pci_set_long(s->dev.config + 0x48, 1);
> +    pci_set_long(s->dev.config + A_PM_IO_BASE, 1);
>     /* SMBus IO base */
> -    pci_set_long(s->dev.config + 0x90, 1);
> +    pci_set_long(s->dev.config + A_SMBUS_IO_BASE, 1);
>
>     acpi_pm1_evt_reset(&s->ar);
>     acpi_pm1_cnt_reset(&s->ar);