From: Julien Grall <julien.grall@citrix.com>
To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com,
stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com,
tim@xen.org, xen-devel@lists.xen.org
Cc: Prasun.Kapoor@caviumnetworks.com,
Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>,
manish.jaggi@caviumnetworks.com
Subject: Re: [PATCH v6 09/31] xen/arm: ITS: Add APIs to add and assign device
Date: Thu, 3 Sep 2015 18:34:53 +0100 [thread overview]
Message-ID: <55E884BD.5080503@citrix.com> (raw)
In-Reply-To: <1441019208-2764-10-git-send-email-vijay.kilari@gmail.com>
Hi Vijay,
On 31/08/15 12:06, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Add APIs to add devices to RB-tree, assign and remove
> devices to domain.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v6: - Moved this patch #19 to patch #8
> - Used col_map to store collection id
> - Use helper functions to update msi_desc members
> v5: - Removed its_detach_device API
> - Pass nr_ites as parameter to its_add_device
> v4: - Introduced helper to populate its_device struct
> - Fixed freeing of its_device memory
> - its_device struct holds domain id
> ---
> xen/arch/arm/gic-v3-its.c | 212 +++++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/gic-its.h | 6 ++
> 2 files changed, 218 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index e70c21a..f14c0f4 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -145,6 +145,19 @@ static struct its_collection *dev_event_to_col(struct its_device *dev,
> return its->collections + dev->event_map.col_map[event];
> }
>
> +static struct its_node *its_get_phys_node(struct dt_device_node *dt)
> +{
> + struct its_node *its;
> +
> + list_for_each_entry(its, &its_nodes, entry)
> + {
> + if ( its->dt_node == dt )
> + return its;
> + }
> +
> + return NULL;
> +}
> +
> /* RB-tree helpers for its_device */
> static struct its_device *its_find_device(u32 devid)
> {
> @@ -522,6 +535,205 @@ static void its_lpi_free(struct its_device *dev)
> xfree(dev->event_map.lpi_map);
> }
>
> +static void its_discard_lpis(struct its_device *dev, u32 ids)
> +{
> + int i;
> +
I would have expected a function more complex than that. If you discard
the LPIs, you also need to free the MSI desc and potentially reset the
IRQ desc.
Otherwise you will left the irq_desc in an unknown state for the next one.
> + for ( i = 0; i < ids; i++)
> + its_send_discard(dev, i);
> +}
> +
> +static inline u32 its_get_plpi(struct its_device *dev, u32 event)
> +{
> + return dev->event_map.lpi_base + event;
> +}
> +
> +static int its_alloc_device_irq(struct its_device *dev, u32 *hwirq)
> +{
> + int idx;
> +
> + idx = find_first_zero_bit(dev->event_map.lpi_map, dev->event_map.nr_lpis);
> + if ( idx == dev->event_map.nr_lpis )
> + return -ENOSPC;
> +
> + *hwirq = its_get_plpi(dev, idx);
> + set_bit(idx, dev->event_map.lpi_map);
> +
> + return 0;
> +}
> +
> +static void its_free_device(struct its_device *dev)
> +{
> + xfree(dev->itt_addr);
> + xfree(dev->event_map.lpi_map);
This should be replaced by its_lpi_free. Otherwise you will never
release the reserved LPI ranges in lpi_bitmap.
> + xfree(dev->event_map.col_map);
> + xfree(dev);
> +}
> +
> +static struct its_device *its_alloc_device(u32 devid, u32 nr_ites,
> + struct dt_device_node *dt_its)
> +{
> + struct its_device *dev;
> + paddr_t *itt;
Why paddr_t? You only allocate it and pass directly to the hardware.
> + unsigned long *lpi_map;
> + int lpi_base, sz;
> + u16 *col_map = NULL;
> +
> + dev = xzalloc(struct its_device);
> + if ( dev == NULL )
> + return NULL;
> +
> + dev->its = its_get_phys_node(dt_its);
> + if (dev->its == NULL)
> + {
> + dprintk(XENLOG_G_ERR,
Why XENLOG_G_ERR?
Also, dprintk will be turned into a no-op on non-debug build. Is it what
you expect?
> + "ITS: Failed to find ITS node for devid 0x%"PRIx32"\n",
> + devid);
> + goto err;
> + }
> +
> + sz = nr_ites * dev->its->ite_size;
> + sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> + itt = xzalloc_bytes(sz);
> + if ( !itt )
> + goto err;
> +
> + lpi_map = its_lpi_alloc_chunks(nr_ites, &lpi_base);
> + if ( !lpi_map )
> + goto lpi_err;
> +
> + col_map = xzalloc_bytes(sizeof(*col_map) * nr_ites);
> + if ( !col_map )
> + goto col_err;
> + dev->itt_addr = itt;
> + dev->event_map.lpi_map = lpi_map;
> + dev->event_map.lpi_base = lpi_base;
> + dev->event_map.col_map = col_map;
> + dev->event_map.nr_lpis = nr_ites;
> + dev->device_id = devid;
> +
> + return dev;
> +
> +col_err:
> + xfree(lpi_map);
This should be replaced by its_lpi_free. Otherwise you will never
release the reserved LPI ranges in lpi_bitmap.
> +lpi_err:
> + xfree(itt);
> +err:
> + xfree(dev);
By reworking a bit this function, you could directly its_free_device.
For instance if you assign lpi_map directly after allocating,...
> + return NULL;
> +}
> +
> +/* Device assignment */
> +int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its)
> +{
> + struct its_device *dev;
> + u32 i, plpi = 0;
> + struct its_collection *col;
> + struct irq_desc *desc;
> + struct msi_desc *msi = NULL;
> + int res = 0;
> +
> + spin_lock(&rb_its_dev_lock);
> + dev = its_find_device(devid);
> + if ( dev )
> + {
> + dprintk(XENLOG_G_ERR, "ITS: Device already exists 0x%"PRIx32"\n",
> + dev->device_id);
> + res = -EEXIST;
> + goto err;
> + }
> +
> + dev = its_alloc_device(devid, nr_ites, dt_its);
> + if ( !dev )
> + {
> + res = -ENOMEM;
> + goto err;
> + }
> +
> + if ( its_insert_device(dev) )
> + {
> + its_free_device(dev);
> + dprintk(XENLOG_G_ERR, "ITS: failed to insert device 0x%"PRIx32"\n",
> + devid);
> + res = -EINVAL;
> + goto err;
> + }
> +
> + DPRINTK("ITS:Add Device 0x%"PRIx32" lpis %"PRIu32" base 0x%"PRIx32"\n",
> + dev->device_id, dev->event_map.nr_lpis, dev->event_map.lpi_base);
> +
> + /* Map device to its ITT */
> + its_send_mapd(dev, 1);
> +
> + for ( i = 0; i < dev->event_map.nr_lpis; i++ )
> + {
> + msi = xzalloc(struct msi_desc);
> + /* Reserve pLPI */
This comment is unclear given that the next section of code is mostly
free code. You may want to move the free in another place.
> + if ( its_alloc_device_irq(dev, &plpi) || !msi )
> + {
> + its_discard_lpis(dev, i);
> + its_lpi_free(dev);
IHMO, this should go in its_free_device.
> + its_send_mapd(dev, 0);
> + its_free_device(dev);
You free the device but never removed from the rb_its_dev. So the
RB-tree will contain an invalid pointer.
It would be nice if you can test the error path manually. For example
you can add a bit of code to say "I want to fail on the 3rd device
allocation" and see what happens...
This is a good way to exercise path that you may enter in common case at
runtime.
> + dprintk(XENLOG_G_ERR, "ITS: Cannot add device 0x%"PRIx32"\n",
> + devid);
> + res = -ENOSPC;
> + goto err;
> + }
> +
> + /* For each pLPI send MAPVI command */
> + col = &dev->its->collections[(i % nr_cpu_ids)];
Please add a comment to explain how you choose the collection.
> + desc = irq_to_desc(plpi);
> +
> + spin_lock(&desc->lock);
> + dev->event_map.col_map[i] = col->col_id;
> + irq_set_msi_desc(desc, msi);
> + irqdesc_set_lpi_event(desc, i);
> + irqdesc_set_its_device(desc, dev);
> + spin_unlock(&desc->lock);
> +
> + its_send_mapvi(dev, plpi, i);
> + }
> +
> +err:
> + spin_unlock(&rb_its_dev_lock);
This can be unlocked after inserting the device (i.e its_insert_device).
> +
> + return res;
> +}
> +
> +int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid)
> +{
> + struct its_device *pdev;
> + u32 plpi, i;
> +
> + DPRINTK("ITS: Assign request for dev 0x%"PRIx32" to domain %"PRIu16"\n",
> + vdevid, d->domain_id);
> +
> + spin_lock(&rb_its_dev_lock);
> + pdev = its_find_device(pdevid);
> + if ( !pdev )
> + {
> + spin_unlock(&rb_its_dev_lock);
> + return -ENODEV;
> + }
> + spin_unlock(&rb_its_dev_lock);
You never check if the device is used by someone else. You don't even
set to invalid domain ID when allocating the device.
While this could be acceptable until guest ITS is not supported. This
should be fixed before any patch series to add guest.
I.e I want this code fixed or a big fat comment explaining what is
missing here.
> +
> + pdev->domain_id = d->domain_id;
> + pdev->virt_device_id = vdevid;
> +
> + DPRINTK("ITS: Assign pdevid 0x%"PRIx32" lpis %"PRIu32" for dom %"PRIu16"\n",
> + pdevid, pdev->event_map.nr_lpis, d->domain_id);
> +
> + for ( i = 0; i < pdev->event_map.nr_lpis; i++ )
> + {
> + plpi = its_get_plpi(pdev, i);
> + /* TODO: Route lpi */
> + }
> +
> + return 0;
> +}
> +
> /*
> * We allocate 64kB for PROPBASE. That gives us at most 64K LPIs to
> * deal with (one configuration byte per interrupt). PENDBASE has to
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index 25c2176..3599c76 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -267,6 +267,10 @@ struct its_device {
> struct event_lpi_map event_map;
> /* Physical Device id */
> u32 device_id;
> + /* Virtual Device id */
> + u32 virt_device_id;
> + /* Domain id */
> + int domain_id;
In general we are storing a pointer to struct domain rather than the
domID because you get directly access to the domain information.
Although, I don't see why you added it as I don't find any usage of it
within this series but here to set the value.
> /* RB-tree entry */
> struct rb_node node;
> };
> @@ -277,6 +281,8 @@ struct its_device *irqdesc_get_its_device(struct irq_desc *desc);
> void irqdesc_set_its_device(struct irq_desc *desc, struct its_device *dev);
> int its_init(struct rdist_prop *rdists);
> int its_cpu_init(void);
> +int its_add_device(u32 devid, u32 nr_ites, struct dt_device_node *dt_its);
> +int its_assign_device(struct domain *d, u32 vdevid, u32 pdevid);
>
> #endif /* __ASM_ARM_GIC_ITS_H__ */
> /*
>
Regards,
--
Julien Grall
next prev parent reply other threads:[~2015-09-03 17:34 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-31 11:06 [PATCH v6 00/31] Add ITS support vijay.kilari
2015-08-31 11:06 ` [PATCH v6 01/31] xen/dt: Handle correctly node with interrupt-map in dt_for_each_irq_map vijay.kilari
2015-08-31 14:20 ` Julien Grall
2015-09-02 15:28 ` Ian Campbell
2015-09-02 15:30 ` Wei Liu
2015-09-02 15:45 ` Julien Grall
2015-09-02 15:52 ` Ian Campbell
2015-09-04 14:41 ` Ian Campbell
2015-08-31 11:06 ` [PATCH v6 02/31] xen/arm: Add bitmap_find_next_zero_area helper function vijay.kilari
2015-08-31 11:06 ` [PATCH v6 03/31] xen: Add log2 functionality vijay.kilari
2015-08-31 11:21 ` Jan Beulich
2015-08-31 11:06 ` [PATCH v6 04/31] xen/arm: Set nr_cpu_ids to available number of cpus vijay.kilari
2015-08-31 14:25 ` Julien Grall
2015-09-09 12:48 ` Ian Campbell
2015-08-31 11:06 ` [PATCH v6 05/31] xen/arm: Rename NR_IRQs and vgic_num_irqs helper function vijay.kilari
2015-08-31 14:40 ` Julien Grall
2015-09-09 13:08 ` Ian Campbell
2015-09-09 13:23 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 06/31] xen/arm: ITS: Port ITS driver to Xen vijay.kilari
2015-08-31 15:41 ` Julien Grall
2015-09-03 17:02 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 07/31] xen/arm: ITS: Add helper functions to manage its_devices vijay.kilari
2015-08-31 11:06 ` [PATCH v6 08/31] xen/arm: ITS: Introduce msi_desc for LPIs vijay.kilari
2015-08-31 16:20 ` Julien Grall
2015-09-09 13:16 ` Ian Campbell
2015-09-09 13:28 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 09/31] xen/arm: ITS: Add APIs to add and assign device vijay.kilari
2015-09-03 17:34 ` Julien Grall [this message]
2015-09-09 13:28 ` Ian Campbell
2015-09-09 13:44 ` Julien Grall
2015-09-09 15:07 ` Ian Campbell
2015-09-09 16:19 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 10/31] xen/arm: ITS: Introduce gic_is_lpi helper function vijay.kilari
2015-08-31 16:49 ` Julien Grall
2015-09-01 9:02 ` Vijay Kilari
2015-09-01 11:40 ` Julien Grall
2015-09-01 11:56 ` Vijay Kilari
2015-09-01 13:02 ` Julien Grall
2015-09-03 6:32 ` Vijay Kilari
2015-09-03 9:48 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 11/31] xen/arm: ITS: Enable compilation of physical ITS driver vijay.kilari
2015-08-31 11:06 ` [PATCH v6 12/31] xen/arm: Move vgic locking inside get_irq_priority callback vijay.kilari
2015-08-31 16:34 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 13/31] xen/arm: ITS: implement hw_irq_controller for LPIs vijay.kilari
2015-08-31 17:53 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 14/31] xen/arm: ITS: Initialize physical ITS and export lpi support vijay.kilari
2015-08-31 18:35 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 15/31] xen/arm: ITS: Add virtual ITS driver vijay.kilari
2015-09-02 17:20 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 16/31] xen/arm: ITS: Add virtual ITS commands support vijay.kilari
2015-09-03 15:07 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 17/31] xen/arm: ITS: Store LPIs allocated and IRQ ID bits per domain vijay.kilari
2015-09-03 16:25 ` Julien Grall
2015-09-07 6:59 ` Vijay Kilari
2015-09-07 10:56 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 18/31] xen/arm: ITS: Enable virtual ITS driver vijay.kilari
2015-08-31 11:06 ` [PATCH v6 19/31] xen/arm: ITS: Export ITS info to Virtual ITS vijay.kilari
2015-09-03 16:48 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 20/31] xen/arm: ITS: Introduce helper to get number of event IDs vijay.kilari
2015-09-03 17:51 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 21/31] xen/arm: ITS: Add GITS registers emulation vijay.kilari
2015-09-07 13:14 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 22/31] xen/arm: ITS: Add virtual ITS availability check helper vijay.kilari
2015-09-07 13:41 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 23/31] xen/arm: ITS: Add 32-bit access to GICR_TYPER vijay.kilari
2015-08-31 16:06 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 24/31] xen/arm: ITS: Add GICR register emulation vijay.kilari
2015-09-07 14:20 ` Julien Grall
2015-09-07 15:26 ` Vijay Kilari
2015-09-09 13:55 ` Ian Campbell
2015-09-09 16:11 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 25/31] xen/arm: ITS: Allocate irq descriptors for LPIs vijay.kilari
2015-08-31 11:06 ` [PATCH v6 26/31] xen/arm: ITS: Allocate pending_lpi " vijay.kilari
2015-08-31 11:06 ` [PATCH v6 27/31] xen/arm: ITS: Route LPIs vijay.kilari
2015-08-31 11:06 ` [PATCH v6 28/31] xen/arm: ITS: Add domain specific ITS initialization vijay.kilari
2015-08-31 11:06 ` [PATCH v6 29/31] xen/arm: ITS: Map ITS translation space vijay.kilari
2015-08-31 19:07 ` Julien Grall
2015-08-31 11:06 ` [PATCH v6 30/31] xen/arm: ITS: Generate ITS node for Dom0 vijay.kilari
2015-08-31 11:06 ` [PATCH v6 31/31] xen/arm: ITS: Add pci devices in ThunderX vijay.kilari
2015-09-09 15:22 ` Ian Campbell
2015-09-02 15:38 ` [PATCH v6 00/31] Add ITS support Ian Campbell
2015-09-02 15:52 ` Ian Campbell
2015-09-03 16:45 ` Julien Grall
2015-09-09 15:29 ` Ian Campbell
2015-09-14 11:00 ` Vijay Kilari
2015-09-14 11:09 ` Julien Grall
2015-09-14 13:04 ` Vijay Kilari
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55E884BD.5080503@citrix.com \
--to=julien.grall@citrix.com \
--cc=Ian.Campbell@citrix.com \
--cc=Prasun.Kapoor@caviumnetworks.com \
--cc=Vijaya.Kumar@caviumnetworks.com \
--cc=manish.jaggi@caviumnetworks.com \
--cc=stefano.stabellini@citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=tim@xen.org \
--cc=vijay.kilari@gmail.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.