Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set

[Laszlo Ersek]

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