From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCh8-0003H2-Lp for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:36:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwCh7-00016Q-Hh for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:36:34 -0400 Received: from david.siemens.de ([192.35.17.14]:23797) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCh7-00016M-9K for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:36:33 -0400 Message-ID: <4E54F04F.2040800@siemens.com> Date: Wed, 24 Aug 2011 14:36:31 +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> In-Reply-To: <20110824123430.GA18717@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: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 has = 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 gi= ven >>>>> 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 offs= et) >>>>> { =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[prev]= ); >>>>> 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 >=20 > Something like the below is accurate, right? >=20 > /* 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 Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux