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

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

Hi Jan,

On 05/17/2016 09:25 AM, Jan Beulich wrote:
>>>> On 13.05.16 at 21:54, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> [...]
>> @@ -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?

There are some duplicated error message (in get_last_bdf_ivhd() and 
parse_ivhd_block(). So, I just want to differentiate them a bit. But 
this is not a big deal. I can just get rid of this change.

>> [...]
>> +        {
>> +            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?

That is not in the spec. Although it seems to the convention.

> And anyway - this if() appears to be redundant with the
> enclosing one.

I am not sure what you mean by this comment. Could you please elaborate?

>
>> +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.

This logic was actually duplicated from the 
amd_iommu_update_ivrs_mapping_acpi(). I believe this was added by the

     commit 992fdf6f46252a459c6b1b8d971b2c71f01460f8
         honor ACPI v4 FADT flags

It might make more sense to pull this out to just check once in the 
amd_iommu_init() along with adding some explanation.

>> @@ -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 ...

Sorry, my bad. I'll fix this.

Thanks,
Suravee

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