From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge. Date: Wed, 26 Jun 2013 08:53:01 -0400 Message-ID: <20130626125301.GB4222@phenom.dumpdata.com> References: <1360253528-5424-4-git-send-email-firemeteor@users.sourceforge.net> <5113E71B02000078000BCF86@nat28.tlf.novell.com> <5114BDC202000078000BD138@nat28.tlf.novell.com> <20130621180335.GC15809@phenom.dumpdata.com> <51C9A44E.1050309@citrix.com> <20130625145438.GA28904@phenom.dumpdata.com> <51C9B0B9.4000409@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "G.R." Cc: Ian Campbell , Ross Philipson , Stefano Stabellini , Jan Beulich , xen-devel , Jean Guyader List-Id: xen-devel@lists.xenproject.org On Wed, Jun 26, 2013 at 12:18:02PM +0800, G.R. wrote: > On Tue, Jun 25, 2013 at 11:01 PM, Ross Philipson > wrote: > > On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote: > >> > >> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote: > >>> > >>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote: > >>>> > >>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote: > >>>>> > >>>>> I'm going to rework this patch to address Jan's concern. > >>>>> Here is my proposal, please review and comment before I begin: > >>>>> > >>>>> The proposal is to read a shadow copy of the exposed host register into > >>>>> the config space of the emulated host bridge and relies on the > >>>>> pci_default_read_config() function > >>>>> to provide proper access. > >>>>> > >>>>> This methodology only works for constant values, which is our case > >>>>> here. > >>>>> The exposed value is either read-only or write-locked (for BIOS). > >>>>> > >>>>> The only exception is that the PAVPC (0x58) register is write-locked > >>>>> but not for BIOS. > >>>> > >>>> > >>>> So only SeaBIOS or hvmloader should touch it? > >>>> > >>>>> This is exposed for RW and my proposal is to perform write-through in > >>>>> the register write-support. > >>>> > >>>> > >>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what > >>>> would happen? Is there a list of the appropiate values it should > >>>> accept? > >>>> > >>>>> > >>>>>> > >>>>>> Also, why would removing the next capability be correct here, > >>>>>> when you're not removing _all_ other capabilities? > >>>>> > >>>>> I have no answer about this question. *Jean*, could you help comment > >>>>> since this is from your code? > >>>> > >>>> > >>>> > >>>> If he doesn't answer - if you don't remove the capability does it > >>>> still work? > >>> > >>> > >>> So I actually originally found this issue with the vendor > >>> capabilities and created the original patch for it. This was quite > >>> some time ago so I had to go back and look. IIRC the vendor specific > >>> capabilities were always the first one in the chain and the > >>> unchaining code would drop all further capabilities (which we did > >>> not want to pass directly to the guest). > >> > >> > >> OK, so blacklisting. > >>> > >>> > >>> We never saw a configuration where the vendor capabilities were not > >>> the first. I guess the suggestion is that to make the patch > >>> consistent, preceding capabilities should be detected and handled. I > >>> am not sure what the best way to do it would be. Perhaps scanning > >>> through the chain until 0x09 is found and reporting its offset > >>> through 0x34 instead of what is there would be the way to go. Then > >>> unchain anything past the 0x09 caps too as is currently done. > >> > >> > >> Or just scan through the capabilities, and chain only the ones > >> that we want to "Whitelist" and the rest are to be blacklisted. > >> The rest can also have its values set to some bogus value (0xdeadbeef?) > >> Perhaps only when built with 'debug=y'. > > > > > > That sounds about right. Back when I first did the patch (in an old qemu) > > there were no other capabilities on the piix4 host bridge so it was simple. > > Not sure if other capabilities will be an issue now. > > It's still the case as for the IVB host bridge. > And from what I can find from the datasheet for the Haswell, it's > still the case. > > Note that the datasheet explicitly documents the offset of the > CAPABILITY registers. > I guess there will be code that rely on this offset that been publicly > documented. > > Btw. Ross, now that you appears to be the original author (sorry for > mess you up with Jean), > could you also comment on my rework proposal? Jan believe the current > form is not clean enough. > > Currently we use a whitelist of registers to pass-through.How do you > come up with the current list? > The shadow copy way appears to work for the current list. OK. > But what if we are going to need some special registers that cannot be > handled well? (e.g. has side effect for reading and cannot perform > read-back?) Hopefully the i915 driver in Linux will help in figuring out which ones of those are needed?