From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33983 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOyGj-00018k-RV for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:27:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOyGe-0006fT-L9 for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:27:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54174) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOyGe-0006fI-Dt for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:27:20 -0400 Date: Wed, 16 Jun 2010 22:22:23 +0300 From: "Michael S. Tsirkin" Message-ID: <20100616192223.GA7530@redhat.com> References: <20100615091207.GA1365@redhat.com> <20100616022002.GB7932@valinux.co.jp> <20100616085425.GA4637@redhat.com> <20100616185134.GC7183@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Isaku Yamahata , qemu-devel@nongnu.org On Wed, Jun 16, 2010 at 07:02:54PM +0000, Blue Swirl wrote: > On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin wr= ote: > > On Wed, Jun 16, 2010 at 06:41:22PM +0000, Blue Swirl wrote: > >> On Wed, Jun 16, 2010 at 8:54 AM, Michael S. Tsirkin = wrote: > >> > On Wed, Jun 16, 2010 at 11:20:02AM +0900, Isaku Yamahata wrote: > >> >> On Tue, Jun 15, 2010 at 12:12:07PM +0300, Michael S. Tsirkin wrot= e: > >> >> > On Tue, Jun 15, 2010 at 02:06:46PM +0900, Isaku Yamahata wrote: > >> >> > > Don't overwrite pci header type. > >> >> > > Otherwise, multi function bit which pci_init_header_type() se= ts > >> >> > > appropriately is lost. > >> >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary t= o zero > >> >> > > which is already zero cleared. > >> >> > > > >> >> > > Signed-off-by: Isaku Yamahata > >> >> > > >> >> > ... > >> >> > > >> >> > > diff --git a/hw/apb_pci.c b/hw/apb_pci.c > >> >> > > index 31c8d70..cdf3bc2 100644 > >> >> > > --- a/hw/apb_pci.c > >> >> > > +++ b/hw/apb_pci.c > >> >> > > @@ -428,7 +428,8 @@ static int pbm_pci_host_init(PCIDevice *d= ) > >> >> > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCI_STATUS_DEVSEL_MEDIUM)= ; > >> >> > > =A0 =A0 =A0pci_config_set_class(d->config, PCI_CLASS_BRIDGE_H= OST); > >> >> > > =A0 =A0 =A0pci_set_byte(d->config + PCI_HEADER_TYPE, > >> >> > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCI_HEADER_TYPE_NORMAL); > >> >> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (pci_get_byte(d->config + P= CI_HEADER_TYPE) & > >> >> > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PCI_HEADER_TYPE_MULTI_FU= NCTION) | PCI_HEADER_TYPE_NORMAL); > >> >> > > >> >> > what is this doing? > >> >> > >> >> It changes the header type to normal device(bit 1-7) without over= writing > >> >> multi function bit(bit 8). > >> > > >> > Don't we know what the multi function bit value is? > >> > > >> >> Apb host bridge specifies PCI_HEADER_TYPE_BRIDGE in PCIDeviceInfo= , > >> >> on the other hand pbc_pci_host_init() sets the register > >> >> to PCI_HEADER_TYPE_NORMAL. > >> >> To be honest I don't know why it does so, but that is what Blue w= ants. > >> > > >> > BTW I think it would be prettier to have is_bridge instead of head= er_type > >> > as a qdev property. Agree? > >> > >> Good idea. > >> > >> >> So I touch only multi function bit(bit 8) and leave other bit (bi= t 1-7) > >> >> unchanged. > >> >> > >> >> If you don't like this hunk, I'll drop this hunk and leave it to = Blue. > >> >> What do you think? > >> > > >> > Blue Swirl, could you comment on this please? > >> > >> I'd go for is_bridge and drop the override for header type in apb_pc= i.c then. > > > > Yes, but what header type does it need? >=20 > The type should be bridge (to allow writes to bridge registers), but > PCI header should use PCI_HEADER_TYPE_NORMAL (because the PBM > specification says so). I can no longer get the PBM specs now: are there alternative links? Need to fix links in code. > >> >> static PCIDeviceInfo pbm_pci_host_info =3D { > >> >> =A0 =A0 .qdev.name =3D "pbm", > >> >> =A0 =A0 .qdev.size =3D sizeof(PCIDevice), > >> >> =A0 =A0 .init =A0 =A0 =A0=3D pbm_pci_host_init, > >> >> =A0 =A0 .header_type =A0=3D PCI_HEADER_TYPE_BRIDGE, =A0 <<<<< Her= e > >> >> }; > >> >> > >> >> -- > >> >> yamahata > >> > > >