Attention is currently required from: Marc Jones, Furquan Shaikh, Duncan Laurie, Tim Wawrzynczak, David Hendricks, Angel Pons. Rocky Phagura has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49286 )
Change subject: src/acpi: Add APEI EINJ support ......................................................................
Patch Set 11:
(6 comments)
Patchset:
PS11:
mostly some suggestions, the only thing that I think requires a change is the acpi_gen_regaddr1 is u […]
The reason for adding this struct is to match the ACPI spec. The existing acpi_gen_regaddr uses 2 32 bit fields for the address, when it should be 1 64bit address. I added the 64bit address to prevent unnecessary shifting of data during runtime.
Currently I have SOC specific code (which has not been upstreamed yet) that uses it in multiple places (for EINJ and HEST).
File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/49286/comment/e942ee95_2cc46a57 PS11, Line 972: UC_NON_FATAL
nit: the other names for uncorrectable non-fatal errors are suffixed with ` _UCE`
Agreed. Good catch. Change made.
https://review.coreboot.org/c/coreboot/+/49286/comment/cf8dd3d0_588d0a5a PS11, Line 993:
What about `GET_EXECUTE_OPERATION_TIMINGS` ?
I haven't been able to test anything with this, but I'm hoping others can add it once they need it. I think this is optional, so its okay to leave out.
https://review.coreboot.org/c/coreboot/+/49286/comment/b64c42b3_1207db09 PS11, Line 1001: /* EINJ (Error Injection Table) */ : typedef struct acpi_gen_regaddr1 { : u8 space_id; /* Address space ID */ : u8 bit_width; /* Register size in bits */ : u8 bit_offset; /* Register bit offset */ : u8 access_size; /* Access size since ACPI 2.0c */ : u64 addr; /* Register address */ : } __packed acpi_addr64_t; :
This is basically a redefinition of `acpi_gen_regaddr` from `acpi. […]
Yes, that's correct but it matches the ACPI spec. The same comment as above applies here.
...The reason for adding this struct is to match the ACPI spec. The existing acpi_gen_regaddr uses 2 32 bit addresses in the fields, when it should be 1 64bit address. I added the 64bit address to prevent shifting of data during runtime.
Currently I have soc specific code (which has not been upstreamed yet) that uses it.
https://review.coreboot.org/c/coreboot/+/49286/comment/6d0d8306_0b4c150c PS11, Line 1031: acpi_einj_action_table_t trigger_action[1];
may as well just use `acpi_einj_action_table_t trigger_action` ?
This implies that multiple trigger action tables can be used. This works great as is and I'm hesitant to change this because it requires another round of testing.
https://review.coreboot.org/c/coreboot/+/49286/comment/1e3530c4_ac16615e PS11, Line 1064: ()
you could take the address as an argument here and use that to make the definitions one line shorter […]
Done...