Re: [Xen-devel] [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 21:54, <suravee.suthikulpanit@xxxxxxx> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -821,13 +821,23 @@ static u16 __init parse_ivhd_device_special(
>      return dev_length;
>  }
>  
> +static inline int get_ivhd_header_size(const struct acpi_ivrs_hardware 
> *ivhd_block)
> +{
> +    if ( ivhd_block->header.type == ACPI_IVRS_TYPE_HARDWARE)
> +        return 24;
> +    if ( ivhd_block->header.type == ACPI_IVRS_TYPE_HARDWARE_11H)
> +        return 40;

Can these become some sizeof() or offsetof() please?

Also both if()-s are lacking a blank before the closing paren. Perhaps
better to be made a switch() anyway.

And the function's return type should be an unsigned one.

>  static int __init parse_ivhd_block(const struct acpi_ivrs_hardware 
> *ivhd_block)
>  {
>      const union acpi_ivhd_device *ivhd_device;
>      u16 block_length, dev_length;
> +    int hdr_size = get_ivhd_header_size(ivhd_block) ;

As should be this variable's.

> @@ -901,7 +911,7 @@ static int __init parse_ivhd_block(const struct 
> acpi_ivrs_hardware *ivhd_block)
>                  ivhd_block->header.length, block_length, iommu);
>              break;
>          default:
> -            AMD_IOMMU_DEBUG("IVHD Error: Invalid Device Type!\n");
> +            AMD_IOMMU_DEBUG("IVHD Error: %s: Invalid Device Type!\n", 
> __func__);

Why?

> @@ -978,6 +960,21 @@ static void __init dump_acpi_table_header(struct 
> acpi_table_header *table)
>  
>  }
>  
> +#define to_ivhd_block(hdr) \
> +    ( container_of(hdr, const struct acpi_ivrs_hardware, header) )
> +#define to_ivmd_block(hdr) \
> +    (container_of(hdr, const struct acpi_ivrs_memory, header) )

Pointless parentheses (and mixed use of blanks). But thanks for
using container_of() here instead of casts.

> +#define is_ivhd_block(x) \
> +    ( x == ACPI_IVRS_TYPE_HARDWARE || \
> +      x == ACPI_IVRS_TYPE_HARDWARE_11H )
> +
> +#define is_ivmd_block(x) \
> +    ( x == ACPI_IVRS_TYPE_MEMORY_ALL || \
> +      x == ACPI_IVRS_TYPE_MEMORY_ONE || \
> +      x == ACPI_IVRS_TYPE_MEMORY_RANGE || \
> +      x == ACPI_IVRS_TYPE_MEMORY_IOMMU )

All instances of x should get properly parenthesized.

> @@ -1177,12 +1176,9 @@ static int __init get_last_bdf_acpi(struct 
> acpi_table_header *table)
>          ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
>          if ( table->length < (length + ivrs_block->length) )
>              return -ENODEV;
> -        if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
> +        if ( ivrs_block->type == ivhd_type )
>          {
> -            int ret = get_last_bdf_ivhd(
> -                 container_of(ivrs_block, const struct acpi_ivrs_hardware,
> -                              header));
> -
> +            int ret = get_last_bdf_ivhd(to_ivhd_block(ivrs_block));
>              if ( ret < 0 )

Stray deletion of a blank line.

> +static int __init
> +get_supported_ivhd_type(struct acpi_table_header *table)
> +{
> +    unsigned long length = sizeof(struct acpi_table_ivrs);
> +    const struct acpi_ivrs_header *ivrs_block, *blk = NULL;
> +
> +    while ( table->length > (length + sizeof(*ivrs_block)) )
> +    {
> +        ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
> +
> +        if ( table->length < (length + ivrs_block->length) )
> +        {
> +            AMD_IOMMU_DEBUG("IVRS Error: "
> +                            "Table Length Exceeded: %#x -> %#lx\n",
> +                            table->length,
> +                            (length + ivrs_block->length));
> +            return -ENODEV;
> +        }
> +
> +        if ( is_ivhd_block (ivrs_block->type) )

Stray blank before inner opening paren.

> +        {
> +            AMD_IOMMU_DEBUG("IVRS Block: Found type %#x flags %#x len %#x id 
> %#x\n",
> +                            ivrs_block->type, ivrs_block->flags,
> +                            ivrs_block->length, ivrs_block->device_id);
> +            if ( ivrs_block->type > IVHD_HIGHEST_SUPPORT_TYPE )
> +                break;

Is there a requirement for the table elements to appear in numerical
order? And anyway - this if() appears to be redundant with the
enclosing one.

> +int __init amd_iommu_get_supported_ivhd_type(void)
> +{
> +    if ( unlikely(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_MSI) )
> +        return -EPERM;

This check appears out of the blue, and isn't being mentioned in
the description. Best would probably be to split it out, but at the
very least it needs to be (briefly) explained.

> @@ -1233,8 +1234,11 @@ int __init amd_iommu_init(void)
>           amd_sp5100_erratum28() )
>          goto error_out;
>  
> -    ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
> +    ivhd_type = amd_iommu_get_supported_ivhd_type();
> +    if ( !ivhd_type )
> +        goto error_out;

Is the ! here meant to catch errors? The function returns -E...
values to indicate errors ...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel