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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.16 at 08:30, <Suravee.Suthikulpanit@xxxxxxx> wrote:
> On 05/17/2016 09:25 AM, Jan Beulich wrote:
>>>>> 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
>>> [...]
>>> @@ -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.

In that case perhaps better to adjust the wording so the messages
become distinguishable?

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

I.e. we shouldn't rely on it.

>> 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?

You've removed too much context, so here the code fragment
again:

        if ( is_ivhd_block (ivrs_block->type) )
        {
            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 )

with

#define is_ivhd_block(x) \
    ( x == ACPI_IVRS_TYPE_HARDWARE || \
      x == ACPI_IVRS_TYPE_HARDWARE_11H )

That means if the outer if()'s condition is true, the inner if()'s one
can never be.

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

Does it actually need duplicating? I.e. doesn't the error that results
from amd_iommu_update_ivrs_mapping_acpi() (if the flag is clear)
not suffice?

Jan


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