From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=36663 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OOyyN-00078B-SO for qemu-devel@nongnu.org; Wed, 16 Jun 2010 16:12:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OOyyM-0006Rq-2V for qemu-devel@nongnu.org; Wed, 16 Jun 2010 16:12:31 -0400 Received: from mail-yw0-f190.google.com ([209.85.211.190]:57982) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OOyyL-0006Rj-U9 for qemu-devel@nongnu.org; Wed, 16 Jun 2010 16:12:30 -0400 Received: by ywh28 with SMTP id 28so5111154ywh.28 for ; Wed, 16 Jun 2010 13:12:29 -0700 (PDT) Message-ID: <4C193029.8040704@codemonkey.ws> Date: Wed, 16 Jun 2010 15:12:25 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH 2/2] pci: don't overwrite pci header type. References: <20100615091207.GA1365@redhat.com> <20100616022002.GB7932@valinux.co.jp> <20100616085425.GA4637@redhat.com> <20100616185134.GC7183@redhat.com> <20100616192223.GA7530@redhat.com> In-Reply-To: <20100616192223.GA7530@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Blue Swirl , Isaku Yamahata , qemu-devel@nongnu.org On 06/16/2010 02:22 PM, Michael S. Tsirkin wrote: > 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 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 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? >>>>> >>>>> >>>>>> 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? >>>>> >>>> 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). >> > I can no longer get the PBM specs now: are there > alternative links? Need to fix links in code. > BTW, I set up http://wiki.qemu.org/Documentation/HardwareManuals so we could start archiving these specification when allowed. Regards, Anthony Liguori > >>>>>> 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 >>>>>> >>>>> >>> >