From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40841 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PVI86-0003gf-LV for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:24:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PVI7g-0003cx-D4 for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:24:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42836) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PVI7g-0003cc-5f for qemu-devel@nongnu.org; Wed, 22 Dec 2010 01:24:28 -0500 Date: Wed, 22 Dec 2010 08:24:02 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space Message-ID: <20101222062401.GA7814@redhat.com> References: <1292973623-2455-1-git-send-email-andreas.faerber@web.de> <20101222025014.GA20337@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20101222025014.GA20337@valinux.co.jp> Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: Andreas =?iso-8859-1?Q?F=E4rber?= , =?iso-8859-1?Q?Herv=E9?= Poussineau , qemu-devel@nongnu.org On Wed, Dec 22, 2010 at 11:50:14AM +0900, Isaku Yamahata wrote: > On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas F=E4rber wrote: > > From: Herv=E9 Poussineau > >=20 > > v1: > > * Rebased. > >=20 > > Signed-off-by: Herv=E9 Poussineau > > Cc: Michael S. Tsirkin > > Signed-off-by: Andreas F=E4rber > > --- > > =20 > > Hello Michael, > > =20 > > Could you please take a look at this? I'm out of my field here. > > =20 > > The intention of the first part appears to be to save (val & ~mask), > > whereas the inline helper would've returned (val & mask). >=20 > 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. >=20 >=20 > > The second part makes existing code conditional on that value. >=20 > What issue are you addressing? Yes I'd like to know that too: > 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 c= lear > those registers. > Anyway after cold/warm reset firmware sets up BARs, so it doesn't matte= r. > You, however, seem to want to keep BARs over resets. >=20 > thanks, >=20 >=20 > > =20 > > Regards, > > Andreas > > =20 > > hw/pci.c | 22 ++++++++++++++-------- > > 1 files changed, 14 insertions(+), 8 deletions(-) > >=20 > > 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; > > } > > =20 > > - 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))) { You want to make memory/io bits in the command register read-only and hardwired to 1? This seems out of spec, no? > > + /* Reset region address, as it it is not read-only */ > > + 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 > >=20 > >=20 >=20 > --=20 > yamahata