From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Philipson Subject: Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge. Date: Wed, 26 Jun 2013 13:26:14 -0400 Message-ID: <51CB2436.8060808@citrix.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> <20130626125301.GB4222@phenom.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20130626125301.GB4222@phenom.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Ian Campbell , "G.R." , xen-devel , Stefano Stabellini , Jan Beulich , Jean Guyader List-Id: xen-devel@lists.xenproject.org On 06/26/2013 08:53 AM, Konrad Rzeszutek Wilk wrote: > 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. IIRC I think that may be the case; probably should proceed with that assumption. >> >> Btw. Ross, now that you appears to be the original author (sorry for >> mess you up with Jean), No worries - Jean and I used to work together. He took the patch, cleaned it up and upstreamed it. >> could you also comment on my rework proposal? Jan believe the current >> form is not clean enough. Using the shadow registers? That sounds fine to me. >> >> 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. Originally the only registers being dealt with were the vendor capabilities. The other registers came along later and I am not sure who identified them or why. For example, I looked up the PAVPC register and it is pretty much undocumented in the spec I have. It says it is disabled by TXT which I assume means if you don't have TXT (or it is disabled), this register would not be disabled for write. I am not sure the best way to handle all that. Maybe set the PAVPLCK bit at the appropriate time and just disable it. > > 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? That sounds like a good suggestion since a lot of those registers are minimally documented.