Hi Jan,
On 05/17/2016 09:25 AM, Jan Beulich wrote:
On 13.05.16 at 21:54, <suravee.suthikulpa...@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
