From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to have some abstract way to add new device to the IOMMU
based on the generic IOMMU DT bindings [1] which can be used for
both DT (right now) and ACPI (in future).

For that reason we can borrow the idea used in Linux these days
called "iommu_fwspec". Having this in, it will be possible
to configure IOMMU master interfaces of the device (device IDs)
from a single common place and avoid keeping almost identical look-up
implementations in each IOMMU driver.

There is no need to port the whole implementation of "iommu_fwspec"
to Xen, we could, probably, end up with a much simpler solution,
some "stripped down" version which fits our requirements.

So, this patch adds the following:
1. A common structure "iommu_fwspec" to hold the the per-device
   firmware data
2. New member "iommu_fwspec" of struct device
3. Functions/helpers to deal with "dev->iommu_fwspec"

It should be noted that in comparison of the original "iommu_fwspec"
Xen's variant doesn't contain some fields, which are not really
needed at the moment (ops, flag) and "iommu_fwnode" field was replaced
by "iommu_dev" to avoid porting a lot of code (to support "fwnode_handle")
with little benefit.

The "iommu_fwspec" support is based on the Linux's commit:
f74c2bb98776e2de508f4d607cd519873065118e "Linux 5.3-rc8"

Subsequent patches will use of that support.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
Changes V3 -> V4:
    - modified iommu_fwspec_add_ids() to use new implementation of
      xrealloc_flex_struct()
    - mentioned exact Linux version we are based on
    - fixed Grammatical error

Changes V2 -> V3:
    - added Copyright from Linux
    - ordered the headers alphabetically
    - removed check for not a NULL before calling xfree()
    - used unsigned for variables which can't be negative
    - removed #include <asm/iommu_fwspec.h> from iommu.h
    - added check to iommu_fwspec_init() to not allow overriding
    - clarified comments in code
    - modified iommu_fwspec_add_ids() to use type-safe xrealloc_flex_struct()
---
 xen/drivers/passthrough/arm/Makefile       |  2 +-
 xen/drivers/passthrough/arm/iommu_fwspec.c | 93 ++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h               |  1 +
 xen/include/asm-arm/iommu_fwspec.h         | 68 ++++++++++++++++++++++
 4 files changed, 163 insertions(+), 1 deletion(-)
 create mode 100644 xen/drivers/passthrough/arm/iommu_fwspec.c
 create mode 100644 xen/include/asm-arm/iommu_fwspec.h

diff --git a/xen/drivers/passthrough/arm/Makefile b/xen/drivers/passthrough/arm/Makefile
index 4abb87a..5fbad45 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,2 @@
-obj-y += iommu.o iommu_helpers.o
+obj-y += iommu.o iommu_helpers.o iommu_fwspec.o
 obj-$(CONFIG_ARM_SMMU) += smmu.o
diff --git a/xen/drivers/passthrough/arm/iommu_fwspec.c b/xen/drivers/passthrough/arm/iommu_fwspec.c
new file mode 100644
index 0000000..7046faf
--- /dev/null
+++ b/xen/drivers/passthrough/arm/iommu_fwspec.c
@@ -0,0 +1,93 @@
+/*
+ * xen/drivers/passthrough/arm/iommu_fwspec.c
+ *
+ * Contains functions to maintain per-device firmware data
+ *
+ * Based on Linux's iommu_fwspec support you can find at:
+ *    drivers/iommu/iommu.c
+ *
+ * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
+ *
+ * Copyright (C) 2019 EPAM Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/iommu.h>
+#include <xen/lib.h>
+
+#include <asm/device.h>
+#include <asm/iommu_fwspec.h>
+
+int iommu_fwspec_init(struct device *dev, struct device *iommu_dev)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+    if ( fwspec )
+    {
+        /* We expect the device to be protected by only one IOMMU. */
+        if ( fwspec->iommu_dev != iommu_dev )
+            return -EINVAL;
+
+        return 0;
+    }
+
+    fwspec = xzalloc(struct iommu_fwspec);
+    if ( !fwspec )
+        return -ENOMEM;
+
+    fwspec->iommu_dev = iommu_dev;
+    dev_iommu_fwspec_set(dev, fwspec);
+
+    return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+    xfree(fwspec);
+    dev_iommu_fwspec_set(dev, NULL);
+}
+
+int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids,
+                         unsigned int num_ids)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+    unsigned int i;
+
+    if ( !fwspec )
+        return -EINVAL;
+
+    fwspec = xrealloc_flex_struct(fwspec, ids, fwspec->num_ids + num_ids);
+    if ( !fwspec )
+        return -ENOMEM;
+
+    dev_iommu_fwspec_set(dev, fwspec);
+
+    for ( i = 0; i < num_ids; i++ )
+        fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+    fwspec->num_ids += num_ids;
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index ee1c3bc..ee7cff2 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -18,6 +18,7 @@ struct device
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
     struct dev_archdata archdata;
+    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
 };
 
 typedef struct device device_t;
diff --git a/xen/include/asm-arm/iommu_fwspec.h b/xen/include/asm-arm/iommu_fwspec.h
new file mode 100644
index 0000000..8ce4da1
--- /dev/null
+++ b/xen/include/asm-arm/iommu_fwspec.h
@@ -0,0 +1,68 @@
+/*
+ * xen/include/asm-arm/iommu_fwspec.h
+ *
+ * Contains a common structure to hold the per-device firmware data and
+ * declaration of functions used to maintain that data
+ *
+ * Based on Linux's iommu_fwspec support you can find at:
+ *    include/linux/iommu.h
+ *
+ * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
+ *
+ * Copyright (C) 2019 EPAM Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ARCH_ARM_IOMMU_FWSPEC_H__
+#define __ARCH_ARM_IOMMU_FWSPEC_H__
+
+/* per-device IOMMU instance data */
+struct iommu_fwspec {
+    /* this device's IOMMU */
+    struct device *iommu_dev;
+    /* IOMMU driver private data for this device */
+    void *iommu_priv;
+    /* number of associated device IDs */
+    unsigned int num_ids;
+    /* IDs which this device may present to the IOMMU */
+    uint32_t ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct device *iommu_dev);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids,
+                         unsigned int num_ids);
+
+static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
+{
+    return dev->iommu_fwspec;
+}
+
+static inline void dev_iommu_fwspec_set(struct device *dev,
+                                        struct iommu_fwspec *fwspec)
+{
+    dev->iommu_fwspec = fwspec;
+}
+
+#endif /* __ARCH_ARM_IOMMU_FWSPEC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 13.09.2019 17:35, Oleksandr Tyshchenko wrote:
> --- /dev/null
> +++ b/xen/include/asm-arm/iommu_fwspec.h
> @@ -0,0 +1,68 @@
> +/*
> + * xen/include/asm-arm/iommu_fwspec.h
> + *
> + * Contains a common structure to hold the per-device firmware data and
> + * declaration of functions used to maintain that data
> + *
> + * Based on Linux's iommu_fwspec support you can find at:
> + *    include/linux/iommu.h
> + *
> + * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
> + *
> + * Copyright (C) 2019 EPAM Systems Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __ARCH_ARM_IOMMU_FWSPEC_H__
> +#define __ARCH_ARM_IOMMU_FWSPEC_H__
> +
> +/* per-device IOMMU instance data */
> +struct iommu_fwspec {
> +    /* this device's IOMMU */
> +    struct device *iommu_dev;
> +    /* IOMMU driver private data for this device */
> +    void *iommu_priv;
> +    /* number of associated device IDs */
> +    unsigned int num_ids;
> +    /* IDs which this device may present to the IOMMU */
> +    uint32_t ids[1];
> +};

Note that you abuse xrealloc_flex_struct() when using it with such
a type: The last field is _not_ a flexible array member. Compilers
might legitimately warn if they can prove that you access
p->ids[1] anywhere, despite you (presumably) having allocated enough
space. (I haven't been able to think of a way for the macro to
actually detect and hence refuse such wrong uses.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 16.09.19 13:40, Jan Beulich wrote:

Hi, Jan

>
>> +
>> +/* per-device IOMMU instance data */
>> +struct iommu_fwspec {
>> +    /* this device's IOMMU */
>> +    struct device *iommu_dev;
>> +    /* IOMMU driver private data for this device */
>> +    void *iommu_priv;
>> +    /* number of associated device IDs */
>> +    unsigned int num_ids;
>> +    /* IDs which this device may present to the IOMMU */
>> +    uint32_t ids[1];
>> +};
> Note that you abuse xrealloc_flex_struct() when using it with such
> a type: The last field is _not_ a flexible array member. Compilers
> might legitimately warn if they can prove that you access
> p->ids[1] anywhere, despite you (presumably) having allocated enough
> space. (I haven't been able to think of a way for the macro to
> actually detect and hence refuse such wrong uses.)

Indeed, you are right. I am in doubt, whether to retain ported from 
Linux code (ids[1])

and mention about such abuse or change it to deal with real flexible 
array member (ids[]). Any thoughts?

-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 16.09.2019 20:08, Oleksandr wrote:
> On 16.09.19 13:40, Jan Beulich wrote:
>>> +/* per-device IOMMU instance data */
>>> +struct iommu_fwspec {
>>> +    /* this device's IOMMU */
>>> +    struct device *iommu_dev;
>>> +    /* IOMMU driver private data for this device */
>>> +    void *iommu_priv;
>>> +    /* number of associated device IDs */
>>> +    unsigned int num_ids;
>>> +    /* IDs which this device may present to the IOMMU */
>>> +    uint32_t ids[1];
>>> +};
>> Note that you abuse xrealloc_flex_struct() when using it with such
>> a type: The last field is _not_ a flexible array member. Compilers
>> might legitimately warn if they can prove that you access
>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>> space. (I haven't been able to think of a way for the macro to
>> actually detect and hence refuse such wrong uses.)
> 
> Indeed, you are right. I am in doubt, whether to retain ported from 
> Linux code (ids[1])
> 
> and mention about such abuse or change it to deal with real flexible 
> array member (ids[]). Any thoughts?

I'm of the strong opinion that you should switch to [] (or at
least [0]) notation.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 17.09.19 09:12, Jan Beulich wrote:

Hi, Jan

> On 16.09.2019 20:08, Oleksandr wrote:
>> On 16.09.19 13:40, Jan Beulich wrote:
>>>> +/* per-device IOMMU instance data */
>>>> +struct iommu_fwspec {
>>>> +    /* this device's IOMMU */
>>>> +    struct device *iommu_dev;
>>>> +    /* IOMMU driver private data for this device */
>>>> +    void *iommu_priv;
>>>> +    /* number of associated device IDs */
>>>> +    unsigned int num_ids;
>>>> +    /* IDs which this device may present to the IOMMU */
>>>> +    uint32_t ids[1];
>>>> +};
>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>> a type: The last field is _not_ a flexible array member. Compilers
>>> might legitimately warn if they can prove that you access
>>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>>> space. (I haven't been able to think of a way for the macro to
>>> actually detect and hence refuse such wrong uses.)
>> Indeed, you are right. I am in doubt, whether to retain ported from
>> Linux code (ids[1])
>>
>> and mention about such abuse or change it to deal with real flexible
>> array member (ids[]). Any thoughts?
> I'm of the strong opinion that you should switch to [] (or at
> least [0]) notation.

I got it. Well, will switch to ids[] if there are no objections.

Thank you.

-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Hi,

On 17/09/2019 19:18, Oleksandr wrote:
> 
> On 17.09.19 09:12, Jan Beulich wrote:
> 
> Hi, Jan
> 
>> On 16.09.2019 20:08, Oleksandr wrote:
>>> On 16.09.19 13:40, Jan Beulich wrote:
>>>>> +/* per-device IOMMU instance data */
>>>>> +struct iommu_fwspec {
>>>>> +    /* this device's IOMMU */
>>>>> +    struct device *iommu_dev;
>>>>> +    /* IOMMU driver private data for this device */
>>>>> +    void *iommu_priv;
>>>>> +    /* number of associated device IDs */
>>>>> +    unsigned int num_ids;
>>>>> +    /* IDs which this device may present to the IOMMU */
>>>>> +    uint32_t ids[1];
>>>>> +};
>>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>>> a type: The last field is _not_ a flexible array member. Compilers
>>>> might legitimately warn if they can prove that you access
>>>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>>>> space. (I haven't been able to think of a way for the macro to
>>>> actually detect and hence refuse such wrong uses.)
>>> Indeed, you are right. I am in doubt, whether to retain ported from
>>> Linux code (ids[1])
>>>
>>> and mention about such abuse or change it to deal with real flexible
>>> array member (ids[]). Any thoughts?
>> I'm of the strong opinion that you should switch to [] (or at
>> least [0]) notation.
> 
> I got it. Well, will switch to ids[] if there are no objections.

I suspect the rationale to use 1 rather than 0 is to avoid the re-allocation in 
the common case where a device has a single ID.

I would like to retain the similar behavior. The ids[1] is probably the most 
pretty way to do it.

Another solution would to use xmalloc_bytes() for the initial allocation of 
xmalloc_bytes().

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 19.09.2019 12:12, Julien Grall wrote:
> Hi,
> 
> On 17/09/2019 19:18, Oleksandr wrote:
>>
>> On 17.09.19 09:12, Jan Beulich wrote:
>>
>> Hi, Jan
>>
>>> On 16.09.2019 20:08, Oleksandr wrote:
>>>> On 16.09.19 13:40, Jan Beulich wrote:
>>>>>> +/* per-device IOMMU instance data */
>>>>>> +struct iommu_fwspec {
>>>>>> +    /* this device's IOMMU */
>>>>>> +    struct device *iommu_dev;
>>>>>> +    /* IOMMU driver private data for this device */
>>>>>> +    void *iommu_priv;
>>>>>> +    /* number of associated device IDs */
>>>>>> +    unsigned int num_ids;
>>>>>> +    /* IDs which this device may present to the IOMMU */
>>>>>> +    uint32_t ids[1];
>>>>>> +};
>>>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>>>> a type: The last field is _not_ a flexible array member. Compilers
>>>>> might legitimately warn if they can prove that you access
>>>>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>>>>> space. (I haven't been able to think of a way for the macro to
>>>>> actually detect and hence refuse such wrong uses.)
>>>> Indeed, you are right. I am in doubt, whether to retain ported from
>>>> Linux code (ids[1])
>>>>
>>>> and mention about such abuse or change it to deal with real flexible
>>>> array member (ids[]). Any thoughts?
>>> I'm of the strong opinion that you should switch to [] (or at
>>> least [0]) notation.
>>
>> I got it. Well, will switch to ids[] if there are no objections.
> 
> I suspect the rationale to use 1 rather than 0 is to avoid the re-allocation in 
> the common case where a device has a single ID.
> 
> I would like to retain the similar behavior. The ids[1] is probably the most 
> pretty way to do it.
> 
> Another solution would to use xmalloc_bytes() for the initial allocation of 
> xmalloc_bytes().

Why not use xmalloc_flex_<whatever>() on the first allocation, too?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Hi, all.


>>>>>>> +struct iommu_fwspec {
>>>>>>> +    /* this device's IOMMU */
>>>>>>> +    struct device *iommu_dev;
>>>>>>> +    /* IOMMU driver private data for this device */
>>>>>>> +    void *iommu_priv;
>>>>>>> +    /* number of associated device IDs */
>>>>>>> +    unsigned int num_ids;
>>>>>>> +    /* IDs which this device may present to the IOMMU */
>>>>>>> +    uint32_t ids[1];
>>>>>>> +};
>>>>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>>>>> a type: The last field is _not_ a flexible array member. Compilers
>>>>>> might legitimately warn if they can prove that you access
>>>>>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>>>>>> space. (I haven't been able to think of a way for the macro to
>>>>>> actually detect and hence refuse such wrong uses.)
>>>>> Indeed, you are right. I am in doubt, whether to retain ported from
>>>>> Linux code (ids[1])
>>>>>
>>>>> and mention about such abuse or change it to deal with real flexible
>>>>> array member (ids[]). Any thoughts?
>>>> I'm of the strong opinion that you should switch to [] (or at
>>>> least [0]) notation.
>>> I got it. Well, will switch to ids[] if there are no objections.
>> I suspect the rationale to use 1 rather than 0 is to avoid the re-allocation in
>> the common case where a device has a single ID.
>>
>> I would like to retain the similar behavior. The ids[1] is probably the most
>> pretty way to do it.
>>
>> Another solution would to use xmalloc_bytes() for the initial allocation of
>> xmalloc_bytes().
> Why not use xmalloc_flex_<whatever>() on the first allocation, too?
Hmm, why not? Looks like the xmalloc_flex_struct fits here.

The first allocation would be:

fwspec = xmalloc_flex_struct(struct iommu_fwspec,  ids, 1);


The re-allocation for the devices with single ID would do effectively 
nothing (assuming that _xrealloc will recognize that size is the same):

fwspec = xrealloc_flex_struct(fwspec, ids, fwspec->num_ids + num_ids);


Julien, what do you think?


-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 19/09/2019 11:57, Oleksandr wrote:
> 
> Hi, all.

Hi,

> 
> 
>>>>>>>> +struct iommu_fwspec {
>>>>>>>> +    /* this device's IOMMU */
>>>>>>>> +    struct device *iommu_dev;
>>>>>>>> +    /* IOMMU driver private data for this device */
>>>>>>>> +    void *iommu_priv;
>>>>>>>> +    /* number of associated device IDs */
>>>>>>>> +    unsigned int num_ids;
>>>>>>>> +    /* IDs which this device may present to the IOMMU */
>>>>>>>> +    uint32_t ids[1];
>>>>>>>> +};
>>>>>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>>>>>> a type: The last field is _not_ a flexible array member. Compilers
>>>>>>> might legitimately warn if they can prove that you access
>>>>>>> p->ids[1] anywhere, despite you (presumably) having allocated enough
>>>>>>> space. (I haven't been able to think of a way for the macro to
>>>>>>> actually detect and hence refuse such wrong uses.)
>>>>>> Indeed, you are right. I am in doubt, whether to retain ported from
>>>>>> Linux code (ids[1])
>>>>>>
>>>>>> and mention about such abuse or change it to deal with real flexible
>>>>>> array member (ids[]). Any thoughts?
>>>>> I'm of the strong opinion that you should switch to [] (or at
>>>>> least [0]) notation.
>>>> I got it. Well, will switch to ids[] if there are no objections.
>>> I suspect the rationale to use 1 rather than 0 is to avoid the re-allocation in
>>> the common case where a device has a single ID.
>>>
>>> I would like to retain the similar behavior. The ids[1] is probably the most
>>> pretty way to do it.
>>>
>>> Another solution would to use xmalloc_bytes() for the initial allocation of
>>> xmalloc_bytes().
>> Why not use xmalloc_flex_<whatever>() on the first allocation, too?
> Hmm, why not? Looks like the xmalloc_flex_struct fits here.
> 
> The first allocation would be:
> 
> fwspec = xmalloc_flex_struct(struct iommu_fwspec,  ids, 1);
> 
> 
> The re-allocation for the devices with single ID would do effectively nothing 
> (assuming that _xrealloc will recognize that size is the same):
> 
> fwspec = xrealloc_flex_struct(fwspec, ids, fwspec->num_ids + num_ids);
> 
> 
> Julien, what do you think?

I am happy with that. The first allocation would need a comment on top 
explaining the reason of the "1".

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Hi, all.



>>
>>>>>>>>> +struct iommu_fwspec {
>>>>>>>>> +    /* this device's IOMMU */
>>>>>>>>> +    struct device *iommu_dev;
>>>>>>>>> +    /* IOMMU driver private data for this device */
>>>>>>>>> +    void *iommu_priv;
>>>>>>>>> +    /* number of associated device IDs */
>>>>>>>>> +    unsigned int num_ids;
>>>>>>>>> +    /* IDs which this device may present to the IOMMU */
>>>>>>>>> +    uint32_t ids[1];
>>>>>>>>> +};
>>>>>>>> Note that you abuse xrealloc_flex_struct() when using it with such
>>>>>>>> a type: The last field is _not_ a flexible array member. Compilers
>>>>>>>> might legitimately warn if they can prove that you access
>>>>>>>> p->ids[1] anywhere, despite you (presumably) having allocated 
>>>>>>>> enough
>>>>>>>> space. (I haven't been able to think of a way for the macro to
>>>>>>>> actually detect and hence refuse such wrong uses.)
>>>>>>> Indeed, you are right. I am in doubt, whether to retain ported from
>>>>>>> Linux code (ids[1])
>>>>>>>
>>>>>>> and mention about such abuse or change it to deal with real 
>>>>>>> flexible
>>>>>>> array member (ids[]). Any thoughts?
>>>>>> I'm of the strong opinion that you should switch to [] (or at
>>>>>> least [0]) notation.
>>>>> I got it. Well, will switch to ids[] if there are no objections.
>>>> I suspect the rationale to use 1 rather than 0 is to avoid the 
>>>> re-allocation in
>>>> the common case where a device has a single ID.
>>>>
>>>> I would like to retain the similar behavior. The ids[1] is probably 
>>>> the most
>>>> pretty way to do it.
>>>>
>>>> Another solution would to use xmalloc_bytes() for the initial 
>>>> allocation of
>>>> xmalloc_bytes().
>>> Why not use xmalloc_flex_<whatever>() on the first allocation, too?
>> Hmm, why not? Looks like the xmalloc_flex_struct fits here.
>>
>> The first allocation would be:
>>
>> fwspec = xmalloc_flex_struct(struct iommu_fwspec,  ids, 1);
>>
>>
>> The re-allocation for the devices with single ID would do effectively 
>> nothing (assuming that _xrealloc will recognize that size is the same):
>>
>> fwspec = xrealloc_flex_struct(fwspec, ids, fwspec->num_ids + num_ids);
>>
>>
>> Julien, what do you think?
>
> I am happy with that. The first allocation would need a comment on top 
> explaining the reason of the "1".

Sure, will add.



-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel