On 26.08.2021 14:30, Andrew Cooper wrote:
> On 26/08/2021 08:23, Jan Beulich wrote:
>> Doing this in amd_iommu_prepare() is too late for it, in particular, to
>> be used in amd_iommu_detect_one_acpi(), as a subsequent change will want
>> to do. Moving it immediately ahead of amd_iommu_detect_acpi() is
>> (luckily) pretty simple, (pretty importantly) without breaking
>> amd_iommu_prepare()'s logic to prevent multiple processing.
>>
>> This involves moving table checksumming, as
>> amd_iommu_get_supported_ivhd_type() -> get_supported_ivhd_type() will
>> now be invoked before amd_iommu_detect_acpi() -> detect_iommu_acpi(). In
>> the course of dojng so stop open-coding acpi_tb_checksum(), seeing that
>
> doing.
>
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -1150,20 +1152,7 @@ static int __init parse_ivrs_table(struc
>> static int __init detect_iommu_acpi(struct acpi_table_header *table)
>> {
>> const struct acpi_ivrs_header *ivrs_block;
>> - unsigned long i;
>> unsigned long length = sizeof(struct acpi_table_ivrs);
>> - u8 checksum, *raw_table;
>> -
>> - /* validate checksum: sum of entire table == 0 */
>> - checksum = 0;
>> - raw_table = (u8 *)table;
>> - for ( i = 0; i < table->length; i++ )
>> - checksum += raw_table[i];
>> - if ( checksum )
>> - {
>> - AMD_IOMMU_DEBUG("IVRS Error: Invalid Checksum %#x\n", checksum);
>> - return -ENODEV;
>> - }
>>
>> while ( table->length > (length + sizeof(*ivrs_block)) )
>> {
>> @@ -1300,6 +1289,15 @@ get_supported_ivhd_type(struct acpi_tabl
>> {
>> size_t length = sizeof(struct acpi_table_ivrs);
>> const struct acpi_ivrs_header *ivrs_block, *blk = NULL;
>> + uint8_t checksum;
>> +
>> + /* Validate checksum: Sum of entire table == 0. */
>> + checksum = acpi_tb_checksum(ACPI_CAST_PTR(uint8_t, table),
>> table->length);
>> + if ( checksum )
>> + {
>> + AMD_IOMMU_DEBUG("IVRS Error: Invalid Checksum %#x\n", checksum);
>
> I know you're just moving code, but this really needs to be a visible
> error. It's "I'm turning off the IOMMU because the ACPI table is bad",
> which is about as serious as errors come.
I'll wait for us settling on patch 1 in this regard, and then follow
the same model here.
Jan
