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

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: "G.R." <firemeteor@users.sourceforge.net>
Cc: Ian Campbell <ian.campbell@citrix.com>,
	Ross Philipson <ross.philipson@citrix.com>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xen.org>,
	Jean Guyader <Jean.guyader@gmail.com>
Subject: Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.
Date: Wed, 26 Jun 2013 08:53:01 -0400	[thread overview]
Message-ID: <20130626125301.GB4222@phenom.dumpdata.com> (raw)
In-Reply-To: <CAKhsbWahd=qXg7PhpMf7aPk4tz9Pk-q9jfZcVqoyZvCDHk6e9w@mail.gmail.com>

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@citrix.com> 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?