[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.