From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVqon-0007Jy-Od for qemu-devel@nongnu.org; Sun, 12 Jun 2011 15:59:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QVqom-0000mM-3h for qemu-devel@nongnu.org; Sun, 12 Jun 2011 15:59:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37984) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVqol-0000mH-L5 for qemu-devel@nongnu.org; Sun, 12 Jun 2011 15:59:32 -0400 Date: Sun, 12 Jun 2011 22:59:46 +0300 From: "Michael S. Tsirkin" Message-ID: <20110612195946.GB20615@redhat.com> References: <1292973623-2455-1-git-send-email-andreas.faerber@web.de> <20101222025014.GA20337@valinux.co.jp> <4D119B09.30103@reactos.org> <20101222064921.GE7814@redhat.com> <193FFA10-B7C4-4928-ADDE-C874C56BE70C@web.de> <4DF4B3C3.7000907@reactos.org> <20110612193327.GA20531@redhat.com> <4DF51895.4030302@reactos.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <4DF51895.4030302@reactos.org> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Herv=E9?= Poussineau Cc: Isaku Yamahata , Andreas =?iso-8859-1?Q?F=E4rber?= , qemu-devel Developers On Sun, Jun 12, 2011 at 09:50:45PM +0200, Herv=E9 Poussineau wrote: > Michael S. Tsirkin a =E9crit : > >On Sun, Jun 12, 2011 at 02:40:35PM +0200, Herv=E9 Poussineau wrote: > >>Andreas, > >> > >>Andreas F=E4rber a =E9crit : > >>>Herv=E9, > >>> > >>>Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin: > >>> > >>>>On Wed, Dec 22, 2010 at 07:30:33AM +0100, Herv=E9 Poussineau wrote: > >>>>>Isaku Yamahata a =E9crit : > >>>>>>On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas F=E4rber wrote: > >>>>>>>From: Herv=E9 Poussineau > >>>>>>> > >>>>>>>v1: > >>>>>>>* Rebased. > >>>>>>> > >>>>>>>Signed-off-by: Herv=E9 Poussineau > >>>>>>>Cc: Michael S. Tsirkin > >>>>>>>Signed-off-by: Andreas F=E4rber > >>>>>>>--- > >>>>>>>Hello Michael, > >>>>>>>Could you please take a look at this? I'm out of my field here. > >>>>>>>The intention of the first part appears to be to save (val & ~ma= sk), > >>>>>>>whereas the inline helper would've returned (val & mask). > >>>>>>Such behavior is intended. > >>>>>>The returned value is just discarded in this case. > >>>>>>test-and-clear means > >>>>>> clear the bits > >>>>>> return if those cleared bits were really set. > >>>>>> > >>>>What about this first chunk? Is it necessary. > >>>> > >>>>>>>The second part makes existing code conditional on that value. > >>>>>>What issue are you addressing? > >>>>>>Although the spec doesn't says about the default value of > >>>>>>BAR registers > >>>>>>after reset, the current code assumes that almost all the > >>>>>>pci devices clear > >>>>>>those registers. > >>>>>>Anyway after cold/warm reset firmware sets up BARs, so it > >>>>>>doesn't matter. > >>>>>>You, however, seem to want to keep BARs over resets. > >>>>>> > >>>>>>thanks, > >>>>>> > >>>>>> > >>>>>As you have seen, the intend here is to be able to keep BARs > >>>>>over resets. > >>>>>It is required for some really specific devices, like a PCI to ISA > >>>>>bridge, where MMIO is always at the same address. > >>>>>In that case, the device keeps PCI_COMMAND_MEMORY and/or > >>>>>PCI_COMMAND_IO flags as read-only. > >>>>> > >>>>>Herv=E9 > >>>>Aha. Are the BARs still writeable? If not maybe that's the right t= hing > >>>>to check? If yes maybe the device simply should have a reset > >>>>handler to rewrite them? > >>>I haven't noticed a follow-up patch of yours. > >>> > >>>Since I don't know what to do here, I'll have to take this out of > >>>the PReP queue for now. > >>>Without this patch, I get to at least the second bootloader icon, > >>>the PCI graphics still work. What particular symptoms did you > >>>observe wrt the i82378 that we can reproduce? > >>> > >>>Thanks, > >>>Andreas > >>> > >>Try do do info qtree with and without this patch, and check the > >>i82378 device > >>With this patch, I have: > >> bar 0: mem at 0x80000000 [0x8000ffff] > >> bar 1: mem at 0xc0000000 [0xc0ffffff] > >> > >>Without it, I have: > >> bar 0: mem at 0xffffffffffffffff [0xfffe] > >> bar 1: mem at 0xffffffffffffffff [0xfffffe] > >> > >>I think that firmware doesn't initialize BARs for this device. > > > >Interesting. So what set the BARs to these values (0x80000000 > >and 0xc0000000) > Currently, those default values are stored in qdev properties of device= , > and device fills in BARs addresses in its init function >=20 > +static int pci_i82378_init(PCIDevice *pci_dev) > ... > + /* Make addresses read only */ > + pci_set_word(pci_dev->wmask + PCI_COMMAND, > + PCI_COMMAND_SPECIAL); > + pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 0 * 4, pci->isa_io_ba= se); > + pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 1 * 4, pci->isa_mem_b= ase); >=20 > Regards >=20 > Herv=E9 >=20 I see. So the right thing to do wrt BARs is probably to restore these values on system reset as well? It might be a good idea to replace 'Make addresses read only' with 'only special cycle enable bit is writeable'. --=20 MST