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.