These functions are memory region callbacks so we have to check
against relative address not the mapped address. Also reduce
indentation by returning early and log unimplemented accesses.
Additionally we remove separate index value from SuperIOConfig and
store the index at reg 0 which is reserved and returns 0 on read. This
simplifies object state.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 63 ++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 28 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 5df9be8ff4..2921d5c55c 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -26,6 +26,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/i2c/pm_smbus.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/range.h"
 #include "qemu/timer.h"
@@ -250,7 +251,6 @@ static const TypeInfo vt8231_pm_info = {
 
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
-    uint8_t index;
     MemoryRegion io;
 } SuperIOConfig;
 
@@ -258,42 +258,49 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
                               unsigned size)
 {
     SuperIOConfig *sc = opaque;
+    uint8_t idx = sc->regs[0];
 
-    if (addr == 0x3f0) { /* config index register */
-        sc->index = data & 0xff;
-    } else {
-        bool can_write = true;
-        /* 0x3f1, config data register */
-        trace_via_superio_write(sc->index, data & 0xff);
-        switch (sc->index) {
-        case 0x00 ... 0xdf:
-        case 0xe4:
-        case 0xe5:
-        case 0xe9 ... 0xed:
-        case 0xf3:
-        case 0xf5:
-        case 0xf7:
-        case 0xf9 ... 0xfb:
-        case 0xfd ... 0xff:
-            can_write = false;
-            break;
-        /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
-        default:
-            break;
+    if (addr == 0) { /* config index register */
+        sc->regs[0] = data;
+        return;
+    }
 
-        }
-        if (can_write) {
-            sc->regs[sc->index] = data & 0xff;
-        }
+    /* config data register */
+    trace_via_superio_write(idx, data);
+    switch (idx) {
+    case 0x00 ... 0xdf:
+    case 0xe4:
+    case 0xe5:
+    case 0xe9 ... 0xed:
+    case 0xf3:
+    case 0xf5:
+    case 0xf7:
+    case 0xf9 ... 0xfb:
+    case 0xfd ... 0xff:
+        /* ignore write to read only registers */
+        return;
+    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "via_superio_cfg: unimplemented register 0x%x\n", idx);
+        break;
     }
+    sc->regs[idx] = data;
 }
 
 static uint64_t superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
 {
     SuperIOConfig *sc = opaque;
-    uint8_t val = sc->regs[sc->index];
+    uint8_t idx = sc->regs[0];
+    uint8_t val = sc->regs[idx];
 
-    trace_via_superio_read(sc->index, val);
+    if (addr == 0) {
+        return idx;
+    }
+    if (addr == 1 && idx == 0) {
+        val = 0; /* reading reg 0 where we store index value */
+    }
+    trace_via_superio_read(idx, val);
     return val;
 }
 
-- 
2.21.3

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> These functions are memory region callbacks so we have to check
> against relative address not the mapped address. Also reduce
> indentation by returning early and log unimplemented accesses.
> Additionally we remove separate index value from SuperIOConfig and
> store the index at reg 0 which is reserved and returns 0 on read. This
> simplifies object state.

Again... Why are you putting so many changes in the same patch?

I split it in 5 distinct patches trivial to review. I probably
shouldn't spend that amount of maintainer time with your series,
but this time I did it, my bad.

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 63 ++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 28 deletions(-)

On Sat, 20 Feb 2021, Philippe Mathieu-Daudé wrote:
> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>> These functions are memory region callbacks so we have to check
>> against relative address not the mapped address. Also reduce
>> indentation by returning early and log unimplemented accesses.
>> Additionally we remove separate index value from SuperIOConfig and
>> store the index at reg 0 which is reserved and returns 0 on read. This
>> simplifies object state.
>
> Again... Why are you putting so many changes in the same patch?

I thought these changes were simple enough to include in one patch so I 
don't get a series with 25 one line patches when I already have 13 
patches. Also I've split these up after having it working and it was hard 
to separate changes after the fact even if I went through doing it again 
from scratch to create the separate patches.

> I split it in 5 distinct patches trivial to review. I probably
> shouldn't spend that amount of maintainer time with your series,
> but this time I did it, my bad.

You don't have to do that, you can just tell me in review how you want 
these split up and then I can change it accordingly. Thanks a lot for 
doing it though, I can't see how this patch could be split up more than 
intno 2-3 patches so doing it was probably faster than explaining it in 
this case.

Regards,
BALATON Zoltan

>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c | 63 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 35 insertions(+), 28 deletions(-)
>
>