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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

~Andrew