[PATCH v2 00/13] vt82c686b clean ups and vt8231 emulation

[PATCH v2 00/13] vt82c686b clean ups and vt8231 emulation

[BALATON Zoltan]

Version 2 of remaining patches for VT8231 emulation addressing review
comments:

- Split off making vt82c686b-pm an abstract class to separate patch
- Use constants for PCI IDs

Regards,
BALATON Zoltan


BALATON Zoltan (13):
  vt82c686: Move superio memory region to SuperIOConfig struct
  vt82c686: Reorganise code
  vt82c686: Fix SMBus IO base and configuration registers
  vt82c686: Fix up power management io base and config
  vt82c686: Set user_creatable=false for VT82C686B_PM
  vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm
    based on it
  vt82c686: Simplify vt82c686b_realize()
  vt82c686: Move creation of ISA devices to the ISA bridge
  vt82c686: Fix superio_cfg_{read,write}() functions
  vt82c686: Implement control of serial port io ranges via config regs
  vt82c686: QOM-ify superio related functionality
  vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO
  vt82c686: Add emulation of VT8231 south bridge

 hw/isa/trace-events       |   2 +
 hw/isa/vt82c686.c         | 891 ++++++++++++++++++++++++++++----------
 hw/mips/fuloong2e.c       |  33 +-
 include/hw/isa/vt82c686.h |   3 +-
 include/hw/pci/pci_ids.h  |   6 +-
 5 files changed, 684 insertions(+), 251 deletions(-)

-- 
2.21.3

[PATCH v2 04/13] vt82c686: Fix up power management io base and config

[BALATON Zoltan]

Similar to the SMBus io registers there is a power management io range
that is set via similar base address reg and enable bit. Some handling
of this was already there but with several problems: using the wrong
registers and bits, wrong size range, not acually updating mapping and
handling reset correctly, nor emulating any of the actual io
registers. Some of these errors are fixed up here.

After this patch we use the correct base address register, enable bit
and region size and allow guests to map/unmap this region and
correctly reset all registers to default values on reset but we still
don't emulate any of the registers in this range.

Previously just an empty RAM region was mapped on realize, now we add
an empty io range logging access instead. I think the pm timer should
be hooked up here but not sure guests need it. PMON on fuloong2e sets
a base address but does not seem to enable region; the pegasos2
firmware pokes some regs but continues anyway so don't know if
anything would make use of these facilities. Therefore this is just a
clean up of previous state for now and not intending to fully
implement missing functionality which could be done later if some
guests need it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/trace-events |  2 ++
 hw/isa/vt82c686.c   | 56 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/hw/isa/trace-events b/hw/isa/trace-events
index d267d3e652..641d69eedf 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -17,5 +17,7 @@ apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
 # vt82c686.c
 via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
+via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
+via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
 via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9c4d153022..fc2a1f4430 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -39,14 +39,11 @@ struct VT686PMState {
 
 static void pm_io_space_update(VT686PMState *s)
 {
-    uint32_t pm_io_base;
-
-    pm_io_base = pci_get_long(s->dev.config + 0x40);
-    pm_io_base &= 0xffc0;
+    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
 
     memory_region_transaction_begin();
-    memory_region_set_enabled(&s->io, s->dev.config[0x80] & 1);
-    memory_region_set_address(&s->io, pm_io_base);
+    memory_region_set_address(&s->io, pmbase);
+    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));
     memory_region_transaction_commit();
 }
 
@@ -92,6 +89,13 @@ 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 (range_covers_byte(addr, len, 0x41)) {
+        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);
@@ -102,6 +106,27 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
     }
 }
 
+static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
+{
+    trace_via_pm_io_write(addr, data, size);
+}
+
+static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
+{
+    trace_via_pm_io_read(addr, 0, size);
+    return 0;
+}
+
+static const MemoryRegionOps pm_io_ops = {
+    .read = pm_io_read,
+    .write = pm_io_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void pm_update_sci(VT686PMState *s)
 {
     int sci_level, pmsts;
@@ -128,35 +153,34 @@ static void vt82c686b_pm_reset(DeviceState *d)
 {
     VT686PMState *s = VT82C686B_PM(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);
     /* SMBus IO base */
     pci_set_long(s->dev.config + 0x90, 1);
-    s->dev.config[0xd2] = 0;
 
+    pm_io_space_update(s);
     smb_io_space_update(s);
 }
 
 static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 {
     VT686PMState *s = VT82C686B_PM(dev);
-    uint8_t *pci_conf;
 
-    pci_conf = s->dev.config;
-    pci_set_word(pci_conf + PCI_COMMAND, 0);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
+    pci_set_word(dev->config + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
 
-    /* 0x48-0x4B is Power Management I/O Base */
-    pci_set_long(pci_conf + 0x48, 0x00000001);
-
     pm_smbus_init(DEVICE(s), &s->smb, false);
     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
     memory_region_set_enabled(&s->smb.io, false);
 
     apm_init(dev, &s->apm, NULL, s);
 
-    memory_region_init(&s->io, OBJECT(dev), "vt82c686-pm", 64);
+    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s,
+                          "vt82c686-pm", 0x100);
+    memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
     memory_region_set_enabled(&s->io, false);
-    memory_region_add_subregion(get_system_io(), 0, &s->io);
 
     acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
     acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
-- 
2.21.3

Re: [PATCH v2 04/13] vt82c686: Fix up power management io base and config

[Philippe Mathieu-Daudé]

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> Similar to the SMBus io registers there is a power management io range
> that is set via similar base address reg and enable bit. Some handling
> of this was already there but with several problems: using the wrong
> registers and bits, wrong size range, not acually updating mapping and
> handling reset correctly, nor emulating any of the actual io
> registers. Some of these errors are fixed up here.
> 
> After this patch we use the correct base address register, enable bit
> and region size and allow guests to map/unmap this region and
> correctly reset all registers to default values on reset but we still
> don't emulate any of the registers in this range.

^^ One change,

vv Another change.

> Previously just an empty RAM region was mapped on realize, now we add
> an empty io range logging access instead. I think the pm timer should
> be hooked up here but not sure guests need it. PMON on fuloong2e sets
> a base address but does not seem to enable region; the pegasos2
> firmware pokes some regs but continues anyway so don't know if
> anything would make use of these facilities. Therefore this is just a
> clean up of previous state for now and not intending to fully
> implement missing functionality which could be done later if some
> guests need it.

I split your patch in 2.

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/trace-events |  2 ++
>  hw/isa/vt82c686.c   | 56 ++++++++++++++++++++++++++++++++-------------
>  2 files changed, 42 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/isa/trace-events b/hw/isa/trace-events
> index d267d3e652..641d69eedf 100644
> --- a/hw/isa/trace-events
> +++ b/hw/isa/trace-events
> @@ -17,5 +17,7 @@ apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
>  # vt82c686.c
>  via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>  via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> +via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> +via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>  via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
>  via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 9c4d153022..fc2a1f4430 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -39,14 +39,11 @@ struct VT686PMState {
>  
>  static void pm_io_space_update(VT686PMState *s)
>  {
> -    uint32_t pm_io_base;
> -
> -    pm_io_base = pci_get_long(s->dev.config + 0x40);
> -    pm_io_base &= 0xffc0;
> +    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;

0x48 is Power Management I/O Base,
0xff80UL is mask for Power Management I/O Register Base Address,
OK.

>      memory_region_transaction_begin();
> -    memory_region_set_enabled(&s->io, s->dev.config[0x80] & 1);
> -    memory_region_set_address(&s->io, pm_io_base);
> +    memory_region_set_address(&s->io, pmbase);
> +    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));

0x41 is General Configuration 1,
bit 7 is I/O Enable for ACPI I/O Base,
OK.

>      memory_region_transaction_commit();
>  }
>  
> @@ -92,6 +89,13 @@ 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);

bit 0 is always set, OK.

> +    }
> +    if (range_covers_byte(addr, len, 0x41)) {

Access to General Configuration. Why not use both registers?

       if (ranges_overlap(addr, len, 0x40, 2)) {

> +        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);
> @@ -102,6 +106,27 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>      }
>  }
>  
> +static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
> +{
> +    trace_via_pm_io_write(addr, data, size);
> +}
> +
> +static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
> +{
> +    trace_via_pm_io_read(addr, 0, size);
> +    return 0;
> +}
> +
> +static const MemoryRegionOps pm_io_ops = {
> +    .read = pm_io_read,
> +    .write = pm_io_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
>  static void pm_update_sci(VT686PMState *s)
>  {
>      int sci_level, pmsts;
> @@ -128,35 +153,34 @@ static void vt82c686b_pm_reset(DeviceState *d)
>  {
>      VT686PMState *s = VT82C686B_PM(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);

bit 1 always set, OK.

>      /* SMBus IO base */
>      pci_set_long(s->dev.config + 0x90, 1);
> -    s->dev.config[0xd2] = 0;
>  
> +    pm_io_space_update(s);
>      smb_io_space_update(s);
>  }
>  
>  static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
>  {
>      VT686PMState *s = VT82C686B_PM(dev);
> -    uint8_t *pci_conf;
>  
> -    pci_conf = s->dev.config;
> -    pci_set_word(pci_conf + PCI_COMMAND, 0);
> -    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
> +    pci_set_word(dev->config + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                   PCI_STATUS_DEVSEL_MEDIUM);
>  
> -    /* 0x48-0x4B is Power Management I/O Base */
> -    pci_set_long(pci_conf + 0x48, 0x00000001);
> -
>      pm_smbus_init(DEVICE(s), &s->smb, false);
>      memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
>      memory_region_set_enabled(&s->smb.io, false);
>  
>      apm_init(dev, &s->apm, NULL, s);
>  
> -    memory_region_init(&s->io, OBJECT(dev), "vt82c686-pm", 64);
> +    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s,
> +                          "vt82c686-pm", 0x100);

Section "Configuration Space Power Management Registers" describes:

  4B-48: Power Mgmt I/O Base (256 Bytes)

Section "Offset 4B-48 – Power Management I/O Base" describes:

  Port Address for the base of the 128-byte Power
  Management I/O Register block, corresponding to
  AD[15:7].

At least we are sure 64 bytes isn't enough indeed, but is it 128 or 256?

> +    memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
>      memory_region_set_enabled(&s->io, false);
> -    memory_region_add_subregion(get_system_io(), 0, &s->io);
>  
>      acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
>      acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);

TBH it took me almost 1h to review this single patch.

Re: [PATCH v2 04/13] vt82c686: Fix up power management io base and config

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 8599 bytes --]

On Sat, 20 Feb 2021, Philippe Mathieu-Daudé wrote:
> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>> Similar to the SMBus io registers there is a power management io range
>> that is set via similar base address reg and enable bit. Some handling
>> of this was already there but with several problems: using the wrong
>> registers and bits, wrong size range, not acually updating mapping and
>> handling reset correctly, nor emulating any of the actual io
>> registers. Some of these errors are fixed up here.
>>
>> After this patch we use the correct base address register, enable bit
>> and region size and allow guests to map/unmap this region and
>> correctly reset all registers to default values on reset but we still
>> don't emulate any of the registers in this range.
>
> ^^ One change,
>
> vv Another change.

This again is just two hunks and a bit from this patch that is fairly 
trivial so I did not think that would need a separate patch but in this 
case maybe simplifies the main patch so you don't have to mentally ignore 
the pm_io_ops parts while reviewing it.

>> Previously just an empty RAM region was mapped on realize, now we add
>> an empty io range logging access instead. I think the pm timer should
>> be hooked up here but not sure guests need it. PMON on fuloong2e sets
>> a base address but does not seem to enable region; the pegasos2
>> firmware pokes some regs but continues anyway so don't know if
>> anything would make use of these facilities. Therefore this is just a
>> clean up of previous state for now and not intending to fully
>> implement missing functionality which could be done later if some
>> guests need it.
>
> I split your patch in 2.

Thanks again.

>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/trace-events |  2 ++
>>  hw/isa/vt82c686.c   | 56 ++++++++++++++++++++++++++++++++-------------
>>  2 files changed, 42 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/isa/trace-events b/hw/isa/trace-events
>> index d267d3e652..641d69eedf 100644
>> --- a/hw/isa/trace-events
>> +++ b/hw/isa/trace-events
>> @@ -17,5 +17,7 @@ apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
>>  # vt82c686.c
>>  via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>>  via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>> +via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>> +via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
>>  via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
>>  via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 9c4d153022..fc2a1f4430 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -39,14 +39,11 @@ struct VT686PMState {
>>
>>  static void pm_io_space_update(VT686PMState *s)
>>  {
>> -    uint32_t pm_io_base;
>> -
>> -    pm_io_base = pci_get_long(s->dev.config + 0x40);
>> -    pm_io_base &= 0xffc0;
>> +    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
>
> 0x48 is Power Management I/O Base,
> 0xff80UL is mask for Power Management I/O Register Base Address,
> OK.
>
>>      memory_region_transaction_begin();
>> -    memory_region_set_enabled(&s->io, s->dev.config[0x80] & 1);
>> -    memory_region_set_address(&s->io, pm_io_base);
>> +    memory_region_set_address(&s->io, pmbase);
>> +    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));
>
> 0x41 is General Configuration 1,
> bit 7 is I/O Enable for ACPI I/O Base,
> OK.
>
>>      memory_region_transaction_commit();
>>  }
>>
>> @@ -92,6 +89,13 @@ 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);
>
> bit 0 is always set, OK.
>
>> +    }
>> +    if (range_covers_byte(addr, len, 0x41)) {
>
> Access to General Configuration. Why not use both registers?
>
>       if (ranges_overlap(addr, len, 0x40, 2)) {

Hmm, not sure, I've forgotten by now. I think we only have to do anything 
if the memory region is enabled or disabled which is the bit in 0x41 so if 
other parts of the reg are written we don't have to call 
pm_io_space_update(). We just store the value in the reg in that case but 
only need to update the region when the enable bit is changed.

>> +        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);
>> @@ -102,6 +106,27 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>>      }
>>  }
>>
>> +static void pm_io_write(void *op, hwaddr addr, uint64_t data, unsigned size)
>> +{
>> +    trace_via_pm_io_write(addr, data, size);
>> +}
>> +
>> +static uint64_t pm_io_read(void *op, hwaddr addr, unsigned size)
>> +{
>> +    trace_via_pm_io_read(addr, 0, size);
>> +    return 0;
>> +}
>> +
>> +static const MemoryRegionOps pm_io_ops = {
>> +    .read = pm_io_read,
>> +    .write = pm_io_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
>> +    },
>> +};
>> +
>>  static void pm_update_sci(VT686PMState *s)
>>  {
>>      int sci_level, pmsts;
>> @@ -128,35 +153,34 @@ static void vt82c686b_pm_reset(DeviceState *d)
>>  {
>>      VT686PMState *s = VT82C686B_PM(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);
>
> bit 1 always set, OK.
>
>>      /* SMBus IO base */
>>      pci_set_long(s->dev.config + 0x90, 1);
>> -    s->dev.config[0xd2] = 0;
>>
>> +    pm_io_space_update(s);
>>      smb_io_space_update(s);
>>  }
>>
>>  static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
>>  {
>>      VT686PMState *s = VT82C686B_PM(dev);
>> -    uint8_t *pci_conf;
>>
>> -    pci_conf = s->dev.config;
>> -    pci_set_word(pci_conf + PCI_COMMAND, 0);
>> -    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>> +    pci_set_word(dev->config + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>                   PCI_STATUS_DEVSEL_MEDIUM);
>>
>> -    /* 0x48-0x4B is Power Management I/O Base */
>> -    pci_set_long(pci_conf + 0x48, 0x00000001);
>> -
>>      pm_smbus_init(DEVICE(s), &s->smb, false);
>>      memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
>>      memory_region_set_enabled(&s->smb.io, false);
>>
>>      apm_init(dev, &s->apm, NULL, s);
>>
>> -    memory_region_init(&s->io, OBJECT(dev), "vt82c686-pm", 64);
>> +    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s,
>> +                          "vt82c686-pm", 0x100);
>
> Section "Configuration Space Power Management Registers" describes:
>
>  4B-48: Power Mgmt I/O Base (256 Bytes)
>
> Section "Offset 4B-48 – Power Management I/O Base" describes:
>
>  Port Address for the base of the 128-byte Power
>  Management I/O Register block, corresponding to
>  AD[15:7].
>
> At least we are sure 64 bytes isn't enough indeed, but is it 128 or 256?

The regs in this range for 82c686b go up to 0x4C so it's definitely over 
64 but seems to be less than 128. The vt8231 docs also say 128 byte here 
so maybe this was a typo and 128 bytes length would be enough.

>> +    memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
>>      memory_region_set_enabled(&s->io, false);
>> -    memory_region_add_subregion(get_system_io(), 0, &s->io);
>>
>>      acpi_pm_tmr_init(&s->ar, pm_tmr_timer, &s->io);
>>      acpi_pm1_evt_init(&s->ar, pm_tmr_timer, &s->io);
>
> TBH it took me almost 1h to review this single patch.

Yes, sorry but these kinds of patches need reading the chip docs which 
takes some time. It took me a few days to write this simgle patch though 
because I had to learn how the chip works and understand the code already 
there and testing with guests so reviewing it is still less than that. ;-)

Thanks for your efforts, I hope to get this series in so I can submit the 
remaining ones adding pegasos2 emulation so it can get in for the next 
release. Let me know if I can help your review by whatever changes you 
would need in other patches.

Regards,
BALATON Zoltan

[PATCH v2 02/13] vt82c686: Reorganise code

[BALATON Zoltan]

Move lines around so that object definitions become consecutive and
not scattered around. This brings functions belonging to an object
together so it's clearer what is defined and what parts belong to
which object.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 279 +++++++++++++++++++++++-----------------------
 1 file changed, 140 insertions(+), 139 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 30fe02f4c6..fe8961b057 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -26,112 +26,7 @@
 #include "exec/address-spaces.h"
 #include "trace.h"
 
-typedef struct SuperIOConfig {
-    uint8_t regs[0x100];
-    uint8_t index;
-    MemoryRegion io;
-} SuperIOConfig;
-
-struct VT82C686BISAState {
-    PCIDevice dev;
-    SuperIOConfig superio_cfg;
-};
-
-OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
-
-static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
-                              unsigned size)
-{
-    SuperIOConfig *sc = opaque;
-
-    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 (can_write) {
-            sc->regs[sc->index] = data & 0xff;
-        }
-    }
-}
-
-static uint64_t superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
-{
-    SuperIOConfig *sc = opaque;
-    uint8_t val = sc->regs[sc->index];
-
-    trace_via_superio_read(sc->index, val);
-    return val;
-}
-
-static const MemoryRegionOps superio_cfg_ops = {
-    .read = superio_cfg_read,
-    .write = superio_cfg_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 1,
-    },
-};
-
-static void vt82c686b_isa_reset(DeviceState *dev)
-{
-    VT82C686BISAState *s = VT82C686B_ISA(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
-                 PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
-
-    pci_conf[0x48] = 0x01; /* Miscellaneous Control 3 */
-    pci_conf[0x4a] = 0x04; /* IDE interrupt Routing */
-    pci_conf[0x4f] = 0x03; /* DMA/Master Mem Access Control 3 */
-    pci_conf[0x50] = 0x2d; /* PnP DMA Request Control */
-    pci_conf[0x59] = 0x04;
-    pci_conf[0x5a] = 0x04; /* KBC/RTC Control*/
-    pci_conf[0x5f] = 0x04;
-    pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
-
-    s->superio_cfg.regs[0xe0] = 0x3c; /* Device ID */
-    s->superio_cfg.regs[0xe2] = 0x03; /* Function select */
-    s->superio_cfg.regs[0xe3] = 0xfc; /* Floppy ctrl base addr */
-    s->superio_cfg.regs[0xe6] = 0xde; /* Parallel port base addr */
-    s->superio_cfg.regs[0xe7] = 0xfe; /* Serial port 1 base addr */
-    s->superio_cfg.regs[0xe8] = 0xbe; /* Serial port 2 base addr */
-}
-
-/* write config pci function0 registers. PCI-ISA bridge */
-static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
-                                   uint32_t val, int len)
-{
-    VT82C686BISAState *s = VT82C686B_ISA(d);
-
-    trace_via_isa_write(addr, val, len);
-    pci_default_write_config(d, addr, val, len);
-    if (addr == 0x85) {
-        /* BIT(1): enable or disable superio config io ports */
-        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
-    }
-}
+OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
 
 struct VT686PMState {
     PCIDevice dev;
@@ -142,30 +37,6 @@ struct VT686PMState {
     uint32_t smb_io_base;
 };
 
-OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
-
-static void pm_update_sci(VT686PMState *s)
-{
-    int sci_level, pmsts;
-
-    pmsts = acpi_pm1_evt_get_sts(&s->ar);
-    sci_level = (((pmsts & s->ar.pm1.evt.en) &
-                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
-                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
-                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
-                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
-    pci_set_irq(&s->dev, sci_level);
-    /* schedule a timer interruption if needed */
-    acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
-                       !(pmsts & ACPI_BITMASK_TIMER_STATUS));
-}
-
-static void pm_tmr_timer(ACPIREGS *ar)
-{
-    VT686PMState *s = container_of(ar, VT686PMState, ar);
-    pm_update_sci(s);
-}
-
 static void pm_io_space_update(VT686PMState *s)
 {
     uint32_t pm_io_base;
@@ -179,12 +50,6 @@ static void pm_io_space_update(VT686PMState *s)
     memory_region_transaction_commit();
 }
 
-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);
-}
-
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
     VT686PMState *s = opaque;
@@ -210,7 +75,34 @@ static const VMStateDescription vmstate_acpi = {
     }
 };
 
-/* vt82c686 pm init */
+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);
+}
+
+static void pm_update_sci(VT686PMState *s)
+{
+    int sci_level, pmsts;
+
+    pmsts = acpi_pm1_evt_get_sts(&s->ar);
+    sci_level = (((pmsts & s->ar.pm1.evt.en) &
+                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
+                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
+                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
+                   ACPI_BITMASK_TIMER_ENABLE)) != 0);
+    pci_set_irq(&s->dev, sci_level);
+    /* schedule a timer interruption if needed */
+    acpi_pm_tmr_update(&s->ar, (s->ar.pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
+                       !(pmsts & ACPI_BITMASK_TIMER_STATUS));
+}
+
+static void pm_tmr_timer(ACPIREGS *ar)
+{
+    VT686PMState *s = container_of(ar, VT686PMState, ar);
+    pm_update_sci(s);
+}
+
 static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 {
     VT686PMState *s = VT82C686B_PM(dev);
@@ -276,6 +168,87 @@ static const TypeInfo via_pm_info = {
     },
 };
 
+
+typedef struct SuperIOConfig {
+    uint8_t regs[0x100];
+    uint8_t index;
+    MemoryRegion io;
+} SuperIOConfig;
+
+static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
+                              unsigned size)
+{
+    SuperIOConfig *sc = opaque;
+
+    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 (can_write) {
+            sc->regs[sc->index] = data & 0xff;
+        }
+    }
+}
+
+static uint64_t superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
+{
+    SuperIOConfig *sc = opaque;
+    uint8_t val = sc->regs[sc->index];
+
+    trace_via_superio_read(sc->index, val);
+    return val;
+}
+
+static const MemoryRegionOps superio_cfg_ops = {
+    .read = superio_cfg_read,
+    .write = superio_cfg_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+
+OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
+
+struct VT82C686BISAState {
+    PCIDevice dev;
+    SuperIOConfig superio_cfg;
+};
+
+static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
+                                   uint32_t val, int len)
+{
+    VT82C686BISAState *s = VT82C686B_ISA(d);
+
+    trace_via_isa_write(addr, val, len);
+    pci_default_write_config(d, addr, val, len);
+    if (addr == 0x85) {
+        /* BIT(1): enable or disable superio config io ports */
+        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
+    }
+}
+
 static const VMStateDescription vmstate_via = {
     .name = "vt82c686b",
     .version_id = 1,
@@ -286,7 +259,33 @@ static const VMStateDescription vmstate_via = {
     }
 };
 
-/* init the PCI-to-ISA bridge */
+static void vt82c686b_isa_reset(DeviceState *dev)
+{
+    VT82C686BISAState *s = VT82C686B_ISA(dev);
+    uint8_t *pci_conf = s->dev.config;
+
+    pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
+                 PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
+
+    pci_conf[0x48] = 0x01; /* Miscellaneous Control 3 */
+    pci_conf[0x4a] = 0x04; /* IDE interrupt Routing */
+    pci_conf[0x4f] = 0x03; /* DMA/Master Mem Access Control 3 */
+    pci_conf[0x50] = 0x2d; /* PnP DMA Request Control */
+    pci_conf[0x59] = 0x04;
+    pci_conf[0x5a] = 0x04; /* KBC/RTC Control*/
+    pci_conf[0x5f] = 0x04;
+    pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
+
+    s->superio_cfg.regs[0xe0] = 0x3c; /* Device ID */
+    s->superio_cfg.regs[0xe2] = 0x03; /* Function select */
+    s->superio_cfg.regs[0xe3] = 0xfc; /* Floppy ctrl base addr */
+    s->superio_cfg.regs[0xe6] = 0xde; /* Parallel port base addr */
+    s->superio_cfg.regs[0xe7] = 0xfe; /* Serial port 1 base addr */
+    s->superio_cfg.regs[0xe8] = 0xbe; /* Serial port 2 base addr */
+}
+
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
     VT82C686BISAState *s = VT82C686B_ISA(d);
@@ -354,6 +353,7 @@ static const TypeInfo via_info = {
     },
 };
 
+
 static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
 {
     ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
@@ -372,11 +372,12 @@ static const TypeInfo via_superio_info = {
     .class_init    = vt82c686b_superio_class_init,
 };
 
+
 static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
-    type_register_static(&via_superio_info);
     type_register_static(&via_info);
+    type_register_static(&via_superio_info);
 }
 
 type_init(vt82c686b_register_types)
-- 
2.21.3

[PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

Currently the ISA devices that are part of the VIA south bridge,
superio chip are wired up by board code. Move creation of these ISA
devices to the VIA ISA bridge model so that board code does not need
to access ISA bus. This also allows vt82c686b-superio to be made
internal to vt82c686 which allows implementing its configuration via
registers in subseqent commits.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
 hw/mips/fuloong2e.c | 29 +++++------------------------
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 58c0bba1d0..5df9be8ff4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -16,6 +16,11 @@
 #include "hw/qdev-properties.h"
 #include "hw/isa/isa.h"
 #include "hw/isa/superio.h"
+#include "hw/intc/i8259.h"
+#include "hw/irq.h"
+#include "hw/dma/i8257.h"
+#include "hw/timer/i8254.h"
+#include "hw/rtc/mc146818rtc.h"
 #include "migration/vmstate.h"
 #include "hw/isa/apm.h"
 #include "hw/acpi/acpi.h"
@@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
 
 struct VT82C686BISAState {
     PCIDevice dev;
+    qemu_irq cpu_intr;
     SuperIOConfig superio_cfg;
 };
 
+static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
+{
+    VT82C686BISAState *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
 static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
@@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     VT82C686BISAState *s = VT82C686B_ISA(d);
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
+    qemu_irq *isa_irq;
     int i;
 
+    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
                           &error_fatal);
+    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+    i8254_pit_init(isa_bus, 0x40, 0, NULL);
+    i8257_dma_init(isa_bus, 0);
+    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
+    mc146818_rtc_init(isa_bus, 2000, NULL);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
         if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index fbdd6122b3..0fc3288556 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -25,9 +25,6 @@
 #include "qapi/error.h"
 #include "cpu.h"
 #include "hw/clock.h"
-#include "hw/intc/i8259.h"
-#include "hw/dma/i8257.h"
-#include "hw/isa/superio.h"
 #include "net/net.h"
 #include "hw/boards.h"
 #include "hw/i2c/smbus_eeprom.h"
@@ -38,13 +35,13 @@
 #include "qemu/log.h"
 #include "hw/loader.h"
 #include "hw/ide/pci.h"
+#include "hw/qdev-properties.h"
 #include "elf.h"
 #include "hw/isa/vt82c686.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "hw/timer/i8254.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
 #include "sysemu/reset.h"
+#include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
 
 #define ENVP_PADDR              0x2000
@@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque)
 }
 
 static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
-                                       I2CBus **i2c_bus, ISABus **p_isa_bus)
+                                       I2CBus **i2c_bus)
 {
-    qemu_irq *i8259;
-    ISABus *isa_bus;
     PCIDevice *dev;
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
-    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
-    assert(isa_bus);
-    *p_isa_bus = isa_bus;
-    /* Interrupt controller */
-    /* The 8259 -> IP5  */
-    i8259 = i8259_init(isa_bus, intc);
-    isa_bus_irqs(isa_bus, i8259);
-    /* init other devices */
-    i8254_pit_init(isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(isa_bus, 0);
-    /* Super I/O */
-    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
+    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
     pci_ide_create_devs(dev);
@@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine)
     uint64_t kernel_entry;
     PCIDevice *pci_dev;
     PCIBus *pci_bus;
-    ISABus *isa_bus;
     I2CBus *smbus;
     Clock *cpuclk;
     MIPSCPU *cpu;
@@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine)
 
     /* South bridge -> IP5 */
     vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
-                               &smbus, &isa_bus);
+                               &smbus);
 
     /* GPU */
     if (vga_interface_type != VGA_NONE) {
@@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine)
     spd_data = spd_data_generate(DDR, machine->ram_size);
     smbus_eeprom_init_one(smbus, 0x50, spd_data);
 
-    mc146818_rtc_init(isa_bus, 2000, NULL);
-
     /* Network card: RTL8139D */
     network_init(pci_bus);
 }
-- 
2.21.3

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> Currently the ISA devices that are part of the VIA south bridge,
> superio chip are wired up by board code. Move creation of these ISA
> devices to the VIA ISA bridge model so that board code does not need
> to access ISA bus. This also allows vt82c686b-superio to be made
> internal to vt82c686 which allows implementing its configuration via
> registers in subseqent commits.

Is this patch dependent of the VT82C686B_PM changes
or can it be applied before them?

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>  hw/mips/fuloong2e.c | 29 +++++------------------------
>  2 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 58c0bba1d0..5df9be8ff4 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -16,6 +16,11 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/isa/isa.h"
>  #include "hw/isa/superio.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/irq.h"
> +#include "hw/dma/i8257.h"
> +#include "hw/timer/i8254.h"
> +#include "hw/rtc/mc146818rtc.h"
>  #include "migration/vmstate.h"
>  #include "hw/isa/apm.h"
>  #include "hw/acpi/acpi.h"
> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
>  
>  struct VT82C686BISAState {
>      PCIDevice dev;
> +    qemu_irq cpu_intr;
>      SuperIOConfig superio_cfg;
>  };
>  
> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> +{
> +    VT82C686BISAState *s = opaque;
> +    qemu_set_irq(s->cpu_intr, level);
> +}
> +
>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>                                     uint32_t val, int len)
>  {
> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      VT82C686BISAState *s = VT82C686B_ISA(d);
>      DeviceState *dev = DEVICE(d);
>      ISABus *isa_bus;
> +    qemu_irq *isa_irq;
>      int i;
>  
> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);

Why not use the SysBus API?

> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>      isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
>                            &error_fatal);

Isn't it get_system_memory() -> pci_address_space(d)?

> +    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> +    i8257_dma_init(isa_bus, 0);
> +    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    mc146818_rtc_init(isa_bus, 2000, NULL);
>  
>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>          if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index fbdd6122b3..0fc3288556 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -25,9 +25,6 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/clock.h"
> -#include "hw/intc/i8259.h"
> -#include "hw/dma/i8257.h"
> -#include "hw/isa/superio.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
>  #include "hw/i2c/smbus_eeprom.h"
> @@ -38,13 +35,13 @@
>  #include "qemu/log.h"
>  #include "hw/loader.h"
>  #include "hw/ide/pci.h"
> +#include "hw/qdev-properties.h"
>  #include "elf.h"
>  #include "hw/isa/vt82c686.h"
> -#include "hw/rtc/mc146818rtc.h"
> -#include "hw/timer/i8254.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/reset.h"
> +#include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
>  
>  #define ENVP_PADDR              0x2000
> @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque)
>  }
>  
>  static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
> -                                       I2CBus **i2c_bus, ISABus **p_isa_bus)
> +                                       I2CBus **i2c_bus)
>  {
> -    qemu_irq *i8259;
> -    ISABus *isa_bus;
>      PCIDevice *dev;
>  
>      dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>                                            TYPE_VT82C686B_ISA);
> -    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
> -    assert(isa_bus);
> -    *p_isa_bus = isa_bus;
> -    /* Interrupt controller */
> -    /* The 8259 -> IP5  */
> -    i8259 = i8259_init(isa_bus, intc);
> -    isa_bus_irqs(isa_bus, i8259);
> -    /* init other devices */
> -    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
> -    /* Super I/O */
> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>  
>      dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>      pci_ide_create_devs(dev);
> @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>      uint64_t kernel_entry;
>      PCIDevice *pci_dev;
>      PCIBus *pci_bus;
> -    ISABus *isa_bus;
>      I2CBus *smbus;
>      Clock *cpuclk;
>      MIPSCPU *cpu;
> @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>  
>      /* South bridge -> IP5 */
>      vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
> -                               &smbus, &isa_bus);
> +                               &smbus);
>  
>      /* GPU */
>      if (vga_interface_type != VGA_NONE) {
> @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>      spd_data = spd_data_generate(DDR, machine->ram_size);
>      smbus_eeprom_init_one(smbus, 0x50, spd_data);
>  
> -    mc146818_rtc_init(isa_bus, 2000, NULL);
> -
>      /* Network card: RTL8139D */
>      network_init(pci_bus);
>  }
> 

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 6310 bytes --]

On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>> Currently the ISA devices that are part of the VIA south bridge,
>> superio chip are wired up by board code. Move creation of these ISA
>> devices to the VIA ISA bridge model so that board code does not need
>> to access ISA bus. This also allows vt82c686b-superio to be made
>> internal to vt82c686 which allows implementing its configuration via
>> registers in subseqent commits.
>
> Is this patch dependent of the VT82C686B_PM changes
> or can it be applied before them?

I don't know but why would that be better? I thought it's clearer to clean 
up pm related parts first before moving more stuff to this file so that's 
why this patch comes after (and also because that's the order I did it).

>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 58c0bba1d0..5df9be8ff4 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -16,6 +16,11 @@
>>  #include "hw/qdev-properties.h"
>>  #include "hw/isa/isa.h"
>>  #include "hw/isa/superio.h"
>> +#include "hw/intc/i8259.h"
>> +#include "hw/irq.h"
>> +#include "hw/dma/i8257.h"
>> +#include "hw/timer/i8254.h"
>> +#include "hw/rtc/mc146818rtc.h"
>>  #include "migration/vmstate.h"
>>  #include "hw/isa/apm.h"
>>  #include "hw/acpi/acpi.h"
>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
>>
>>  struct VT82C686BISAState {
>>      PCIDevice dev;
>> +    qemu_irq cpu_intr;
>>      SuperIOConfig superio_cfg;
>>  };
>>
>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>> +{
>> +    VT82C686BISAState *s = opaque;
>> +    qemu_set_irq(s->cpu_intr, level);
>> +}
>> +
>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>                                     uint32_t val, int len)
>>  {
>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>      DeviceState *dev = DEVICE(d);
>>      ISABus *isa_bus;
>> +    qemu_irq *isa_irq;
>>      int i;
>>
>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>
> Why not use the SysBus API?

How? This is a PCIDevice not a SysBusDevice.

>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>      isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
>>                            &error_fatal);
>
> Isn't it get_system_memory() -> pci_address_space(d)?

I don't really know. Most other places that create an isa bus seem to also 
use get_system_memory(), only piix4 uses pci_address_space(dev) so I 
thought if those others are OK this should be too.

Regards,
BALATON Zoltan

>> +    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
>> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>> +    i8257_dma_init(isa_bus, 0);
>> +    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>> +    mc146818_rtc_init(isa_bus, 2000, NULL);
>>
>>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>>          if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index fbdd6122b3..0fc3288556 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -25,9 +25,6 @@
>>  #include "qapi/error.h"
>>  #include "cpu.h"
>>  #include "hw/clock.h"
>> -#include "hw/intc/i8259.h"
>> -#include "hw/dma/i8257.h"
>> -#include "hw/isa/superio.h"
>>  #include "net/net.h"
>>  #include "hw/boards.h"
>>  #include "hw/i2c/smbus_eeprom.h"
>> @@ -38,13 +35,13 @@
>>  #include "qemu/log.h"
>>  #include "hw/loader.h"
>>  #include "hw/ide/pci.h"
>> +#include "hw/qdev-properties.h"
>>  #include "elf.h"
>>  #include "hw/isa/vt82c686.h"
>> -#include "hw/rtc/mc146818rtc.h"
>> -#include "hw/timer/i8254.h"
>>  #include "exec/address-spaces.h"
>>  #include "sysemu/qtest.h"
>>  #include "sysemu/reset.h"
>> +#include "sysemu/sysemu.h"
>>  #include "qemu/error-report.h"
>>
>>  #define ENVP_PADDR              0x2000
>> @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque)
>>  }
>>
>>  static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>> -                                       I2CBus **i2c_bus, ISABus **p_isa_bus)
>> +                                       I2CBus **i2c_bus)
>>  {
>> -    qemu_irq *i8259;
>> -    ISABus *isa_bus;
>>      PCIDevice *dev;
>>
>>      dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>>                                            TYPE_VT82C686B_ISA);
>> -    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
>> -    assert(isa_bus);
>> -    *p_isa_bus = isa_bus;
>> -    /* Interrupt controller */
>> -    /* The 8259 -> IP5  */
>> -    i8259 = i8259_init(isa_bus, intc);
>> -    isa_bus_irqs(isa_bus, i8259);
>> -    /* init other devices */
>> -    i8254_pit_init(isa_bus, 0x40, 0, NULL);
>> -    i8257_dma_init(isa_bus, 0);
>> -    /* Super I/O */
>> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>> +    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>>
>>      dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>>      pci_ide_create_devs(dev);
>> @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>>      uint64_t kernel_entry;
>>      PCIDevice *pci_dev;
>>      PCIBus *pci_bus;
>> -    ISABus *isa_bus;
>>      I2CBus *smbus;
>>      Clock *cpuclk;
>>      MIPSCPU *cpu;
>> @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>>
>>      /* South bridge -> IP5 */
>>      vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
>> -                               &smbus, &isa_bus);
>> +                               &smbus);
>>
>>      /* GPU */
>>      if (vga_interface_type != VGA_NONE) {
>> @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>>      spd_data = spd_data_generate(DDR, machine->ram_size);
>>      smbus_eeprom_init_one(smbus, 0x50, spd_data);
>>
>> -    mc146818_rtc_init(isa_bus, 2000, NULL);
>> -
>>      /* Network card: RTL8139D */
>>      network_init(pci_bus);
>>  }
>>
>
>

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Philippe Mathieu-Daudé]

+PCI experts

On 1/10/21 1:43 AM, BALATON Zoltan wrote:
> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
>>
>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>> Currently the ISA devices that are part of the VIA south bridge,
>>> superio chip are wired up by board code. Move creation of these ISA
>>> devices to the VIA ISA bridge model so that board code does not need
>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>> internal to vt82c686 which allows implementing its configuration via
>>> registers in subseqent commits.
>>
>> Is this patch dependent of the VT82C686B_PM changes
>> or can it be applied before them?
> 
> I don't know but why would that be better? I thought it's clearer to
> clean up pm related parts first before moving more stuff to this file so
> that's why this patch comes after (and also because that's the order I
> did it).

Not any better, but easier for me to get your patches integrated,
as I'm reviewing your patches slowly. Finding other reviewers
would certainly help.

>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 58c0bba1d0..5df9be8ff4 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -16,6 +16,11 @@
>>>  #include "hw/qdev-properties.h"
>>>  #include "hw/isa/isa.h"
>>>  #include "hw/isa/superio.h"
>>> +#include "hw/intc/i8259.h"
>>> +#include "hw/irq.h"
>>> +#include "hw/dma/i8257.h"
>>> +#include "hw/timer/i8254.h"
>>> +#include "hw/rtc/mc146818rtc.h"
>>>  #include "migration/vmstate.h"
>>>  #include "hw/isa/apm.h"
>>>  #include "hw/acpi/acpi.h"
>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>> VT82C686B_ISA)
>>>
>>>  struct VT82C686BISAState {
>>>      PCIDevice dev;
>>> +    qemu_irq cpu_intr;
>>>      SuperIOConfig superio_cfg;
>>>  };
>>>
>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>>> +{
>>> +    VT82C686BISAState *s = opaque;
>>> +    qemu_set_irq(s->cpu_intr, level);
>>> +}
>>> +
>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>                                     uint32_t val, int len)
>>>  {
>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>> Error **errp)
>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>      DeviceState *dev = DEVICE(d);
>>>      ISABus *isa_bus;
>>> +    qemu_irq *isa_irq;
>>>      int i;
>>>
>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>
>> Why not use the SysBus API?
> 
> How? This is a PCIDevice not a SysBusDevice.

Indeed :)

>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>> pci_address_space_io(d),
>>>                            &error_fatal);
>>
>> Isn't it get_system_memory() -> pci_address_space(d)?
> 
> I don't really know. Most other places that create an isa bus seem to
> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
> I thought if those others are OK this should be too.

I'm not a PCI expert but my understanding is PCI device functions are
restricted to the PCI bus address space. The host bridge may map this
space within the host.

QEMU might be using get_system_memory() because for some host bridge
the mapping is not implemented so it was easier this way?

Regards,

Phil.

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4703 bytes --]

On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
> +PCI experts
>
> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>> Hi Zoltan,
>>>
>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>> superio chip are wired up by board code. Move creation of these ISA
>>>> devices to the VIA ISA bridge model so that board code does not need
>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>> internal to vt82c686 which allows implementing its configuration via
>>>> registers in subseqent commits.
>>>
>>> Is this patch dependent of the VT82C686B_PM changes
>>> or can it be applied before them?
>>
>> I don't know but why would that be better? I thought it's clearer to
>> clean up pm related parts first before moving more stuff to this file so
>> that's why this patch comes after (and also because that's the order I
>> did it).
>
> Not any better, but easier for me to get your patches integrated,
> as I'm reviewing your patches slowly. Finding other reviewers
> would certainly help.

No problem, I'll wait for your review. Merging parts of the series does 
not help much because the whole series is needed for vt8231 which is 
prerequisite for pegasos2 so eventually all of these are needed so it does 
not matter if this one patch gets in earlier or later.

Not sure who could help with review. Maybe Jiaxun or Huacai as this is 
used by fuloong2e so they might be interested and could have info on this 
chip. Most of these patches just cleaning up the vt82c686b and adding some 
missing features so these can be reused by the vt8231 model in last 3 
patches (which is very similar to 686b only some reg addresses and ids 
seem to be different for what we are concerned).

>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -16,6 +16,11 @@
>>>>  #include "hw/qdev-properties.h"
>>>>  #include "hw/isa/isa.h"
>>>>  #include "hw/isa/superio.h"
>>>> +#include "hw/intc/i8259.h"
>>>> +#include "hw/irq.h"
>>>> +#include "hw/dma/i8257.h"
>>>> +#include "hw/timer/i8254.h"
>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>  #include "migration/vmstate.h"
>>>>  #include "hw/isa/apm.h"
>>>>  #include "hw/acpi/acpi.h"
>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>> VT82C686B_ISA)
>>>>
>>>>  struct VT82C686BISAState {
>>>>      PCIDevice dev;
>>>> +    qemu_irq cpu_intr;
>>>>      SuperIOConfig superio_cfg;
>>>>  };
>>>>
>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>>>> +{
>>>> +    VT82C686BISAState *s = opaque;
>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>> +}
>>>> +
>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>                                     uint32_t val, int len)
>>>>  {
>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>> Error **errp)
>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>      DeviceState *dev = DEVICE(d);
>>>>      ISABus *isa_bus;
>>>> +    qemu_irq *isa_irq;
>>>>      int i;
>>>>
>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>
>>> Why not use the SysBus API?
>>
>> How? This is a PCIDevice not a SysBusDevice.
>
> Indeed :)
>
>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>> pci_address_space_io(d),
>>>>                            &error_fatal);
>>>
>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>
>> I don't really know. Most other places that create an isa bus seem to
>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
>> I thought if those others are OK this should be too.
>
> I'm not a PCI expert but my understanding is PCI device functions are
> restricted to the PCI bus address space. The host bridge may map this
> space within the host.
>
> QEMU might be using get_system_memory() because for some host bridge
> the mapping is not implemented so it was easier this way?

Maybe, also one less indirection which if not really needed is a good 
thing for performance so unless it's found to be needed to use another 
address space here I'm happy with this as it matches what other similar 
devices do and it seems to work. Maybe a separate address space is only 
really needed if we have an iommu?

Regards,
BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Jiaxun Yang]

On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote:
> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
> > +PCI experts
> >
> > On 1/10/21 1:43 AM, BALATON Zoltan wrote:
> >> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:

[...]

> > I'm not a PCI expert but my understanding is PCI device functions are
> > restricted to the PCI bus address space. The host bridge may map this
> > space within the host.
> >
> > QEMU might be using get_system_memory() because for some host bridge
> > the mapping is not implemented so it was easier this way?
> 
> Maybe, also one less indirection which if not really needed is a good 
> thing for performance so unless it's found to be needed to use another 
> address space here I'm happy with this as it matches what other similar 
> devices do and it seems to work. Maybe a separate address space is only 
> really needed if we have an iommu?

Hi Zoltan,

It is possible for bonito to remap PCI address space so maybe it's essential for bonito.

Appreciate for your work. I'm going to help with reviewing as well.

> 
> Regards,
> BALATON Zoltan

-- 
- Jiaxun

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

On Mon, 11 Jan 2021, Jiaxun Yang wrote:
> On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote:
>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>> +PCI experts
>>>
>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>
> [...]
>
>>> I'm not a PCI expert but my understanding is PCI device functions are
>>> restricted to the PCI bus address space. The host bridge may map this
>>> space within the host.
>>>
>>> QEMU might be using get_system_memory() because for some host bridge
>>> the mapping is not implemented so it was easier this way?
>>
>> Maybe, also one less indirection which if not really needed is a good
>> thing for performance so unless it's found to be needed to use another
>> address space here I'm happy with this as it matches what other similar
>> devices do and it seems to work. Maybe a separate address space is only
>> really needed if we have an iommu?
>
> Hi Zoltan,
>
> It is possible for bonito to remap PCI address space so maybe it's essential for bonito.

I'm still not sure it's needed or how that would work. Maybe while it's 
possible to remap these, all guests just map these one-to-one (otherwise 
they may need to do some address translation which is much better avoided) 
so in practice we don't need to emulate this. If we use a different memory 
region maybe we also need some additional iommu code somewhere to connect 
the two but I'm not sure, I haven't tried since most other isa bridges do 
the same way using system_memory and this seems to work. I'm also a bit 
concerned about additional overhead which we could avoid if possible. Just 
to model something that's not really used I don't think it's worth the 
additional complexity and possible performance loss unless it's found to 
be needed to get some guests work.

> Appreciate for your work. I'm going to help with reviewing as well.

Thanks, I hope to get these in now before the freeze so testing and 
reviewing is really appreciated.

Regards,
BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Philippe Mathieu-Daudé]

On 1/11/21 11:28 AM, BALATON Zoltan wrote:
> On Mon, 11 Jan 2021, Jiaxun Yang wrote:
>> On Mon, Jan 11, 2021, at 3:25 AM, BALATON Zoltan wrote:
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> +PCI experts
>>>>
>>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>
>> [...]
>>
>>>> I'm not a PCI expert but my understanding is PCI device functions are
>>>> restricted to the PCI bus address space. The host bridge may map this
>>>> space within the host.
>>>>
>>>> QEMU might be using get_system_memory() because for some host bridge
>>>> the mapping is not implemented so it was easier this way?
>>>
>>> Maybe, also one less indirection which if not really needed is a good
>>> thing for performance so unless it's found to be needed to use another
>>> address space here I'm happy with this as it matches what other similar
>>> devices do and it seems to work. Maybe a separate address space is only
>>> really needed if we have an iommu?
>>
>> Hi Zoltan,
>>
>> It is possible for bonito to remap PCI address space so maybe it's
>> essential for bonito.
> 
> I'm still not sure it's needed or how that would work. Maybe while it's
> possible to remap these, all guests just map these one-to-one (otherwise
> they may need to do some address translation which is much better
> avoided) so in practice we don't need to emulate this. If we use a
> different memory region maybe we also need some additional iommu code
> somewhere to connect the two but I'm not sure, I haven't tried since
> most other isa bridges do the same way using system_memory and this
> seems to work. I'm also a bit concerned about additional overhead which
> we could avoid if possible. Just to model something that's not really
> used I don't think it's worth the additional complexity and possible
> performance loss unless it's found to be needed to get some guests work.

Fine.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5287 bytes --]

On Sun, 10 Jan 2021, BALATON Zoltan wrote:
> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>> +PCI experts
>> 
>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> Hi Zoltan,
>>>> 
>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>> registers in subseqent commits.
>>>> 
>>>> Is this patch dependent of the VT82C686B_PM changes
>>>> or can it be applied before them?
>>> 
>>> I don't know but why would that be better? I thought it's clearer to
>>> clean up pm related parts first before moving more stuff to this file so
>>> that's why this patch comes after (and also because that's the order I
>>> did it).
>> 
>> Not any better, but easier for me to get your patches integrated,
>> as I'm reviewing your patches slowly. Finding other reviewers
>> would certainly help.
>
> No problem, I'll wait for your review. Merging parts of the series does not 
> help much because the whole series is needed for vt8231 which is prerequisite 
> for pegasos2 so eventually all of these are needed so it does not matter if 
> this one patch gets in earlier or later.
>
> Not sure who could help with review. Maybe Jiaxun or Huacai as this is used 
> by fuloong2e so they might be interested and could have info on this chip. 
> Most of these patches just cleaning up the vt82c686b and adding some missing 
> features so these can be reused by the vt8231 model in last 3 patches (which 
> is very similar to 686b only some reg addresses and ids seem to be different 
> for what we are concerned).

Ping? There are still a few patches needing review:

http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512

Jiaxun, Hiacai, or anybody else could you please help reviewing or testing 
if this works with fuloong2e?

Thank you,
BALATON Zoltan

>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>> 
>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>>> --- a/hw/isa/vt82c686.c
>>>>> +++ b/hw/isa/vt82c686.c
>>>>> @@ -16,6 +16,11 @@
>>>>>  #include "hw/qdev-properties.h"
>>>>>  #include "hw/isa/isa.h"
>>>>>  #include "hw/isa/superio.h"
>>>>> +#include "hw/intc/i8259.h"
>>>>> +#include "hw/irq.h"
>>>>> +#include "hw/dma/i8257.h"
>>>>> +#include "hw/timer/i8254.h"
>>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>>  #include "migration/vmstate.h"
>>>>>  #include "hw/isa/apm.h"
>>>>>  #include "hw/acpi/acpi.h"
>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>>> VT82C686B_ISA)
>>>>> 
>>>>>  struct VT82C686BISAState {
>>>>>      PCIDevice dev;
>>>>> +    qemu_irq cpu_intr;
>>>>>      SuperIOConfig superio_cfg;
>>>>>  };
>>>>> 
>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>>>>> +{
>>>>> +    VT82C686BISAState *s = opaque;
>>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>>> +}
>>>>> +
>>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>>                                     uint32_t val, int len)
>>>>>  {
>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>>> Error **errp)
>>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>>      DeviceState *dev = DEVICE(d);
>>>>>      ISABus *isa_bus;
>>>>> +    qemu_irq *isa_irq;
>>>>>      int i;
>>>>> 
>>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>> 
>>>> Why not use the SysBus API?
>>> 
>>> How? This is a PCIDevice not a SysBusDevice.
>> 
>> Indeed :)
>> 
>>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>>> pci_address_space_io(d),
>>>>>                            &error_fatal);
>>>> 
>>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>> 
>>> I don't really know. Most other places that create an isa bus seem to
>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
>>> I thought if those others are OK this should be too.
>> 
>> I'm not a PCI expert but my understanding is PCI device functions are
>> restricted to the PCI bus address space. The host bridge may map this
>> space within the host.
>> 
>> QEMU might be using get_system_memory() because for some host bridge
>> the mapping is not implemented so it was easier this way?
>
> Maybe, also one less indirection which if not really needed is a good thing 
> for performance so unless it's found to be needed to use another address 
> space here I'm happy with this as it matches what other similar devices do 
> and it seems to work. Maybe a separate address space is only really needed if 
> we have an iommu?
>
> Regards,
> BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Jiaxun Yang]

在 2021/2/2 上午4:04, BALATON Zoltan 写道:
> On Sun, 10 Jan 2021, BALATON Zoltan wrote:
>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>> +PCI experts
>>>
>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>>> Hi Zoltan,
>>>>>
>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>>> registers in subseqent commits.
>>>>>
>>>>> Is this patch dependent of the VT82C686B_PM changes
>>>>> or can it be applied before them?
>>>>
>>>> I don't know but why would that be better? I thought it's clearer to
>>>> clean up pm related parts first before moving more stuff to this 
>>>> file so
>>>> that's why this patch comes after (and also because that's the order I
>>>> did it).
>>>
>>> Not any better, but easier for me to get your patches integrated,
>>> as I'm reviewing your patches slowly. Finding other reviewers
>>> would certainly help.
>>
>> No problem, I'll wait for your review. Merging parts of the series 
>> does not help much because the whole series is needed for vt8231 
>> which is prerequisite for pegasos2 so eventually all of these are 
>> needed so it does not matter if this one patch gets in earlier or later.
>>
>> Not sure who could help with review. Maybe Jiaxun or Huacai as this 
>> is used by fuloong2e so they might be interested and could have info 
>> on this chip. Most of these patches just cleaning up the vt82c686b 
>> and adding some missing features so these can be reused by the vt8231 
>> model in last 3 patches (which is very similar to 686b only some reg 
>> addresses and ids seem to be different for what we are concerned).
>
> Ping? There are still a few patches needing review:
>
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512
>
> Jiaxun, Hiacai, or anybody else could you please help reviewing or 
> testing if this works with fuloong2e?

Tested the series against Fuloong2E PMON. Fuloong's kernel doesn't have 
much to do with
VIA ISA.

Which patch is pending for test or review?

Thanks.

- Jiaxun

>
> Thank you,
> BALATON Zoltan
>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -16,6 +16,11 @@
>>>>>>  #include "hw/qdev-properties.h"
>>>>>>  #include "hw/isa/isa.h"
>>>>>>  #include "hw/isa/superio.h"
>>>>>> +#include "hw/intc/i8259.h"
>>>>>> +#include "hw/irq.h"
>>>>>> +#include "hw/dma/i8257.h"
>>>>>> +#include "hw/timer/i8254.h"
>>>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>>>  #include "migration/vmstate.h"
>>>>>>  #include "hw/isa/apm.h"
>>>>>>  #include "hw/acpi/acpi.h"
>>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>>>> VT82C686B_ISA)
>>>>>>
>>>>>>  struct VT82C686BISAState {
>>>>>>      PCIDevice dev;
>>>>>> +    qemu_irq cpu_intr;
>>>>>>      SuperIOConfig superio_cfg;
>>>>>>  };
>>>>>>
>>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int 
>>>>>> level)
>>>>>> +{
>>>>>> +    VT82C686BISAState *s = opaque;
>>>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>>>> +}
>>>>>> +
>>>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>>>                                     uint32_t val, int len)
>>>>>>  {
>>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>>>> Error **errp)
>>>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>>>      DeviceState *dev = DEVICE(d);
>>>>>>      ISABus *isa_bus;
>>>>>> +    qemu_irq *isa_irq;
>>>>>>      int i;
>>>>>>
>>>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>>>
>>>>> Why not use the SysBus API?
>>>>
>>>> How? This is a PCIDevice not a SysBusDevice.
>>>
>>> Indeed :)
>>>
>>>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>>>> pci_address_space_io(d),
>>>>>>                            &error_fatal);
>>>>>
>>>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>>>
>>>> I don't really know. Most other places that create an isa bus seem to
>>>> also use get_system_memory(), only piix4 uses 
>>>> pci_address_space(dev) so
>>>> I thought if those others are OK this should be too.
>>>
>>> I'm not a PCI expert but my understanding is PCI device functions are
>>> restricted to the PCI bus address space. The host bridge may map this
>>> space within the host.
>>>
>>> QEMU might be using get_system_memory() because for some host bridge
>>> the mapping is not implemented so it was easier this way?
>>
>> Maybe, also one less indirection which if not really needed is a good 
>> thing for performance so unless it's found to be needed to use 
>> another address space here I'm happy with this as it matches what 
>> other similar devices do and it seems to work. Maybe a separate 
>> address space is only really needed if we have an iommu?
>>
>> Regards,
>> BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]

On Thu, 4 Feb 2021, Jiaxun Yang wrote:
> 在 2021/2/2 上午4:04, BALATON Zoltan 写道:
>> On Sun, 10 Jan 2021, BALATON Zoltan wrote:
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> +PCI experts
>>>> 
>>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Zoltan,
>>>>>> 
>>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>>>> registers in subseqent commits.
>>>>>> 
>>>>>> Is this patch dependent of the VT82C686B_PM changes
>>>>>> or can it be applied before them?
>>>>> 
>>>>> I don't know but why would that be better? I thought it's clearer to
>>>>> clean up pm related parts first before moving more stuff to this file so
>>>>> that's why this patch comes after (and also because that's the order I
>>>>> did it).
>>>> 
>>>> Not any better, but easier for me to get your patches integrated,
>>>> as I'm reviewing your patches slowly. Finding other reviewers
>>>> would certainly help.
>>> 
>>> No problem, I'll wait for your review. Merging parts of the series does 
>>> not help much because the whole series is needed for vt8231 which is 
>>> prerequisite for pegasos2 so eventually all of these are needed so it does 
>>> not matter if this one patch gets in earlier or later.
>>> 
>>> Not sure who could help with review. Maybe Jiaxun or Huacai as this is 
>>> used by fuloong2e so they might be interested and could have info on this 
>>> chip. Most of these patches just cleaning up the vt82c686b and adding some 
>>> missing features so these can be reused by the vt8231 model in last 3 
>>> patches (which is very similar to 686b only some reg addresses and ids 
>>> seem to be different for what we are concerned).
>> 
>> Ping? There are still a few patches needing review:
>> 
>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512
>> 
>> Jiaxun, Hiacai, or anybody else could you please help reviewing or testing 
>> if this works with fuloong2e?
>
> Tested the series against Fuloong2E PMON. Fuloong's kernel doesn't have much 
> to do with
> VIA ISA.
>
> Which patch is pending for test or review?

Thank you. In the above patchwork link there are numbers next to patches 
showing the number of tags, those which don't have any need review, mostly 
the last few. Unfortunately patchwork seems to be down at the moment from 
here. Pathes that don't have review yet are: vt82c686: Fix up power 
management io base and config and those starting with vt82c686: Fix 
superio_cfg_{read,write}() functions and after that. The patchew link for 
the series is here:

https://patchew.org/QEMU/cover.1610223396.git.balaton@eik.bme.hu/

Regards,
BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5356 bytes --]

On Mon, 1 Feb 2021, BALATON Zoltan wrote:
> On Sun, 10 Jan 2021, BALATON Zoltan wrote:
>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>> +PCI experts
>>> 
>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>>> Hi Zoltan,
>>>>> 
>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>>> registers in subseqent commits.
>>>>> 
>>>>> Is this patch dependent of the VT82C686B_PM changes
>>>>> or can it be applied before them?
>>>> 
>>>> I don't know but why would that be better? I thought it's clearer to
>>>> clean up pm related parts first before moving more stuff to this file so
>>>> that's why this patch comes after (and also because that's the order I
>>>> did it).
>>> 
>>> Not any better, but easier for me to get your patches integrated,
>>> as I'm reviewing your patches slowly. Finding other reviewers
>>> would certainly help.
>> 
>> No problem, I'll wait for your review. Merging parts of the series does not 
>> help much because the whole series is needed for vt8231 which is 
>> prerequisite for pegasos2 so eventually all of these are needed so it does 
>> not matter if this one patch gets in earlier or later.
>> 
>> Not sure who could help with review. Maybe Jiaxun or Huacai as this is used 
>> by fuloong2e so they might be interested and could have info on this chip. 
>> Most of these patches just cleaning up the vt82c686b and adding some 
>> missing features so these can be reused by the vt8231 model in last 3 
>> patches (which is very similar to 686b only some reg addresses and ids seem 
>> to be different for what we are concerned).
>
> Ping? There are still a few patches needing review:
>
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512
>
> Jiaxun, Hiacai, or anybody else could you please help reviewing or testing if 
> this works with fuloong2e?

Ping^2

> Thank you,
> BALATON Zoltan
>
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>>>> --- a/hw/isa/vt82c686.c
>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>> @@ -16,6 +16,11 @@
>>>>>>  #include "hw/qdev-properties.h"
>>>>>>  #include "hw/isa/isa.h"
>>>>>>  #include "hw/isa/superio.h"
>>>>>> +#include "hw/intc/i8259.h"
>>>>>> +#include "hw/irq.h"
>>>>>> +#include "hw/dma/i8257.h"
>>>>>> +#include "hw/timer/i8254.h"
>>>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>>>  #include "migration/vmstate.h"
>>>>>>  #include "hw/isa/apm.h"
>>>>>>  #include "hw/acpi/acpi.h"
>>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>>>> VT82C686B_ISA)
>>>>>> 
>>>>>>  struct VT82C686BISAState {
>>>>>>      PCIDevice dev;
>>>>>> +    qemu_irq cpu_intr;
>>>>>>      SuperIOConfig superio_cfg;
>>>>>>  };
>>>>>> 
>>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int 
>>>>>> level)
>>>>>> +{
>>>>>> +    VT82C686BISAState *s = opaque;
>>>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>>>> +}
>>>>>> +
>>>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>>>                                     uint32_t val, int len)
>>>>>>  {
>>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>>>> Error **errp)
>>>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>>>      DeviceState *dev = DEVICE(d);
>>>>>>      ISABus *isa_bus;
>>>>>> +    qemu_irq *isa_irq;
>>>>>>      int i;
>>>>>> 
>>>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>>> 
>>>>> Why not use the SysBus API?
>>>> 
>>>> How? This is a PCIDevice not a SysBusDevice.
>>> 
>>> Indeed :)
>>> 
>>>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>>>> pci_address_space_io(d),
>>>>>>                            &error_fatal);
>>>>> 
>>>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>>> 
>>>> I don't really know. Most other places that create an isa bus seem to
>>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
>>>> I thought if those others are OK this should be too.
>>> 
>>> I'm not a PCI expert but my understanding is PCI device functions are
>>> restricted to the PCI bus address space. The host bridge may map this
>>> space within the host.
>>> 
>>> QEMU might be using get_system_memory() because for some host bridge
>>> the mapping is not implemented so it was easier this way?
>> 
>> Maybe, also one less indirection which if not really needed is a good thing 
>> for performance so unless it's found to be needed to use another address 
>> space here I'm happy with this as it matches what other similar devices do 
>> and it seems to work. Maybe a separate address space is only really needed 
>> if we have an iommu?
>> 
>> Regards,
>> BALATON Zoltan

Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]

On Tue, 9 Feb 2021, BALATON Zoltan wrote:
> On Mon, 1 Feb 2021, BALATON Zoltan wrote:
>> On Sun, 10 Jan 2021, BALATON Zoltan wrote:
>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>> +PCI experts
>>>> 
>>>> On 1/10/21 1:43 AM, BALATON Zoltan wrote:
>>>>> On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
>>>>>> Hi Zoltan,
>>>>>> 
>>>>>> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>>>>>>> Currently the ISA devices that are part of the VIA south bridge,
>>>>>>> superio chip are wired up by board code. Move creation of these ISA
>>>>>>> devices to the VIA ISA bridge model so that board code does not need
>>>>>>> to access ISA bus. This also allows vt82c686b-superio to be made
>>>>>>> internal to vt82c686 which allows implementing its configuration via
>>>>>>> registers in subseqent commits.
>>>>>> 
>>>>>> Is this patch dependent of the VT82C686B_PM changes
>>>>>> or can it be applied before them?
>>>>> 
>>>>> I don't know but why would that be better? I thought it's clearer to
>>>>> clean up pm related parts first before moving more stuff to this file so
>>>>> that's why this patch comes after (and also because that's the order I
>>>>> did it).
>>>> 
>>>> Not any better, but easier for me to get your patches integrated,
>>>> as I'm reviewing your patches slowly. Finding other reviewers
>>>> would certainly help.
>>> 
>>> No problem, I'll wait for your review. Merging parts of the series does 
>>> not help much because the whole series is needed for vt8231 which is 
>>> prerequisite for pegasos2 so eventually all of these are needed so it does 
>>> not matter if this one patch gets in earlier or later.
>>> 
>>> Not sure who could help with review. Maybe Jiaxun or Huacai as this is 
>>> used by fuloong2e so they might be interested and could have info on this 
>>> chip. Most of these patches just cleaning up the vt82c686b and adding some 
>>> missing features so these can be reused by the vt8231 model in last 3 
>>> patches (which is very similar to 686b only some reg addresses and ids 
>>> seem to be different for what we are concerned).
>> 
>> Ping? There are still a few patches needing review:
>> 
>> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512
>> 
>> Jiaxun, Hiacai, or anybody else could you please help reviewing or testing 
>> if this works with fuloong2e?
>
> Ping^2

Ping^3

>> Thank you,
>> BALATON Zoltan
>> 
>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>> ---
>>>>>>>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>>>>>>>  hw/mips/fuloong2e.c | 29 +++++------------------------
>>>>>>>  2 files changed, 25 insertions(+), 24 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>>>>> index 58c0bba1d0..5df9be8ff4 100644
>>>>>>> --- a/hw/isa/vt82c686.c
>>>>>>> +++ b/hw/isa/vt82c686.c
>>>>>>> @@ -16,6 +16,11 @@
>>>>>>>  #include "hw/qdev-properties.h"
>>>>>>>  #include "hw/isa/isa.h"
>>>>>>>  #include "hw/isa/superio.h"
>>>>>>> +#include "hw/intc/i8259.h"
>>>>>>> +#include "hw/irq.h"
>>>>>>> +#include "hw/dma/i8257.h"
>>>>>>> +#include "hw/timer/i8254.h"
>>>>>>> +#include "hw/rtc/mc146818rtc.h"
>>>>>>>  #include "migration/vmstate.h"
>>>>>>>  #include "hw/isa/apm.h"
>>>>>>>  #include "hw/acpi/acpi.h"
>>>>>>> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState,
>>>>>>> VT82C686B_ISA)
>>>>>>> 
>>>>>>>  struct VT82C686BISAState {
>>>>>>>      PCIDevice dev;
>>>>>>> +    qemu_irq cpu_intr;
>>>>>>>      SuperIOConfig superio_cfg;
>>>>>>>  };
>>>>>>> 
>>>>>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int 
>>>>>>> level)
>>>>>>> +{
>>>>>>> +    VT82C686BISAState *s = opaque;
>>>>>>> +    qemu_set_irq(s->cpu_intr, level);
>>>>>>> +}
>>>>>>> +
>>>>>>>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>>>>>>>                                     uint32_t val, int len)
>>>>>>>  {
>>>>>>> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d,
>>>>>>> Error **errp)
>>>>>>>      VT82C686BISAState *s = VT82C686B_ISA(d);
>>>>>>>      DeviceState *dev = DEVICE(d);
>>>>>>>      ISABus *isa_bus;
>>>>>>> +    qemu_irq *isa_irq;
>>>>>>>      int i;
>>>>>>> 
>>>>>>> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>>>> 
>>>>>> Why not use the SysBus API?
>>>>> 
>>>>> How? This is a PCIDevice not a SysBusDevice.
>>>> 
>>>> Indeed :)
>>>> 
>>>>>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>>>>>      isa_bus = isa_bus_new(dev, get_system_memory(),
>>>>>>> pci_address_space_io(d),
>>>>>>>                            &error_fatal);
>>>>>> 
>>>>>> Isn't it get_system_memory() -> pci_address_space(d)?
>>>>> 
>>>>> I don't really know. Most other places that create an isa bus seem to
>>>>> also use get_system_memory(), only piix4 uses pci_address_space(dev) so
>>>>> I thought if those others are OK this should be too.
>>>> 
>>>> I'm not a PCI expert but my understanding is PCI device functions are
>>>> restricted to the PCI bus address space. The host bridge may map this
>>>> space within the host.
>>>> 
>>>> QEMU might be using get_system_memory() because for some host bridge
>>>> the mapping is not implemented so it was easier this way?
>>> 
>>> Maybe, also one less indirection which if not really needed is a good 
>>> thing for performance so unless it's found to be needed to use another 
>>> address space here I'm happy with this as it matches what other similar 
>>> devices do and it seems to work. Maybe a separate address space is only 
>>> really needed if we have an iommu?
>>> 
>>> Regards,
>>> BALATON Zoltan

[PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers

[BALATON Zoltan]

The base address of the SMBus io ports and its enabled status is set
by registers in the PCI config space but this was not correctly
emulated. Instead the SMBus registers were mapped on realize to the
base address set by a property to the address expected by fuloong2e
firmware.

Fix the base and config register handling to more closely model
hardware which allows to remove the property and allows the guest to
control this mapping. Do all this in reset instead of realize so it's
correctly updated on reset.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c   | 49 +++++++++++++++++++++++++++++++++------------
 hw/mips/fuloong2e.c |  4 +---
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index fe8961b057..9c4d153022 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -22,6 +22,7 @@
 #include "hw/i2c/pm_smbus.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "qemu/range.h"
 #include "qemu/timer.h"
 #include "exec/address-spaces.h"
 #include "trace.h"
@@ -34,7 +35,6 @@ struct VT686PMState {
     ACPIREGS ar;
     APMState apm;
     PMSMBus smb;
-    uint32_t smb_io_base;
 };
 
 static void pm_io_space_update(VT686PMState *s)
@@ -50,11 +50,22 @@ static void pm_io_space_update(VT686PMState *s)
     memory_region_transaction_commit();
 }
 
+static void smb_io_space_update(VT686PMState *s)
+{
+    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
+
+    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_transaction_commit();
+}
+
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
     VT686PMState *s = opaque;
 
     pm_io_space_update(s);
+    smb_io_space_update(s);
     return 0;
 }
 
@@ -77,8 +88,18 @@ static const VMStateDescription vmstate_acpi = {
 
 static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
 {
+    VT686PMState *s = VT82C686B_PM(d);
+
     trace_via_pm_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
+    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 (range_covers_byte(addr, len, 0xd2)) {
+        s->dev.config[0xd2] &= 0xf;
+        smb_io_space_update(s);
+    }
 }
 
 static void pm_update_sci(VT686PMState *s)
@@ -103,6 +124,17 @@ static void pm_tmr_timer(ACPIREGS *ar)
     pm_update_sci(s);
 }
 
+static void vt82c686b_pm_reset(DeviceState *d)
+{
+    VT686PMState *s = VT82C686B_PM(d);
+
+    /* SMBus IO base */
+    pci_set_long(s->dev.config + 0x90, 1);
+    s->dev.config[0xd2] = 0;
+
+    smb_io_space_update(s);
+}
+
 static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 {
     VT686PMState *s = VT82C686B_PM(dev);
@@ -116,13 +148,9 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     /* 0x48-0x4B is Power Management I/O Base */
     pci_set_long(pci_conf + 0x48, 0x00000001);
 
-    /* SMB ports:0xeee0~0xeeef */
-    s->smb_io_base = ((s->smb_io_base & 0xfff0) + 0x0);
-    pci_conf[0x90] = s->smb_io_base | 1;
-    pci_conf[0x91] = s->smb_io_base >> 8;
-    pci_conf[0xd2] = 0x90;
     pm_smbus_init(DEVICE(s), &s->smb, false);
-    memory_region_add_subregion(get_system_io(), s->smb_io_base, &s->smb.io);
+    memory_region_add_subregion(pci_address_space_io(dev), 0, &s->smb.io);
+    memory_region_set_enabled(&s->smb.io, false);
 
     apm_init(dev, &s->apm, NULL, s);
 
@@ -135,11 +163,6 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2);
 }
 
-static Property via_pm_properties[] = {
-    DEFINE_PROP_UINT32("smb_io_base", VT686PMState, smb_io_base, 0),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void via_pm_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -151,10 +174,10 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_VIA_ACPI;
     k->class_id = PCI_CLASS_BRIDGE_OTHER;
     k->revision = 0x40;
+    dc->reset = vt82c686b_pm_reset;
     dc->desc = "PM";
     dc->vmsd = &vmstate_acpi;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    device_class_set_props(dc, via_pm_properties);
 }
 
 static const TypeInfo via_pm_info = {
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 29805242ca..fbdd6122b3 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -251,9 +251,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
 
-    dev = pci_new(PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
-    qdev_prop_set_uint32(DEVICE(dev), "smb_io_base", 0xeee1);
-    pci_realize_and_unref(dev, pci_bus, &error_fatal);
+    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM);
     *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
     /* Audio support */
-- 
2.21.3

Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers

[Jiaxun Yang]

在 2021/1/10 上午4:16, BALATON Zoltan 写道:
> The base address of the SMBus io ports and its enabled status is set
> by registers in the PCI config space but this was not correctly
> emulated. Instead the SMBus registers were mapped on realize to the
> base address set by a property to the address expected by fuloong2e
> firmware.
>
> Fix the base and config register handling to more closely model
> hardware which allows to remove the property and allows the guest to
> control this mapping. Do all this in reset instead of realize so it's
> correctly updated on reset.

Hi,

Thanks for your patch!

>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/isa/vt82c686.c   | 49 +++++++++++++++++++++++++++++++++------------
>   hw/mips/fuloong2e.c |  4 +---
>   2 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index fe8961b057..9c4d153022 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -22,6 +22,7 @@
>   #include "hw/i2c/pm_smbus.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "qemu/range.h"
>   #include "qemu/timer.h"
>   #include "exec/address-spaces.h"
>   #include "trace.h"
> @@ -34,7 +35,6 @@ struct VT686PMState {
>       ACPIREGS ar;
>       APMState apm;
>       PMSMBus smb;
> -    uint32_t smb_io_base;
>   };
>   
>   static void pm_io_space_update(VT686PMState *s)
> @@ -50,11 +50,22 @@ static void pm_io_space_update(VT686PMState *s)
>       memory_region_transaction_commit();
>   }
>   
> +static void smb_io_space_update(VT686PMState *s)
> +{
> +    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
> +
> +    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_transaction_commit();
> +}
> +
>   static int vmstate_acpi_post_load(void *opaque, int version_id)
>   {
>       VT686PMState *s = opaque;
>   
>       pm_io_space_update(s);
> +    smb_io_space_update(s);
>       return 0;
>   }
>   
> @@ -77,8 +88,18 @@ static const VMStateDescription vmstate_acpi = {
>   
>   static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>   {
> +    VT686PMState *s = VT82C686B_PM(d);
> +
>       trace_via_pm_write(addr, val, len);
>       pci_default_write_config(d, addr, val, len);
> +    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);

What does this "or 1" do?
The datasheet I found only mentioned the default value of BASE is 0000 0001
but didn't say anything about it's function :-/

> +    }
> +    if (range_covers_byte(addr, len, 0xd2)) {
> +        s->dev.config[0xd2] &= 0xf;
> +        smb_io_space_update(s);
> +    }
>   }
>   
>   static void pm_update_sci(VT686PMState *s)
> @@ -103,6 +124,17 @@ static void pm_tmr_timer(ACPIREGS *ar)
>       pm_update_sci(s);
>   }
>   
> +static void vt82c686b_pm_reset(DeviceState *d)
> +{
> +    VT686PMState *s = VT82C686B_PM(d);
> +
> +    /* SMBus IO base */
> +    pci_set_long(s->dev.config + 0x90, 1);

Theoretically this kind of magic number should be avoided but
as the rest of the file was written in such style it seems fine for me.

[...]

Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4137 bytes --]

On Tue, 12 Jan 2021, Jiaxun Yang wrote:
> 在 2021/1/10 上午4:16, BALATON Zoltan 写道:
>> The base address of the SMBus io ports and its enabled status is set
>> by registers in the PCI config space but this was not correctly
>> emulated. Instead the SMBus registers were mapped on realize to the
>> base address set by a property to the address expected by fuloong2e
>> firmware.
>> 
>> Fix the base and config register handling to more closely model
>> hardware which allows to remove the property and allows the guest to
>> control this mapping. Do all this in reset instead of realize so it's
>> correctly updated on reset.
>
> Hi,
>
> Thanks for your patch!
>
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/isa/vt82c686.c   | 49 +++++++++++++++++++++++++++++++++------------
>>   hw/mips/fuloong2e.c |  4 +---
>>   2 files changed, 37 insertions(+), 16 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index fe8961b057..9c4d153022 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -22,6 +22,7 @@
>>   #include "hw/i2c/pm_smbus.h"
>>   #include "qapi/error.h"
>>   #include "qemu/module.h"
>> +#include "qemu/range.h"
>>   #include "qemu/timer.h"
>>   #include "exec/address-spaces.h"
>>   #include "trace.h"
>> @@ -34,7 +35,6 @@ struct VT686PMState {
>>       ACPIREGS ar;
>>       APMState apm;
>>       PMSMBus smb;
>> -    uint32_t smb_io_base;
>>   };
>>     static void pm_io_space_update(VT686PMState *s)
>> @@ -50,11 +50,22 @@ static void pm_io_space_update(VT686PMState *s)
>>       memory_region_transaction_commit();
>>   }
>>   +static void smb_io_space_update(VT686PMState *s)
>> +{
>> +    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
>> +
>> +    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_transaction_commit();
>> +}
>> +
>>   static int vmstate_acpi_post_load(void *opaque, int version_id)
>>   {
>>       VT686PMState *s = opaque;
>>         pm_io_space_update(s);
>> +    smb_io_space_update(s);
>>       return 0;
>>   }
>>   @@ -77,8 +88,18 @@ static const VMStateDescription vmstate_acpi = {
>>     static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, 
>> int len)
>>   {
>> +    VT686PMState *s = VT82C686B_PM(d);
>> +
>>       trace_via_pm_write(addr, val, len);
>>       pci_default_write_config(d, addr, val, len);
>> +    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);
>
> What does this "or 1" do?
> The datasheet I found only mentioned the default value of BASE is 0000 0001
> but didn't say anything about it's function :-/

It says that in the summary table but later in data sheet there's also 
detailed description of registers for each part where it says:

Offset 93-90 – SMBus I/O Base ... RW
3-0 Fixed ... always reads 0001b

The above mask and | 1 ensures this. I don't know why lowest bit is always 
1 but that seems to be the case for all such regs. Maybe internally these 
are implemented like PCI BARs where lowest bit means IO space.

>> +    }
>> +    if (range_covers_byte(addr, len, 0xd2)) {
>> +        s->dev.config[0xd2] &= 0xf;
>> +        smb_io_space_update(s);
>> +    }
>>   }
>>     static void pm_update_sci(VT686PMState *s)
>> @@ -103,6 +124,17 @@ static void pm_tmr_timer(ACPIREGS *ar)
>>       pm_update_sci(s);
>>   }
>>   +static void vt82c686b_pm_reset(DeviceState *d)
>> +{
>> +    VT686PMState *s = VT82C686B_PM(d);
>> +
>> +    /* SMBus IO base */
>> +    pci_set_long(s->dev.config + 0x90, 1);
>
> Theoretically this kind of magic number should be avoided but
> as the rest of the file was written in such style it seems fine for me.

I could add defines for register offsets but did not think that would make 
it much more readable to have random names instead of random numbers. 
Likely you'll have to consult the data sheet to find out their meaning 
anyway.

Regards,
BALATON Zoltan

Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers

[Jiaxun Yang]

在 2021/1/13 上午6:25, BALATON Zoltan 写道:
> On Tue, 12 Jan 2021, Jiaxun Yang wrote:
>> 在 2021/1/10 上午4:16, BALATON Zoltan 写道:
>>> The base address of the SMBus io ports and its enabled status is set
>>> by registers in the PCI config space but this was not correctly
>>> emulated. Instead the SMBus registers were mapped on realize to the
>>> base address set by a property to the address expected by fuloong2e
>>> firmware.
>>>
>>> Fix the base and config register handling to more closely model
>>> hardware which allows to remove the property and allows the guest to
>>> control this mapping. Do all this in reset instead of realize so it's
>>> correctly updated on reset.
>>
>> Hi,
>>
>> Thanks for your patch!
>>
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/isa/vt82c686.c   | 49 
>>> +++++++++++++++++++++++++++++++++------------
>>>   hw/mips/fuloong2e.c |  4 +---
>>>   2 files changed, 37 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index fe8961b057..9c4d153022 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -22,6 +22,7 @@
>>>   #include "hw/i2c/pm_smbus.h"
>>>   #include "qapi/error.h"
>>>   #include "qemu/module.h"
>>> +#include "qemu/range.h"
>>>   #include "qemu/timer.h"
>>>   #include "exec/address-spaces.h"
>>>   #include "trace.h"
>>> @@ -34,7 +35,6 @@ struct VT686PMState {
>>>       ACPIREGS ar;
>>>       APMState apm;
>>>       PMSMBus smb;
>>> -    uint32_t smb_io_base;
>>>   };
>>>     static void pm_io_space_update(VT686PMState *s)
>>> @@ -50,11 +50,22 @@ static void pm_io_space_update(VT686PMState *s)
>>>       memory_region_transaction_commit();
>>>   }
>>>   +static void smb_io_space_update(VT686PMState *s)
>>> +{
>>> +    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
>>> +
>>> +    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_transaction_commit();
>>> +}
>>> +
>>>   static int vmstate_acpi_post_load(void *opaque, int version_id)
>>>   {
>>>       VT686PMState *s = opaque;
>>>         pm_io_space_update(s);
>>> +    smb_io_space_update(s);
>>>       return 0;
>>>   }
>>>   @@ -77,8 +88,18 @@ static const VMStateDescription vmstate_acpi = {
>>>     static void pm_write_config(PCIDevice *d, uint32_t addr, 
>>> uint32_t val, int len)
>>>   {
>>> +    VT686PMState *s = VT82C686B_PM(d);
>>> +
>>>       trace_via_pm_write(addr, val, len);
>>>       pci_default_write_config(d, addr, val, len);
>>> +    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);
>>
>> What does this "or 1" do?
>> The datasheet I found only mentioned the default value of BASE is 
>> 0000 0001
>> but didn't say anything about it's function :-/
>
> It says that in the summary table but later in data sheet there's also 
> detailed description of registers for each part where it says:
>
> Offset 93-90 – SMBus I/O Base ... RW
> 3-0 Fixed ... always reads 0001b
>
> The above mask and | 1 ensures this. I don't know why lowest bit is 
> always 1 but that seems to be the case for all such regs. Maybe 
> internally these are implemented like PCI BARs where lowest bit means 
> IO space.

Thanks!

In this case:

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

>
>>> +    }
>>> +    if (range_covers_byte(addr, len, 0xd2)) {
>>> +        s->dev.config[0xd2] &= 0xf;
>>> +        smb_io_space_update(s);
>>> +    }
>>>   }
>>>     static void pm_update_sci(VT686PMState *s)
>>> @@ -103,6 +124,17 @@ static void pm_tmr_timer(ACPIREGS *ar)
>>>       pm_update_sci(s);
>>>   }
>>>   +static void vt82c686b_pm_reset(DeviceState *d)
>>> +{
>>> +    VT686PMState *s = VT82C686B_PM(d);
>>> +
>>> +    /* SMBus IO base */
>>> +    pci_set_long(s->dev.config + 0x90, 1);
>>
>> Theoretically this kind of magic number should be avoided but
>> as the rest of the file was written in such style it seems fine for me.
>
> I could add defines for register offsets but did not think that would 
> make it much more readable to have random names instead of random 
> numbers. Likely you'll have to consult the data sheet to find out 
> their meaning anyway.

Agreed.

- Jiaxun

>
> Regards,
> BALATON Zoltan

[PATCH v2 13/13] vt82c686: Add emulation of VT8231 south bridge

[BALATON Zoltan]

Add emulation of VT8231 south bridge ISA part based on the similar
VT82C686B but implemented in a separate subclass that holds the
differences while reusing parts that can be shared.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 154 ++++++++++++++++++++++++++++++--------
 include/hw/isa/vt82c686.h |   1 +
 include/hw/pci/pci_ids.h  |   3 +-
 3 files changed, 126 insertions(+), 32 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f27aea1643..50181e22dd 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -8,6 +8,9 @@
  *
  * Contributions after 2012-01-13 are licensed under the terms of the
  * GNU GPL, version 2 or (at your option) any later version.
+ *
+ * VT8231 south bridge support and general clean up to allow it
+ * Copyright (c) 2018-2020 BALATON Zoltan
  */
 
 #include "qemu/osdep.h"
@@ -609,24 +612,48 @@ static const TypeInfo vt8231_superio_info = {
 };
 
 
-OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
+#define TYPE_VIA_ISA "via-isa"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaISAState, VIA_ISA)
 
-struct VT82C686BISAState {
+struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
     ViaSuperIOState *via_sio;
 };
 
+static const VMStateDescription vmstate_via = {
+    .name = "via-isa",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, ViaISAState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const TypeInfo via_isa_info = {
+    .name          = TYPE_VIA_ISA,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(ViaISAState),
+    .abstract      = true,
+    .interfaces    = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
 {
-    VT82C686BISAState *s = opaque;
+    ViaISAState *s = opaque;
     qemu_set_irq(s->cpu_intr, level);
 }
 
+/* TYPE_VT82C686B_ISA */
+
 static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
-    VT82C686BISAState *s = VT82C686B_ISA(d);
+    ViaISAState *s = VIA_ISA(d);
 
     trace_via_isa_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
@@ -636,19 +663,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
     }
 }
 
-static const VMStateDescription vmstate_via = {
-    .name = "vt82c686b",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, VT82C686BISAState),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
 static void vt82c686b_isa_reset(DeviceState *dev)
 {
-    VT82C686BISAState *s = VT82C686B_ISA(dev);
+    ViaISAState *s = VIA_ISA(dev);
     uint8_t *pci_conf = s->dev.config;
 
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
@@ -668,7 +685,7 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
-    VT82C686BISAState *s = VT82C686B_ISA(d);
+    ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
     qemu_irq *isa_irq;
@@ -692,7 +709,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     }
 }
 
-static void via_class_init(ObjectClass *klass, void *data)
+static void vt82c686b_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -700,28 +717,101 @@ static void via_class_init(ObjectClass *klass, void *data)
     k->realize = vt82c686b_realize;
     k->config_write = vt82c686b_write_config;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-    k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE;
+    k->device_id = PCI_DEVICE_ID_VIA_82C686B_ISA;
     k->class_id = PCI_CLASS_BRIDGE_ISA;
     k->revision = 0x40;
     dc->reset = vt82c686b_isa_reset;
     dc->desc = "ISA bridge";
     dc->vmsd = &vmstate_via;
-    /*
-     * Reason: part of VIA VT82C686 southbridge, needs to be wired up,
-     * e.g. by mips_fuloong2e_init()
-     */
+    /* Reason: part of VIA VT82C686 southbridge, needs to be wired up */
     dc->user_creatable = false;
 }
 
-static const TypeInfo via_info = {
+static const TypeInfo vt82c686b_isa_info = {
     .name          = TYPE_VT82C686B_ISA,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VT82C686BISAState),
-    .class_init    = via_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
+    .parent        = TYPE_VIA_ISA,
+    .instance_size = sizeof(ViaISAState),
+    .class_init    = vt82c686b_class_init,
+};
+
+/* TYPE_VT8231_ISA */
+
+static void vt8231_write_config(PCIDevice *d, uint32_t addr,
+                                uint32_t val, int len)
+{
+    ViaISAState *s = VIA_ISA(d);
+
+    trace_via_isa_write(addr, val, len);
+    pci_default_write_config(d, addr, val, len);
+    if (addr == 0x50) {
+        /* BIT(2): enable or disable superio config io ports */
+        via_superio_io_enable(s->via_sio, val & BIT(2));
+    }
+}
+
+static void vt8231_isa_reset(DeviceState *dev)
+{
+    ViaISAState *s = VIA_ISA(dev);
+    uint8_t *pci_conf = s->dev.config;
+
+    pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
+                 PCI_COMMAND_MASTER | PCI_COMMAND_SPECIAL);
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
+
+    pci_conf[0x58] = 0x40; /* Miscellaneous Control 0 */
+    pci_conf[0x67] = 0x08; /* Fast IR Config */
+    pci_conf[0x6b] = 0x01; /* Fast IR I/O Base */
+}
+
+static void vt8231_realize(PCIDevice *d, Error **errp)
+{
+    ViaISAState *s = VIA_ISA(d);
+    DeviceState *dev = DEVICE(d);
+    ISABus *isa_bus;
+    qemu_irq *isa_irq;
+    int i;
+
+    qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
+    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+                          &error_fatal);
+    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
+    i8254_pit_init(isa_bus, 0x40, 0, NULL);
+    i8257_dma_init(isa_bus, 0);
+    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus, TYPE_VT8231_SUPERIO));
+    mc146818_rtc_init(isa_bus, 2000, NULL);
+
+    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
+        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
+            d->wmask[i] = 0;
+        }
+    }
+}
+
+static void vt8231_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = vt8231_realize;
+    k->config_write = vt8231_write_config;
+    k->vendor_id = PCI_VENDOR_ID_VIA;
+    k->device_id = PCI_DEVICE_ID_VIA_8231_ISA;
+    k->class_id = PCI_CLASS_BRIDGE_ISA;
+    k->revision = 0x10;
+    dc->reset = vt8231_isa_reset;
+    dc->desc = "ISA bridge";
+    dc->vmsd = &vmstate_via;
+    /* Reason: part of VIA VT8231 southbridge, needs to be wired up */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo vt8231_isa_info = {
+    .name          = TYPE_VT8231_ISA,
+    .parent        = TYPE_VIA_ISA,
+    .instance_size = sizeof(ViaISAState),
+    .class_init    = vt8231_class_init,
 };
 
 
@@ -733,7 +823,9 @@ static void vt82c686b_register_types(void)
     type_register_static(&via_superio_info);
     type_register_static(&vt82c686b_superio_info);
     type_register_static(&vt8231_superio_info);
-    type_register_static(&via_info);
+    type_register_static(&via_isa_info);
+    type_register_static(&vt82c686b_isa_info);
+    type_register_static(&vt8231_isa_info);
 }
 
 type_init(vt82c686b_register_types)
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 0692b9a527..0f01aaa471 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -3,6 +3,7 @@
 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
+#define TYPE_VT8231_ISA "vt8231-isa"
 #define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 7c183d16f9..463ffa054e 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -201,12 +201,13 @@
 #define PCI_VENDOR_ID_XILINX             0x10ee
 
 #define PCI_VENDOR_ID_VIA                0x1106
-#define PCI_DEVICE_ID_VIA_ISA_BRIDGE     0x0686
+#define PCI_DEVICE_ID_VIA_82C686B_ISA    0x0686
 #define PCI_DEVICE_ID_VIA_IDE            0x0571
 #define PCI_DEVICE_ID_VIA_UHCI           0x3038
 #define PCI_DEVICE_ID_VIA_82C686B_PM     0x3057
 #define PCI_DEVICE_ID_VIA_AC97           0x3058
 #define PCI_DEVICE_ID_VIA_MC97           0x3068
+#define PCI_DEVICE_ID_VIA_8231_ISA       0x8231
 #define PCI_DEVICE_ID_VIA_8231_PM        0x8235
 
 #define PCI_VENDOR_ID_MARVELL            0x11ab
-- 
2.21.3

[PATCH v2 09/13] vt82c686: Fix superio_cfg_{read,write}() functions

[BALATON Zoltan]

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

Re: [PATCH v2 09/13] vt82c686: Fix superio_cfg_{read,write}() functions

[Philippe Mathieu-Daudé]

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

Re: [PATCH v2 09/13] vt82c686: Fix superio_cfg_{read,write}() functions

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

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

[PATCH v2 01/13] vt82c686: Move superio memory region to SuperIOConfig struct

[BALATON Zoltan]

The superio memory region holds the io space index/data registers used
to access the superio config registers that are implemented in struct
SuperIOConfig. To keep these related things together move the memory
region to SuperIOConfig and rename it accordingly.
Also remove the unused "data" member of SuperIOConfig which is not
needed as we store actual data values in the regs array.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index a6f5a0843d..30fe02f4c6 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -29,12 +29,11 @@
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
     uint8_t index;
-    uint8_t data;
+    MemoryRegion io;
 } SuperIOConfig;
 
 struct VT82C686BISAState {
     PCIDevice dev;
-    MemoryRegion superio;
     SuperIOConfig superio_cfg;
 };
 
@@ -128,8 +127,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
 
     trace_via_isa_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
-    if (addr == 0x85) {  /* enable or disable super IO configure */
-        memory_region_set_enabled(&s->superio, val & 0x2);
+    if (addr == 0x85) {
+        /* BIT(1): enable or disable superio config io ports */
+        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
     }
 }
 
@@ -311,15 +311,15 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
         }
     }
 
-    memory_region_init_io(&s->superio, OBJECT(d), &superio_cfg_ops,
-                          &s->superio_cfg, "superio", 2);
-    memory_region_set_enabled(&s->superio, false);
+    memory_region_init_io(&s->superio_cfg.io, OBJECT(d), &superio_cfg_ops,
+                          &s->superio_cfg, "superio_cfg", 2);
+    memory_region_set_enabled(&s->superio_cfg.io, false);
     /*
      * The floppy also uses 0x3f0 and 0x3f1.
      * But we do not emulate a floppy, so just set it here.
      */
     memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
-                                &s->superio);
+                                &s->superio_cfg.io);
 }
 
 static void via_class_init(ObjectClass *klass, void *data)
-- 
2.21.3

Re: [PATCH v2 01/13] vt82c686: Move superio memory region to SuperIOConfig struct

[Philippe Mathieu-Daudé]

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> The superio memory region holds the io space index/data registers used
> to access the superio config registers that are implemented in struct
> SuperIOConfig. To keep these related things together move the memory
> region to SuperIOConfig and rename it accordingly.
> Also remove the unused "data" member of SuperIOConfig which is not
> needed as we store actual data values in the regs array.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 01/13] vt82c686: Move superio memory region to SuperIOConfig struct

[Jiaxun Yang]

在 2021/1/10 上午4:16, BALATON Zoltan 写道:
> The superio memory region holds the io space index/data registers used
> to access the superio config registers that are implemented in struct
> SuperIOConfig. To keep these related things together move the memory
> region to SuperIOConfig and rename it accordingly.
> Also remove the unused "data" member of SuperIOConfig which is not
> needed as we store actual data values in the regs array.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

> ---
>   hw/isa/vt82c686.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index a6f5a0843d..30fe02f4c6 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -29,12 +29,11 @@
>   typedef struct SuperIOConfig {
>       uint8_t regs[0x100];
>       uint8_t index;
> -    uint8_t data;
> +    MemoryRegion io;
>   } SuperIOConfig;
>   
>   struct VT82C686BISAState {
>       PCIDevice dev;
> -    MemoryRegion superio;
>       SuperIOConfig superio_cfg;
>   };
>   
> @@ -128,8 +127,9 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>   
>       trace_via_isa_write(addr, val, len);
>       pci_default_write_config(d, addr, val, len);
> -    if (addr == 0x85) {  /* enable or disable super IO configure */
> -        memory_region_set_enabled(&s->superio, val & 0x2);
> +    if (addr == 0x85) {
> +        /* BIT(1): enable or disable superio config io ports */
> +        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
>       }
>   }
>   
> @@ -311,15 +311,15 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>           }
>       }
>   
> -    memory_region_init_io(&s->superio, OBJECT(d), &superio_cfg_ops,
> -                          &s->superio_cfg, "superio", 2);
> -    memory_region_set_enabled(&s->superio, false);
> +    memory_region_init_io(&s->superio_cfg.io, OBJECT(d), &superio_cfg_ops,
> +                          &s->superio_cfg, "superio_cfg", 2);
> +    memory_region_set_enabled(&s->superio_cfg.io, false);
>       /*
>        * The floppy also uses 0x3f0 and 0x3f1.
>        * But we do not emulate a floppy, so just set it here.
>        */
>       memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
> -                                &s->superio);
> +                                &s->superio_cfg.io);
>   }
>   
>   static void via_class_init(ObjectClass *klass, void *data)

[PATCH v2 05/13] vt82c686: Set user_creatable=false for VT82C686B_PM

[BALATON Zoltan]

This device is part of the multifunction VIA superio/south bridge chip
so not useful in itself.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index fc2a1f4430..9b16660e9d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -200,8 +200,9 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
     k->revision = 0x40;
     dc->reset = vt82c686b_pm_reset;
     dc->desc = "PM";
+    /* Reason: part of VIA south bridge, does not exist stand alone */
+    dc->user_creatable = false;
     dc->vmsd = &vmstate_acpi;
-    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
 }
 
 static const TypeInfo via_pm_info = {
-- 
2.21.3

Re: [PATCH v2 05/13] vt82c686: Set user_creatable=false for VT82C686B_PM

[Philippe Mathieu-Daudé]

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> This device is part of the multifunction VIA superio/south bridge chip
> so not useful in itself.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 05/13] vt82c686: Set user_creatable=false for VT82C686B_PM

[Jiaxun Yang]

在 2021/1/10 上午4:16, BALATON Zoltan 写道:
> This device is part of the multifunction VIA superio/south bridge chip
> so not useful in itself.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>   hw/isa/vt82c686.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index fc2a1f4430..9b16660e9d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -200,8 +200,9 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
>       k->revision = 0x40;
>       dc->reset = vt82c686b_pm_reset;
>       dc->desc = "PM";
> +    /* Reason: part of VIA south bridge, does not exist stand alone */
> +    dc->user_creatable = false;
>       dc->vmsd = &vmstate_acpi;
> -    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>   }
>   
>   static const TypeInfo via_pm_info = {

[PATCH v2 07/13] vt82c686: Simplify vt82c686b_realize()

[BALATON Zoltan]

Remove unneeded variables and setting value to 0 on zero initialised
data and replace check for error with error_fatal. Rationalise loop
that sets PCI config header fields read only.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c257926579..58c0bba1d0 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -363,24 +363,16 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
     VT82C686BISAState *s = VT82C686B_ISA(d);
-    uint8_t *pci_conf;
+    DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
-    uint8_t *wmask;
     int i;
 
-    isa_bus = isa_bus_new(DEVICE(d), get_system_memory(),
-                          pci_address_space_io(d), errp);
-    if (!isa_bus) {
-        return;
-    }
-
-    pci_conf = d->config;
-    pci_config_set_prog_interface(pci_conf, 0x0);
+    isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
+                          &error_fatal);
 
-    wmask = d->wmask;
-    for (i = 0x00; i < 0xff; i++) {
-        if (i <= 0x03 || (i >= 0x08 && i <= 0x3f)) {
-            wmask[i] = 0x00;
+    for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
+        if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
+            d->wmask[i] = 0;
         }
     }
 
-- 
2.21.3

[PATCH v2 11/13] vt82c686: QOM-ify superio related functionality

[BALATON Zoltan]

Collect superio functionality and its controlling config registers
handling in an abstract VIA_SUPERIO class that is a subclass of
ISA_SUPERIO and put vt82c686b specific parts in a subclass of this
abstract class.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 240 ++++++++++++++++++++++++--------------
 include/hw/isa/vt82c686.h |   1 -
 2 files changed, 150 insertions(+), 91 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 18bd86285b..f23fb2ea25 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -249,12 +249,21 @@ static const TypeInfo vt8231_pm_info = {
 };
 
 
-typedef struct SuperIOConfig {
+#define TYPE_VIA_SUPERIO "via-superio"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaSuperIOState, VIA_SUPERIO)
+
+struct ViaSuperIOState {
+    ISASuperIODevice superio;
     uint8_t regs[0x100];
+    const MemoryRegionOps *io_ops;
     MemoryRegion io;
-    ISASuperIODevice *superio;
     MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
-} SuperIOConfig;
+};
+
+static inline void via_superio_io_enable(ViaSuperIOState *s, bool enable)
+{
+    memory_region_set_enabled(&s->io, enable);
+}
 
 static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
                                     int offs)
@@ -270,10 +279,76 @@ static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
     return mr;
 }
 
-static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
-                              unsigned size)
+static void via_superio_realize(DeviceState *d, Error **errp)
+{
+    ViaSuperIOState *s = VIA_SUPERIO(d);
+    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
+    int i;
+
+    assert(s->io_ops);
+    ic->parent_realize(d, errp);
+    if (*errp) {
+        return;
+    }
+    /* Grab io regions of serial devices so we can control them */
+    for (i = 0; i < ic->serial.count; i++) {
+        ISADevice *sd = s->superio.serial[i];
+        MemoryRegion *io = isa_address_space_io(sd);
+        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
+        if (!mr) {
+            error_setg(errp, "Could not get io region for serial %d", i);
+            return;
+        }
+        s->serial_io[i] = mr;
+    }
+
+    memory_region_init_io(&s->io, OBJECT(d), s->io_ops, s, "via-superio", 2);
+    memory_region_set_enabled(&s->io, false);
+    /* The floppy also uses 0x3f0 and 0x3f1 but this seems to work anyway */
+    memory_region_add_subregion(isa_address_space_io(ISA_DEVICE(s)), 0x3f0,
+                                &s->io);
+}
+
+static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ViaSuperIOState *sc = opaque;
+    uint8_t idx = sc->regs[0];
+    uint8_t val = sc->regs[idx];
+
+    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;
+}
+
+static void via_superio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    sc->parent_realize = dc->realize;
+    dc->realize = via_superio_realize;
+}
+
+static const TypeInfo via_superio_info = {
+    .name          = TYPE_VIA_SUPERIO,
+    .parent        = TYPE_ISA_SUPERIO,
+    .instance_size = sizeof(ViaSuperIOState),
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = via_superio_class_init,
+    .abstract      = true,
+};
+
+#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+
+static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
+                                        uint64_t data, unsigned size)
 {
-    SuperIOConfig *sc = opaque;
+    ViaSuperIOState *sc = opaque;
     uint8_t idx = sc->regs[0];
 
     if (addr == 0) { /* config index register */
@@ -295,29 +370,29 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     case 0xfd ... 0xff:
         /* ignore write to read only registers */
         return;
-    case 0xe2:
+    case 0xe2: /* Function select */
     {
         data &= 0x1f;
         if (data & BIT(2)) { /* Serial port 1 enable */
-            ISADevice *dev = sc->superio->serial[0];
+            ISADevice *dev = sc->superio.serial[0];
             if (!memory_region_is_mapped(sc->serial_io[0])) {
                 memory_region_add_subregion(isa_address_space_io(dev),
                                             dev->ioport_id, sc->serial_io[0]);
             }
         } else {
-            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
+            MemoryRegion *io = isa_address_space_io(sc->superio.serial[0]);
             if (memory_region_is_mapped(sc->serial_io[0])) {
                 memory_region_del_subregion(io, sc->serial_io[0]);
             }
         }
         if (data & BIT(3)) { /* Serial port 2 enable */
-            ISADevice *dev = sc->superio->serial[1];
+            ISADevice *dev = sc->superio.serial[1];
             if (!memory_region_is_mapped(sc->serial_io[1])) {
                 memory_region_add_subregion(isa_address_space_io(dev),
                                             dev->ioport_id, sc->serial_io[1]);
             }
         } else {
-            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
+            MemoryRegion *io = isa_address_space_io(sc->superio.serial[1]);
             if (memory_region_is_mapped(sc->serial_io[1])) {
                 memory_region_del_subregion(io, sc->serial_io[1]);
             }
@@ -327,7 +402,7 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     case 0xe7: /* Serial port 1 io base address */
     {
         data &= 0xfe;
-        sc->superio->serial[0]->ioport_id = data << 2;
+        sc->superio.serial[0]->ioport_id = data << 2;
         if (memory_region_is_mapped(sc->serial_io[0])) {
             memory_region_set_address(sc->serial_io[0], data << 2);
         }
@@ -336,7 +411,7 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     case 0xe8: /* Serial port 2 io base address */
     {
         data &= 0xfe;
-        sc->superio->serial[1]->ioport_id = data << 2;
+        sc->superio.serial[1]->ioport_id = data << 2;
         if (memory_region_is_mapped(sc->serial_io[1])) {
             memory_region_set_address(sc->serial_io[1], data << 2);
         }
@@ -350,25 +425,9 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     sc->regs[idx] = data;
 }
 
-static uint64_t superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
-{
-    SuperIOConfig *sc = opaque;
-    uint8_t idx = sc->regs[0];
-    uint8_t val = sc->regs[idx];
-
-    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;
-}
-
-static const MemoryRegionOps superio_cfg_ops = {
-    .read = superio_cfg_read,
-    .write = superio_cfg_write,
+static const MemoryRegionOps vt82c686b_superio_cfg_ops = {
+    .read = via_superio_cfg_read,
+    .write = vt82c686b_superio_cfg_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -376,13 +435,66 @@ static const MemoryRegionOps superio_cfg_ops = {
     },
 };
 
+static void vt82c686b_superio_reset(DeviceState *dev)
+{
+    ViaSuperIOState *s = VIA_SUPERIO(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+    /* Device ID */
+    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
+    /* Function select - all disabled */
+    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
+    /* Floppy ctrl base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
+    /* Parallel port base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
+    /* Serial port 1 base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
+    /* Serial port 2 base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
+
+    vt82c686b_superio_cfg_write(s, 0, 0, 1);
+}
+
+static void vt82c686b_superio_init(Object *obj)
+{
+    VIA_SUPERIO(obj)->io_ops = &vt82c686b_superio_cfg_ops;
+}
+
+static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    dc->reset = vt82c686b_superio_reset;
+    sc->serial.count = 2;
+    sc->parallel.count = 1;
+    sc->ide.count = 0; /* emulated by via-ide */
+    sc->floppy.count = 1;
+}
+
+static const TypeInfo vt82c686b_superio_info = {
+    .name          = TYPE_VT82C686B_SUPERIO,
+    .parent        = TYPE_VIA_SUPERIO,
+    .instance_size = sizeof(ViaSuperIOState),
+    .instance_init = vt82c686b_superio_init,
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = vt82c686b_superio_class_init,
+};
+
 
 OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
 
 struct VT82C686BISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
-    SuperIOConfig superio_cfg;
+    ViaSuperIOState *via_sio;
 };
 
 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
@@ -400,7 +512,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
     pci_default_write_config(d, addr, val, len);
     if (addr == 0x85) {
         /* BIT(1): enable or disable superio config io ports */
-        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
+        via_superio_io_enable(s->via_sio, val & BIT(1));
     }
 }
 
@@ -432,13 +544,6 @@ static void vt82c686b_isa_reset(DeviceState *dev)
     pci_conf[0x5a] = 0x04; /* KBC/RTC Control*/
     pci_conf[0x5f] = 0x04;
     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
-
-    s->superio_cfg.regs[0xe0] = 0x3c; /* Device ID */
-    s->superio_cfg.regs[0xe2] = 0x03; /* Function select */
-    s->superio_cfg.regs[0xe3] = 0xfc; /* Floppy ctrl base addr */
-    s->superio_cfg.regs[0xe6] = 0xde; /* Parallel port base addr */
-    s->superio_cfg.regs[0xe7] = 0xfe; /* Serial port 1 base addr */
-    s->superio_cfg.regs[0xe8] = 0xbe; /* Serial port 2 base addr */
 }
 
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
@@ -447,7 +552,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
     qemu_irq *isa_irq;
-    ISASuperIOClass *ic;
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
@@ -457,9 +561,8 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
-                                                      TYPE_VT82C686B_SUPERIO));
-    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
+    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus,
+                                               TYPE_VT82C686B_SUPERIO));
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
@@ -467,31 +570,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
             d->wmask[i] = 0;
         }
     }
-
-    memory_region_init_io(&s->superio_cfg.io, OBJECT(d), &superio_cfg_ops,
-                          &s->superio_cfg, "superio_cfg", 2);
-    memory_region_set_enabled(&s->superio_cfg.io, false);
-    /*
-     * The floppy also uses 0x3f0 and 0x3f1.
-     * But we do not emulate a floppy, so just set it here.
-     */
-    memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
-                                &s->superio_cfg.io);
-
-    /* Grab io regions of serial devices so we can control them */
-    for (i = 0; i < ic->serial.count; i++) {
-        ISADevice *sd = s->superio_cfg.superio->serial[i];
-        MemoryRegion *io = isa_address_space_io(sd);
-        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
-        if (!mr) {
-            error_setg(errp, "Could not get io region for serial %d", i);
-            return;
-        }
-        s->superio_cfg.serial_io[i] = mr;
-        if (memory_region_is_mapped(mr)) {
-            memory_region_del_subregion(io, mr);
-        }
-    }
 }
 
 static void via_class_init(ObjectClass *klass, void *data)
@@ -527,32 +605,14 @@ static const TypeInfo via_info = {
 };
 
 
-static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
-{
-    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
-
-    sc->serial.count = 2;
-    sc->parallel.count = 1;
-    sc->ide.count = 0;
-    sc->floppy.count = 1;
-}
-
-static const TypeInfo via_superio_info = {
-    .name          = TYPE_VT82C686B_SUPERIO,
-    .parent        = TYPE_ISA_SUPERIO,
-    .instance_size = sizeof(ISASuperIODevice),
-    .class_size    = sizeof(ISASuperIOClass),
-    .class_init    = vt82c686b_superio_class_init,
-};
-
-
 static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
     type_register_static(&vt82c686b_pm_info);
     type_register_static(&vt8231_pm_info);
-    type_register_static(&via_info);
     type_register_static(&via_superio_info);
+    type_register_static(&vt82c686b_superio_info);
+    type_register_static(&via_info);
 }
 
 type_init(vt82c686b_register_types)
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 9b6d610e83..0692b9a527 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -2,7 +2,6 @@
 #define HW_VT82C686_H
 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
-#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
-- 
2.21.3

[PATCH v2 12/13] vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO

[BALATON Zoltan]

The VT8231 south bridge is very similar to VT82C686B but there are
some differences in register addresses and functionality, e.g. the
VT8231 only has one serial port. This commit adds VT8231_SUPERIO
subclass based on the abstract VIA_SUPERIO class to emulate the
superio part of VT8231.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f23fb2ea25..f27aea1643 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -489,6 +489,126 @@ static const TypeInfo vt82c686b_superio_info = {
 };
 
 
+#define TYPE_VT8231_SUPERIO "vt8231-superio"
+
+static void vt8231_superio_cfg_write(void *opaque, hwaddr addr,
+                                     uint64_t data, unsigned size)
+{
+    ViaSuperIOState *sc = opaque;
+    uint8_t idx = sc->regs[0];
+
+    if (addr == 0) { /* config index register */
+        sc->regs[0] = data;
+        return;
+    }
+
+    /* config data register */
+    trace_via_superio_write(idx, data);
+    switch (idx) {
+    case 0x00 ... 0xdf:
+    case 0xe7 ... 0xef:
+    case 0xf0 ... 0xf1:
+    case 0xf5:
+    case 0xf8:
+    case 0xfd:
+        /* ignore write to read only registers */
+        return;
+    case 0xf2: /* Function select */
+    {
+        data &= 0x17;
+        if (data & BIT(2)) { /* Serial port enable */
+            ISADevice *dev = sc->superio.serial[0];
+            if (!memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_add_subregion(isa_address_space_io(dev),
+                                            dev->ioport_id, sc->serial_io[0]);
+            }
+        } else {
+            MemoryRegion *io = isa_address_space_io(sc->superio.serial[0]);
+            if (memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_del_subregion(io, sc->serial_io[0]);
+            }
+        }
+        break;
+    }
+    case 0xf4: /* Serial port io base address */
+    {
+        data &= 0xfe;
+        sc->superio.serial[0]->ioport_id = data << 2;
+        if (memory_region_is_mapped(sc->serial_io[0])) {
+            memory_region_set_address(sc->serial_io[0], data << 2);
+        }
+        break;
+    }
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "via_superio_cfg: unimplemented register 0x%x\n", idx);
+        break;
+    }
+    sc->regs[idx] = data;
+}
+
+static const MemoryRegionOps vt8231_superio_cfg_ops = {
+    .read = via_superio_cfg_read,
+    .write = vt8231_superio_cfg_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static void vt8231_superio_reset(DeviceState *dev)
+{
+    ViaSuperIOState *s = VIA_SUPERIO(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+    /* Device ID */
+    s->regs[0xf0] = 0x3c;
+    /* Device revision */
+    s->regs[0xf1] = 0x01;
+    /* Function select - all disabled */
+    vt8231_superio_cfg_write(s, 0, 0xf2, 1);
+    vt8231_superio_cfg_write(s, 1, 0x03, 1);
+    /* Serial port base addr */
+    vt8231_superio_cfg_write(s, 0, 0xf4, 1);
+    vt8231_superio_cfg_write(s, 1, 0xfe, 1);
+    /* Parallel port base addr */
+    vt8231_superio_cfg_write(s, 0, 0xf6, 1);
+    vt8231_superio_cfg_write(s, 1, 0xde, 1);
+    /* Floppy ctrl base addr */
+    vt8231_superio_cfg_write(s, 0, 0xf7, 1);
+    vt8231_superio_cfg_write(s, 1, 0xfc, 1);
+
+    vt8231_superio_cfg_write(s, 0, 0, 1);
+}
+
+static void vt8231_superio_init(Object *obj)
+{
+    VIA_SUPERIO(obj)->io_ops = &vt8231_superio_cfg_ops;
+}
+
+static void vt8231_superio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    dc->reset = vt8231_superio_reset;
+    sc->serial.count = 1;
+    sc->parallel.count = 1;
+    sc->ide.count = 0; /* emulated by via-ide */
+    sc->floppy.count = 1;
+}
+
+static const TypeInfo vt8231_superio_info = {
+    .name          = TYPE_VT8231_SUPERIO,
+    .parent        = TYPE_VIA_SUPERIO,
+    .instance_size = sizeof(ViaSuperIOState),
+    .instance_init = vt8231_superio_init,
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = vt8231_superio_class_init,
+};
+
+
 OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)
 
 struct VT82C686BISAState {
@@ -612,6 +732,7 @@ static void vt82c686b_register_types(void)
     type_register_static(&vt8231_pm_info);
     type_register_static(&via_superio_info);
     type_register_static(&vt82c686b_superio_info);
+    type_register_static(&vt8231_superio_info);
     type_register_static(&via_info);
 }
 
-- 
2.21.3

[PATCH v2 06/13] vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm based on it

[BALATON Zoltan]

The vt82c686b-pm model can be shared between VT82C686B and VT8231. The
only difference between the two is the device id in what we emulate so
make an abstract via-pm model by renaming appropriately and add types
for vt82c686b-pm and vt8231-pm based on it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 84 ++++++++++++++++++++++++++-------------
 include/hw/isa/vt82c686.h |  1 +
 include/hw/pci/pci_ids.h  |  3 +-
 3 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 9b16660e9d..c257926579 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -27,9 +27,10 @@
 #include "exec/address-spaces.h"
 #include "trace.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
+#define TYPE_VIA_PM "via-pm"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
-struct VT686PMState {
+struct ViaPMState {
     PCIDevice dev;
     MemoryRegion io;
     ACPIREGS ar;
@@ -37,7 +38,7 @@ struct VT686PMState {
     PMSMBus smb;
 };
 
-static void pm_io_space_update(VT686PMState *s)
+static void pm_io_space_update(ViaPMState *s)
 {
     uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
 
@@ -47,7 +48,7 @@ static void pm_io_space_update(VT686PMState *s)
     memory_region_transaction_commit();
 }
 
-static void smb_io_space_update(VT686PMState *s)
+static void smb_io_space_update(ViaPMState *s)
 {
     uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
 
@@ -59,7 +60,7 @@ static void smb_io_space_update(VT686PMState *s)
 
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
-    VT686PMState *s = opaque;
+    ViaPMState *s = opaque;
 
     pm_io_space_update(s);
     smb_io_space_update(s);
@@ -72,20 +73,20 @@ static const VMStateDescription vmstate_acpi = {
     .minimum_version_id = 1,
     .post_load = vmstate_acpi_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.evt.sts, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.evt.en, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.cnt.cnt, VT686PMState),
-        VMSTATE_STRUCT(apm, VT686PMState, 0, vmstate_apm, APMState),
-        VMSTATE_TIMER_PTR(ar.tmr.timer, VT686PMState),
-        VMSTATE_INT64(ar.tmr.overflow_time, VT686PMState),
+        VMSTATE_PCI_DEVICE(dev, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.evt.sts, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.evt.en, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.cnt.cnt, ViaPMState),
+        VMSTATE_STRUCT(apm, ViaPMState, 0, vmstate_apm, APMState),
+        VMSTATE_TIMER_PTR(ar.tmr.timer, ViaPMState),
+        VMSTATE_INT64(ar.tmr.overflow_time, ViaPMState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
 {
-    VT686PMState *s = VT82C686B_PM(d);
+    ViaPMState *s = VIA_PM(d);
 
     trace_via_pm_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
@@ -127,7 +128,7 @@ static const MemoryRegionOps pm_io_ops = {
     },
 };
 
-static void pm_update_sci(VT686PMState *s)
+static void pm_update_sci(ViaPMState *s)
 {
     int sci_level, pmsts;
 
@@ -145,13 +146,13 @@ static void pm_update_sci(VT686PMState *s)
 
 static void pm_tmr_timer(ACPIREGS *ar)
 {
-    VT686PMState *s = container_of(ar, VT686PMState, ar);
+    ViaPMState *s = container_of(ar, ViaPMState, ar);
     pm_update_sci(s);
 }
 
-static void vt82c686b_pm_reset(DeviceState *d)
+static void via_pm_reset(DeviceState *d)
 {
-    VT686PMState *s = VT82C686B_PM(d);
+    ViaPMState *s = VIA_PM(d);
 
     memset(s->dev.config + PCI_CONFIG_HEADER_SIZE, 0,
            PCI_CONFIG_SPACE_SIZE - PCI_CONFIG_HEADER_SIZE);
@@ -164,9 +165,9 @@ static void vt82c686b_pm_reset(DeviceState *d)
     smb_io_space_update(s);
 }
 
-static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
+static void via_pm_realize(PCIDevice *dev, Error **errp)
 {
-    VT686PMState *s = VT82C686B_PM(dev);
+    ViaPMState *s = VIA_PM(dev);
 
     pci_set_word(dev->config + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
@@ -177,8 +178,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 
     apm_init(dev, &s->apm, NULL, s);
 
-    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s,
-                          "vt82c686-pm", 0x100);
+    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 0x100);
     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
     memory_region_set_enabled(&s->io, false);
 
@@ -187,35 +187,61 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2);
 }
 
+typedef struct via_pm_init_info {
+    uint16_t device_id;
+} ViaPMInitInfo;
+
 static void via_pm_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    ViaPMInitInfo *info = data;
 
-    k->realize = vt82c686b_pm_realize;
+    k->realize = via_pm_realize;
     k->config_write = pm_write_config;
     k->vendor_id = PCI_VENDOR_ID_VIA;
-    k->device_id = PCI_DEVICE_ID_VIA_ACPI;
+    k->device_id = info->device_id;
     k->class_id = PCI_CLASS_BRIDGE_OTHER;
     k->revision = 0x40;
-    dc->reset = vt82c686b_pm_reset;
-    dc->desc = "PM";
+    dc->reset = via_pm_reset;
     /* Reason: part of VIA south bridge, does not exist stand alone */
     dc->user_creatable = false;
     dc->vmsd = &vmstate_acpi;
 }
 
 static const TypeInfo via_pm_info = {
-    .name          = TYPE_VT82C686B_PM,
+    .name          = TYPE_VIA_PM,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VT686PMState),
-    .class_init    = via_pm_class_init,
+    .instance_size = sizeof(ViaPMState),
+    .abstract      = true,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { },
     },
 };
 
+static const ViaPMInitInfo vt82c686b_pm_init_info = {
+    .device_id = PCI_DEVICE_ID_VIA_82C686B_PM,
+};
+
+static const TypeInfo vt82c686b_pm_info = {
+    .name          = TYPE_VT82C686B_PM,
+    .parent        = TYPE_VIA_PM,
+    .class_init    = via_pm_class_init,
+    .class_data    = (void *)&vt82c686b_pm_init_info,
+};
+
+static const ViaPMInitInfo vt8231_pm_init_info = {
+    .device_id = PCI_DEVICE_ID_VIA_8231_PM,
+};
+
+static const TypeInfo vt8231_pm_info = {
+    .name          = TYPE_VT8231_PM,
+    .parent        = TYPE_VIA_PM,
+    .class_init    = via_pm_class_init,
+    .class_data    = (void *)&vt8231_pm_init_info,
+};
+
 
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
@@ -424,6 +450,8 @@ static const TypeInfo via_superio_info = {
 static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
+    type_register_static(&vt82c686b_pm_info);
+    type_register_static(&vt8231_pm_info);
     type_register_static(&via_info);
     type_register_static(&via_superio_info);
 }
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 5b0a1ffe72..9b6d610e83 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -4,6 +4,7 @@
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
+#define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"
 
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 11f8ab7149..7c183d16f9 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -204,9 +204,10 @@
 #define PCI_DEVICE_ID_VIA_ISA_BRIDGE     0x0686
 #define PCI_DEVICE_ID_VIA_IDE            0x0571
 #define PCI_DEVICE_ID_VIA_UHCI           0x3038
-#define PCI_DEVICE_ID_VIA_ACPI           0x3057
+#define PCI_DEVICE_ID_VIA_82C686B_PM     0x3057
 #define PCI_DEVICE_ID_VIA_AC97           0x3058
 #define PCI_DEVICE_ID_VIA_MC97           0x3068
+#define PCI_DEVICE_ID_VIA_8231_PM        0x8235
 
 #define PCI_VENDOR_ID_MARVELL            0x11ab
 
-- 
2.21.3

Re: [PATCH v2 06/13] vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm based on it

[Philippe Mathieu-Daudé]

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> The vt82c686b-pm model can be shared between VT82C686B and VT8231. The
> only difference between the two is the device id in what we emulate so
> make an abstract via-pm model by renaming appropriately and add types
> for vt82c686b-pm and vt8231-pm based on it.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 84 ++++++++++++++++++++++++++-------------
>  include/hw/isa/vt82c686.h |  1 +
>  include/hw/pci/pci_ids.h  |  3 +-
>  3 files changed, 59 insertions(+), 29 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 10/13] vt82c686: Implement control of serial port io ranges via config regs

[BALATON Zoltan]

In VIA super south bridge the io ranges of superio components
(parallel and serial ports and FDC) can be controlled by superio
config registers to set their base address and enable/disable them.
This is not easy to implement in QEMU because ISA emulation is only
designed to set io base address once on creating the device and io
ranges are registered at creation and cannot easily be disabled or
moved later.

In this patch we hack around that but only for serial ports because
those have a single io range at port base that's relatively easy to
handle and it's what guests actually use and set address different
than the default.

We do not attempt to handle controlling the parallel and FDC regions
because those have multiple io ranges so handling them would be messy
and guests either don't change their deafult or don't care. We could
even get away with disabling and not emulating them, but since they
are already there, this patch leaves them mapped at their default
address just in case this could be useful for a guest in the future.

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

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 2921d5c55c..18bd86285b 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
     MemoryRegion io;
+    ISASuperIODevice *superio;
+    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
 } SuperIOConfig;
 
+static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
+                                    int offs)
+{
+    MemoryRegion *subregion, *mr = NULL;
+
+    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
+        if (subregion->addr == offs) {
+            mr = subregion;
+            break;
+        }
+    }
+    return mr;
+}
+
 static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
                               unsigned size)
 {
@@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
     case 0xfd ... 0xff:
         /* ignore write to read only registers */
         return;
-    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
+    case 0xe2:
+    {
+        data &= 0x1f;
+        if (data & BIT(2)) { /* Serial port 1 enable */
+            ISADevice *dev = sc->superio->serial[0];
+            if (!memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_add_subregion(isa_address_space_io(dev),
+                                            dev->ioport_id, sc->serial_io[0]);
+            }
+        } else {
+            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
+            if (memory_region_is_mapped(sc->serial_io[0])) {
+                memory_region_del_subregion(io, sc->serial_io[0]);
+            }
+        }
+        if (data & BIT(3)) { /* Serial port 2 enable */
+            ISADevice *dev = sc->superio->serial[1];
+            if (!memory_region_is_mapped(sc->serial_io[1])) {
+                memory_region_add_subregion(isa_address_space_io(dev),
+                                            dev->ioport_id, sc->serial_io[1]);
+            }
+        } else {
+            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
+            if (memory_region_is_mapped(sc->serial_io[1])) {
+                memory_region_del_subregion(io, sc->serial_io[1]);
+            }
+        }
+        break;
+    }
+    case 0xe7: /* Serial port 1 io base address */
+    {
+        data &= 0xfe;
+        sc->superio->serial[0]->ioport_id = data << 2;
+        if (memory_region_is_mapped(sc->serial_io[0])) {
+            memory_region_set_address(sc->serial_io[0], data << 2);
+        }
+        break;
+    }
+    case 0xe8: /* Serial port 2 io base address */
+    {
+        data &= 0xfe;
+        sc->superio->serial[1]->ioport_id = data << 2;
+        if (memory_region_is_mapped(sc->serial_io[1])) {
+            memory_region_set_address(sc->serial_io[1], data << 2);
+        }
+        break;
+    }
     default:
         qemu_log_mask(LOG_UNIMP,
                       "via_superio_cfg: unimplemented register 0x%x\n", idx);
@@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
     qemu_irq *isa_irq;
+    ISASuperIOClass *ic;
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
@@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
+    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
+                                                      TYPE_VT82C686B_SUPERIO));
+    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
@@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
      */
     memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
                                 &s->superio_cfg.io);
+
+    /* Grab io regions of serial devices so we can control them */
+    for (i = 0; i < ic->serial.count; i++) {
+        ISADevice *sd = s->superio_cfg.superio->serial[i];
+        MemoryRegion *io = isa_address_space_io(sd);
+        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
+        if (!mr) {
+            error_setg(errp, "Could not get io region for serial %d", i);
+            return;
+        }
+        s->superio_cfg.serial_io[i] = mr;
+        if (memory_region_is_mapped(mr)) {
+            memory_region_del_subregion(io, mr);
+        }
+    }
 }
 
 static void via_class_init(ObjectClass *klass, void *data)
-- 
2.21.3

Re: [PATCH v2 10/13] vt82c686: Implement control of serial port io ranges via config regs

[Philippe Mathieu-Daudé]

Cc'ing Paolo (Memory API maintainer).

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> In VIA super south bridge the io ranges of superio components
> (parallel and serial ports and FDC) can be controlled by superio
> config registers to set their base address and enable/disable them.
> This is not easy to implement in QEMU because ISA emulation is only
> designed to set io base address once on creating the device and io
> ranges are registered at creation and cannot easily be disabled or
> moved later.
> 
> In this patch we hack around that but only for serial ports because
> those have a single io range at port base that's relatively easy to
> handle and it's what guests actually use and set address different
> than the default.
> 
> We do not attempt to handle controlling the parallel and FDC regions
> because those have multiple io ranges so handling them would be messy
> and guests either don't change their deafult or don't care. We could
> even get away with disabling and not emulating them, but since they
> are already there, this patch leaves them mapped at their default
> address just in case this could be useful for a guest in the future.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 2921d5c55c..18bd86285b 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
>  typedef struct SuperIOConfig {
>      uint8_t regs[0x100];
>      MemoryRegion io;
> +    ISASuperIODevice *superio;
> +    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
>  } SuperIOConfig;
>  
> +static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
> +                                    int offs)
> +{
> +    MemoryRegion *subregion, *mr = NULL;
> +
> +    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
> +        if (subregion->addr == offs) {
> +            mr = subregion;

Shouldn't we use memory_region_find() here?

Also, why not have a proper helper in "hw/isa/isa.h"?

> +            break;
> +        }
> +    }
> +    return mr;
> +}
> +
>  static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>                                unsigned size)
>  {
> @@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>      case 0xfd ... 0xff:
>          /* ignore write to read only registers */
>          return;
> -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
> +    case 0xe2:
> +    {
> +        data &= 0x1f;
> +        if (data & BIT(2)) { /* Serial port 1 enable */
> +            ISADevice *dev = sc->superio->serial[0];
> +            if (!memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[0]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
> +            if (memory_region_is_mapped(sc->serial_io[0])) {
> +                memory_region_del_subregion(io, sc->serial_io[0]);
> +            }
> +        }
> +        if (data & BIT(3)) { /* Serial port 2 enable */
> +            ISADevice *dev = sc->superio->serial[1];
> +            if (!memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_add_subregion(isa_address_space_io(dev),
> +                                            dev->ioport_id, sc->serial_io[1]);
> +            }
> +        } else {
> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
> +            if (memory_region_is_mapped(sc->serial_io[1])) {
> +                memory_region_del_subregion(io, sc->serial_io[1]);
> +            }
> +        }
> +        break;
> +    }
> +    case 0xe7: /* Serial port 1 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[0]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[0])) {
> +            memory_region_set_address(sc->serial_io[0], data << 2);
> +        }
> +        break;
> +    }
> +    case 0xe8: /* Serial port 2 io base address */
> +    {
> +        data &= 0xfe;
> +        sc->superio->serial[1]->ioport_id = data << 2;
> +        if (memory_region_is_mapped(sc->serial_io[1])) {
> +            memory_region_set_address(sc->serial_io[1], data << 2);
> +        }
> +        break;
> +    }
>      default:
>          qemu_log_mask(LOG_UNIMP,
>                        "via_superio_cfg: unimplemented register 0x%x\n", idx);
> @@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      DeviceState *dev = DEVICE(d);
>      ISABus *isa_bus;
>      qemu_irq *isa_irq;
> +    ISASuperIOClass *ic;
>      int i;
>  
>      qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> @@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>      isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
>      i8254_pit_init(isa_bus, 0x40, 0, NULL);
>      i8257_dma_init(isa_bus, 0);
> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
> +                                                      TYPE_VT82C686B_SUPERIO));
> +    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
>      mc146818_rtc_init(isa_bus, 2000, NULL);
>  
>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> @@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>       */
>      memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
>                                  &s->superio_cfg.io);
> +
> +    /* Grab io regions of serial devices so we can control them */
> +    for (i = 0; i < ic->serial.count; i++) {
> +        ISADevice *sd = s->superio_cfg.superio->serial[i];
> +        MemoryRegion *io = isa_address_space_io(sd);
> +        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
> +        if (!mr) {
> +            error_setg(errp, "Could not get io region for serial %d", i);
> +            return;
> +        }
> +        s->superio_cfg.serial_io[i] = mr;
> +        if (memory_region_is_mapped(mr)) {
> +            memory_region_del_subregion(io, mr);
> +        }
> +    }
>  }
>  
>  static void via_class_init(ObjectClass *klass, void *data)
> 

Re: [PATCH v2 10/13] vt82c686: Implement control of serial port io ranges via config regs

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 8493 bytes --]

On Sat, 20 Feb 2021, Philippe Mathieu-Daudé wrote:
> Cc'ing Paolo (Memory API maintainer).
>
> On 1/9/21 9:16 PM, BALATON Zoltan wrote:
>> In VIA super south bridge the io ranges of superio components
>> (parallel and serial ports and FDC) can be controlled by superio
>> config registers to set their base address and enable/disable them.
>> This is not easy to implement in QEMU because ISA emulation is only
>> designed to set io base address once on creating the device and io
>> ranges are registered at creation and cannot easily be disabled or
>> moved later.
>>
>> In this patch we hack around that but only for serial ports because
>> those have a single io range at port base that's relatively easy to
>> handle and it's what guests actually use and set address different
>> than the default.
>>
>> We do not attempt to handle controlling the parallel and FDC regions
>> because those have multiple io ranges so handling them would be messy
>> and guests either don't change their deafult or don't care. We could
>> even get away with disabling and not emulating them, but since they
>> are already there, this patch leaves them mapped at their default
>> address just in case this could be useful for a guest in the future.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c | 84 +++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 2921d5c55c..18bd86285b 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -252,8 +252,24 @@ static const TypeInfo vt8231_pm_info = {
>>  typedef struct SuperIOConfig {
>>      uint8_t regs[0x100];
>>      MemoryRegion io;
>> +    ISASuperIODevice *superio;
>> +    MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
>>  } SuperIOConfig;
>>
>> +static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
>> +                                    int offs)
>> +{
>> +    MemoryRegion *subregion, *mr = NULL;
>> +
>> +    QTAILQ_FOREACH(subregion, &parent->subregions, subregions_link) {
>> +        if (subregion->addr == offs) {
>> +            mr = subregion;
>
> Shouldn't we use memory_region_find() here?

I think I've looked at that but it did not do what I needed (which is 
returning the actual region at the address) but this approach was used 
instead by some s390x code I think where I've got it from.

> Also, why not have a proper helper in "hw/isa/isa.h"?

Maybe because nothing needed it so far? Also I've tried that first but 
some ISA devices have multiple memory regions so it's not easy to return 
all of them so a helper would only work for ISA devices that have a single 
region.

As the patch description says this is basically a hack to be able to 
implement this behaviour of this VIA chip without rewriting ISA emulation 
that I'd rather not do because 1) it could break a lot of other machines 
and 2) takes a lot of time so I went with this approach that's not so bad 
for just this use case and could be cleaned up later if somebody would 
want to clean up ISA emulation to support this use case as well. Currently 
ISA seems to be designed to only handle devices that are created once at 
the start and not changed later but these VIA superio chips have ISA 
devices that can be enabled/disabled and their IO ranges changed by 
writing registers. That's not easily emulated in current QEMU so this is a 
compromise to make it work with the guests but avoid needing extensive 
changes to QEMU ISA parts. (We only really need to change serial address 
because VT8231 has a single serial port and pegasos2 firmware puts it at 
0x2f8 instead of the usual default of 0x3f8 so we can't get away with just 
mapping the serial device at the default address. It could work by putting 
it at 0x2f8 but that may break other guests and this way we can actually 
emulate what the chip does so it works for both 82c686b and 8231 and more 
guests should be happy with it so I think this should be good enough for 
now.)

Regards,
BALATON Zoltan

>> +            break;
>> +        }
>> +    }
>> +    return mr;
>> +}
>> +
>>  static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>>                                unsigned size)
>>  {
>> @@ -279,7 +295,53 @@ static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
>>      case 0xfd ... 0xff:
>>          /* ignore write to read only registers */
>>          return;
>> -    /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */
>> +    case 0xe2:
>> +    {
>> +        data &= 0x1f;
>> +        if (data & BIT(2)) { /* Serial port 1 enable */
>> +            ISADevice *dev = sc->superio->serial[0];
>> +            if (!memory_region_is_mapped(sc->serial_io[0])) {
>> +                memory_region_add_subregion(isa_address_space_io(dev),
>> +                                            dev->ioport_id, sc->serial_io[0]);
>> +            }
>> +        } else {
>> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
>> +            if (memory_region_is_mapped(sc->serial_io[0])) {
>> +                memory_region_del_subregion(io, sc->serial_io[0]);
>> +            }
>> +        }
>> +        if (data & BIT(3)) { /* Serial port 2 enable */
>> +            ISADevice *dev = sc->superio->serial[1];
>> +            if (!memory_region_is_mapped(sc->serial_io[1])) {
>> +                memory_region_add_subregion(isa_address_space_io(dev),
>> +                                            dev->ioport_id, sc->serial_io[1]);
>> +            }
>> +        } else {
>> +            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
>> +            if (memory_region_is_mapped(sc->serial_io[1])) {
>> +                memory_region_del_subregion(io, sc->serial_io[1]);
>> +            }
>> +        }
>> +        break;
>> +    }
>> +    case 0xe7: /* Serial port 1 io base address */
>> +    {
>> +        data &= 0xfe;
>> +        sc->superio->serial[0]->ioport_id = data << 2;
>> +        if (memory_region_is_mapped(sc->serial_io[0])) {
>> +            memory_region_set_address(sc->serial_io[0], data << 2);
>> +        }
>> +        break;
>> +    }
>> +    case 0xe8: /* Serial port 2 io base address */
>> +    {
>> +        data &= 0xfe;
>> +        sc->superio->serial[1]->ioport_id = data << 2;
>> +        if (memory_region_is_mapped(sc->serial_io[1])) {
>> +            memory_region_set_address(sc->serial_io[1], data << 2);
>> +        }
>> +        break;
>> +    }
>>      default:
>>          qemu_log_mask(LOG_UNIMP,
>>                        "via_superio_cfg: unimplemented register 0x%x\n", idx);
>> @@ -385,6 +447,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>      DeviceState *dev = DEVICE(d);
>>      ISABus *isa_bus;
>>      qemu_irq *isa_irq;
>> +    ISASuperIOClass *ic;
>>      int i;
>>
>>      qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>> @@ -394,7 +457,9 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>      isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
>>      i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>      i8257_dma_init(isa_bus, 0);
>> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>> +    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
>> +                                                      TYPE_VT82C686B_SUPERIO));
>> +    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
>>      mc146818_rtc_init(isa_bus, 2000, NULL);
>>
>>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>> @@ -412,6 +477,21 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>       */
>>      memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
>>                                  &s->superio_cfg.io);
>> +
>> +    /* Grab io regions of serial devices so we can control them */
>> +    for (i = 0; i < ic->serial.count; i++) {
>> +        ISADevice *sd = s->superio_cfg.superio->serial[i];
>> +        MemoryRegion *io = isa_address_space_io(sd);
>> +        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
>> +        if (!mr) {
>> +            error_setg(errp, "Could not get io region for serial %d", i);
>> +            return;
>> +        }
>> +        s->superio_cfg.serial_io[i] = mr;
>> +        if (memory_region_is_mapped(mr)) {
>> +            memory_region_del_subregion(io, mr);
>> +        }
>> +    }
>>  }
>>
>>  static void via_class_init(ObjectClass *klass, void *data)
>>
>
>

Re: [PATCH v2 00/13] vt82c686b clean ups and vt8231 emulation

[Philippe Mathieu-Daudé]

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> Version 2 of remaining patches for VT8231 emulation addressing review
> comments:
> 
> - Split off making vt82c686b-pm an abstract class to separate patch
> - Use constants for PCI IDs
> 
> Regards,
> BALATON Zoltan
> 
> 
> BALATON Zoltan (13):
>   vt82c686: Move superio memory region to SuperIOConfig struct
>   vt82c686: Reorganise code
>   vt82c686: Fix SMBus IO base and configuration registers
>   vt82c686: Fix up power management io base and config
>   vt82c686: Set user_creatable=false for VT82C686B_PM
>   vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm
>     based on it
>   vt82c686: Simplify vt82c686b_realize()
>   vt82c686: Move creation of ISA devices to the ISA bridge
>   vt82c686: Fix superio_cfg_{read,write}() functions
>   vt82c686: Implement control of serial port io ranges via config regs
>   vt82c686: QOM-ify superio related functionality
>   vt82c686: Add VT8231_SUPERIO based on VIA_SUPERIO
>   vt82c686: Add emulation of VT8231 south bridge
> 
>  hw/isa/trace-events       |   2 +
>  hw/isa/vt82c686.c         | 891 ++++++++++++++++++++++++++++----------
>  hw/mips/fuloong2e.c       |  33 +-
>  include/hw/isa/vt82c686.h |   3 +-
>  include/hw/pci/pci_ids.h  |   6 +-
>  5 files changed, 684 insertions(+), 251 deletions(-)

I'm queuing patches 1-9 via mips-next. I'd rather see the last
4 patches go via the PPC tree or via Paolo's misc tree.

Thanks,

Phil.