From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46568 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOxtO-0004Am-OV for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:03:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOxtN-0002H7-AP for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:03:18 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:40039) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOxtN-0002Gz-5J for qemu-devel@nongnu.org; Wed, 16 Jun 2010 15:03:17 -0400 Received: by pvg11 with SMTP id 11so2487391pvg.4 for ; Wed, 16 Jun 2010 12:03:16 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20100616185134.GC7183@redhat.com> References: <20100615091207.GA1365@redhat.com> <20100616022002.GB7932@valinux.co.jp> <20100616085425.GA4637@redhat.com> <20100616185134.GC7183@redhat.com> From: Blue Swirl Date: Wed, 16 Jun 2010 19:02:54 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 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: "Michael S. Tsirkin" Cc: Isaku Yamahata , qemu-devel@nongnu.org On Wed, Jun 16, 2010 at 6:51 PM, Michael S. Tsirkin wrote: > 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 wro= te: >> > 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 wrote: >> >> > 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() sets >> >> > > appropriately is lost. >> >> > > Anyway PCI_HEADER_TYPE_NORMAL is zero, so it is unnecessary to ze= ro >> >> > > 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) >> >> > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PC= I_STATUS_DEVSEL_MEDIUM); >> >> > > =C2=A0 =C2=A0 =C2=A0pci_config_set_class(d->config, PCI_CLASS_BRI= DGE_HOST); >> >> > > =C2=A0 =C2=A0 =C2=A0pci_set_byte(d->config + PCI_HEADER_TYPE, >> >> > > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 PCI_HEA= DER_TYPE_NORMAL); >> >> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (pci_ge= t_byte(d->config + PCI_HEADER_TYPE) & >> >> > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0P= CI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); >> >> > >> >> > what is this doing? >> >> >> >> It changes the header type to normal device(bit 1-7) without overwrit= ing >> >> 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 wants= . >> > >> > BTW I think it would be prettier to have is_bridge instead of header_t= ype >> > as a qdev property. Agree? >> >> Good idea. >> >> >> So I touch only multi function bit(bit 8) and leave other bit (bit 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_pci.c = then. > > Yes, but what header type does it need? 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). >> >> static PCIDeviceInfo pbm_pci_host_info =3D { >> >> =C2=A0 =C2=A0 .qdev.name =3D "pbm", >> >> =C2=A0 =C2=A0 .qdev.size =3D sizeof(PCIDevice), >> >> =C2=A0 =C2=A0 .init =C2=A0 =C2=A0 =C2=A0=3D pbm_pci_host_init, >> >> =C2=A0 =C2=A0 .header_type =C2=A0=3D PCI_HEADER_TYPE_BRIDGE, =C2=A0 <= <<<< Here >> >> }; >> >> >> >> -- >> >> yamahata >> > >