From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45205 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PVIWC-0005wu-8J for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:49:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PVIWA-0001uR-Fc for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:49:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61649) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PVIWA-0001uH-5l for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:49:46 -0500 Date: Wed, 22 Dec 2010 08:49:22 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space Message-ID: <20101222064921.GE7814@redhat.com> References: <1292973623-2455-1-git-send-email-andreas.faerber@web.de> <20101222025014.GA20337@valinux.co.jp> <4D119B09.30103@reactos.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <4D119B09.30103@reactos.org> Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org 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@nongnu.org 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 & ~mask), > >> 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 register= s > >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 matt= er. > >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 reset= s. > 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. >=20 > Herv=E9 Aha. Are the BARs still writeable? If not maybe that's the right thing to check? If yes maybe the device simply should have a reset handler to rewrite them? Also I would like the above text (maybe a bit shortened) to replace the c= omment: /* Reset region address, as it it is not read-only */ the general idea is to document the reason the code is what it is, not restate what it does. Something similar should also go into the commit message I think. > >> Regards, > >> Andreas > >> hw/pci.c | 22 ++++++++++++++-------- > >> 1 files changed, 14 insertions(+), 8 deletions(-) > >> > >>diff --git a/hw/pci.c b/hw/pci.c > >>index ef00d20..4db4b1f 100644 > >>--- a/hw/pci.c > >>+++ b/hw/pci.c > >>@@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev) > >> static void pci_device_reset(PCIDevice *dev) > >> { > >> int r; > >>+ uint16_t cmd; > >> /* TODO: call the below unconditionally once all pci devices > >> * are qdevified */ > >> if (dev->qdev.info) { > >>@@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev) > >> dev->irq_state =3D 0; > >> pci_update_irq_status(dev); > >> /* Clear all writeable bits */ > >>- pci_word_test_and_clear_mask(dev->config + PCI_COMMAND, > >>- pci_get_word(dev->wmask + PCI_COMMA= ND) | > >>- pci_get_word(dev->w1cmask + PCI_COM= MAND)); > >>+ cmd =3D pci_get_word(dev->config + PCI_COMMAND); > >>+ cmd &=3D ~(pci_get_word(dev->wmask + PCI_COMMAND) | > >>+ pci_get_word(dev->w1cmask + PCI_COMMAND)); > >>+ pci_set_word(dev->config + PCI_COMMAND, cmd); > >> pci_word_test_and_clear_mask(dev->config + PCI_STATUS, > >> pci_get_word(dev->wmask + PCI_STATU= S) | > >> pci_get_word(dev->w1cmask + PCI_STA= TUS)); > >>@@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev) > >> continue; > >> } > >>- if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) && > >>- region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) { > >>- pci_set_quad(dev->config + pci_bar(dev, r), region->type= ); > >>- } else { > >>- pci_set_long(dev->config + pci_bar(dev, r), region->type= ); > >>+ if ((region->type =3D=3D PCI_BASE_ADDRESS_SPACE_IO && !(cmd = & PCI_COMMAND_MEMORY)) || > >>+ (region->type =3D=3D PCI_BASE_ADDRESS_SPACE_MEMORY && !(= cmd & PCI_COMMAND_IO))) { These lines are too long. > >>+ /* Reset region address, as it it is not read-only */ Checking wmask would be more straight-forward? > >>+ if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) && > >>+ region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) { > >>+ pci_set_quad(dev->config + pci_bar(dev, r), region->= type); > >>+ } else { > >>+ pci_set_long(dev->config + pci_bar(dev, r), region->= type); > >>+ } > >> } > >> } > >> pci_update_mappings(dev); > >>--=20 > >>1.7.3.4 > >> > >> > >