Re: [Xen-devel] [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Ross Philipson <ross.philipson@xxxxxxxxxx>]

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
> > <ross.philipson@xxxxxxxxxx>  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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel