From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36739 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOqjY-0005di-Cx for qemu-devel@nongnu.org; Wed, 16 Jun 2010 07:24:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOqjW-0001S8-Jj for qemu-devel@nongnu.org; Wed, 16 Jun 2010 07:24:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46573) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOqjW-0001Rv-Bt for qemu-devel@nongnu.org; Wed, 16 Jun 2010 07:24:38 -0400 Date: Wed, 16 Jun 2010 14:19:44 +0300 From: "Michael S. Tsirkin" Message-ID: <20100616111944.GA5661@redhat.com> References: <20100615091207.GA1365@redhat.com> <20100616022002.GB7932@valinux.co.jp> <20100616085425.GA4637@redhat.com> <20100616094353.GB23748@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100616094353.GB23748@valinux.co.jp> 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: Isaku Yamahata Cc: blauwirbel@gmail.com, qemu-devel@nongnu.org On Wed, Jun 16, 2010 at 06:43:53PM +0900, Isaku Yamahata wrote: > On Wed, Jun 16, 2010 at 11:54:25AM +0300, 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 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 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) > > > > > PCI_STATUS_DEVSEL_MEDIUM); > > > > > pci_config_set_class(d->config, PCI_CLASS_BRIDGE_HOST); > > > > > pci_set_byte(d->config + PCI_HEADER_TYPE, > > > > > - PCI_HEADER_TYPE_NORMAL); > > > > > + (pci_get_byte(d->config + PCI_HEADER_TYPE) & > > > > > + PCI_HEADER_TYPE_MULTI_FUNCTION) | PCI_HEADER_TYPE_NORMAL); > > > > > > > > what is this doing? > > > > > > It changes the header type to normal device(bit 1-7) without overwriting > > > multi function bit(bit 8). > > > > Don't we know what the multi function bit value is? > > pci generic initialization, pci_qdev_init(), in pci.c sets (or clears) the bit > and then calls the device specific initialization function, pbm_pci_host_init() > in this case. > So we shouldn't clear the bit unconditionally. > > > > > 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_type > > as a qdev property. Agree? > > The spec version 3.0 defines three header types. > 0:normal device, 1:pci-to-pci bridge, 2:card bus bridge > So I'd like the name a bit more generic than is_bridge. > Any suggestion? Could we just have functions that set up header for each type, such as pci_init_normal_header() pci_init_p2p_bridge_header() pci_init_cardbus_header() > > > 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? > > > > > static PCIDeviceInfo pbm_pci_host_info = { > > > .qdev.name = "pbm", > > > .qdev.size = sizeof(PCIDevice), > > > .init = pbm_pci_host_init, > > > .header_type = PCI_HEADER_TYPE_BRIDGE, <<<<< Here > > > }; > > > > > > -- > > > yamahata > > > > -- > yamahata