Re: [PATCH] PCI: quirks: Fix register location for UPDCR

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 18 Sep 2019 10:46:51 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed, 18 Sep 2019 13:09:18 +0100
> Andrew Murray <andrew.murray@arm.com> wrote:
> 
> > On Wed, Sep 18, 2019 at 02:02:59PM +0200, Steffen Liebergeld wrote:  
> > > On 18/09/2019 12:42, Andrew Murray wrote:    
> > > > On Tue, Sep 17, 2019 at 08:07:13PM +0200, Steffen Liebergeld wrote:    
> > > >> According to documentation [0] the correct offset for the
> > > >> Upstream Peer Decode Configuration Register (UPDCR) is 0x1014.
> > > >> It was previously defined as 0x1114. This patch fixes it.
> > > >>
> > > >> [0]
> > > >> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-mobile-i-o-datasheet.pdf
> > > >> (page 325)
> > > >>
> > > >> Signed-off-by: Steffen Liebergeld <steffen.liebergeld@kernkonzept.com>    
> > > > 
> > > > You may also like to add:
> > > > 
> > > > Fixes: d99321b63b1f ("PCI: Enable quirks for PCIe ACS on Intel PCH root ports")
> > > > Reviewed-by: Andrew Murray <andrew.murray@arm.com>
> > > > 
> > > > As well as CC'ing stable.    
> > > 
> > > Ok. Thank you.
> > >     
> > > > I guess the side effect of this bug is that we claim to have peer
> > > > isolation when we do not. This fix ensures that we get the advertised
> > > > isolation.    
> > > Yes, that is also my understanding. Should I explain that in the commit
> > > message?    
> > 
> > I think something similar to that would be helpful.  
> 
> This is unfortunate, but my initial impression is that this may have
> just been a typo that slipped by everyone.  It's difficult to actually
> test for isolation.  Maybe someone from Intel could review this.  Also,
> Steffen discussed this with me prior to posting and I believe this is
> untested, so while trivial from inspection, it would be preferable to
> know that some sample of hardware doesn't fall over as a result.

I've looked at 4 different systems, two 6-series (desktop + laptop), an
8-series desktop, and an X79 workstation.  None of the 6/8 series even
enter the branch where we read the UPDCR register, the value read from
the BSPR register doesn't require it.  In the case of the X79, using
0x1014 for UPDCR, the value read from the register is zero so code
would not proceed into the inner branch to write the register, but
using the current 0x1114 address, we read a non-zero value and changing
it does stick on re-read.  Neither address is defined in the public
specs for this chipset, we're basing the information on word of mouth
and ack from Intel as noted in commit 1a30fd0dba77.

So of these systems, if 0x1014 is the correct UPDCR register address,
nothing actually changes with respect to isolation other than we're not
changing the value in mystery register 0x1114.  Intel?  Thanks,

Alex