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

["G.R." <firemeteor@xxxxxxxxxxxxxxxxxxxxx>]

On Wed, Jun 26, 2013 at 8:53 PM, Konrad Rzeszutek Wilk
<konrad.wilk@xxxxxxxxxx> 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.
>>
>> 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?
I remember the vendor cap fix only helps windows guest.
Linux guest just run happily without this.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel