[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control R
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set |
Date: |
Fri, 11 Jan 2013 13:20:20 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.11) Gecko/20121116 Thunderbird/10.0.11 |
On 01/09/13 22:01, Blue Swirl wrote:
> On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <address@hidden> wrote:
>> static int i440fx_pcihost_initfn(SysBusDevice *dev)
>> {
>> PCIHostState *s = PCI_HOST_BRIDGE(dev);
>>
>> - memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
>> + i440fx_host_conf_ops.read = pci_host_conf_le_ops.read;
>
> It would be cleaner to introduce a new memory region (without this
> copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches
> accesses to 0xcf9. This may mean that pci_host_config_{read,write}
> will need to be exposed.
(3) Do you have a single region in mind that covers [ 0xcf8 ..0xcff ]
contiguously? That would require exposing pci_host_data_{read,write}
too, not only pci_host_config_{read,write}.
Plus, the config & data regions of I440FXState.parent_obj have distinct
names now ("pci-conf-idx" and "pci-conf-data"); merging them (and
inventing a new name) might be user-visible via the "info mtree" monitor
command. (At least it would differ from many of the PCI host
implementations in the tree.)
What if I change v1 like this:
--------*--------
diff --git a/hw/pci/pci_host.h b/hw/pci/pci_host.h
index 1845d4d..f5b6487 100644
--- a/hw/pci/pci_host.h
+++ b/hw/pci/pci_host.h
@@ -53,6 +53,9 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev,
uint32_t addr,
void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
+void pci_host_config_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned len);
+uint64_t pci_host_config_read(void *opaque, hwaddr addr, unsigned len);
extern const MemoryRegionOps pci_host_conf_le_ops;
extern const MemoryRegionOps pci_host_conf_be_ops;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 265c324..b0f359d 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -94,8 +94,8 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
return val;
}
-static void pci_host_config_write(void *opaque, hwaddr addr,
- uint64_t val, unsigned len)
+void pci_host_config_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned len)
{
PCIHostState *s = opaque;
@@ -107,8 +107,7 @@ static void pci_host_config_write(void *opaque, hwaddr addr,
s->config_reg = val;
}
-static uint64_t pci_host_config_read(void *opaque, hwaddr addr,
- unsigned len)
+uint64_t pci_host_config_read(void *opaque, hwaddr addr, unsigned len)
{
PCIHostState *s = opaque;
uint32_t val = s->config_reg;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 89d694c..168e20d 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -190,11 +190,11 @@ static void i440fx_host_config_write(void *opaque, hwaddr
addr,
}
return;
}
- pci_host_conf_le_ops.write(opaque, addr, val, len);
+ pci_host_config_write(opaque, addr, val, len);
}
-static MemoryRegionOps i440fx_host_conf_ops = {
- .read = NULL,
+static const MemoryRegionOps i440fx_host_conf_ops = {
+ .read = pci_host_conf_read,
.write = i440fx_host_config_write,
.endianness = DEVICE_LITTLE_ENDIAN
};
@@ -203,7 +203,6 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
{
PCIHostState *s = PCI_HOST_BRIDGE(dev);
- i440fx_host_conf_ops.read = pci_host_conf_le_ops.read;
memory_region_init_io(&s->conf_mem, &i440fx_host_conf_ops, s,
"pci-conf-idx", 4);
sysbus_add_io(dev, 0xcf8, &s->conf_mem);
--------*--------
I'm not sure at which depth you want me to stop duplicating the "base class":
- call its functions via pci_host_conf_le_ops.FIELD (v1 rfc),
- call its functions by their direct names (exposing them first, the
above),
- instead of reusing "pci_host_data_le_ops" for "pci-conf-data", create
an "i440fx_host_DATA_ops" global as well:
- pointing at pci_host_data_{read,write} (exposing them first),
- pointing at private functions calling pci_host_data_{read,write}...
Thanks,
Laszlo