[M] Change in coreboot[main]: acpi: Add support for DHRD size reporting

[Angel Pons (Code Review)]

Attention is currently required from: Arthur Heymans, Chen, Gang C, Christian Walter, Cliff Huang, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jeff Daly, Jincheng Li, Johnny Lin, Jonathan Zhang, Kapil Porwal, Lance Zhao, Lean Sheng Tan, Nick Vaccaro, Nico Huber, Patrick Rudolph, Paul Menzel, Sean Rhodes, Shuo Liu, Subrata Banik, Tarun, Tim Chu, Tim Wawrzynczak, Vanessa Eusebio, Werner Zeh.

Angel Pons has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/82429?usp=email )

Change subject: acpi: Add support for DHRD size reporting
......................................................................

Patch Set 15:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/82429/comment/b0cb3a2c_cd33ed81?usp... :
PS15, Line 7: DHRD
typo: DRHD

https://review.coreboot.org/c/coreboot/+/82429/comment/3be958e3_75053b3e?usp... :
PS15, Line 11: the register set is 2^N 4 KB pages.
If the value of N is zero, what happens? I didn't find an answer in the VT-d spec (`D51397-016`).

File src/acpi/acpi_dmar.c:

https://review.coreboot.org/c/coreboot/+/82429/comment/b292fb68_15386f17?usp... :
PS15, Line 50: 	drhd->bar = bar;
VT-d spec (`D51397-016`) says:
> This address must be aligned according to the size of the register set size reported in the Size field of this structure.

```

```

https://review.coreboot.org/c/coreboot/+/82429/comment/c094befc_53b75ea8?usp... :
PS15, Line 51: drhd->size = log2(size) - 12;
             : 	/*
             : 	 * Refer to chapter 8.3 of https://cdrdv2.intel.com/v1/dl/getContent/671081
             : 	 * only bit 3:0 are allowed to be used
             : 	 */
             : 	assert(!(drhd->size >> 4));
Although this check is correct, I find it rather convoluted. How about:

```suggestion
    drhd->size = log2(size) - 12;
    /*
     * Refer to chapter 8.3 of https://cdrdv2.intel.com/v1/dl/getContent/671081
     * only bit 3:0 are allowed to be used
     */
    assert(size <= (1 << 15) * (4 * KiB)); /* max 2^15 pages, 4 KiB each */
```

I intentionally kept the multiplications explicit: it's easier to see what the numbers mean, and the compiler will do constant folding anyway.

Even better: since size is always a multiple of 4 KiB (page size), why not have a `pages` parameter instead? It would avoid having to put `4 * KiB` on every call site.

```suggestion
    /*
     * Refer to chapter 8.3 of https://cdrdv2.intel.com/v1/dl/getContent/671081
     * only bit 3:0 are allowed to be used
     */
    assert(pages <= 1 << 15); /* max 2^15 pages */
    drhd->size = log2(pages);
```

File src/include/acpi/acpi.h:

https://review.coreboot.org/c/coreboot/+/82429/comment/fbf49e69_bc841c71?usp... :
PS15, Line 644: } __packed dmar_entry_t;
I think this used to be a generic DMAR entry structure, but it has become a DRHD structure. VT-d spec `(D51397-016)` says only the first two fields are common across all entries.

Anyway, nothing to do in this patchset.

https://review.coreboot.org/c/coreboot/+/82429/comment/b02365fa_8a08d135?usp... :
PS15, Line 1852: unsigned long acpi_create_dmar_drhd(unsigned long current, u8 flags,
               : 				    u16 segment, u64 bar, u32 size);
To avoid having to touch all platforms, would it make sense to have a specialised `acpi_create_dmar_drhd_4k` function?

I would make a preparation change that renames the existing `acpi_create_dmar_drhd` function to `acpi_create_dmar_drhd_4k`, then this change would (re-)add `acpi_create_dmar_drhd` with the size parameter.

End result would be:

```suggestion
unsigned long acpi_create_dmar_drhd(unsigned long current, u8 flags,
    			    u16 segment, u64 bar, u32 size);
static inline unsigned long acpi_create_dmar_drhd_4k(unsigned long current, u8 flags,
    					     u16 segment, u64 bar)
{
    return acpi_create_dmar_drhd(current, flags, segment, bar, 4 * KiB);
}
```

If changing the function signature to use pages instead of size, this also needs to be adapted.