|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v3 10/18] xen/arm: ITS: Add APIs to add and assign device
Hi Vijay,
On 22/06/15 13:01, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>
> Add APIs to add devices to RB-tree, assign and remove
> devices to domain.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> ---
> xen/arch/arm/gic-v3-its.c | 246
> ++++++++++++++++++++++++++++++++++++++++-
> xen/include/asm-arm/gic-its.h | 4 +-
> 2 files changed, 246 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 2a4fa97..4471669 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -94,6 +94,7 @@ static LIST_HEAD(its_nodes);
> static DEFINE_SPINLOCK(its_lock);
> static struct rdist_prop *gic_rdists;
> static struct rb_root rb_its_dev;
> +static DEFINE_SPINLOCK(rb_its_dev_lock);
Can't you use the its_lock to handle locking for the rb_its_dev?
>
> #define gic_data_rdist() (per_cpu(rdist, smp_processor_id()))
> #define gic_data_rdist_rd_base() (per_cpu(rdist,
> smp_processor_id()).rbase)
> @@ -152,6 +153,21 @@ u32 its_get_nr_events(void)
> return (1 << id_bits);
> }
>
> +static struct its_node * its_get_phys_node(u32 dev_id)
> +{
> + struct its_node *its;
> +
> + /* TODO: For now return ITS0 node.
> + * Need Query PCI helper function to get on which
> + * ITS node the device is attached
> + */
> + list_for_each_entry(its, &its_nodes, entry)
> + {
> + return its;
> + }
> +
> + return NULL;
> +}
> /* RB-tree helpers for its_device */
> struct its_device * find_its_device(struct rb_root *root, u32 devid)
> {
> @@ -459,7 +475,7 @@ void its_send_inv(struct its_device *dev, struct
> its_collection *col,
> its_send_single_command(dev->its, its_build_inv_cmd, &desc);
> }
>
> -void its_send_mapd(struct its_device *dev, int valid)
> +static void its_send_mapd(struct its_device *dev, int valid)
I would prefer to see this static where the function has been introduced
and delay the compilation until the end.
> {
> struct its_cmd_desc desc;
>
> @@ -493,7 +509,7 @@ void its_send_mapvi(struct its_device *dev, struct
> its_collection *col,
> its_send_single_command(dev->its, its_build_mapvi_cmd, &desc);
> }
>
> -void its_send_movi(struct its_device *dev, struct its_collection *col,
> +static void its_send_movi(struct its_device *dev, struct its_collection *col,
> u32 event)
Ditto
> {
> struct its_cmd_desc desc;
> @@ -596,7 +612,7 @@ int its_lpi_init(u32 id_bits)
> return 0;
> }
>
> -unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids)
> +static unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids)
Ditto
> {
> unsigned long *bitmap = NULL;
> int chunk_id;
> @@ -636,6 +652,230 @@ out:
> return bitmap;
> }
>
> +static void its_lpi_free(unsigned long *bitmap, int base, int nr_ids)
> +{
> + int lpi;
> +
> + spin_lock(&lpi_lock);
> +
> + for ( lpi = base; lpi < (base + nr_ids); lpi += IRQS_PER_CHUNK )
> + {
> + int chunk = its_lpi_to_chunk(lpi);
> +
> + BUG_ON(chunk > lpi_chunks);
> + if ( test_bit(chunk, lpi_bitmap) )
> + clear_bit(chunk, lpi_bitmap);
> + else
> + its_err("Bad LPI chunk %d\n", chunk);
> + }
> +
> + spin_unlock(&lpi_lock);
> +
> + xfree(bitmap);
> +}
> +
> +static int its_alloc_device_irq(struct its_device *dev, u32 *hwirq)
> +{
> + int idx;
> +
> + idx = find_first_zero_bit(dev->lpi_map, dev->nr_lpis);
> + if ( idx == dev->nr_lpis )
> + return -ENOSPC;
> +
> + *hwirq = dev->lpi_base + idx;
You could use its_get_plpi here.
> + set_bit(idx, dev->lpi_map);
> +
> + return 0;
> +}
> +
> +static u32 its_get_plpi(struct its_device *dev, u32 event)
> +{
> + ASSERT(event < dev->nr_lpis);
> + return dev->lpi_base + event;
> +}
> +
> +/* Device assignment. Should be called from pci_device_add */
I guess you mean PHYSDEVOP_pci_device_add?
> +int its_add_device(struct domain *d, u32 devid)
Why do you pass a domain in parameter? This function only adds a device
into the ITS, there is nothing related to a specific domain.
For more abstraction, I would create an helper which take a struct
device in parameter and retrieving all the necessary informations
(devid, number of MSI...) and then call this function.
> +{
> + struct its_device *dev;
> + unsigned long *lpi_map;
> + void *itt;
> + int lpi_base, nr_lpis, sz;
> + u32 i, nr_ites, plpi, nr_cpus;
> + struct its_collection *col;
> +
> + spin_lock(&rb_its_dev_lock);
> + dev = find_its_device(&rb_its_dev, devid);
> + if ( dev )
This check is not useful. As you release the lock before adding the
device someone else can have time to add the device in the RB-tree.
> + {
> + spin_unlock(&rb_its_dev_lock);
> + dprintk(XENLOG_G_ERR, "ITS:d%dv%d Device already exists dev 0x%x\n",
> + d->domain_id, current->vcpu_id, dev->device_id);
> + return -EEXIST;
> + }
> + spin_unlock(&rb_its_dev_lock);
> +
> + DPRINTK("ITS:d%dv%d Add device devid 0x%x\n",
> + d->domain_id, current->vcpu_id, devid);
> +
> + dev = xzalloc(struct its_device);
> + if ( dev == NULL )
> + return -ENOMEM;
> +
> + dev->its = its_get_phys_node(devid);
> + /* TODO: Use pci helper to get nvecs */
> + nr_ites = 64;
How did you choose this value?
> + sz = nr_ites * dev->its->ite_size;
> + sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> + itt = xzalloc_bytes(sz);
> + if ( !itt )
> + return -ENOMEM;
You forgot to free dev if we fail to allocate itt.
As Linux does, you can do the allocation check in one place. But you
will have to create a local variable for the ITS.
> +
> + lpi_map = its_lpi_alloc_chunks(nr_ites, &lpi_base, &nr_lpis);
> + if ( !lpi_map || (nr_lpis != nr_ites) )
The check "nr_lpis != nr_ites" is here to ensure that
its_lpi_alloc_chuncks allocate the right number of LPI, right? If so,
given this is the only place you call its_lpi_alloc_chunks, why don't
you put the check within the function? It would avoid one parameter.
> + {
> + xfree(dev);
> + xfree(itt);
> + xfree(lpi_map);
> + return -EINVAL;
> + }
> +
> + dev->itt_addr = (u64)itt;
The cast is not necessary.
> + dev->nr_ites = nr_ites;
> + dev->lpi_map = lpi_map;
> + dev->lpi_base = lpi_base;
> + /* nr_lpis should be always equal to nvecs */
nvecs doesn't have any meaning within this function. Do you mean nr_ites?
> + dev->nr_lpis = nr_lpis;
> + dev->device_id = devid;
> +
> + DPRINTK("ITS:d%dv%d Adding Device with id 0x%x nvecs %d lpi_base 0x%x\n",
> + d->domain_id, current->vcpu_id, dev->device_id,
> + dev->nr_lpis, dev->lpi_base);
> +
> + /* Map device to its ITT */
> + its_send_mapd(dev, 1);
> +
> + nr_cpus = num_online_cpus();
> + for ( i = 0; i < nr_ites; i++ )
> + {
> + /* Reserve pLPI */
> + if ( its_alloc_device_irq(dev, &plpi) )
> + return -ENOSPC;
> +
> + /* For each pLPI send MAPVI command */
> + col = &dev->its->collections[(i % nr_cpus)];
Hmmmm.... In patch #7 you are using a different way to get the
collection for the LPIs. Can you introduce an helper to get the collection?
> + its_send_mapvi(dev, col, plpi, i);
> + }
> +
> + spin_lock(&rb_its_dev_lock);
> + if ( insert_its_device(&rb_its_dev, dev) )
You have to undo the mapd, mapvi, and free all the allocated memory if
the insertion fail.
You may want to rework the code in to avoid taking care of that.
> + {
> + spin_unlock(&rb_its_dev_lock);
> + return -EINVAL;
> + }
> + spin_unlock(&rb_its_dev_lock);
> +
> + return 0;
> +}
> +
> +int its_assign_device(struct domain *d, u32 vdevid)
> +{
> + struct its_device *pdev;
> + struct vits_device *vdev;
> + struct irq_desc *desc;
> + u32 plpi, i, pdevid;
> +
> + DPRINTK("ITS:d%dv%d Assign request for virtual device 0x%x\n",
> + d->domain_id, current->vcpu_id, vdevid);
Please don't mix d and current. The first one point to the domain where
the device will be assigned, the latter is the vcpu which issue the
hypercall to assign.
You don't need to print current here.
> + vdev = find_vits_device(&d->arch.vits->dev_root, vdevid);
> + if ( vdev )
> + return -ENODEV;
ENODEV mean there is not device. In this case the vDevID already exists.
You may want to use -EEXISTS.
> +
> + /* TODO: Use PCI helper to convert vDevid to pDevid */
> + pdevid = vdevid;
How the PCI helper would convert a vDevID to pDevID? This is entirely up
to the toolstack.
It would be preferrable that this function take a device and the vDevID
in parameter.
> + spin_lock(&rb_its_dev_lock);
> + pdev = find_its_device(&rb_its_dev, pdevid);
> + if ( !pdev )
> + {
> + spin_unlock(&rb_its_dev_lock);
> + return -ENODEV;
> + }
> + spin_unlock(&rb_its_dev_lock);
Same remark as in its_add_device for finding a device.
> +
> + vdev = xzalloc_bytes(sizeof(struct vits_device));
> + if ( !pdev )
> + return -ENOMEM;
> +
> + vdev->its_dev = pdev;
> + vdev->vdevid = vdevid;
> + vdev->pdevid = pdevid;
> +
> + /* Insert to domains' list */
> + if ( insert_vits_device(&d->arch.vits->dev_root, vdev) )
> + return -EINVAL;
> +
> + DPRINTK("ITS:d%dv%d Assign pdevid 0x%x with nvecs %d\n",
> + d->domain_id, current->vcpu_id, pdev->device_id, pdev->nr_lpis);
ditto for d and current
> +
> + /* XXX: Event id always starts from 0? */
> + for ( i = 0; i < pdev->nr_lpis; i++ )
> + {
> + plpi = its_get_plpi(pdev, i);
> + route_irq_to_guest(d, i, plpi, "LPI");
> + /* Enable all the event in LPI configuration table */
> + desc = irq_to_desc(plpi);
> + set_irq_device(desc, pdev);
> + lpi_set_config(desc, 1);
route_irq_to_guest expects to see a vIRQ in parameter and not an
eventID. Actually, all the check are based on this assumption (even
after you changes in patch #16).
The correct solution is to introduce a new function to route the LPI and
set correctly the required information.
> + }
> +
> + return 0;
> +}
> +
> +int its_detach_device(struct domain *d, u32 vdevid)
> +{
> + struct its_device *pdev;
> + struct vits_device *vdev;
> + struct irq_desc *desc;
> + u32 plpi, i, pdevid;
> +
> + DPRINTK("ITS:d%dv%d Detach request for virtual device 0x%x\n",
> + d->domain_id, current->vcpu_id, vdevid);
ditto for d and current.
> + vdev = find_vits_device(&d->arch.vits->dev_root, vdevid);
> + if ( !vdev )
> + return -EINVAL;
> +
> + /* XXX: convert vDevid to pDevid ? */
> + pdevid = vdevid;
> + spin_lock(&rb_its_dev_lock);
> + pdev = find_its_device(&rb_its_dev, pdevid);
> + if ( !pdev )
> + {
> + spin_unlock(&rb_its_dev_lock);
> + return -EINVAL;
> + }
> + spin_unlock(&rb_its_dev_lock);
Why do you need this? You have the vDev which contains a pointer to the
its_device structure.
> +
> + DPRINTK("ITS:d%dv%d Detach pdevid 0x%x with nvecs %d\n",
> + d->domain_id, current->vcpu_id, pdev->device_id, pdev->nr_lpis);
> + /* Remove vits_device from domain list */
> + remove_vits_device(&d->arch.vits->dev_root, vdev);
> + xfree(vdev);
> +
> + for ( i = 0; i < pdev->nr_lpis; i++ )
> + {
> + plpi = its_get_plpi(pdev, i);
> + /* XXX: Event id always starts from 0 */
> + release_irq(plpi, d);
> + /* Disable all the event in LPI configuration table */
> + desc = irq_to_desc(plpi);
> + set_irq_device(desc, NULL);
release_irq free irq_guest so this will segfault. As for
route_irq_to_guest please introduce a function to release the LPI.
> + lpi_set_config(desc, 0);
The interrupt should be disabled before deassigning from the guest.
IHMO, all this stuff should be done the a new release function.
> + }
> +
> + its_lpi_free(pdev->lpi_map, pdev->lpi_base, pdev->nr_lpis);
Why do you free the physical LPI chunk? We want to be able to reassign
the guest later.
> + 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 a1099a1..8f898a6 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -263,7 +263,9 @@ static inline uint32_t its_decode_devid(struct domain *d,
> its_cmd_block *cmd)
> void its_set_affinity(struct irq_desc *desc, int cpu);
> void lpi_set_config(struct irq_desc *desc, int enable);
> uint32_t its_get_nr_events(void);
> -
> +struct vits_device * find_vits_device(struct rb_root *root, uint32_t devid);
Coding style. And in general, the function prototype should be declared
in the patch where the function has been added not when it's used.
> +int insert_vits_device(struct rb_root *root, struct vits_device *dev);
> +int remove_vits_device(struct rb_root *root, struct vits_device *dev);
> #endif /* __ASM_ARM_GIC_ITS_H__ */
>
> /*
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |