* [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h
@ 2016-05-13 19:54 suravee.suthikulpanit
2016-05-17 14:25 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: suravee.suthikulpanit @ 2016-05-13 19:54 UTC (permalink / raw)
To: xen-devel, jbeulich; +Cc: Suravee Suthikulpanit
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Along with the IVHD block type 10h, newer AMD platforms also come with
types 11h, which is a superset of the older one. Having multiple IVHD
block types in the same platform allows backward compatibility of newer
systems to work with existing drivers. The driver should only parse
the highest-level (newest) type of IVHD block that it can support.
However, the current driver returns error when encounters with unknown
IVHD block type. This causes existing driver to unnecessarily fail IOMMU
initialization on new systems.
This patch introduces a new logic, which scans through IVRS table looking
for the highest-level supporsted IVHD block type. It also adds support
for the new IVHD block type 11h. More information about the IVHD type 11h
can be found in the AMD I/O Virtualization Technology (IOMMU) Specification
rev 2.62.
http://support.amd.com/TechDocs/48882_IOMMU.pdf
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/drivers/passthrough/amd/iommu_acpi.c | 133 +++++++++++++++++---------
xen/drivers/passthrough/amd/iommu_init.c | 6 +-
xen/include/acpi/actbl2.h | 7 +-
xen/include/asm-x86/amd-iommu.h | 1 +
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 1 +
5 files changed, 103 insertions(+), 45 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 79c1f8c..2a2d61b 100644
--- 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;
+ return 0;
+}
+
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) ;
struct amd_iommu *iommu;
- if ( ivhd_block->header.length < sizeof(*ivhd_block) )
+ if ( ivhd_block->header.length < hdr_size )
{
AMD_IOMMU_DEBUG("IVHD Error: Invalid Block Length!\n");
return -ENODEV;
@@ -845,7 +855,7 @@ static int __init parse_ivhd_block(const struct acpi_ivrs_hardware *ivhd_block)
}
/* parse Device Entries */
- block_length = sizeof(*ivhd_block);
+ block_length = hdr_size;
while ( ivhd_block->header.length >=
(block_length + sizeof(struct acpi_ivrs_de_header)) )
{
@@ -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__);
dev_length = 0;
break;
}
@@ -914,34 +924,6 @@ static int __init parse_ivhd_block(const struct acpi_ivrs_hardware *ivhd_block)
return 0;
}
-static int __init parse_ivrs_block(const struct acpi_ivrs_header *ivrs_block)
-{
- const struct acpi_ivrs_hardware *ivhd_block;
- const struct acpi_ivrs_memory *ivmd_block;
-
- switch ( ivrs_block->type )
- {
- case ACPI_IVRS_TYPE_HARDWARE:
- ivhd_block = container_of(ivrs_block, const struct acpi_ivrs_hardware,
- header);
- return parse_ivhd_block(ivhd_block);
-
- case ACPI_IVRS_TYPE_MEMORY_ALL:
- case ACPI_IVRS_TYPE_MEMORY_ONE:
- case ACPI_IVRS_TYPE_MEMORY_RANGE:
- case ACPI_IVRS_TYPE_MEMORY_IOMMU:
- ivmd_block = container_of(ivrs_block, const struct acpi_ivrs_memory,
- header);
- return parse_ivmd_block(ivmd_block);
-
- default:
- AMD_IOMMU_DEBUG("IVRS Error: Invalid Block Type!\n");
- return -ENODEV;
- }
-
- return 0;
-}
-
static void __init dump_acpi_table_header(struct acpi_table_header *table)
{
int i;
@@ -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) )
+
+#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 )
+
static int __init parse_ivrs_table(struct acpi_table_header *table)
{
const struct acpi_ivrs_header *ivrs_block;
@@ -1010,7 +1007,10 @@ static int __init parse_ivrs_table(struct acpi_table_header *table)
return -ENODEV;
}
- error = parse_ivrs_block(ivrs_block);
+ if ( ivrs_block->type == ivhd_type )
+ error = parse_ivhd_block(to_ivhd_block(ivrs_block));
+ else if ( is_ivmd_block (ivrs_block->type) )
+ error = parse_ivmd_block(to_ivmd_block(ivrs_block));
length += ivrs_block->length;
}
@@ -1083,9 +1083,7 @@ static int __init detect_iommu_acpi(struct acpi_table_header *table)
if ( table->length < (length + ivrs_block->length) )
return -ENODEV;
if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
- amd_iommu_detect_one_acpi(
- container_of(ivrs_block, const struct acpi_ivrs_hardware,
- header)) != 0 )
+ amd_iommu_detect_one_acpi(to_ivhd_block(ivrs_block)) != 0 )
return -ENODEV;
length += ivrs_block->length;
}
@@ -1102,15 +1100,16 @@ static int __init get_last_bdf_ivhd(
{
const union acpi_ivhd_device *ivhd_device;
u16 block_length, dev_length;
+ int hdr_size = get_ivhd_header_size(ivhd_block) ;
int last_bdf = 0;
- if ( ivhd_block->header.length < sizeof(*ivhd_block) )
+ if ( ivhd_block->header.length < hdr_size )
{
AMD_IOMMU_DEBUG("IVHD Error: Invalid Block Length!\n");
return -ENODEV;
}
- block_length = sizeof(*ivhd_block);
+ block_length = hdr_size;
while ( ivhd_block->header.length >=
(block_length + sizeof(struct acpi_ivrs_de_header)) )
{
@@ -1153,7 +1152,7 @@ static int __init get_last_bdf_ivhd(
dev_length = sizeof(ivhd_device->special);
break;
default:
- AMD_IOMMU_DEBUG("IVHD Error: Invalid Device Type!\n");
+ AMD_IOMMU_DEBUG("IVHD Error: %s: Invalid Device Type!\n", __func__);
dev_length = 0;
break;
}
@@ -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 )
return ret;
UPDATE_LAST_BDF(ret);
@@ -1212,3 +1208,54 @@ int __init amd_iommu_update_ivrs_mapping_acpi(void)
return acpi_table_parse(ACPI_SIG_IVRS, parse_ivrs_table);
}
+
+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) )
+ {
+ 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;
+ blk = ivrs_block;
+ }
+
+ length += ivrs_block->length;
+ }
+
+ if ( !blk )
+ {
+ printk(XENLOG_ERR "Cannot find supported IVHD type.\n");
+ return -ENODEV;
+ }
+
+ AMD_IOMMU_DEBUG("Using IVHD type %#x\n", blk->type);
+
+ return blk->type;
+}
+
+int __init amd_iommu_get_supported_ivhd_type(void)
+{
+ if ( unlikely(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_MSI) )
+ return -EPERM;
+
+ return acpi_table_parse(ACPI_SIG_IVRS, get_supported_ivhd_type);
+}
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index d90a2d2..d76c993 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -35,6 +35,7 @@ static int __initdata nr_amd_iommus;
static struct tasklet amd_iommu_irq_tasklet;
unsigned int __read_mostly ivrs_bdf_entries;
+unsigned int __read_mostly ivhd_type;
static struct radix_tree_root ivrs_maps;
struct list_head amd_iommu_head;
struct table_struct device_table;
@@ -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;
+ ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
if ( !ivrs_bdf_entries )
goto error_out;
diff --git a/xen/include/acpi/actbl2.h b/xen/include/acpi/actbl2.h
index 87bc6b3..694d87e 100644
--- a/xen/include/acpi/actbl2.h
+++ b/xen/include/acpi/actbl2.h
@@ -527,12 +527,15 @@ struct acpi_ivrs_header {
enum acpi_ivrs_type {
ACPI_IVRS_TYPE_HARDWARE = 0x10,
+ ACPI_IVRS_TYPE_HARDWARE_11H = 0x11,
ACPI_IVRS_TYPE_MEMORY_ALL /* _MEMORY1 */ = 0x20,
ACPI_IVRS_TYPE_MEMORY_ONE /* _MEMORY2 */ = 0x21,
ACPI_IVRS_TYPE_MEMORY_RANGE /* _MEMORY3 */ = 0x22,
ACPI_IVRS_TYPE_MEMORY_IOMMU = 0x23
};
+#define IVHD_HIGHEST_SUPPORT_TYPE ACPI_IVRS_TYPE_HARDWARE_11H
+
/* Masks for Flags field above for IVHD subtable */
#define ACPI_IVHD_TT_ENABLE (1)
@@ -560,7 +563,9 @@ struct acpi_ivrs_hardware {
u64 base_address; /* IOMMU control registers */
u16 pci_segment_group;
u16 info; /* MSI number and unit ID */
- u32 reserved;
+ u32 iommu_attr;
+ u64 efr_image; /* Extd feature register */
+ u64 reserved;
};
/* Masks for Info field above */
diff --git a/xen/include/asm-x86/amd-iommu.h b/xen/include/asm-x86/amd-iommu.h
index e9fa9c2..722e729 100644
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -126,6 +126,7 @@ struct ivrs_mappings {
};
extern unsigned int ivrs_bdf_entries;
+extern unsigned int ivhd_type;
struct ivrs_mappings *get_ivrs_mappings(u16 seg);
int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 9c51172..e6065d3 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -42,6 +42,7 @@ struct acpi_ivrs_hardware;
/* amd-iommu-detect functions */
int amd_iommu_get_ivrs_dev_entries(void);
+int amd_iommu_get_supported_ivhd_type(void);
int amd_iommu_detect_one_acpi(const struct acpi_ivrs_hardware *);
int amd_iommu_detect_acpi(void);
void get_iommu_features(struct amd_iommu *iommu);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h
2016-05-13 19:54 [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h suravee.suthikulpanit
@ 2016-05-17 14:25 ` Jan Beulich
2016-05-19 6:30 ` Suravee Suthikulpanit
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2016-05-17 14:25 UTC (permalink / raw)
To: suravee.suthikulpanit; +Cc: xen-devel
>>> 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
> @@ -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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h
2016-05-17 14:25 ` Jan Beulich
@ 2016-05-19 6:30 ` Suravee Suthikulpanit
2016-05-19 9:09 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Suravee Suthikulpanit @ 2016-05-19 6:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h
2016-05-19 6:30 ` Suravee Suthikulpanit
@ 2016-05-19 9:09 ` Jan Beulich
2016-05-19 20:38 ` Suravee Suthikulpanit
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2016-05-19 9:09 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: xen-devel
>>> On 19.05.16 at 08:30, <Suravee.Suthikulpanit@amd.com> wrote:
> 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.
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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h
2016-05-19 9:09 ` Jan Beulich
@ 2016-05-19 20:38 ` Suravee Suthikulpanit
0 siblings, 0 replies; 5+ messages in thread
From: Suravee Suthikulpanit @ 2016-05-19 20:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
Hi Jan,
On 05/19/2016 04:09 AM, Jan Beulich wrote:
>>>> >>>+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
>
No, it doesn't need duplicated. It was accidentally duplicated.
Howerver, as you mentioned earlier, it probably should be moved to the
beginning of amd_iommu_init() before any parsing of the IVRS table.
Suravee.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-19 20:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-13 19:54 [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h suravee.suthikulpanit
2016-05-17 14:25 ` Jan Beulich
2016-05-19 6:30 ` Suravee Suthikulpanit
2016-05-19 9:09 ` Jan Beulich
2016-05-19 20:38 ` Suravee Suthikulpanit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).