From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49823) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtdZE-000742-I2 for qemu-devel@nongnu.org; Fri, 11 Jan 2013 07:18:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TtdZ7-0006Tw-0T for qemu-devel@nongnu.org; Fri, 11 Jan 2013 07:18:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11526) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TtdZ6-0006Tl-Pb for qemu-devel@nongnu.org; Fri, 11 Jan 2013 07:18:28 -0500 Message-ID: <50F00384.10907@redhat.com> Date: Fri, 11 Jan 2013 13:20:20 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1357681452-24963-1-git-send-email-lersek@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: akong@redhat.com, qemu-devel@nongnu.org On 01/09/13 22:01, Blue Swirl wrote: > On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek 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