From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49031 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P8B6E-0003a0-LP for qemu-devel@nongnu.org; Tue, 19 Oct 2010 08:15:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P8B6D-0006XE-CT for qemu-devel@nongnu.org; Tue, 19 Oct 2010 08:15:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43358) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P8B6D-0006X9-5b for qemu-devel@nongnu.org; Tue, 19 Oct 2010 08:15:25 -0400 Date: Tue, 19 Oct 2010 14:07:28 +0200 From: "Michael S. Tsirkin" Message-ID: <20101019120728.GA6970@redhat.com> References: <8370077d844866e4b4b2866f16c21a948f84c792.1287478251.git.yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8370077d844866e4b4b2866f16c21a948f84c792.1287478251.git.yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH v5 04/14] pci/bridge: fix pci_bridge_reset() List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: skandasa@cisco.com, adnan@khaleel.us, etmartin@cisco.com, qemu-devel@nongnu.org, wexu2@cisco.com On Tue, Oct 19, 2010 at 06:06:31PM +0900, Isaku Yamahata wrote: > The default value of base/limit registers aren't specified in the spec. > So pci_bridge_reset() shouldn't touch them. > Instead, introduced two functions to reset those registers in a way > of typical implementation. zero base/limit registers or disable forwarding. > They will be used later. > > Signed-off-by: Isaku Yamahata > --- I have some second thoughts: 1. pci_bridge_reset is used in several devices and I have no idea what the reset change will do there, how do real devices behave or whether guests depend on a specific behaviour. It seems harmless to leave the current implementation in place, and simply add pci_bridge_disable_base_limit which devices can call after pci_bridge_reset. 2. _zero and _disable describe what function does already, so we should drop _reset from function name. Thus we only get 1 new function, pci_bridge_disable_base_limit. > Changes v4 -> v5: > - drop the lines in pci_bridge_reset() > - introduced two functions to reset base/limit registers. > --- > hw/pci_bridge.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++------- > hw/pci_bridge.h | 2 + > 2 files changed, 51 insertions(+), 8 deletions(-) > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c > index 638e3b3..de75e6a 100644 > --- a/hw/pci_bridge.c > +++ b/hw/pci_bridge.c > @@ -151,6 +151,46 @@ void pci_bridge_write_config(PCIDevice *d, > } > } > > +void pci_bridge_reset_zero_base_limit(PCIDevice *dev) > +{ > + uint8_t *conf = dev->config; > + > + pci_byte_test_and_clear_mask(conf + PCI_IO_BASE, > + PCI_IO_RANGE_MASK & 0xff); > + pci_byte_test_and_clear_mask(conf + PCI_IO_LIMIT, > + PCI_IO_RANGE_MASK & 0xff); > + pci_word_test_and_clear_mask(conf + PCI_MEMORY_BASE, > + PCI_MEMORY_RANGE_MASK & 0xffff); > + pci_word_test_and_clear_mask(conf + PCI_MEMORY_LIMIT, > + PCI_MEMORY_RANGE_MASK & 0xffff); > + pci_word_test_and_clear_mask(conf + PCI_PREF_MEMORY_BASE, > + PCI_PREF_RANGE_MASK & 0xffff); > + pci_word_test_and_clear_mask(conf + PCI_PREF_MEMORY_LIMIT, > + PCI_PREF_RANGE_MASK & 0xffff); > + pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0); > + pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0); > +} > + > +void pci_bridge_reset_disable_base_limit(PCIDevice *dev) > +{ > + uint8_t *conf = dev->config; > + > + pci_byte_test_and_set_mask(conf + PCI_IO_BASE, > + PCI_IO_RANGE_MASK & 0xff); > + pci_byte_test_and_clear_mask(conf + PCI_IO_LIMIT, > + PCI_IO_RANGE_MASK & 0xff); > + pci_word_test_and_set_mask(conf + PCI_MEMORY_BASE, > + PCI_MEMORY_RANGE_MASK & 0xffff); > + pci_word_test_and_clear_mask(conf + PCI_MEMORY_LIMIT, > + PCI_MEMORY_RANGE_MASK & 0xffff); > + pci_word_test_and_set_mask(conf + PCI_PREF_MEMORY_BASE, > + PCI_PREF_RANGE_MASK & 0xffff); > + pci_word_test_and_clear_mask(conf + PCI_PREF_MEMORY_LIMIT, > + PCI_PREF_RANGE_MASK & 0xffff); > + pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0); > + pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0); > +} > + > /* reset bridge specific configuration registers */ > void pci_bridge_reset_reg(PCIDevice *dev) > { > @@ -161,14 +201,15 @@ void pci_bridge_reset_reg(PCIDevice *dev) > conf[PCI_SUBORDINATE_BUS] = 0; > conf[PCI_SEC_LATENCY_TIMER] = 0; > > - conf[PCI_IO_BASE] = 0; > - conf[PCI_IO_LIMIT] = 0; > - pci_set_word(conf + PCI_MEMORY_BASE, 0); > - pci_set_word(conf + PCI_MEMORY_LIMIT, 0); > - pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0); > - pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0); > - pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0); > - pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0); > + /* > + * the default values for base/limit registers aren't specified > + * in the PCI-to-PCI-bridge spec. So we don't thouch them here. > + * Each implementation can override it. > + * typical implementation does > + * - zero registers: pci_bridge_reset_zer_base_limit() > + * or > + * - disable forwarding: pci_bridge_reset_disable_base_limit() > + */ > > pci_set_word(conf + PCI_BRIDGE_CONTROL, 0); > } > diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h > index f6fade0..2359684 100644 > --- a/hw/pci_bridge.h > +++ b/hw/pci_bridge.h > @@ -39,6 +39,8 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type); > > void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len); > +void pci_bridge_reset_zero_base_limit(PCIDevice *dev); > +void pci_bridge_reset_disable_base_limit(PCIDevice *dev); > void pci_bridge_reset_reg(PCIDevice *dev); > void pci_bridge_reset(DeviceState *qdev); > > -- > 1.7.1.1