Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
nb/intel/haswell/northbridge.c: Correct DPR handling
DPR size is in MiB, but the range boundaries are expressed in KiB. In addition, DPR and TSEG use the same attributes, so unify both regions. Also improve a comment about DPR, since `is special` is uninformative.
Change-Id: I4479483e17890b5a4c39165138fa1c5f8215bc84 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/haswell/northbridge.c 1 file changed, 11 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/46987/1
diff --git a/src/northbridge/intel/haswell/northbridge.c b/src/northbridge/intel/haswell/northbridge.c index 64a274d..e31337b 100644 --- a/src/northbridge/intel/haswell/northbridge.c +++ b/src/northbridge/intel/haswell/northbridge.c @@ -289,7 +289,12 @@ mc_read_map_entries(dev, &mc_values[0]); mc_report_map_entries(dev, &mc_values[0]);
- /* The DPR register is special */ + /* + * DMA Protected Range can be reserved below TSEG for PCODE patch + * or TXT/BootGuard related data. Rather than report a base address, + * the DPR register reports the TOP of the region, which is the same + * as TSEG base. The region size is reported in MiB in bits 11:4. + */ const union dpr_register dpr = { .raw = pci_read_config32(dev, DPR), }; @@ -328,23 +333,15 @@ size_k = (0xa0000 >> 10) - base_k; ram_resource(dev, index++, base_k, size_k);
- /* 0xc0000 -> DPR base */ + /* 0xc0000 -> TSEG - DPR */ base_k = 0xc0000 >> 10; - size_k = (unsigned long)(mc_values[TSEG_REG] >> 10) - (base_k + dpr.size); + size_k = (unsigned long)(mc_values[TSEG_REG] >> 10) - base_k; + size_k -= dpr.size >> 10; ram_resource(dev, index++, base_k, size_k);
- /* DPR base -> TSEG */ - if (dpr.size) { - resource = new_resource(dev, index++); - resource->base = (dpr.top - dpr.size) * MiB; - resource->size = dpr.size * MiB; - resource->flags = IORESOURCE_MEM | IORESOURCE_STORED | IORESOURCE_CACHEABLE | - IORESOURCE_RESERVE | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; - } - - /* TSEG -> BGSM */ + /* TSEG - DPR -> BGSM */ resource = new_resource(dev, index++); - resource->base = mc_values[TSEG_REG]; + resource->base = mc_values[TSEG_REG] - dpr.size; resource->size = mc_values[BGSM_REG] - resource->base; resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED | IORESOURCE_RESERVE | IORESOURCE_ASSIGNED | IORESOURCE_CACHEABLE;
Attention is currently required from: Nico Huber, Christian Walter, Angel Pons, Arthur Heymans, Michael Niewöhner. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
Patch Set 9: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
nb/intel/haswell/northbridge.c: Correct DPR handling
DPR size is in MiB, but the range boundaries are expressed in KiB. In addition, DPR and TSEG use the same attributes, so unify both regions. Also improve a comment about DPR, since `is special` is uninformative.
Change-Id: I4479483e17890b5a4c39165138fa1c5f8215bc84 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46987 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/haswell/northbridge.c 1 file changed, 11 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/northbridge/intel/haswell/northbridge.c b/src/northbridge/intel/haswell/northbridge.c index 130c0ff..df9cc18 100644 --- a/src/northbridge/intel/haswell/northbridge.c +++ b/src/northbridge/intel/haswell/northbridge.c @@ -251,7 +251,12 @@ mc_read_map_entries(dev, &mc_values[0]); mc_report_map_entries(dev, &mc_values[0]);
- /* The DPR register is special */ + /* + * DMA Protected Range can be reserved below TSEG for PCODE patch + * or TXT/BootGuard related data. Rather than report a base address, + * the DPR register reports the TOP of the region, which is the same + * as TSEG base. The region size is reported in MiB in bits 11:4. + */ const union dpr_register dpr = { .raw = pci_read_config32(dev, DPR), }; @@ -290,23 +295,15 @@ size_k = (0xa0000 >> 10) - base_k; ram_resource(dev, index++, base_k, size_k);
- /* 0xc0000 -> DPR base */ + /* 0xc0000 -> TSEG - DPR */ base_k = 0xc0000 >> 10; - size_k = (unsigned long)(mc_values[TSEG_REG] >> 10) - (base_k + dpr.size); + size_k = (unsigned long)(mc_values[TSEG_REG] >> 10) - base_k; + size_k -= dpr.size >> 10; ram_resource(dev, index++, base_k, size_k);
- /* DPR base -> TSEG */ - if (dpr.size) { - resource = new_resource(dev, index++); - resource->base = (dpr.top - dpr.size) * MiB; - resource->size = dpr.size * MiB; - resource->flags = IORESOURCE_MEM | IORESOURCE_STORED | IORESOURCE_CACHEABLE | - IORESOURCE_RESERVE | IORESOURCE_ASSIGNED | IORESOURCE_FIXED; - } - - /* TSEG -> BGSM */ + /* TSEG - DPR -> BGSM */ resource = new_resource(dev, index++); - resource->base = mc_values[TSEG_REG]; + resource->base = mc_values[TSEG_REG] - dpr.size; resource->size = mc_values[BGSM_REG] - resource->base; resource->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_STORED | IORESOURCE_RESERVE | IORESOURCE_ASSIGNED | IORESOURCE_CACHEABLE;
Attention is currently required from: Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
Patch Set 10:
(1 comment)
File src/northbridge/intel/haswell/northbridge.c:
https://review.coreboot.org/c/coreboot/+/46987/comment/ecca8bfc_81a18f6d PS10, Line 301: >> <<
found by coverity
Attention is currently required from: Angel Pons. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
Patch Set 10:
(1 comment)
File src/northbridge/intel/haswell/northbridge.c:
https://review.coreboot.org/c/coreboot/+/46987/comment/fe291466_83d407a3 PS10, Line 301: >>
<< […]
dpr.size is in MB, needs to be converted to KB here
Attention is currently required from: Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
Patch Set 10:
(3 comments)
File src/northbridge/intel/haswell/northbridge.c:
https://review.coreboot.org/c/coreboot/+/46987/comment/532c3b06_b10fe96d PS10, Line 302: resource->size = dpr.size * MiB; Looks like this was actually correct.
https://review.coreboot.org/c/coreboot/+/46987/comment/ac8a37e2_9796fa47 PS10, Line 301: >>
<< […]
or *MiB/KiB
https://review.coreboot.org/c/coreboot/+/46987/comment/2cac87a0_2b8de6a0 PS10, Line 306: dpr.size << 20 ?
(Maybe stop shifting and use MiB/KiB instead)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46987 )
Change subject: nb/intel/haswell/northbridge.c: Correct DPR handling ......................................................................
Patch Set 10:
(1 comment)
File src/northbridge/intel/haswell/northbridge.c:
https://review.coreboot.org/c/coreboot/+/46987/comment/b6bb2970_e987e87b PS10, Line 301: >>
or *MiB/KiB
urgh
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)