[PATCH 0/5] PCI Bridge Emulation changes for v5.8

[PATCH 0/5] PCI Bridge Emulation changes for v5.8

[Jon Derrick]

Hi Bjorn,

These are the changes for pci-bridge-emul as applies to your origin/next.

The first two patches are fixes for existing issues.
The third patch is the conversion to GENMASK() and BIT() for clarity.
The fourth adds the missing bit definitions from PCIe 4.0 and PCIe 5.0.
The fifth patch optimizes away the unnecessary reserved member.

Jon Derrick (5):
  PCI: pci-bridge-emul: Fix PCIe bit conflicts
  PCI: pci-bridge-emul: Fix Root Cap/Status comment
  PCI: pci-bridge-emul: Convert to GENMASK and BIT
  PCI: pci-bridge-emul: Update for PCIe 5.0 r1.0
  PCI: pci-bridge-emul: Eliminate the 'reserved' member

 drivers/pci/pci-bridge-emul.c | 78 +++++++++++++++++------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

-- 
2.18.1

[PATCH 1/5] PCI: pci-bridge-emul: Fix PCIe bit conflicts

[Jon Derrick]

This patch fixes two bit conflicts in the pci-bridge-emul driver:

1. Bit 3 of Device Status (19 of Device Control) is marked as both
   Write-1-to-Clear and Read-Only. It should be Write-1-to-Clear.
   The Read-Only and Reserved bitmasks are shifted by 1 bit due to this
   error.

2. Bit 12 of Slot Control is marked as both Read-Write and Reserved.
   It should be Read-Write.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci-bridge-emul.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index 4f4f54bc732e..faa414655f33 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -185,8 +185,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 * RO, the rest is reserved
 		 */
 		.w1c = GENMASK(19, 16),
-		.ro = GENMASK(20, 19),
-		.rsvd = GENMASK(31, 21),
+		.ro = GENMASK(21, 20),
+		.rsvd = GENMASK(31, 22),
 	},
 
 	[PCI_EXP_LNKCAP / 4] = {
@@ -226,7 +226,7 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
 		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
 		       PCI_EXP_SLTSTA_EIS) << 16,
-		.rsvd = GENMASK(15, 12) | (GENMASK(15, 9) << 16),
+		.rsvd = GENMASK(15, 13) | (GENMASK(15, 9) << 16),
 	},
 
 	[PCI_EXP_RTCTL / 4] = {
-- 
2.18.1

Re: [PATCH 1/5] PCI: pci-bridge-emul: Fix PCIe bit conflicts

[Rob Herring]

On Tue, 14 Apr 2020 16:30:01 -0400, Jon Derrick wrote:
> This patch fixes two bit conflicts in the pci-bridge-emul driver:
> 
> 1. Bit 3 of Device Status (19 of Device Control) is marked as both
>    Write-1-to-Clear and Read-Only. It should be Write-1-to-Clear.
>    The Read-Only and Reserved bitmasks are shifted by 1 bit due to this
>    error.
> 
> 2. Bit 12 of Slot Control is marked as both Read-Write and Reserved.
>    It should be Read-Write.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci-bridge-emul.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 2/5] PCI: pci-bridge-emul: Fix Root Cap/Status comment

[Jon Derrick]

The upper 16-bits of Root Control contain the Root Capabilities
register. The code instead describes the Root Status register in the
upper 16-bits, although it uses the correct bit definition for Root
Capabilities, and for Root Status in the next definition.

Fix this comment and add a comment describing the Root Status register.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci-bridge-emul.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index faa414655f33..c00c30ffb198 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -234,7 +234,7 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 * Root control has bits [4:0] RW, the rest is
 		 * reserved.
 		 *
-		 * Root status has bit 0 RO, the rest is reserved.
+		 * Root capabilities has bit 0 RO, the rest is reserved.
 		 */
 		.rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE |
 		       PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE |
@@ -244,6 +244,10 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 	},
 
 	[PCI_EXP_RTSTA / 4] = {
+		/*
+		 * Root status has bits 17 and [15:0] RO, bit 16 W1C, the rest
+		 * is reserved.
+		 */
 		.ro = GENMASK(15, 0) | PCI_EXP_RTSTA_PENDING,
 		.w1c = PCI_EXP_RTSTA_PME,
 		.rsvd = GENMASK(31, 18),
-- 
2.18.1

Re: [PATCH 2/5] PCI: pci-bridge-emul: Fix Root Cap/Status comment

[Rob Herring]

On Tue, 14 Apr 2020 16:30:02 -0400, Jon Derrick wrote:
> The upper 16-bits of Root Control contain the Root Capabilities
> register. The code instead describes the Root Status register in the
> upper 16-bits, although it uses the correct bit definition for Root
> Capabilities, and for Root Status in the next definition.
> 
> Fix this comment and add a comment describing the Root Status register.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci-bridge-emul.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 3/5] PCI: pci-bridge-emul: Convert to GENMASK and BIT

[Jon Derrick]

In order to make pci-bridge-emul easier to keep up-to-date with new PCIe
features, convert all named register bits to GENMASK and BIT pairs. This
patch doesn't alter any of the PCI configuration space as these bits are
fully defined.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci-bridge-emul.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index c00c30ffb198..bbcccadca85e 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 * as reserved bits.
 		 */
 		.rw = GENMASK(12, 0),
-		.w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
-			PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
-			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
-		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
-		       PCI_EXP_SLTSTA_EIS) << 16,
+		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
+		.ro = GENMASK(7, 5) << 16,
 		.rsvd = GENMASK(15, 13) | (GENMASK(15, 9) << 16),
 	},
 
@@ -236,10 +233,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 *
 		 * Root capabilities has bit 0 RO, the rest is reserved.
 		 */
-		.rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE |
-		       PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE |
-		       PCI_EXP_RTCTL_CRSSVE),
-		.ro = PCI_EXP_RTCAP_CRSVIS << 16,
+		.rw = GENMASK(4, 0),
+		.ro = BIT(0) << 16,
 		.rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
 	},
 
@@ -248,8 +243,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 * Root status has bits 17 and [15:0] RO, bit 16 W1C, the rest
 		 * is reserved.
 		 */
-		.ro = GENMASK(15, 0) | PCI_EXP_RTSTA_PENDING,
-		.w1c = PCI_EXP_RTSTA_PME,
+		.ro = BIT(17) | GENMASK(15, 0),
+		.w1c = BIT(16),
 		.rsvd = GENMASK(31, 18),
 	},
 };
-- 
2.18.1

Re: [PATCH 3/5] PCI: pci-bridge-emul: Convert to GENMASK and BIT

[Christoph Hellwig]

On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote:
> In order to make pci-bridge-emul easier to keep up-to-date with new PCIe
> features, convert all named register bits to GENMASK and BIT pairs. This
> patch doesn't alter any of the PCI configuration space as these bits are
> fully defined.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci-bridge-emul.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> index c00c30ffb198..bbcccadca85e 100644
> --- a/drivers/pci/pci-bridge-emul.c
> +++ b/drivers/pci/pci-bridge-emul.c
> @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
>  		 * as reserved bits.
>  		 */
>  		.rw = GENMASK(12, 0),
> -		.w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> -			PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> -			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> -		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> -		       PCI_EXP_SLTSTA_EIS) << 16,
> +		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> +		.ro = GENMASK(7, 5) << 16,

FYI, I find the previous version a lot more readable.  Or rather I find
it readable while the new one looks like intentionally obsfucated
garbage to me.

Re: [PATCH 3/5] PCI: pci-bridge-emul: Convert to GENMASK and BIT

[Derrick, Jonathan]

On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote:
> On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote:
> > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe
> > features, convert all named register bits to GENMASK and BIT pairs. This
> > patch doesn't alter any of the PCI configuration space as these bits are
> > fully defined.
> > 
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> >  drivers/pci/pci-bridge-emul.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> > index c00c30ffb198..bbcccadca85e 100644
> > --- a/drivers/pci/pci-bridge-emul.c
> > +++ b/drivers/pci/pci-bridge-emul.c
> > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
> >  		 * as reserved bits.
> >  		 */
> >  		.rw = GENMASK(12, 0),
> > -		.w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > -			PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> > -			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> > -		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> > -		       PCI_EXP_SLTSTA_EIS) << 16,
> > +		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> > +		.ro = GENMASK(7, 5) << 16,
> 
> FYI, I find the previous version a lot more readable.  Or rather I find
> it readable while the new one looks like intentionally obsfucated
> garbage to me.

Well I guess that's entirely subjective. But I do think if all the
existing BIT and GENMASK were converted to named registers instead, it
would be a lot easier to overlook mistakes.

Re: [PATCH 3/5] PCI: pci-bridge-emul: Convert to GENMASK and BIT

[Lorenzo Pieralisi]

On Thu, Apr 16, 2020 at 02:35:59PM +0000, Derrick, Jonathan wrote:
> On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote:
> > On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote:
> > > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe
> > > features, convert all named register bits to GENMASK and BIT pairs. This
> > > patch doesn't alter any of the PCI configuration space as these bits are
> > > fully defined.
> > > 
> > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > ---
> > >  drivers/pci/pci-bridge-emul.c | 17 ++++++-----------
> > >  1 file changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> > > index c00c30ffb198..bbcccadca85e 100644
> > > --- a/drivers/pci/pci-bridge-emul.c
> > > +++ b/drivers/pci/pci-bridge-emul.c
> > > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
> > >  		 * as reserved bits.
> > >  		 */
> > >  		.rw = GENMASK(12, 0),
> > > -		.w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > > -			PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> > > -			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> > > -		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> > > -		       PCI_EXP_SLTSTA_EIS) << 16,
> > > +		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> > > +		.ro = GENMASK(7, 5) << 16,
> > 
> > FYI, I find the previous version a lot more readable.  Or rather I find
> > it readable while the new one looks like intentionally obsfucated
> > garbage to me.
> 
> Well I guess that's entirely subjective. But I do think if all the
> existing BIT and GENMASK were converted to named registers instead, it
> would be a lot easier to overlook mistakes.

Hi Jon,

where are we with this patch ? If we drop it I assume you will have
to rebase subsequent patches - I do think Christoph has a point here.

Thanks,
Lorenzo

Re: [PATCH 3/5] PCI: pci-bridge-emul: Convert to GENMASK and BIT

[Derrick, Jonathan]

On Mon, 2020-05-11 at 11:06 +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 16, 2020 at 02:35:59PM +0000, Derrick, Jonathan wrote:
> > On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote:
> > > On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote:
> > > > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe
> > > > features, convert all named register bits to GENMASK and BIT pairs. This
> > > > patch doesn't alter any of the PCI configuration space as these bits are
> > > > fully defined.
> > > > 
> > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > > > ---
> > > >  drivers/pci/pci-bridge-emul.c | 17 ++++++-----------
> > > >  1 file changed, 6 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
> > > > index c00c30ffb198..bbcccadca85e 100644
> > > > --- a/drivers/pci/pci-bridge-emul.c
> > > > +++ b/drivers/pci/pci-bridge-emul.c
> > > > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
> > > >  		 * as reserved bits.
> > > >  		 */
> > > >  		.rw = GENMASK(12, 0),
> > > > -		.w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
> > > > -			PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC |
> > > > -			PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16,
> > > > -		.ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS |
> > > > -		       PCI_EXP_SLTSTA_EIS) << 16,
> > > > +		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
> > > > +		.ro = GENMASK(7, 5) << 16,
> > > 
> > > FYI, I find the previous version a lot more readable.  Or rather I find
> > > it readable while the new one looks like intentionally obsfucated
> > > garbage to me.
> > 
> > Well I guess that's entirely subjective. But I do think if all the
> > existing BIT and GENMASK were converted to named registers instead, it
> > would be a lot easier to overlook mistakes.
> 
> Hi Jon,
> 
> where are we with this patch ? If we drop it I assume you will have
> to rebase subsequent patches - I do think Christoph has a point here.
> 
> Thanks,
> Lorenzo

I'll post v2 without 3 in a bit

[PATCH 4/5] PCI: pci-bridge-emul: Update for PCIe 5.0 r1.0

[Jon Derrick]

Add missing bits from PCIe 4.0 and updates for PCIe 5.0 r1.0.

PCIe 4.0:
Device Status bit 6 - W1C - Emergency Power Reduction Detected
Link Control bits 15:14 - RW - DRS Signaling Control
Slot Control bit 13 - RW - Auto Slow Power Limit Disable

PCIe 5.0:
Slot Control bit 14 - RW - In-Band PD Disable

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci-bridge-emul.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index bbcccadca85e..5c0dffa601f3 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -181,12 +181,12 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		.rw = GENMASK(15, 0),
 
 		/*
-		 * Device status register has 4 bits W1C, then 2 bits
-		 * RO, the rest is reserved
+		 * Device status register has bits 6 and [3:0] W1C, [5:4] RO,
+		 * the rest is reserved
 		 */
-		.w1c = GENMASK(19, 16),
-		.ro = GENMASK(21, 20),
-		.rsvd = GENMASK(31, 22),
+		.w1c = (BIT(6) | GENMASK(3, 0)) << 16,
+		.ro = GENMASK(5, 4) << 16,
+		.rsvd = GENMASK(15, 7) << 16,
 	},
 
 	[PCI_EXP_LNKCAP / 4] = {
@@ -197,15 +197,16 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 
 	[PCI_EXP_LNKCTL / 4] = {
 		/*
-		 * Link control has bits [1:0] and [11:3] RW, the
-		 * other bits are reserved.
-		 * Link status has bits [13:0] RO, and bits [14:15]
+		 * Link control has bits [15:14], [11:3] and [1:0] RW, the
+		 * rest is reserved.
+		 *
+		 * Link status has bits [13:0] RO, and bits [15:14]
 		 * W1C.
 		 */
-		.rw = GENMASK(11, 3) | GENMASK(1, 0),
+		.rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
 		.ro = GENMASK(13, 0) << 16,
 		.w1c = GENMASK(15, 14) << 16,
-		.rsvd = GENMASK(15, 12) | BIT(2),
+		.rsvd = GENMASK(13, 12) | BIT(2),
 	},
 
 	[PCI_EXP_SLTCAP / 4] = {
@@ -214,16 +215,16 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 
 	[PCI_EXP_SLTCTL / 4] = {
 		/*
-		 * Slot control has bits [12:0] RW, the rest is
+		 * Slot control has bits [14:0] RW, the rest is
 		 * reserved.
 		 *
-		 * Slot status has a mix of W1C and RO bits, as well
-		 * as reserved bits.
+		 * Slot status has bits 8 and [4:0] W1C, bits [7:5] RO, the
+		 * rest is reserved.
 		 */
-		.rw = GENMASK(12, 0),
+		.rw = GENMASK(14, 0),
 		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
 		.ro = GENMASK(7, 5) << 16,
-		.rsvd = GENMASK(15, 13) | (GENMASK(15, 9) << 16),
+		.rsvd = BIT(15) | (GENMASK(15, 9) << 16),
 	},
 
 	[PCI_EXP_RTCTL / 4] = {
-- 
2.18.1

Re: [PATCH 4/5] PCI: pci-bridge-emul: Update for PCIe 5.0 r1.0

[Rob Herring]

On Tue, 14 Apr 2020 16:30:04 -0400, Jon Derrick wrote:
> Add missing bits from PCIe 4.0 and updates for PCIe 5.0 r1.0.
> 
> PCIe 4.0:
> Device Status bit 6 - W1C - Emergency Power Reduction Detected
> Link Control bits 15:14 - RW - DRS Signaling Control
> Slot Control bit 13 - RW - Auto Slow Power Limit Disable
> 
> PCIe 5.0:
> Slot Control bit 14 - RW - In-Band PD Disable
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci-bridge-emul.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 5/5] PCI: pci-bridge-emul: Eliminate the 'reserved' member

[Jon Derrick]

Per PCIe 5.0 r1.0, Terms and Acronyms, Page 80:

  Reserved register fields must be read only and must return 0 (all 0's
  for multi-bit fields) when read. Reserved encodings for register and
  packet fields must not be used. Any implementation dependence on a
  Reserved field value or encoding will result in an implementation that
  is not PCI Express-compliant.

This patch ensures reads will return 0 for any bit not in the Read-Only,
Read-Write, or Write-1-to-Clear bitmasks.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 drivers/pci/pci-bridge-emul.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index 5c0dffa601f3..aa563c8fd81e 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -24,6 +24,17 @@
 #define PCI_CAP_PCIE_START	PCI_BRIDGE_CONF_END
 #define PCI_CAP_PCIE_END	(PCI_CAP_PCIE_START + PCI_EXP_SLTSTA2 + 2)
 
+/**
+ * struct pci_bridge_reg_behavior - register bits behaviors
+ * @ro:		Read-Only bits
+ * @rw:		Read-Write bits
+ * @w1c:	Write-1-to-Clear bits
+ *
+ * Reads and Writes will be filtered by specified behavior. All other bits not
+ * declared are assumed 'Reserved' and will return 0 on reads, per PCIe 5.0:
+ * "Reserved register fields must be read only and must return 0 (all 0's for
+ * multi-bit fields) when read".
+ */
 struct pci_bridge_reg_behavior {
 	/* Read-only bits */
 	u32 ro;
@@ -33,9 +44,6 @@ struct pci_bridge_reg_behavior {
 
 	/* Write-1-to-clear bits */
 	u32 w1c;
-
-	/* Reserved bits (hardwired to 0) */
-	u32 rsvd;
 };
 
 static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
@@ -49,7 +57,6 @@ static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
 			PCI_COMMAND_FAST_BACK) |
 		       (PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ |
 			PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MASK) << 16),
-		.rsvd = GENMASK(15, 10) | ((BIT(6) | GENMASK(3, 0)) << 16),
 		.w1c = PCI_STATUS_ERROR_BITS << 16,
 	},
 	[PCI_CLASS_REVISION / 4] = { .ro = ~0 },
@@ -96,8 +103,6 @@ static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
 		       GENMASK(11, 8) | GENMASK(3, 0)),
 
 		.w1c = PCI_STATUS_ERROR_BITS << 16,
-
-		.rsvd = ((BIT(6) | GENMASK(4, 0)) << 16),
 	},
 
 	[PCI_MEMORY_BASE / 4] = {
@@ -130,12 +135,10 @@ static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
 
 	[PCI_CAPABILITY_LIST / 4] = {
 		.ro = GENMASK(7, 0),
-		.rsvd = GENMASK(31, 8),
 	},
 
 	[PCI_ROM_ADDRESS1 / 4] = {
 		.rw = GENMASK(31, 11) | BIT(0),
-		.rsvd = GENMASK(10, 1),
 	},
 
 	/*
@@ -158,8 +161,6 @@ static const struct pci_bridge_reg_behavior pci_regs_behavior[] = {
 		.ro = (GENMASK(15, 8) | ((PCI_BRIDGE_CTL_FAST_BACK) << 16)),
 
 		.w1c = BIT(10) << 16,
-
-		.rsvd = (GENMASK(15, 12) | BIT(4)) << 16,
 	},
 };
 
@@ -186,13 +187,11 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 */
 		.w1c = (BIT(6) | GENMASK(3, 0)) << 16,
 		.ro = GENMASK(5, 4) << 16,
-		.rsvd = GENMASK(15, 7) << 16,
 	},
 
 	[PCI_EXP_LNKCAP / 4] = {
 		/* All bits are RO, except bit 23 which is reserved */
 		.ro = lower_32_bits(~BIT(23)),
-		.rsvd = BIT(23),
 	},
 
 	[PCI_EXP_LNKCTL / 4] = {
@@ -206,7 +205,6 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		.rw = GENMASK(15, 14) | GENMASK(11, 3) | GENMASK(1, 0),
 		.ro = GENMASK(13, 0) << 16,
 		.w1c = GENMASK(15, 14) << 16,
-		.rsvd = GENMASK(13, 12) | BIT(2),
 	},
 
 	[PCI_EXP_SLTCAP / 4] = {
@@ -224,7 +222,6 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		.rw = GENMASK(14, 0),
 		.w1c = (BIT(8) | GENMASK(4, 0)) << 16,
 		.ro = GENMASK(7, 5) << 16,
-		.rsvd = BIT(15) | (GENMASK(15, 9) << 16),
 	},
 
 	[PCI_EXP_RTCTL / 4] = {
@@ -236,7 +233,6 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 */
 		.rw = GENMASK(4, 0),
 		.ro = BIT(0) << 16,
-		.rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16),
 	},
 
 	[PCI_EXP_RTSTA / 4] = {
@@ -246,7 +242,6 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = {
 		 */
 		.ro = BIT(17) | GENMASK(15, 0),
 		.w1c = BIT(16),
-		.rsvd = GENMASK(31, 18),
 	},
 };
 
@@ -354,7 +349,8 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
 	 * Make sure we never return any reserved bit with a value
 	 * different from 0.
 	 */
-	*value &= ~behavior[reg / 4].rsvd;
+	*value &= behavior[reg / 4].ro | behavior[reg / 4].rw |
+		  behavior[reg / 4].w1c;
 
 	if (size == 1)
 		*value = (*value >> (8 * (where & 3))) & 0xff;
-- 
2.18.1

Re: [PATCH 5/5] PCI: pci-bridge-emul: Eliminate the 'reserved' member

[Rob Herring]

On Tue, 14 Apr 2020 16:30:05 -0400, Jon Derrick wrote:
> Per PCIe 5.0 r1.0, Terms and Acronyms, Page 80:
> 
>   Reserved register fields must be read only and must return 0 (all 0's
>   for multi-bit fields) when read. Reserved encodings for register and
>   packet fields must not be used. Any implementation dependence on a
>   Reserved field value or encoding will result in an implementation that
>   is not PCI Express-compliant.
> 
> This patch ensures reads will return 0 for any bit not in the Read-Only,
> Read-Write, or Write-1-to-Clear bitmasks.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  drivers/pci/pci-bridge-emul.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>