From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59647) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2Kh9-0006zo-NN for qemu-devel@nongnu.org; Mon, 16 May 2016 11:44:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2Kh4-0008QP-HK for qemu-devel@nongnu.org; Mon, 16 May 2016 11:44:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60037) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2Kh4-0008QB-9T for qemu-devel@nongnu.org; Mon, 16 May 2016 11:44:30 -0400 Date: Mon, 16 May 2016 09:44:28 -0600 From: Alex Williamson Message-ID: <20160516094428.51c641d7@t450s.home> In-Reply-To: <20160516095818.GC17782@pxdev.xzpeter.org> References: <1463382800-25863-1-git-send-email-peterx@redhat.com> <20160516104802-mutt-send-email-mst@redhat.com> <20160516090005.GB17782@pxdev.xzpeter.org> <20160516121635-mutt-send-email-mst@redhat.com> <20160516095818.GC17782@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] pci: fix pci_requester_id() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, pbonzini@redhat.com, jan.kiszka@web.de, rkrcmar@redhat.com, Marcel Apfelbaum On Mon, 16 May 2016 17:58:18 +0800 Peter Xu wrote: > On Mon, May 16, 2016 at 12:21:54PM +0300, Michael S. Tsirkin wrote: > [...] > > > "Legacy PCI bus, override requester ID with the bridge's BDF > > > upstream. The root complex of legacy PCI system can only get > > > requester ID from directly attached devices (including bridges). > > > > When do legacy pci systems use requester id at all? > > PCI spec does not mention this concept. > > I see some descriptions about this in vt-d spec, e.g., chap > 3.9.2. Maybe somewhere else too, but I cannot remember. In the spec, > it is told something like: > > "...the source-id in the DMA requests is the requester-id of the > bridge device..." > > Similar thing on interrupt remapping desc in 5.1.1. > > Actually I am curious about how generic PCI system delivers > requester ID (if there is)... For PCIe, we have encoded TLP header, > and requester ID is filled in the specific field of the header. > However for legacy PCI system, all the data is transmitted via the > parallel interface (no matter 32/64 bits) and I found no place that > the requester ID can be included. I was assuming there is some way > for the root complex to know it (when request comes, the root > complex should be able to know where the request come from, or say, > its connected BDF). Never digger into the details, or am I wrong? There's no such thing as a requester ID on conventional PCI. We should probably be making use of pci_bus_is_express() to determine whether we have a valid requester ID and error if we hit pci_bus_is_root() and we still don't have an express bus. And as MST says, testing for bus number zero is not a valid test for the root bus. Thanks, Alex > > > > > If > > > devices are attached under specific bridge (no matter > > > > should be "no matter if" > > > > > there are one > > > or more bridges), only the requester ID of the bridge that directly > > > > should be "that is directly" > > Will fix above two. > > > > > > attached to the root complex can be recognized." > > > > > > > > > > > > + result = pci_get_bdf(dev); > > > > > > > > Won't dev be NULL for a root bus? > > > > > > Should not. The above while() is checking whether dev's parent bus > > > number (N) is zero, > > > > OK but from pci perspective it's not a given that it's zero. > > I think it isn't for pci expander. > > BTW did you try this with expander bridges? > > Nop... I used the same test in as in v1 (Radim's one, with IR > patchset applied, since until now IR seems the only one that uses > this field), since I found it hard to cover all the combinations > (include different PCI/PCIX/PCIe buses, PCI/PCIe devices, and all > kinds of topologies, etc.). Do you think I should do thorough tests > for this change? If so, do you have suggestion on which test cases I > should (at least) cover? > > > > > > and reach here only if it's non-zero. Here, dev > > > is already re-used to store the PCIDevice struct for the bus device, > > > whose secondary bus number is N (as checked in the while > > > condition). So it should never be the root pci bus (which has > > > so-called secondary bus number 0). > > > > Pls don't make this assumption. If you want to know > > whether it's a root, call pci_bus_is_root. > > Ah, yes, I should use that. > > Thanks, > > -- peterx