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

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

[Steffen Liebergeld]

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>
---
  drivers/pci/quirks.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 208aacf39329..7e184beb2aa4 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4602,7 +4602,7 @@ int pci_dev_specific_acs_enabled(struct pci_dev 
*dev, u16 acs_flags)
  #define INTEL_BSPR_REG_BPPD  (1 << 9)
   /* Upstream Peer Decode Configuration Register */
-#define INTEL_UPDCR_REG 0x1114
+#define INTEL_UPDCR_REG 0x1014
  /* 5:0 Peer Decode Enable bits */
  #define INTEL_UPDCR_REG_MASK 0x3f
  -- 2.11.0

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

[Andrew Murray]

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.

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.

Thanks,

Andrew Murray

> ---
>  drivers/pci/quirks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 208aacf39329..7e184beb2aa4 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4602,7 +4602,7 @@ int pci_dev_specific_acs_enabled(struct pci_dev *dev,
> u16 acs_flags)
>  #define INTEL_BSPR_REG_BPPD  (1 << 9)
>   /* Upstream Peer Decode Configuration Register */
> -#define INTEL_UPDCR_REG 0x1114
> +#define INTEL_UPDCR_REG 0x1014
>  /* 5:0 Peer Decode Enable bits */
>  #define INTEL_UPDCR_REG_MASK 0x3f
>  -- 2.11.0

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

[Steffen Liebergeld]

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?

Best,
Steffen
-- 
Steffen Liebergeld +49-351-41 888 613

Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
Geschäftsführer: Dr.-Ing. Michael Hohmuth

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

[Andrew Murray]

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.

Thanks,

Andrew Murray

> 
> Best,
> Steffen
> -- 
> Steffen Liebergeld +49-351-41 888 613
> 
> Kernkonzept GmbH.  Sitz: Dresden.  Amtsgericht Dresden, HRB 31129.
> Geschäftsführer: Dr.-Ing. Michael Hohmuth

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

[Alex Williamson]

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.
Thanks,

Alex

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

[Alex Williamson]

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

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

[Raj, Ashok]

Hi Alex

On Wed, Sep 18, 2019 at 04:04:54PM -0600, Alex Williamson wrote:
> > > > >>
> > > > >> 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,

I have asked the chipset folks if the spec has a typo.. waiting for
an answer..