From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v3] pci: correct pci config size default for cap version 2 endpoints Date: Sun, 24 Jul 2011 11:12:44 +0300 Message-ID: <20110724081244.GB24483@redhat.com> References: <20110722155338.43049.12587.stgit@dddsys0.bos.redhat.com> <20110722212422.GH9766@sequoia.sous-sol.org> <1311370232.2653.100.camel@bling.home> <20110722213547.GI9766@sequoia.sous-sol.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alex Williamson , Donald Dutile , kvm@vger.kernel.org To: Chris Wright Return-path: Received: from mx1.redhat.com ([209.132.183.28]:54829 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751816Ab1GXIMV (ORCPT ); Sun, 24 Jul 2011 04:12:21 -0400 Content-Disposition: inline In-Reply-To: <20110722213547.GI9766@sequoia.sous-sol.org> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Jul 22, 2011 at 02:35:47PM -0700, Chris Wright wrote: > * Alex Williamson (alex.williamson@redhat.com) wrote: > > On Fri, 2011-07-22 at 14:24 -0700, Chris Wright wrote: > > > * Donald Dutile (ddutile@redhat.com) wrote: > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > > > > index 36ad6b0..34db52e 100644 > > > > --- a/hw/device-assignment.c > > > > +++ b/hw/device-assignment.c > > > > @@ -1419,16 +1419,18 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev) > > > > } > > > > > > > > if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_EXP, 0))) { > > > > - uint8_t version; > > > > + uint8_t version, size; > > > > uint16_t type, devctl, lnkcap, lnksta; > > > > uint32_t devcap; > > > > - int size = 0x3c; /* version 2 size */ > > > > > > > > version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS); > > > > version &= PCI_EXP_FLAGS_VERS; > > > > if (version == 1) { > > > > size = 0x14; > > > > - } else if (version > 2) { > > > > + } else if (version == 2) { > > > > + /* don't include slot cap/stat/ctrl 2 regs; only support endpoints */ > > > > + size = 0x34; > > > > > > That doesn't look correct to me. The size is fixed, just that some > > > registers are Reserved Zero when they do not apply (e.g. endpoint only). > > > > Apparently it can be interpreted differently. In this case, we've seen > > a tg3 device expose a v2 PCI express capability at offset 0xcc. Using > > 0x3c bytes, we extend 8 bytes past the legacy config space area :( > > Wow, that device sounds broken to me. The spec is pretty clear. Yes, I agree it's broken. Looks like something that happens when a device is designed in parallel with the spec. What bothers me is this patch seems to make devices that do behave correctly out of spec (registers will be writeable by default) - correct? How about we check for overflow and only do the hacks if it happens? Also, the code to initialize slot and root control registers is still there: it would seem that running it will corrupt memmory beyond the config array? -- MST