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/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.
> Anyhow, if we retain the location (aka offset) of the capabilities
> then if there is some silly code that expects those to be at
> specific locations it should still work?
Yes, that could be a definite concern. When I was dredging up memories 
of finding the issue, I had a notion that this might actually be the 
case. That is why I suggest leaving them at their offset.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel