by Oleksandr Tyshchenko

[permalink] [raw]

Subject: Re: [PATCH V2 2/7] xen/grants: support allocating consecutive grants

On 07.05.22 21:19, Oleksandr Tyshchenko wrote:

Hello Boris, Stefano

> From: Juergen Gross <[email protected]>
>
> For support of virtio via grant mappings in rare cases larger mappings
> using consecutive grants are needed. Support those by adding a bitmap
> of free grants.
>
> As consecutive grants will be needed only in very rare cases (e.g. when
> configuring a virtio device with a multi-page ring), optimize for the
> normal case of non-consecutive allocations.
>
> Signed-off-by: Juergen Gross <[email protected]>
> ---
> Changes RFC -> V1:
> - no changes
>
> Changes V1 -> V2:
> - no changes

May I please ask for the review here?

This patch has been tested in various scenarios:

1. Guest with Xen PV drivers only (gnttab_alloc(free)_grant_reference()
usage only)

2. Guest with Virtio drivers only
(gnttab_alloc(free)_grant_reference_seq() usage only)

3. Guest with Virtio and Xen PV drivers (combined
gnttab_alloc(free)_grant_reference() and
gnttab_alloc(free)_grant_reference_seq() usage)

> ---
> drivers/xen/grant-table.c | 238 +++++++++++++++++++++++++++++++++++++++-------
> include/xen/grant_table.h | 4 +
> 2 files changed, 210 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index 8ccccac..1b458c0 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -33,6 +33,7 @@
>
> #define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
>
> +#include <linux/bitmap.h>
> #include <linux/memblock.h>
> #include <linux/sched.h>
> #include <linux/mm.h>
> @@ -72,9 +73,32 @@
>
> static grant_ref_t **gnttab_list;
> static unsigned int nr_grant_frames;
> +
> +/*
> + * Handling of free grants:
> + *
> + * Free grants are in a simple list anchored in gnttab_free_head. They are
> + * linked by grant ref, the last element contains GNTTAB_LIST_END. The number
> + * of free entries is stored in gnttab_free_count.
> + * Additionally there is a bitmap of free entries anchored in
> + * gnttab_free_bitmap. This is being used for simplifying allocation of
> + * multiple consecutive grants, which is needed e.g. for support of virtio.
> + * gnttab_last_free is used to add free entries of new frames at the end of
> + * the free list.
> + * gnttab_free_tail_ptr specifies the variable which references the start
> + * of consecutive free grants ending with gnttab_last_free. This pointer is
> + * updated in a rather defensive way, in order to avoid performance hits in
> + * hot paths.
> + * All those variables are protected by gnttab_list_lock.
> + */
> static int gnttab_free_count;
> -static grant_ref_t gnttab_free_head;
> +static unsigned int gnttab_size;
> +static grant_ref_t gnttab_free_head = GNTTAB_LIST_END;
> +static grant_ref_t gnttab_last_free = GNTTAB_LIST_END;
> +static grant_ref_t *gnttab_free_tail_ptr;
> +static unsigned long *gnttab_free_bitmap;
> static DEFINE_SPINLOCK(gnttab_list_lock);
> +
> struct grant_frames xen_auto_xlat_grant_frames;
> static unsigned int xen_gnttab_version;
> module_param_named(version, xen_gnttab_version, uint, 0);
> @@ -170,16 +194,111 @@ static int get_free_entries(unsigned count)
>
> ref = head = gnttab_free_head;
> gnttab_free_count -= count;
> - while (count-- > 1)
> - head = gnttab_entry(head);
> + while (count--) {
> + bitmap_clear(gnttab_free_bitmap, head, 1);
> + if (gnttab_free_tail_ptr == __gnttab_entry(head))
> + gnttab_free_tail_ptr = &gnttab_free_head;
> + if (count)
> + head = gnttab_entry(head);
> + }
> gnttab_free_head = gnttab_entry(head);
> gnttab_entry(head) = GNTTAB_LIST_END;
>
> + if (!gnttab_free_count) {
> + gnttab_last_free = GNTTAB_LIST_END;
> + gnttab_free_tail_ptr = NULL;
> + }
> +
> spin_unlock_irqrestore(&gnttab_list_lock, flags);
>
> return ref;
> }
>
> +static int get_seq_entry_count(void)
> +{
> + if (gnttab_last_free == GNTTAB_LIST_END || !gnttab_free_tail_ptr ||
> + *gnttab_free_tail_ptr == GNTTAB_LIST_END)
> + return 0;
> +
> + return gnttab_last_free - *gnttab_free_tail_ptr + 1;
> +}
> +
> +/* Rebuilds the free grant list and tries to find count consecutive entries. */
> +static int get_free_seq(unsigned int count)
> +{
> + int ret = -ENOSPC;
> + unsigned int from, to;
> + grant_ref_t *last;
> +
> + gnttab_free_tail_ptr = &gnttab_free_head;
> + last = &gnttab_free_head;
> +
> + for (from = find_first_bit(gnttab_free_bitmap, gnttab_size);
> + from < gnttab_size;
> + from = find_next_bit(gnttab_free_bitmap, gnttab_size, to + 1)) {
> + to = find_next_zero_bit(gnttab_free_bitmap, gnttab_size,
> + from + 1);
> + if (ret < 0 && to - from >= count) {
> + ret = from;
> + bitmap_clear(gnttab_free_bitmap, ret, count);
> + from += count;
> + gnttab_free_count -= count;
> + if (from == to)
> + continue;
> + }
> +
> + while (from < to) {
> + *last = from;
> + last = __gnttab_entry(from);
> + gnttab_last_free = from;
> + from++;
> + }
> + if (to < gnttab_size)
> + gnttab_free_tail_ptr = __gnttab_entry(to - 1);
> + }
> +
> + *last = GNTTAB_LIST_END;
> + if (gnttab_last_free != gnttab_size - 1)
> + gnttab_free_tail_ptr = NULL;
> +
> + return ret;
> +}
> +
> +static int get_free_entries_seq(unsigned int count)
> +{
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&gnttab_list_lock, flags);
> +
> + if (gnttab_free_count < count) {
> + ret = gnttab_expand(count - gnttab_free_count);
> + if (ret < 0)
> + goto out;
> + }
> +
> + if (get_seq_entry_count() < count) {
> + ret = get_free_seq(count);
> + if (ret >= 0)
> + goto out;
> + ret = gnttab_expand(count - get_seq_entry_count());
> + if (ret < 0)
> + goto out;
> + }
> +
> + ret = *gnttab_free_tail_ptr;
> + *gnttab_free_tail_ptr = gnttab_entry(ret + count - 1);
> + gnttab_free_count -= count;
> + if (!gnttab_free_count)
> + gnttab_free_tail_ptr = NULL;
> + bitmap_clear(gnttab_free_bitmap, ret, count);
> +
> + out:
> + spin_unlock_irqrestore(&gnttab_list_lock, flags);
> +
> + return ret;
> +}
> +
> static void do_free_callbacks(void)
> {
> struct gnttab_free_callback *callback, *next;
> @@ -206,17 +325,48 @@ static inline void check_free_callbacks(void)
> do_free_callbacks();
> }
>
> -static void put_free_entry(grant_ref_t ref)
> +static void put_free_entry_locked(grant_ref_t ref)
> {
> - unsigned long flags;
> - spin_lock_irqsave(&gnttab_list_lock, flags);
> gnttab_entry(ref) = gnttab_free_head;
> gnttab_free_head = ref;
> + if (!gnttab_free_count)
> + gnttab_last_free = ref;
> + if (gnttab_free_tail_ptr == &gnttab_free_head)
> + gnttab_free_tail_ptr = __gnttab_entry(ref);
> gnttab_free_count++;
> + bitmap_set(gnttab_free_bitmap, ref, 1);
> +}
> +
> +static void put_free_entry(grant_ref_t ref)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gnttab_list_lock, flags);
> + put_free_entry_locked(ref);
> check_free_callbacks();
> spin_unlock_irqrestore(&gnttab_list_lock, flags);
> }
>
> +static void gnttab_set_free(unsigned int start, unsigned int n)
> +{
> + unsigned int i;
> +
> + for (i = start; i < start + n - 1; i++)
> + gnttab_entry(i) = i + 1;
> +
> + gnttab_entry(i) = GNTTAB_LIST_END;
> + if (!gnttab_free_count) {
> + gnttab_free_head = start;
> + gnttab_free_tail_ptr = &gnttab_free_head;
> + } else {
> + gnttab_entry(gnttab_last_free) = start;
> + }
> + gnttab_free_count += n;
> + gnttab_last_free = i;
> +
> + bitmap_set(gnttab_free_bitmap, start, n);
> +}
> +
> /*
> * Following applies to gnttab_update_entry_v1 and gnttab_update_entry_v2.
> * Introducing a valid entry into the grant table:
> @@ -448,23 +598,31 @@ void gnttab_free_grant_references(grant_ref_t head)
> {
> grant_ref_t ref;
> unsigned long flags;
> - int count = 1;
> - if (head == GNTTAB_LIST_END)
> - return;
> +
> spin_lock_irqsave(&gnttab_list_lock, flags);
> - ref = head;
> - while (gnttab_entry(ref) != GNTTAB_LIST_END) {
> - ref = gnttab_entry(ref);
> - count++;
> + while (head != GNTTAB_LIST_END) {
> + ref = gnttab_entry(head);
> + put_free_entry_locked(head);
> + head = ref;
> }
> - gnttab_entry(ref) = gnttab_free_head;
> - gnttab_free_head = head;
> - gnttab_free_count += count;
> check_free_callbacks();
> spin_unlock_irqrestore(&gnttab_list_lock, flags);
> }
> EXPORT_SYMBOL_GPL(gnttab_free_grant_references);
>
> +void gnttab_free_grant_reference_seq(grant_ref_t head, unsigned int count)
> +{
> + unsigned long flags;
> + unsigned int i;
> +
> + spin_lock_irqsave(&gnttab_list_lock, flags);
> + for (i = count; i > 0; i--)
> + put_free_entry_locked(head + i - 1);
> + check_free_callbacks();
> + spin_unlock_irqrestore(&gnttab_list_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(gnttab_free_grant_reference_seq);
> +
> int gnttab_alloc_grant_references(u16 count, grant_ref_t *head)
> {
> int h = get_free_entries(count);
> @@ -478,6 +636,24 @@ int gnttab_alloc_grant_references(u16 count, grant_ref_t *head)
> }
> EXPORT_SYMBOL_GPL(gnttab_alloc_grant_references);
>
> +int gnttab_alloc_grant_reference_seq(unsigned int count, grant_ref_t *first)
> +{
> + int h;
> +
> + if (count == 1)
> + h = get_free_entries(1);
> + else
> + h = get_free_entries_seq(count);
> +
> + if (h < 0)
> + return -ENOSPC;
> +
> + *first = h;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(gnttab_alloc_grant_reference_seq);
> +
> int gnttab_empty_grant_references(const grant_ref_t *private_head)
> {
> return (*private_head == GNTTAB_LIST_END);
> @@ -570,16 +746,13 @@ static int grow_gnttab_list(unsigned int more_frames)
> goto grow_nomem;
> }
>
> + gnttab_set_free(gnttab_size, extra_entries);
>
> - for (i = grefs_per_frame * nr_grant_frames;
> - i < grefs_per_frame * new_nr_grant_frames - 1; i++)
> - gnttab_entry(i) = i + 1;
> -
> - gnttab_entry(i) = gnttab_free_head;
> - gnttab_free_head = grefs_per_frame * nr_grant_frames;
> - gnttab_free_count += extra_entries;
> + if (!gnttab_free_tail_ptr)
> + gnttab_free_tail_ptr = __gnttab_entry(gnttab_size);
>
> nr_grant_frames = new_nr_grant_frames;
> + gnttab_size += extra_entries;
>
> check_free_callbacks();
>
> @@ -1424,7 +1597,6 @@ int gnttab_init(void)
> int i;
> unsigned long max_nr_grant_frames;
> unsigned int max_nr_glist_frames, nr_glist_frames;
> - unsigned int nr_init_grefs;
> int ret;
>
> gnttab_request_version();
> @@ -1452,6 +1624,13 @@ int gnttab_init(void)
> }
> }
>
> + i = gnttab_interface->grefs_per_grant_frame * max_nr_grant_frames;
> + gnttab_free_bitmap = bitmap_zalloc(i, GFP_KERNEL);
> + if (!gnttab_free_bitmap) {
> + ret = -ENOMEM;
> + goto ini_nomem;
> + }
> +
> ret = arch_gnttab_init(max_nr_grant_frames,
> nr_status_frames(max_nr_grant_frames));
> if (ret < 0)
> @@ -1462,15 +1641,9 @@ int gnttab_init(void)
> goto ini_nomem;
> }
>
> - nr_init_grefs = nr_grant_frames *
> - gnttab_interface->grefs_per_grant_frame;
> -
> - for (i = NR_RESERVED_ENTRIES; i < nr_init_grefs - 1; i++)
> - gnttab_entry(i) = i + 1;
> + gnttab_size = nr_grant_frames * gnttab_interface->grefs_per_grant_frame;
>
> - gnttab_entry(nr_init_grefs - 1) = GNTTAB_LIST_END;
> - gnttab_free_count = nr_init_grefs - NR_RESERVED_ENTRIES;
> - gnttab_free_head = NR_RESERVED_ENTRIES;
> + gnttab_set_free(NR_RESERVED_ENTRIES, gnttab_size - NR_RESERVED_ENTRIES);
>
> printk("Grant table initialized\n");
> return 0;
> @@ -1479,6 +1652,7 @@ int gnttab_init(void)
> for (i--; i >= 0; i--)
> free_page((unsigned long)gnttab_list[i]);
> kfree(gnttab_list);
> + bitmap_free(gnttab_free_bitmap);
> return ret;
> }
> EXPORT_SYMBOL_GPL(gnttab_init);
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index dfd5bf3..d815e1d 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -129,10 +129,14 @@ int gnttab_try_end_foreign_access(grant_ref_t ref);
> */
> int gnttab_alloc_grant_references(u16 count, grant_ref_t *pprivate_head);
>
> +int gnttab_alloc_grant_reference_seq(unsigned int count, grant_ref_t *first);
> +
> void gnttab_free_grant_reference(grant_ref_t ref);
>
> void gnttab_free_grant_references(grant_ref_t head);
>
> +void gnttab_free_grant_reference_seq(grant_ref_t head, unsigned int count);
> +
> int gnttab_empty_grant_references(const grant_ref_t *pprivate_head);
>
> int gnttab_claim_grant_reference(grant_ref_t *pprivate_head);

--
Regards,

Oleksandr Tyshchenko

2022-05-13 22:50:06

On 24.05.22 21:34, Saravana Kannan wrote:

Hello all

> "
>
> On Tue, May 24, 2022 at 9:01 AM Rob Herring <[email protected]> wrote:
>> +Saravana
>>
>> On Mon, May 23, 2022 at 06:58:13PM -0700, Stefano Stabellini wrote:
>>> On Mon, 23 May 2022, Oleksandr wrote:
>>>>>> On Thu, 19 May 2022, Oleksandr wrote:
>>>>>>>> On Wed, May 18, 2022 at 5:06 PM Oleksandr <[email protected]> wrote:
>>>>>>>>> On 18.05.22 17:32, Arnd Bergmann wrote:
>>>>>>>>>> On Sat, May 7, 2022 at 7:19 PM Oleksandr Tyshchenko
>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>> This would mean having a device
>>>>>>>>>> node for the grant-table mechanism that can be referred to using
>>>>>>>>>> the
>>>>>>>>>> 'iommus'
>>>>>>>>>> phandle property, with the domid as an additional argument.
>>>>>>>>> I assume, you are speaking about something like the following?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> xen_dummy_iommu {
>>>>>>>>> compatible = "xen,dummy-iommu";
>>>>>>>>> #iommu-cells = <1>;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> virtio@3000 {
>>>>>>>>> compatible = "virtio,mmio";
>>>>>>>>> reg = <0x3000 0x100>;
>>>>>>>>> interrupts = <41>;
>>>>>>>>>
>>>>>>>>> /* The device is located in Xen domain with ID 1 */
>>>>>>>>> iommus = <&xen_dummy_iommu 1>;
>>>>>>>>> };
>>>>>>>> Right, that's that's the idea,
>>>>>>> thank you for the confirmation
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> except I would not call it a 'dummy'.
>>>>>>>> From the perspective of the DT, this behaves just like an IOMMU,
>>>>>>>> even if the exact mechanism is different from most hardware IOMMU
>>>>>>>> implementations.
>>>>>>> well, agree
>>>>>>>
>>>>>>>
>>>>>>>>>> It does not quite fit the model that Linux currently uses for
>>>>>>>>>> iommus,
>>>>>>>>>> as that has an allocator for dma_addr_t space
>>>>>>>>> yes (# 3/7 adds grant-table based allocator)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> , but it would think it's
>>>>>>>>>> conceptually close enough that it makes sense for the binding.
>>>>>>>>> Interesting idea. I am wondering, do we need an extra actions for
>>>>>>>>> this
>>>>>>>>> to work in Linux guest (dummy IOMMU driver, etc)?
>>>>>>>> It depends on how closely the guest implementation can be made to
>>>>>>>> resemble a normal iommu. If you do allocate dma_addr_t addresses,
>>>>>>>> it may actually be close enough that you can just turn the grant-table
>>>>>>>> code into a normal iommu driver and change nothing else.
>>>>>>> Unfortunately, I failed to find a way how use grant references at the
>>>>>>> iommu_ops level (I mean to fully pretend that we are an IOMMU driver). I
>>>>>>> am
>>>>>>> not too familiar with that, so what is written below might be wrong or
>>>>>>> at
>>>>>>> least not precise.
>>>>>>>
>>>>>>> The normal IOMMU driver in Linux doesn’t allocate DMA addresses by
>>>>>>> itself, it
>>>>>>> just maps (IOVA-PA) what was requested to be mapped by the upper layer.
>>>>>>> The
>>>>>>> DMA address allocation is done by the upper layer (DMA-IOMMU which is
>>>>>>> the glue
>>>>>>> layer between DMA API and IOMMU API allocates IOVA for PA?). But, all
>>>>>>> what we
>>>>>>> need here is just to allocate our specific grant-table based DMA
>>>>>>> addresses
>>>>>>> (DMA address = grant reference + offset in the page), so let’s say we
>>>>>>> need an
>>>>>>> entity to take a physical address as parameter and return a DMA address
>>>>>>> (what
>>>>>>> actually commit #3/7 is doing), and that’s all. So working at the
>>>>>>> dma_ops
>>>>>>> layer we get exactly what we need, with the minimal changes to guest
>>>>>>> infrastructure. In our case the Xen itself acts as an IOMMU.
>>>>>>>
>>>>>>> Assuming that we want to reuse the IOMMU infrastructure somehow for our
>>>>>>> needs.
>>>>>>> I think, in that case we will likely need to introduce a new specific
>>>>>>> IOVA
>>>>>>> allocator (alongside with a generic one) to be hooked up by the
>>>>>>> DMA-IOMMU
>>>>>>> layer if we run on top of Xen. But, even having the specific IOVA
>>>>>>> allocator to
>>>>>>> return what we indeed need (DMA address = grant reference + offset in
>>>>>>> the
>>>>>>> page) we will still need the specific minimal required IOMMU driver to
>>>>>>> be
>>>>>>> present in the system anyway in order to track the mappings(?) and do
>>>>>>> nothing
>>>>>>> with them, returning a success (this specific IOMMU driver should have
>>>>>>> all
>>>>>>> mandatory callbacks implemented).
>>>>>>>
>>>>>>> I completely agree, it would be really nice to reuse generic IOMMU
>>>>>>> bindings
>>>>>>> rather than introducing Xen specific property if what we are trying to
>>>>>>> implement in current patch series fits in the usage of "iommus" in Linux
>>>>>>> more-less. But, if we will have to add more complexity/more components
>>>>>>> to the
>>>>>>> code for the sake of reusing device tree binding, this raises a question
>>>>>>> whether that’s worthwhile.
>>>>>>>
>>>>>>> Or I really missed something?
>>>>>> I think Arnd was primarily suggesting to reuse the IOMMU Device Tree
>>>>>> bindings, not necessarily the IOMMU drivers framework in Linux (although
>>>>>> that would be an added bonus.)
>>>>>>
>>>>>> I know from previous discussions with you that making the grant table
>>>>>> fit in the existing IOMMU drivers model is difficult, but just reusing
>>>>>> the Device Tree bindings seems feasible?
>>>>> I started experimenting with that. As wrote in a separate email, I got a
>>>>> deferred probe timeout,
>>>>>
>>>>> after inserting required nodes into guest device tree, which seems to be a
>>>>> consequence of the unavailability of IOMMU, I will continue to investigate
>>>>> this question.
>>>>
>>>> I have experimented with that. Yes, just reusing the Device Tree bindings is
>>>> technically feasible (and we are able to do this by only touching
>>>> grant-dma-ops.c), although deferred probe timeout still stands (as there is no
>>>> IOMMU driver being present actually).
>>>>
>>>> [ 0.583771] virtio-mmio 2000000.virtio: deferred probe timeout, ignoring
>>>> dependency
>>>> [ 0.615556] virtio_blk virtio0: [vda] 4096000 512-byte logical blocks (2.10
>>>> GB/1.95 GiB)
>>>>
>>>>
>>>> Below the working diff (on top of current series):
>>>>
>>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>>> index da9c7ff..6586152 100644
>>>> --- a/drivers/xen/grant-dma-ops.c
>>>> +++ b/drivers/xen/grant-dma-ops.c
>>>> @@ -272,17 +272,24 @@ static const struct dma_map_ops xen_grant_dma_ops = {
>>>>
>>>> bool xen_is_grant_dma_device(struct device *dev)
>>>> {
>>>> + struct device_node *iommu_np;
>>>> + bool has_iommu;
>>>> +
>>>> /* XXX Handle only DT devices for now */
>>>> if (!dev->of_node)
>>>> return false;
>>>>
>>>> - return of_property_read_bool(dev->of_node, "xen,backend-domid");
>>>> + iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
>>>> + has_iommu = iommu_np && of_device_is_compatible(iommu_np,
>>>> "xen,grant-dma");
>>>> + of_node_put(iommu_np);
>>>> +
>>>> + return has_iommu;
>>>> }
>>>>
>>>> void xen_grant_setup_dma_ops(struct device *dev)
>>>> {
>>>> struct xen_grant_dma_data *data;
>>>> - uint32_t domid;
>>>> + struct of_phandle_args iommu_spec;
>>>>
>>>> data = find_xen_grant_dma_data(dev);
>>>> if (data) {
>>>> @@ -294,16 +301,30 @@ void xen_grant_setup_dma_ops(struct device *dev)
>>>> if (!dev->of_node)
>>>> goto err;
>>>>
>>>> - if (of_property_read_u32(dev->of_node, "xen,backend-domid", &domid)) {
>>>> - dev_err(dev, "xen,backend-domid property is not present\n");
>>>> + if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
>>>> + 0, &iommu_spec)) {
>>>> + dev_err(dev, "Cannot parse iommus property\n");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
>>>> + iommu_spec.args_count != 1) {
>>>> + dev_err(dev, "Incompatible IOMMU node\n");
>>>> + of_node_put(iommu_spec.np);
>>>> goto err;
>>>> }
>>>>
>>>> + of_node_put(iommu_spec.np);
>>>> +
>>>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>>> if (!data)
>>>> goto err;
>>>>
>>>> - data->backend_domid = domid;
>>>> + /*
>>>> + * The endpoint ID here means the ID of the domain where the
>>>> corresponding
>>>> + * backend is running
>>>> + */
>>>> + data->backend_domid = iommu_spec.args[0];
>>>>
>>>> if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
>>>> GFP_KERNEL))) {
>>>> (END)
>>>>
>>>>
>>>>
>>>> Below, the nodes generated by Xen toolstack:
>>>>
>>>> xen_grant_dma {
>> Nit: iommu {
>>
>>>> compatible = "xen,grant-dma";
>>>> #iommu-cells = <0x01>;
>>>> phandle = <0xfde9>;
>>>> };
>>>>
>>>> virtio@2000000 {
>>>> compatible = "virtio,mmio";
>>>> reg = <0x00 0x2000000 0x00 0x200>;
>>>> interrupts = <0x00 0x01 0xf01>;
>>>> interrupt-parent = <0xfde8>;
>>>> dma-coherent;
>>>> iommus = <0xfde9 0x01>;
>>>> };
>>> Not bad! I like it.
>>>
>>>
>>>> I am wondering, would be the proper solution to eliminate deferred probe
>>>> timeout issue in our particular case (without introducing an extra IOMMU
>>>> driver)?
>>> In reality I don't think there is a way to do that. I would create an
>>> empty skelethon IOMMU driver for xen,grant-dma.
>> Does it have to be an empty driver? Originally, IOMMU 'drivers' were not
>> drivers, but they've been getting converted. Can that be done here?
>>
>> Short of that, I think we could have some sort of skip probe list for
>> deferred probe. Not sure if that would be easiest as IOMMU specific or
>> global.
> Hi Oleksandr,
>
> If you do fw_devlink.strict=1, you'll notice that the consumers of
> this "iommu" won't probe at all or will delay the boot by some number
> of seconds. The eventual goal is to go towards fw_devlink.strict=1
> being the default.

ok, I got it.

Let's me please explain our particular case in details, sorry I may
repeat some information which I have already mentioned elsewhere, but it
maybe better to keep the whole context here.

We have Xen grant DMA-mapping layer added by previous commit [1]. For it
to operate properly we need a way to communicate some per-device
information using device-tree,
and this information is Xen specific. This is what the current commit is
doing by introducing new binding to describe that. The next commit [2]
will use that new binding to retrieve required information. There was a
suggestion to consider reusing generic device-tree IOMMU bindings to
communicate this specific information instead of introducing a custom
property.

Although it requires more effort for the Xen toolstack (instead of
adding a code to insert a single "xen,backend-domid" property, we need
to generate fake IOMMU node, reserve phandle for it, etc), from the
device tree PoV it looks indeed good (we reuse endpoint ID to pass the
ID of the domain where the corresponding backend is running), and
resulting code to retrieve this information in our DMA-mapping layer
also looks simple enough [3].

Using generic device-tree IOMMU bindings:

         iommu {
                 compatible = "xen,grant-dma";
                 #iommu-cells = <0x01>;
                 phandle = <0xfde9>;
         };
         virtio@2000000 {
                 compatible = "virtio,mmio";
                 reg = <0x00 0x2000000 0x00 0x200>;
                 interrupts = <0x00 0x01 0xf01>;
                 interrupt-parent = <0xfde8>;
                 dma-coherent;
                 iommus = <0xfde9 0x01>;
         };

Using Xen specific property:

         virtio@2000000 {
                 compatible = "virtio,mmio";
                 reg = <0x00 0x2000000 0x00 0x200>;
                 interrupts = <0x00 0x01 0xf01>;
                 interrupt-parent = <0xfde8>;
                 dma-coherent;
                 xen,backend-domid = <0x01>;
         };

The main problem is that idea doesn't quite fit into how Linux currently
behaves for the "iommus" property. Of course, just reusing IOMMU
bindings (without having a corresponding driver) leads to the deferred
probe timeout issue afterwards, because the IOMMU device never becomes
available. From my understanding, our DMA-mapping layer we are consider
to reuse IOMMU bindings for, *cannot* be converted into the proper IOMMU
driver.

Sure, we will need to find a way how to deal with it, if we really want
to reuse the IOMMU bindings. And yes, one idea was to just implement
stub IOMMU driver for that purpose, I have rechecked, it works fine with
that stub driver [4].

>
> From a fw_devlik perspective, please implement a driver. Ideally a
> real one, but at least an empty one. The empty one doesn't need to be
> an IOMMU driver, but at least just do a return 0 in the probe
> function.

If I got things right, I am afraid, for the "of_iommu" case the empty
driver is not enough. The driver should at least register iommu_ops, but
the "of_xlate" callback should be *not* implemented.
In that case, we will get NO_IOMMU (>0 : there is no IOMMU, or one was
unavailable for non-fatal reasons) which is also a success condition, so
-EPROBE_DEFER won't be returned.

https://elixir.bootlin.com/linux/v5.18/source/drivers/iommu/of_iommu.c#L32

Otherwise, of_iommu_xlate() will call driver_deferred_probe_check_state().

https://elixir.bootlin.com/linux/v5.18/source/drivers/iommu/of_iommu.c#L43

> Also, if it's not a device, why even have a "compatible"
> property (removing it won't necessarily remove the deferred probe
> timeout issue you see)? Will any code be using "xen,grant-dma" to look
> up the node?

Yes

> If so, that driver could be the one that probes this
> device. At least from a fw_devlink perspective, it just needs to have
> a driver that binds to this device.

Agree

>
> Also, if we aren't going to implement a driver and have the supplier
> ("xen,grant-dma") behave like a device (as in, have a driver that
> probes), I'd rather that the iommu binding not be used at all as this
> would be an exception to how every other iommu device behaves.

Agree

Saravana, thank you for the explanation.

To summarize, as I understand, we have three options (the first two are
clear enough, the third is unclear yet):

1. Do not try to reuse IOMMU bindings for current xen-virtio enabling
work, use "xen,backend-domid" property.
2. Reuse IOMMU bindings, for that purpose introduce stub IOMMU driver.
It is a standalone entity in my example, but it can be a part of
grant-dma-ops.c which
actually uses "xen,grant-dma" compatible to look up a node.
3. Try to find other options how to reuse IOMMU bindings but *without*
introducing stub IOMMU driver, such as skip list for deferred probe, etc.

What do the maintainers think regarding the option to go forward?

[1]
https://lore.kernel.org/xen-devel/[email protected]/

[2]
https://lore.kernel.org/xen-devel/[email protected]/

[3]
https://lore.kernel.org/xen-devel/[email protected]/

[4]
https://lore.kernel.org/xen-devel/[email protected]/

>
> -Saravana

--
Regards,

Oleksandr Tyshchenko