Re: [PATCH v7 2/8] AMD/IOMMU: obtain IVHD type to use earlier

[Jan Beulich <jbeulich@xxxxxxxx>]

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