From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41516) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCkE-0006lJ-BQ for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:39:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwCkD-0001tX-2J for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:39:46 -0400 Received: from thoth.sbs.de ([192.35.17.2]:22225) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCkC-0001tH-Od for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:39:45 -0400 Message-ID: <4E54F10F.20200@siemens.com> Date: Wed, 24 Aug 2011 14:39:43 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <4E53E328.90601@siemens.com> <20110824100439.GA17255@redhat.com> <4E54CE18.1080508@siemens.com> <20110824115816.GA18393@redhat.com> <4E54EEB0.2070803@siemens.com> <20110824123430.GA18717@redhat.com> <4E54F04F.2040800@siemens.com> <20110824123929.GB18717@redhat.com> In-Reply-To: <20110824123929.GB18717@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] pci: Error on PCI capability collisions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel On 2011-08-24 14:39, Michael S. Tsirkin wrote: > On Wed, Aug 24, 2011 at 02:36:31PM +0200, Jan Kiszka wrote: >> On 2011-08-24 14:34, Michael S. Tsirkin wrote: >>> On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote: >>>> On 2011-08-24 13:58, Michael S. Tsirkin wrote: >>>>> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote: >>>>>> On 2011-08-24 12:04, Michael S. Tsirkin wrote: >>>>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote: >>>>>>>> From: Alex Williamson >>>>>>>> >>>>>>>> Nothing good can happen when we overlap capabilities >>>>>>>> >>>>>>>> [ Jan: rebased over qemu, minor formatting ] >>>>>>>> >>>>>>>> Signed-off-by: Jan Kiszka >>>>>>> >>>>>>> This doesn't build for me: >>>>>>> >>>>>>> /scm/qemu/hw/pci.c: In function =E2=80=98pci_add_capability=E2=80= =99: >>>>>>> /scm/qemu/hw/pci.c:1970:45: error: =E2=80=98PCIDevice=E2=80=99 ha= s no member named =E2=80=98config_map=E2=80=99 >>>>>> >>>>>> Yeah, sorry, forgot to refresh the commit before posting. >>>>>> >>>>>>> >>>>>>> I think that what that includes is the capability including each = given >>>>>>> offset, right? It would be easy to write some code scanning the >>>>>>> capability list to figure this value out. >>>>>>> Something along the lines of (untested): >>>>>>> >>>>>>> static >>>>>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t of= fset) >>>>>>> { =20 >>>>>>> uint8_t next, prev, found =3D 0; >>>>>>> >>>>>>> if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) >>>>>>> return 0; >>>>>>> >>>>>>> for (prev =3D PCI_CAPABILITY_LIST; (next =3D pdev->config[pre= v]); >>>>>>> prev =3D next + PCI_CAP_LIST_NEXT) >>>>>>> if (next <=3D offset && next > found) >>>>>>> found =3D next; >>>>>>> >>>>>>> return found; >>>>>>> } >>>>>> >>>>>> Sounds useful, will enhance the patch. >>>>>> >>>>>> (Originally, I just wanted to reduce the qemu-kvm delta... :) ) >>>>>> >>>>>> Jan >>>>> >>>>> Also, let's add a comment documenting the >>>>> reason for this check: device assignment >>>>> depends on this check to verify that the device >>>>> is not broken. >>>> >>>> Based on the previous discussion, I don't think this is accurate as = it >>>> will also validate emulated devices. >>>> >>>> Jan >>> >>> Something like the below is accurate, right? >>> >>> /* Device assignment depends on this check to verify that the device >>> is not broken. Should never trigger for emulated devices, >>> but it's helpful for debugging these. >>> */ >> >> I've expressed this in the commit message. Unless there is another >> reason to do v3, maybe you can merge the comment on commit. >> >> Jan >=20 > Sure, I can do that, no need with v3. You are fine with the way > it's formulated? Yep. Jan --=20 Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux