Re: [PATCH v8 6/6] AMD/IOMMU: expose errors and warnings unconditionally

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 22/09/2021 15:38, Jan Beulich wrote:
> Making these dependent upon "iommu=debug" isn't really helpful in the
> field. Where touching respective code anyway also make use of %pp and
> %pd.
>
> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: Paul Durrant <paul@xxxxxxx>

... with one nit below...

> ---
[snip]
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -174,7 +174,7 @@ static int __init reserve_unity_map_for_
>           if ( unity_map->addr + unity_map->length > base &&
>                base + length > unity_map->addr )
>           {
> -            AMD_IOMMU_DEBUG("IVMD Error: overlap [%lx,%lx) vs [%lx,%lx)\n",
> +            AMD_IOMMU_ERROR("IVMD: overlap [%lx,%lx) vs [%lx,%lx)\n",
>                               base, base + length, unity_map->addr,
>                               unity_map->addr + unity_map->length);
>               return -EPERM;
> @@ -248,7 +248,7 @@ static int __init register_range_for_dev
>       iommu = find_iommu_for_device(seg, bdf);
>       if ( !iommu )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: No IOMMU for Dev_Id %#x!\n", bdf);
> +        AMD_IOMMU_ERROR("IVMD: no IOMMU for Dev_Id %#x\n", bdf);
>           return -ENODEV;
>       }
>       req = ivrs_mappings[bdf].dte_requestor_id;
> @@ -318,7 +318,7 @@ static int __init parse_ivmd_device_sele
>       bdf = ivmd_block->header.device_id;
>       if ( bdf >= ivrs_bdf_entries )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: Invalid Dev_Id %#x\n", bdf);
> +        AMD_IOMMU_ERROR("IVMD: invalid Dev_Id %#x\n", bdf);
>           return -ENODEV;
>       }
>   
> @@ -335,16 +335,14 @@ static int __init parse_ivmd_device_rang
>       first_bdf = ivmd_block->header.device_id;
>       if ( first_bdf >= ivrs_bdf_entries )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: "
> -                        "Invalid Range_First Dev_Id %#x\n", first_bdf);
> +        AMD_IOMMU_ERROR("IVMD: invalid Range_First Dev_Id %#x\n", first_bdf);
>           return -ENODEV;
>       }
>   
>       last_bdf = ivmd_block->aux_data;
>       if ( (last_bdf >= ivrs_bdf_entries) || (last_bdf <= first_bdf) )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: "
> -                        "Invalid Range_Last Dev_Id %#x\n", last_bdf);
> +        AMD_IOMMU_ERROR("IVMD: invalid Range_Last Dev_Id %#x\n", last_bdf);
>           return -ENODEV;
>       }
>   
> @@ -367,7 +365,7 @@ static int __init parse_ivmd_device_iomm
>                                       ivmd_block->aux_data);
>       if ( !iommu )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: No IOMMU for Dev_Id %#x Cap %#x\n",
> +        AMD_IOMMU_ERROR("IVMD: no IOMMU for Dev_Id %#x Cap %#x\n",
>                           ivmd_block->header.device_id, ivmd_block->aux_data);
>           return -ENODEV;
>       }
> @@ -384,7 +382,7 @@ static int __init parse_ivmd_block(const
>   
>       if ( ivmd_block->header.length < sizeof(*ivmd_block) )
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: Invalid Block Length!\n");
> +        AMD_IOMMU_ERROR("IVMD: invalid block length\n");
>           return -ENODEV;
>       }
>   
> @@ -402,8 +400,8 @@ static int __init parse_ivmd_block(const
>            (addr_bits < BITS_PER_LONG &&
>             ((start_addr + mem_length - 1) >> addr_bits)) )
>       {
> -        AMD_IOMMU_DEBUG("IVMD: [%lx,%lx) is not IOMMU addressable\n",
> -                        start_addr, start_addr + mem_length);
> +        AMD_IOMMU_WARN("IVMD: [%lx,%lx) is not IOMMU addressable\n",
> +                       start_addr, start_addr + mem_length);
>           return 0;
>       }
>   
> @@ -411,8 +409,8 @@ static int __init parse_ivmd_block(const
>       {
>           paddr_t addr;
>   
> -        AMD_IOMMU_DEBUG("IVMD: [%lx,%lx) is not (entirely) in reserved memory\n",
> -                        base, limit + PAGE_SIZE);
> +        AMD_IOMMU_WARN("IVMD: [%lx,%lx) is not (entirely) in reserved 
> memory\n",
> +                       base, limit + PAGE_SIZE);
>   
>           for ( addr = base; addr <= limit; addr += PAGE_SIZE )
>           {
> @@ -423,7 +421,7 @@ static int __init parse_ivmd_block(const
>                   if ( e820_add_range(&e820, addr, addr + PAGE_SIZE,
>                                       E820_RESERVED) )
>                       continue;
> -                AMD_IOMMU_DEBUG("IVMD Error: Page at %lx couldn't be 
> reserved\n",
> +                AMD_IOMMU_ERROR("IVMD: page at %lx couldn't be reserved\n",
>                                   addr);
>                   return -EIO;
>               }
> @@ -433,8 +431,7 @@ static int __init parse_ivmd_block(const
>                              RAM_TYPE_UNUSABLE)) )
>                   continue;
>   
> -            AMD_IOMMU_DEBUG("IVMD Error: Page at %lx can't be converted\n",
> -                            addr);
> +            AMD_IOMMU_ERROR("IVMD: page at %lx can't be converted\n", addr);
>               return -EIO;
>           }
>       }
> @@ -448,7 +445,7 @@ static int __init parse_ivmd_block(const
>       }
>       else
>       {
> -        AMD_IOMMU_DEBUG("IVMD Error: Invalid Flag Field!\n");
> +        AMD_IOMMU_ERROR("IVMD: invalid flag field\n");
>           return -ENODEV;
>       }
>   
> @@ -471,7 +468,8 @@ static int __init parse_ivmd_block(const
>                                          iw, ir, exclusion);
>   
>       default:
> -        AMD_IOMMU_DEBUG("IVMD Error: Invalid Block Type!\n");
> +        AMD_IOMMU_ERROR("IVMD: unknown block type %#x\n",
> +                        ivmd_block->header.type);
>           return -ENODEV;
>       }
>   }
> @@ -481,7 +479,7 @@ static u16 __init parse_ivhd_device_padd
>   {
>       if ( header_length < (block_length + pad_length) )
>       {
> -        AMD_IOMMU_DEBUG("IVHD Error: Invalid Device_Entry Length!\n");
> +        AMD_IOMMU_ERROR("IVHD: invalid Device_Entry length\n");
>           return 0;
>       }
>   
> @@ -496,7 +494,7 @@ static u16 __init parse_ivhd_device_sele
>       bdf = select->header.id;
>       if ( bdf >= ivrs_bdf_entries )
>       {
> -        AMD_IOMMU_DEBUG("IVHD Error: Invalid Device_Entry Dev_Id %#x\n", bdf);
> +        AMD_IOMMU_ERROR("IVHD: invalid Device_Entry Dev_Id %#x\n", bdf);
>           return 0;
>       }
>   
> @@ -515,14 +513,13 @@ static u16 __init parse_ivhd_device_rang
>       dev_length = sizeof(*range);
>       if ( header_length < (block_length + dev_length) )
>       {
> -        AMD_IOMMU_DEBUG("IVHD Error: Invalid Device_Entry Length!\n");
> +        AMD_IOMMU_ERROR("IVHD: invalid Device_Entry length\n");
>           return 0;
>       }
>   
>       if ( range->end.header.type != ACPI_IVRS_TYPE_END )
>       {
> -        AMD_IOMMU_DEBUG("IVHD Error: "
> -                        "Invalid Range: End_Type %#x\n",
> +        AMD_IOMMU_ERROR("IVHD Error: invalid range: End_Type %#x\n",
NIT: I guess you want to drop the 'Error' here like you did elsewhere.