These four patches try to refine the Intel IOMMU.
Patch 1/2 tries to make it more user friendly by add a zero-sized array in
some dmar data structure.
Patch 3 move the ckeck of Register Base Address ahead to avoid cleanup when it
is NULL.
Patch 4 re-use dmar_walk_dmar_table() to make the code easy to understand.
V2:
* add patch 3 and 4
Wei Yang (4):
iommu/vt-d: replace *hdr with hdr[0] in struct dmar_drhd_unit
iommu/vt-d: use zero-sized array in DMAR related ACPI structures
iommu/vt-d: check Register Base Address at the beginning of
dmar_parse_one_drhd()
iommu/vt-d: refine dmar_acpi_dev_scope_init() with
dmar_walk_dmar_table()
drivers/iommu/dmar.c | 127 ++++++++++++++++++++++++-------------------------
include/acpi/actbl2.h | 31 ++++++------
include/linux/dmar.h | 2 +-
3 files changed, 81 insertions(+), 79 deletions(-)
--
1.7.9.5
hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
at the end of struct dmar_drhd_unit. One zero-sized array may be more
elegant for this purpose.
This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
Besides this, this patch includes other two changes:
1. remove unnecessary type cast in dmar_table_detect()
2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/dmar.c | 15 ++++-----------
include/linux/dmar.h | 2 +-
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 80e3c17..d6dd23f 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -292,8 +292,7 @@ static int dmar_pci_bus_add_dev(struct dmar_pci_notify_info *info)
if (dmaru->include_all)
continue;
- drhd = container_of(dmaru->hdr,
- struct acpi_dmar_hardware_unit, header);
+ drhd = (struct acpi_dmar_hardware_unit *)dmaru->hdr;
ret = dmar_insert_dev_scope(info, (void *)(drhd + 1),
((void *)drhd) + drhd->header.length,
dmaru->segment,
@@ -390,7 +389,6 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
* If header is allocated from slab by ACPI _DSM method, we need to
* copy the content because the memory buffer will be freed on return.
*/
- dmaru->hdr = (void *)(dmaru + 1);
memcpy(dmaru->hdr, header, header->length);
dmaru->reg_base_addr = drhd->address;
dmaru->segment = drhd->segment;
@@ -529,8 +527,7 @@ static int __init dmar_table_detect(void)
/* if we could find DMAR table, then there are DMAR devices */
status = acpi_get_table_with_size(ACPI_SIG_DMAR, 0,
- (struct acpi_table_header **)&dmar_tbl,
- &dmar_tbl_size);
+ &dmar_tbl, &dmar_tbl_size);
if (ACPI_SUCCESS(status) && !dmar_tbl) {
pr_warn("Unable to map DMAR\n");
@@ -663,9 +660,7 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
rcu_read_lock();
for_each_drhd_unit(dmaru) {
- drhd = container_of(dmaru->hdr,
- struct acpi_dmar_hardware_unit,
- header);
+ drhd = (struct acpi_dmar_hardware_unit *)dmaru->hdr;
if (dmaru->include_all &&
drhd->segment == pci_domain_nr(dev->bus))
@@ -693,9 +688,7 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
struct acpi_dmar_pci_path *path;
for_each_drhd_unit(dmaru) {
- drhd = container_of(dmaru->hdr,
- struct acpi_dmar_hardware_unit,
- header);
+ drhd = (struct acpi_dmar_hardware_unit *)dmaru->hdr;
for (scope = (void *)(drhd + 1);
(unsigned long)scope < ((unsigned long)drhd) + drhd->header.length;
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e9bc929..5f6da7e 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -52,7 +52,6 @@ struct dmar_dev_scope {
extern struct acpi_table_header *dmar_tbl;
struct dmar_drhd_unit {
struct list_head list; /* list of drhd units */
- struct acpi_dmar_header *hdr; /* ACPI header */
u64 reg_base_addr; /* register base address*/
struct dmar_dev_scope *devices;/* target device array */
int devices_cnt; /* target device count */
@@ -60,6 +59,7 @@ struct dmar_drhd_unit {
u8 ignored:1; /* ignore drhd */
u8 include_all:1;
struct intel_iommu *iommu;
+ struct acpi_dmar_header hdr[0];/* ACPI header */
};
struct dmar_pci_path {
--
1.7.9.5
1. DMAR table has variable number of remapping entries
2. DMAR hardware unit has variable number of device scope
3. DMAR device scope has variable number of pci path
In current implementation, we use (head + 1) to access these variable
number elements, which may not be obvious for audience. Zero-sized array is
usually used for this purpose.
This patch adds zero-sized array for variable elements in DMAR ACPI
structures, which tries to make the code more audience friendly.
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/dmar.c | 22 ++++++++++------------
include/acpi/actbl2.h | 31 +++++++++++++++++--------------
2 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index d6dd23f..04f199c 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -225,7 +225,6 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
int i, level;
struct device *tmp, *dev = &info->dev->dev;
struct acpi_dmar_device_scope *scope;
- struct acpi_dmar_pci_path *path;
if (segment != info->seg)
return 0;
@@ -236,9 +235,8 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
scope->entry_type != ACPI_DMAR_SCOPE_TYPE_BRIDGE)
continue;
- path = (struct acpi_dmar_pci_path *)(scope + 1);
- level = (scope->length - sizeof(*scope)) / sizeof(*path);
- if (!dmar_match_pci_path(info, scope->bus, path, level))
+ level = (scope->length - sizeof(*scope)) / sizeof(*scope->path);
+ if (!dmar_match_pci_path(info, scope->bus, scope->path, level))
continue;
if ((scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT) ^
@@ -393,7 +391,7 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
dmaru->reg_base_addr = drhd->address;
dmaru->segment = drhd->segment;
dmaru->include_all = drhd->flags & 0x1; /* BIT0: INCLUDE_ALL */
- dmaru->devices = dmar_alloc_dev_scope((void *)(drhd + 1),
+ dmaru->devices = dmar_alloc_dev_scope(drhd->dev_scope,
((void *)drhd) + drhd->header.length,
&dmaru->devices_cnt);
if (dmaru->devices_cnt && dmaru->devices == NULL) {
@@ -579,7 +577,7 @@ static int dmar_walk_remapping_entries(struct acpi_dmar_header *start,
static inline int dmar_walk_dmar_table(struct acpi_table_dmar *dmar,
struct dmar_res_callback *cb)
{
- return dmar_walk_remapping_entries((void *)(dmar + 1),
+ return dmar_walk_remapping_entries(dmar->remapping_entries,
dmar->header.length - sizeof(*dmar), cb);
}
@@ -685,12 +683,11 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
struct acpi_dmar_device_scope *scope;
struct device *tmp;
int i;
- struct acpi_dmar_pci_path *path;
for_each_drhd_unit(dmaru) {
drhd = (struct acpi_dmar_hardware_unit *)dmaru->hdr;
- for (scope = (void *)(drhd + 1);
+ for (scope = drhd->dev_scope;
(unsigned long)scope < ((unsigned long)drhd) + drhd->header.length;
scope = ((void *)scope) + scope->length) {
if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
@@ -698,15 +695,16 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
if (scope->enumeration_id != device_number)
continue;
- path = (void *)(scope + 1);
pr_info("ACPI device \"%s\" under DMAR at %llx as %02x:%02x.%d\n",
dev_name(&adev->dev), dmaru->reg_base_addr,
- scope->bus, path->device, path->function);
+ scope->bus, scope->path->device,
+ scope->path->function);
for_each_dev_scope(dmaru->devices, dmaru->devices_cnt, i, tmp)
if (tmp == NULL) {
dmaru->devices[i].bus = scope->bus;
- dmaru->devices[i].devfn = PCI_DEVFN(path->device,
- path->function);
+ dmaru->devices[i].devfn =
+ PCI_DEVFN(scope->path->device,
+ scope->path->function);
rcu_assign_pointer(dmaru->devices[i].dev,
get_device(&adev->dev));
return;
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 6e28f54..fd5c615 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -403,17 +403,6 @@ struct acpi_table_dbgp {
*
******************************************************************************/
-struct acpi_table_dmar {
- struct acpi_table_header header; /* Common ACPI table header */
- u8 width; /* Host Address Width */
- u8 flags;
- u8 reserved[10];
-};
-
-/* Masks for Flags field above */
-
-#define ACPI_DMAR_INTR_REMAP (1)
-
/* DMAR subtable header */
struct acpi_dmar_header {
@@ -434,12 +423,18 @@ enum acpi_dmar_type {
/* DMAR Device Scope structure */
+struct acpi_dmar_pci_path {
+ u8 device;
+ u8 function;
+};
+
struct acpi_dmar_device_scope {
u8 entry_type;
u8 length;
u16 reserved;
u8 enumeration_id;
u8 bus;
+ struct acpi_dmar_pci_path path[0];
};
/* Values for entry_type in struct acpi_dmar_device_scope - device types */
@@ -454,11 +449,18 @@ enum acpi_dmar_scope_type {
ACPI_DMAR_SCOPE_TYPE_RESERVED = 6 /* 6 and greater are reserved */
};
-struct acpi_dmar_pci_path {
- u8 device;
- u8 function;
+struct acpi_table_dmar {
+ struct acpi_table_header header; /* Common ACPI table header */
+ u8 width; /* Host Address Width */
+ u8 flags;
+ u8 reserved[10];
+ struct acpi_dmar_header remapping_entries[0];
};
+/* Masks for Flags field above */
+
+#define ACPI_DMAR_INTR_REMAP (1)
+
/*
* DMAR Subtables, correspond to Type in struct acpi_dmar_header
*/
@@ -471,6 +473,7 @@ struct acpi_dmar_hardware_unit {
u8 reserved;
u16 segment;
u64 address; /* Register Base Address */
+ struct acpi_dmar_device_scope dev_scope[0];
};
/* Masks for Flags field above */
--
1.7.9.5
A NULL value of Register Base Address in a Hardware Unit Definition means
it is an invalid dmar. Current implementation checks this value in
alloc_iommu(), by when it has already allocated memory to store itself and
device scope.
This patch moves the check at the beginning of dmar_parse_one_drhd(), so
that it notices this is an invalid dmar and avoids related setup jobs.
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/dmar.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 04f199c..82f0b92 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -363,6 +363,18 @@ dmar_find_dmaru(struct acpi_dmar_hardware_unit *drhd)
return NULL;
}
+static void warn_invalid_dmar(u64 addr, const char *message)
+{
+ WARN_TAINT_ONCE(
+ 1, TAINT_FIRMWARE_WORKAROUND,
+ "Your BIOS is broken; DMAR reported at address %llx%s!\n"
+ "BIOS vendor: %s; Ver: %s; Product Version: %s\n",
+ addr, message,
+ dmi_get_system_info(DMI_BIOS_VENDOR),
+ dmi_get_system_info(DMI_BIOS_VERSION),
+ dmi_get_system_info(DMI_PRODUCT_VERSION));
+}
+
/**
* dmar_parse_one_drhd - parses exactly one DMA remapping hardware definition
* structure which uniquely represent one DMA remapping hardware unit
@@ -379,6 +391,11 @@ static int dmar_parse_one_drhd(struct acpi_dmar_header *header, void *arg)
if (dmaru)
goto out;
+ if (!drhd->address) {
+ warn_invalid_dmar(0, "");
+ return -EINVAL;
+ }
+
dmaru = kzalloc(sizeof(*dmaru) + header->length, GFP_KERNEL);
if (!dmaru)
return -ENOMEM;
@@ -808,18 +825,6 @@ int __init dmar_table_init(void)
return dmar_table_initialized < 0 ? dmar_table_initialized : 0;
}
-static void warn_invalid_dmar(u64 addr, const char *message)
-{
- WARN_TAINT_ONCE(
- 1, TAINT_FIRMWARE_WORKAROUND,
- "Your BIOS is broken; DMAR reported at address %llx%s!\n"
- "BIOS vendor: %s; Ver: %s; Product Version: %s\n",
- addr, message,
- dmi_get_system_info(DMI_BIOS_VENDOR),
- dmi_get_system_info(DMI_BIOS_VERSION),
- dmi_get_system_info(DMI_PRODUCT_VERSION));
-}
-
static int __ref
dmar_validate_one_drhd(struct acpi_dmar_header *entry, void *arg)
{
@@ -995,11 +1000,6 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
int msagaw = 0;
int err;
- if (!drhd->reg_base_addr) {
- warn_invalid_dmar(0, "");
- return -EINVAL;
- }
-
iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
if (!iommu)
return -ENOMEM;
--
1.7.9.5
dmar_acpi_dev_scope_init() iterates on the remapping structure and just do
proper job for ANDD structure. This is the what dmar_walk_dmar_table()
does.
This patch improves the code with dmar_walk_dmar_table().
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/dmar.c | 56 ++++++++++++++++++++++++++++----------------------
1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 82f0b92..538e260 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -733,36 +733,44 @@ static void __init dmar_acpi_insert_dev_scope(u8 device_number,
device_number, dev_name(&adev->dev));
}
-static int __init dmar_acpi_dev_scope_init(void)
+static int __dmar_acpi_dev_scope_init(struct acpi_dmar_header *header,
+ void *arg)
{
struct acpi_dmar_andd *andd;
+ acpi_handle h;
+ struct acpi_device *adev;
+
+ andd = (struct acpi_dmar_andd *)header;
+ if (!ACPI_SUCCESS(acpi_get_handle(ACPI_ROOT_OBJECT,
+ andd->device_name,
+ &h))) {
+ pr_err("Failed to find handle for ACPI object %s\n",
+ andd->device_name);
+ return 0;
+ }
+ if (acpi_bus_get_device(h, &adev)) {
+ pr_err("Failed to get device for ACPI object %s\n",
+ andd->device_name);
+ return 0;
+ }
+ dmar_acpi_insert_dev_scope(andd->device_number, adev);
+ return 0;
+}
+
+static int __init dmar_acpi_dev_scope_init(void)
+{
+ struct acpi_table_dmar *dmar;
+ struct dmar_res_callback cb = {
+ .print_entry = false,
+ .ignore_unhandled = true,
+ .cb[ACPI_DMAR_TYPE_NAMESPACE] = &__dmar_acpi_dev_scope_init,
+ };
if (dmar_tbl == NULL)
return -ENODEV;
- for (andd = (void *)dmar_tbl + sizeof(struct acpi_table_dmar);
- ((unsigned long)andd) < ((unsigned long)dmar_tbl) + dmar_tbl->length;
- andd = ((void *)andd) + andd->header.length) {
- if (andd->header.type == ACPI_DMAR_TYPE_NAMESPACE) {
- acpi_handle h;
- struct acpi_device *adev;
-
- if (!ACPI_SUCCESS(acpi_get_handle(ACPI_ROOT_OBJECT,
- andd->device_name,
- &h))) {
- pr_err("Failed to find handle for ACPI object %s\n",
- andd->device_name);
- continue;
- }
- if (acpi_bus_get_device(h, &adev)) {
- pr_err("Failed to get device for ACPI object %s\n",
- andd->device_name);
- continue;
- }
- dmar_acpi_insert_dev_scope(andd->device_number, adev);
- }
- }
- return 0;
+ dmar = (struct acpi_table_dmar *)dmar_tbl;
+ return dmar_walk_dmar_table(dmar, &cb);
}
int __init dmar_dev_scope_init(void)
--
1.7.9.5
On Sun, 20 Mar 2016, Wei Yang wrote:
> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
> at the end of struct dmar_drhd_unit. One zero-sized array may be more
> elegant for this purpose.
You forget to tell why.
> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
>
> Besides this, this patch includes other two changes:
> 1. remove unnecessary type cast in dmar_table_detect()
Again. Why is it not necessary?
> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
Don't even think about doing that. container_of() is there for a reason.
Your change works today, because the embedded structure is the first one in
the containing structure. If the containing structure gets reordered later,
the whole thing will explode in hard to debug ways.
Even if such a reordering is unlikely in that ACPI case, we just don't do
that. It's bad and sloppy coding style. The generated code is the same.
Thanks,
tglx
Hi, Thomas
Glad to receive your comment. Please see my reply below.
On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote:
>On Sun, 20 Mar 2016, Wei Yang wrote:
>
>> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
>> at the end of struct dmar_drhd_unit. One zero-sized array may be more
>> elegant for this purpose.
>
>You forget to tell why.
>
One possible benefit is to save some space.
Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework",
dmaru->hdr just points to the memory region of DMA remapping hardware
definition. In this case, it would have no difference to where we put hdr.
After this commit, DMA remapping hardware definition is copied and
attach to the end of dmaru structure. By replacing a pointer with a zero-sized
array, that would save some space for this structure.
>> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
>>
>> Besides this, this patch includes other two changes:
>> 1. remove unnecessary type cast in dmar_table_detect()
>
>Again. Why is it not necessary?
>
The second parameter's type of function acpi_get_table_with_size() is "struct
acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *".
So without the type cast, the type of parameter and the function definition
matches.
That's why I want to remove it.
>> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
>
>Don't even think about doing that. container_of() is there for a reason.
>
>Your change works today, because the embedded structure is the first one in
>the containing structure. If the containing structure gets reordered later,
>the whole thing will explode in hard to debug ways.
>
>Even if such a reordering is unlikely in that ACPI case, we just don't do
>that. It's bad and sloppy coding style. The generated code is the same.
>
Yes, I agree with you that make this change should be very careful, while I
think the original usage of container_of() is not necessary.
Literally, it converts "struct acpi_dmar_header" to "struct
acpi_dmar_hardware_unit", because the first one is an element "header" of the
second one. While let's look at how the dmaru->hdr is initialized in
dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit"
to a region where dmaru->hdr points to. So the code itself implies "struct
acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit".
Otherwise, we can't do this memcpy in dmar_parse_one_drhd().
BTW, I am thinking changing the type of dmaru->hdr from "struct
acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the
code would be more self explain, and it doesn't need to cast back and forth.
Thanks again for your review, have a good day~
>Thanks,
>
> tglx
--
Wei Yang
Help you, Help me
On Mon, 21 Mar 2016, Wei Yang wrote:
> On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote:
> >On Sun, 20 Mar 2016, Wei Yang wrote:
> >
> >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
> >> at the end of struct dmar_drhd_unit. One zero-sized array may be more
> >> elegant for this purpose.
> >
> >You forget to tell why.
> >
>
> One possible benefit is to save some space.
>
> Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework",
> dmaru->hdr just points to the memory region of DMA remapping hardware
> definition. In this case, it would have no difference to where we put hdr.
>
> After this commit, DMA remapping hardware definition is copied and
> attach to the end of dmaru structure. By replacing a pointer with a zero-sized
> array, that would save some space for this structure.
Sure and exactly that explanation should be in the changelog. Not some
handwaving "may be more elegant". We don't care about elegance, we care about
correctness.
> >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
> >>
> >> Besides this, this patch includes other two changes:
> >> 1. remove unnecessary type cast in dmar_table_detect()
> >
> >Again. Why is it not necessary?
> >
>
> The second parameter's type of function acpi_get_table_with_size() is "struct
> acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *".
>
> So without the type cast, the type of parameter and the function definition
> matches.
That's the information which a changelog wants to have, because otherwise a
reviewer is forced to lookup the prototypes ....
So a simple:
"Remove redundant type case to same type"
would have told me what you are doing.
> >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
> >
> >Don't even think about doing that. container_of() is there for a reason.
> >
> >Your change works today, because the embedded structure is the first one in
> >the containing structure. If the containing structure gets reordered later,
> >the whole thing will explode in hard to debug ways.
> >
> >Even if such a reordering is unlikely in that ACPI case, we just don't do
> >that. It's bad and sloppy coding style. The generated code is the same.
> >
>
> Yes, I agree with you that make this change should be very careful, while I
> think the original usage of container_of() is not necessary.
It's not necessary, but it is correct. Removing it is just putting assumptions
into the code, which is never a good idea.
> Literally, it converts "struct acpi_dmar_header" to "struct
> acpi_dmar_hardware_unit", because the first one is an element "header" of the
> second one. While let's look at how the dmaru->hdr is initialized in
> dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit"
> to a region where dmaru->hdr points to. So the code itself implies "struct
> acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit".
Which is wrong to begin with. Assumption of first elements are crap.
> Otherwise, we can't do this memcpy in dmar_parse_one_drhd().
> BTW, I am thinking changing the type of dmaru->hdr from "struct
> acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the
> code would be more self explain, and it doesn't need to cast back and forth.
Yes, that makes sense.
Thanks,
tglx
On Mon, Mar 21, 2016 at 05:53:15PM +0100, Thomas Gleixner wrote:
>On Mon, 21 Mar 2016, Wei Yang wrote:
>> On Sun, Mar 20, 2016 at 04:42:29PM +0100, Thomas Gleixner wrote:
>> >On Sun, 20 Mar 2016, Wei Yang wrote:
>> >
>> >> hdr in struct dmar_drhd_unit is used to point the DMAR hardware unit copied
>> >> at the end of struct dmar_drhd_unit. One zero-sized array may be more
>> >> elegant for this purpose.
>> >
>> >You forget to tell why.
>> >
>>
>> One possible benefit is to save some space.
>>
>> Before commit 6b1972493a84 "iommu/vt-d: Implement DMAR unit hotplug framework",
>> dmaru->hdr just points to the memory region of DMA remapping hardware
>> definition. In this case, it would have no difference to where we put hdr.
>>
>> After this commit, DMA remapping hardware definition is copied and
>> attach to the end of dmaru structure. By replacing a pointer with a zero-sized
>> array, that would save some space for this structure.
>
>Sure and exactly that explanation should be in the changelog. Not some
>handwaving "may be more elegant". We don't care about elegance, we care about
>correctness.
>
Hey, Thomas
Nice to see your comment again~
You are right, I would add this in the changelog.
>> >> This patch replace *hdr with hdr[0] in struct dmar_drhd_unit.
>> >>
>> >> Besides this, this patch includes other two changes:
>> >> 1. remove unnecessary type cast in dmar_table_detect()
>> >
>> >Again. Why is it not necessary?
>> >
>>
>> The second parameter's type of function acpi_get_table_with_size() is "struct
>> acpi_table_header **", and type of dmar_tbl is "struct acpi_table_header *".
>>
>> So without the type cast, the type of parameter and the function definition
>> matches.
>
>That's the information which a changelog wants to have, because otherwise a
>reviewer is forced to lookup the prototypes ....
>
>So a simple:
>
> "Remove redundant type case to same type"
>
>would have told me what you are doing.
>
Agree, thanks for your suggestion, this really helps reviewers.
>> >> 2. type cast from acpi_dmar_header to acpi_dmar_hardware_unit directly
>> >
>> >Don't even think about doing that. container_of() is there for a reason.
>> >
>> >Your change works today, because the embedded structure is the first one in
>> >the containing structure. If the containing structure gets reordered later,
>> >the whole thing will explode in hard to debug ways.
>> >
>> >Even if such a reordering is unlikely in that ACPI case, we just don't do
>> >that. It's bad and sloppy coding style. The generated code is the same.
>> >
>>
>> Yes, I agree with you that make this change should be very careful, while I
>> think the original usage of container_of() is not necessary.
>
>It's not necessary, but it is correct. Removing it is just putting assumptions
>into the code, which is never a good idea.
>
>> Literally, it converts "struct acpi_dmar_header" to "struct
>> acpi_dmar_hardware_unit", because the first one is an element "header" of the
>> second one. While let's look at how the dmaru->hdr is initialized in
>> dmar_parse_one_drhd(), we copy the memory of "struct acpi_dmar_hardware_unit"
>> to a region where dmaru->hdr points to. So the code itself implies "struct
>> acpi_dmar_header" is the first element of "struct acpi_dmar_hardware_unit".
>u link
>Which is wrong to begin with. Assumption of first elements are crap.
>
>> Otherwise, we can't do this memcpy in dmar_parse_one_drhd().
>
>> BTW, I am thinking changing the type of dmaru->hdr from "struct
>> acpi_dmar_header *" to "struct acpi_dmar_hardware_unit *". By doing so the
>> code would be more self explain, and it doesn't need to cast back and forth.
>
>Yes, that makes sense.
>
Ah, I am happy that you like this idea :-)
Let me format V3. Have a good night:)
>Thanks,
>
> tglx
--
Wei Yang
Help you, Help me