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é <f4...@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);