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.
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.