[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

Hi Jan,

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);
-            AMD_IOMMU_DEBUG("IVHD Error: Invalid Device Type!\n");
+            AMD_IOMMU_DEBUG("IVHD Error: %s: Invalid Device Type!\n", 


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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.