Re: [RFC PATCH v2 13/22] xen/arm: its: Add virtual ITS command support

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 19/03/2015 14:38, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> Add Virtual ITS command processing support to
> Virtual ITS driver. Also add API's to in physical
> ITS driver to send commands from Virtual ITS driver.
>
> In this patch, following are done
>   -Physical ITS driver will allocate physical LPI for
>    virtual LPI request.

Please indent the same way as the other item.

>   - The Device ID is used to find the ITS on which it is attached
>     and ITS command is sent on that physical ITS.
>   - Commands like SYNC and INVALL does not have device id. So these
>     commands are sent on all Physical ITS nodes.
>   - The vTA(virtual target address) is considered unique way to map
>     to Physical target address and collection ids.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
> v2: - put unused code under #if0/endif
>      - changes to redistributor is moved to separate patch
>      - Fixed comments from RFC version
> ---
>   xen/arch/arm/Makefile         |    1 +
>   xen/arch/arm/gic-v3-its.c     |  185 ++++++++-
>   xen/arch/arm/vgic-v3-its.c    |  879 +++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/domain.h  |    9 +
>   xen/include/asm-arm/gic-its.h |   86 +++-
>   5 files changed, 1156 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 66ea264..81a3317 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -32,6 +32,7 @@ obj-y += traps.o
>   obj-y += vgic.o vgic-v2.o
>   obj-$(CONFIG_ARM_64) += vgic-v3.o
>   obj-$(CONFIG_ARM_64) += gic-v3-its.o
> +obj-$(CONFIG_ARM_64) += vgic-v3-its.o
>   obj-y += vtimer.o
>   obj-y += vuart.o
>   obj-y += hvm.o
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 242cf65..a9aab73 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -56,6 +56,14 @@
>
>   #define its_warn(fmt, ...)                                            \
>
> +//#define DEBUG_GIC_ITS
> +
> +#ifdef DEBUG_GIC_ITS
> +# define DPRINTK(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
> +#else
> +# define DPRINTK(fmt, args...) do {} while ( 0 )
> +#endif
> +
>   #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1 << 0)
>
>   #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
> @@ -68,6 +76,7 @@
>   struct its_collection {
>   	u64			target_address;
>   	u16			col_id;
> +	u16			valid;
>   };
>
>   /*
> @@ -80,14 +89,19 @@ struct its_node {
>   	struct list_head	entry;
>   	void __iomem		*base;
>   	unsigned long		phys_base;
> +	unsigned long		phys_size;
>   	struct its_cmd_block	*cmd_base;
>   	struct its_cmd_block	*cmd_write;
>   	void			*tables[GITS_BASER_NR_REGS];
>   	struct its_collection	*collections;
>   	u64			flags;
>   	u32			ite_size;
> +	u32			nr_collections;
> +	struct dt_device_node	*dt_node;
>   };
>
> +uint32_t pta_type;
> +

The physical target address is defined per-ITS. Therefore it should be 
moved in the its_node.

>   #define ITS_ITT_ALIGN		SZ_256
>
>   static LIST_HEAD(its_nodes);
> @@ -127,6 +141,123 @@ struct its_cmd_desc {
>   	};
>   };
>
> +uint32_t its_get_pta_type(void)
> +{

That would require its_get_pta_type to take an its_node in parameter.

> +	return pta_type;
> +}
> +
> +struct its_node * its_get_phys_node(uint32_t 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;
> +}
> +
> +static int its_search_rdist_address(struct domain *d, uint64_t ta,
> +				    uint32_t *col_id)
> +{
> +	int i, rg;
> +	paddr_t start, end;
> +
> +	for (rg = 0; rg < d->arch.vgic.nr_regions; rg++) {
> +		i = 0;
> +		start = d->arch.vgic.rdist_regions[rg].base;
> +		end = d->arch.vgic.rdist_regions[rg].base +
> +			d->arch.vgic.rdist_regions[rg].size;
> +		while ((( start + i * d->arch.vgic.rdist_stride) < end)) {
> +			if ((start + i * d->arch.vgic.rdist_stride) == ta) {
> +				DPRINTK("ITS: Found pta 0x%lx\n", ta);
> +				*col_id = i;
> +				return 0;
> +			}
> +			i++;
> +		}
> +	}
> +	return 1;
> +}
> +

If you consider col_id == vcpu_id, you may want to give a look to 
get_vcpu_from_rdist.

> +int its_get_physical_cid(struct domain *d, uint32_t *col_id, uint64_t ta)

The function returns either 1 or 0. I would use bool_t.

After looking to the code of this function, this looks more a vITS 
specific function rather than an ITS one.

> +{
> +	int i;
> +	struct its_collection *col;
> +
> +	/*
> +	* For Dom0, the target address info is collected
> +	* at boot time.
> +	*/
> +	if (is_hardware_domain(d)) {
> +		struct its_node *its;
> +
> +		list_for_each_entry(its, &its_nodes, entry) {
> +			for (i = 0; i < its->nr_collections; i++) {
> +		                col = &its->collections[i];
> +				if (col->valid && col->target_address == ta) {
> +					DPRINTK("ITS:Match ta 0x%lx ta 0x%lx\n",
> +						col->target_address, ta);
> +					*col_id = col->col_id;
> +					return 0;
> +				}
> +			}
> +			/* All collections are mapped on every physical ITS */

If this is true, you don't need the list_for_each_entry. It will be less 
confusing to understand.

Also, you are assuming that every ITS have the same value in 
GITS_TYPER.PTA. Which may not be true on any platform.

> +			break;
> +		}
> +	}
> +	else
> +	{
> +		/* As per Spec, Target address is re-distributor
> +		 * address/cpu number.
> +		 * We cannot rely on collection id as it can any number.
> +		 * So here we should rely only on vta address to map the
> +		 * collection. For domU, vta != target address.
> +		 * So, check vta is corresponds to which GICR region and
> +		 * consider that vcpu id as collection id.

A collection ID based on the VCPU ID may not be a valid collection on 
the physical ITS.

> +		 */
> +		if (its_get_pta_type()) {
> +			its_search_rdist_address(d, ta, col_id);
> +		}
> +		else
> +		{
> +			*col_id = ta;
> +			return 0;
> +		}
> +	}
> +
> +	DPRINTK("ITS: Cannot find valid pta entry for ta 0x%lx\n", ta);
> +	return 1;
> +}
> +
> +int its_get_target(uint8_t pcid, uint64_t *pta)

Ditto for the return type.

> +{
> +	int i;
> +	struct its_collection *col;
> +	struct its_node *its;
> +
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		for (i = 0; i < its->nr_collections; i++) {
> +			col = &its->collections[i];
> +			if (col->valid && col->col_id == pcid) {
> +				*pta = col->target_address;
> +				DPRINTK("ITS:Match pta 0x%lx vta 0x%lx\n",
> +					col->target_address, *pta);
> +				return 0;
> +			}
> +		}
> +		/* All collections are mapped on every physical ITS */
> +		break;
> +	}
> +
> +	DPRINTK("ITS: Cannot find valid pta entry for vta 0x%lx\n",*pta);
> +	return 1;
> +}
> +
>   #define ITS_CMD_QUEUE_SZ		SZ_64K
>   #define ITS_CMD_QUEUE_NR_ENTRIES	(ITS_CMD_QUEUE_SZ / sizeof(struct its_cmd_block))
>
> @@ -541,7 +672,6 @@ out:
>   	return bitmap;
>   }
>
> -/* TODO: Remove static for the sake of compilation */
>   void its_lpi_free(unsigned long *bitmap, int base, int nr_ids)
>   {
>   	int lpi;
> @@ -859,6 +989,8 @@ static void its_cpu_init_collection(void)
>   		/* Perform collection mapping */
>   		its->collections[cpu].target_address = target;
>   		its->collections[cpu].col_id = cpu;
> +		its->collections[cpu].valid = 1;
> +		its->nr_collections++;
>
>   		its_send_mapc(its, &its->collections[cpu], 1);
>   		its_send_invall(its, &its->collections[cpu]);
> @@ -867,8 +999,7 @@ static void its_cpu_init_collection(void)
>   	spin_unlock(&its_lock);
>   }
>
> -/* TODO: Remove static for the sake of compilation */
> -int its_alloc_device_irq(struct its_device *dev, int *hwirq)
> +int its_alloc_device_irq(struct its_device *dev, uint32_t *hwirq)

In patch #6 "Port ITS driver to Xen" you change irq_hw_number_t to int. 
Now you modify again the type to uint32_t. Why not directly moving to 
the correct type in #6?

>   {
>   	int idx;
>
> @@ -882,6 +1013,47 @@ int its_alloc_device_irq(struct its_device *dev, int *hwirq)
>   	return 0;
>   }
>
> +static int its_send_cmd(struct vcpu *v, struct its_node *its,
> +			struct its_cmd_block *phys_cmd)
> +{
> +	struct its_cmd_block *cmd, *next_cmd;
> +
> +	spin_lock(&its->lock);

spin_lock_irqsave?

> +
> +	cmd = its_allocate_entry(its);
> +	if (!cmd)
> +		return 0;
> +
> +	cmd->raw_cmd[0] = phys_cmd->raw_cmd[0];
> +	cmd->raw_cmd[1] = phys_cmd->raw_cmd[1];
> +	cmd->raw_cmd[2] = phys_cmd->raw_cmd[2];
> +	cmd->raw_cmd[3] = phys_cmd->raw_cmd[3];

memcpy?

> +	its_flush_cmd(its, cmd);
> +
> +	next_cmd = its_post_commands(its);
> +	spin_unlock(&its->lock);
> +
> +	its_wait_for_range_completion(its, cmd, next_cmd);
> +
> +	return 1;
> +}
> +
> +int gic_its_send_cmd(struct vcpu *v, struct its_node *its,
> +		     struct its_cmd_block *phys_cmd, int send_all)
> +{
> +	struct its_node *pits;
> +	int ret = 0;
> +
> +	if (send_all) {
> +		list_for_each_entry(pits, &its_nodes, entry)
> +		ret = its_send_cmd(v, pits, phys_cmd);
> +	}
> +	else
> +		return its_send_cmd(v, its, phys_cmd);
> +
> +	return ret;
> +}
> +
>   static int its_force_quiescent(void __iomem *base)
>   {
>   	u32 count = 1000000;	/* 1s */
> @@ -955,10 +1127,17 @@ static int its_probe(struct dt_device_node *node)
>
>   	spin_lock_init(&its->lock);
>   	INIT_LIST_HEAD(&its->entry);
> +	its->dt_node = node;
>   	its->base = its_base;
>   	its->phys_base = its_addr;
> +	its->phys_size = its_size;

These 2 changes don't belong to this patch.

>   	its->ite_size = ((readl_relaxed(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
>
> +	if ( (readq_relaxed(its->base + GITS_TYPER) & GITS_TYPER_PTA) )
> +		pta_type = 1;
> +	else
> +		pta_type = 0;
> +

Please add a define for those pta_type. It would be easier to understand 
the code.

>   	its->cmd_base = xzalloc_bytes(ITS_CMD_QUEUE_SZ);
>   	if (!its->cmd_base) {
>   		err = -ENOMEM;
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> new file mode 100644
> index 0000000..7530a88
> --- /dev/null
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -0,0 +1,879 @@
> +/*
> + * Copyright (C) 2013, 2014 ARM Limited, All Rights Reserved.
> + * Author: Marc Zyngier <marc.zyngier@arm.com>
> + *
> + * Xen changes:
> + * Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> + * Copyright (C) 2014 Cavium Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms 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/bitops.h>
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/softirq.h>
> +#include <xen/irq.h>
> +#include <xen/list.h>
> +#include <xen/sched.h>
> +#include <xen/sizes.h>
> +#include <xen/xmalloc.h>
> +#include <asm/current.h>
> +#include <asm/device.h>
> +#include <asm/mmio.h>
> +#include <asm/io.h>
> +#include <asm/gic_v3_defs.h>
> +#include <asm/gic.h>
> +#include <asm/vgic.h>
> +#include <asm/gic-its.h>
> +
> +/* GITS register definitions */
> +#define VITS_GITS_TYPER_HCC       (0xffU << 24)
> +#define VITS_GITS_TYPER_PTA_SHIFT (19)
> +#define VITS_GITS_DEV_BITS        (0x14U << 13)
> +#define VITS_GITS_ID_BITS         (0x13U << 8)
> +#define VITS_GITS_ITT_SIZE        (0x7U << 4)
> +#define VITS_GITS_DISTRIBUTED     (0x1U << 3)
> +#define VITS_GITS_PLPIS           (0x1U << 0)
> +
> +/* GITS_PIDRn register values for ARM implementations */
> +#define GITS_PIDR0_VAL            (0x94)
> +#define GITS_PIDR1_VAL            (0xb4)
> +#define GITS_PIDR2_VAL            (0x3b)
> +#define GITS_PIDR3_VAL            (0x00)
> +#define GITS_PIDR4_VAL            (0x04)
> +
> +//#define DEBUG_ITS
> +
> +#ifdef DEBUG_ITS
> +# define DPRINTK(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
> +#else
> +# define DPRINTK(fmt, args...) do {} while ( 0 )
> +#endif
> +
> +#ifdef DEBUG_ITS
> +static void dump_cmd(struct its_cmd_block *cmd)
> +{
> +    printk("CMD[0] = 0x%lx CMD[1] = 0x%lx CMD[2] = 0x%lx CMD[3] = 0x%lx\n",
> +           cmd->raw_cmd[0], cmd->raw_cmd[1], cmd->raw_cmd[2], cmd->raw_cmd[3]);
> +}
> +#endif
> +
> +void vgic_its_disable_lpis(struct vcpu *v, uint32_t lpi)

This function is only used within this file. Therefore it should be static.

Also, why the 's' to lpis? AFAICT, you are only disabling one LPI at the 
time.

> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    p = irq_to_pending(v, lpi);
> +    clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +    gic_remove_from_queues(v, lpi);
> +    if ( p->desc != NULL )
> +    {
> +        spin_lock_irqsave(&p->desc->lock, flags);
> +        p->desc->handler->disable(p->desc);
> +        spin_unlock_irqrestore(&p->desc->lock, flags);
> +    }

This code seems very similar to the content of the loop in 
vgic_disable_irqs. Please make a common helper.

> +}
> +
> +void vgic_its_enable_lpis(struct vcpu *v, uint32_t lpi)

static

Ditto for the 's'

> +{
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    p = irq_to_pending(v, lpi);
> +    set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    if ( !list_empty(&p->inflight) &&
> +         !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> +        gic_raise_guest_irq(v, p->desc->arch.virq, p->priority);
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +    if ( p->desc != NULL )
> +    {
> +        spin_lock_irqsave(&p->desc->lock, flags);
> +        p->desc->handler->enable(p->desc);
> +        spin_unlock_irqrestore(&p->desc->lock, flags);
> +    }

Similar to the content of loop in vgic_enable_irqs.

> +}
> +
> +static int vits_alloc_device_irq(struct its_device *dev, uint32_t id,

It took me a while to understand what is "id". I would rename to 
something more meaningful such as eventID of irqID.

> +                                uint32_t *plpi, uint32_t vlpi, uint32_t vcol_id)

The name is confusing. This function can also retrieve an LPIs...

> +{
> +
> +    int idx, i = 0;
> +
> +    spin_lock(&dev->vlpi_lock);
> +    while ((i = find_next_bit(dev->vlpi_map, dev->nr_lpis, i)) < dev->nr_lpis )
> +    {
> +        if ( dev->vlpi_entries[i].vlpi == vlpi )
> +        {
> +             *plpi = dev->vlpi_entries[i].plpi;
> +             DPRINTK("Found plpi %d for device 0x%x with vlpi %d id %d\n",
> +                      *plpi, dev->dev_id, vlpi, dev->vlpi_entries[i].id);
> +             spin_unlock(&dev->vlpi_lock);
> +             return 0;
> +        }
> +        i++;
> +    }
> +
> +    if ( its_alloc_device_irq(dev, plpi) )
> +        BUG_ON(1);

Why this BUG_ON()? It looks like to me that we should return an error if 
we can't allocate the LPIs for the device. Otherwise a malicious guest 
can trigger the BUG_ON and crash the whole platform.

> +
> +    idx = find_first_zero_bit(dev->vlpi_map, dev->nr_lpis);
> +    dev->vlpi_entries[idx].plpi = *plpi;
> +    dev->vlpi_entries[idx].vlpi = vlpi;
> +    dev->vlpi_entries[idx].id  = id;
> +    set_bit(idx, dev->vlpi_map);
> +
> +    spin_unlock(&dev->vlpi_lock);
> +
> +    DPRINTK("Allocated plpi %d for device 0x%x with vlpi %d id %d @idx %d\n",
> +            *plpi, dev->dev_id, vlpi, id, idx);
> +
> +    return 0;
> +}
> +
> +/* Should be called with its lock held */

Please add an ASSERT in the function to verify the assumption.

> +static void vgic_its_unmap_id(struct vcpu *v, struct its_device *dev,
> +                              uint32_t id, int trash)
> +{
> +    int i = 0;
> +
> +    DPRINTK("vITS: unmap id for device 0x%x id %d trash %d\n",
> +             dev->dev_id, id, trash);
> +
> +    spin_lock(&dev->vlpi_lock);
> +    while ((i = find_next_bit(dev->vlpi_map, dev->nr_lpis, i)) < dev->nr_lpis )

Missing space after the first parenthesis.

while ( ... )

> +    {
> +        if ( dev->vlpi_entries[i].id == id )
> +        {
> +            DPRINTK("vITS: un mapped id for device 0x%x id %d lpi %d\n",
> +                     dev->dev_id, dev->vlpi_entries[i].id,
> +                     dev->vlpi_entries[i].plpi);
> +            vgic_its_disable_lpis(v, dev->vlpi_entries[i].plpi);
> +            release_irq(dev->vlpi_entries[i].plpi, v->domain);

That's definitely wrong. The vITS should not be able to unmap an LPI 
like that.

> +            dev->vlpi_entries[i].plpi = 0;
> +            dev->vlpi_entries[i].vlpi = 0;
> +            dev->vlpi_entries[i].id = 0;
> +            /* XXX: Clear LPI base here? */
> +            clear_bit(dev->vlpi_entries[i].plpi - dev->lpi_base, dev->lpi_map);
> +            clear_bit(i, dev->vlpi_map);
> +            goto out;
> +        }
> +        i++;
> +    }
> +
> +    spin_unlock(&dev->vlpi_lock);
> +    dprintk(XENLOG_ERR, "vITS: id %d not found for device 0x%x to unmap\n",
> +           id, dev->device_id);

XENLOG_ERR is not rate-limited, you have to use XENLOG_G_ERR.

Also please use printk rather than dprintk. The latter will be drop on 
non-debug build.

Lastly, I would print the domain, vCPU and the vITS ID. It would be 
easier for debugging.

All these comment are valid for every dprint/DPRINTK message in this file.

> +
> +    return;
> +out:
> +    if ( bitmap_empty(dev->lpi_map, dev->nr_lpis) )
> +    {
> +        its_lpi_free(dev->lpi_map, dev->lpi_base, dev->nr_lpis);
> +        DPRINTK("vITS: Freeing lpi chunk\n");
> +    }
> +    /* XXX: Device entry is not removed on empty lpi list */
> +    spin_unlock(&dev->vlpi_lock);
> +}
> +
> +static int vgic_its_check_device_id(struct vcpu *v, struct its_device *dev,
> +                                    uint32_t id)
> +{
> +    int i = 0;
> +
> +    spin_lock(&dev->vlpi_lock);
> +    while ((i = find_next_bit(dev->vlpi_map, dev->nr_lpis, i)) < dev->nr_lpis )
> +    {
> +        if ( dev->vlpi_entries[i].id == id )
> +        {
> +            spin_unlock(&dev->vlpi_lock);
> +            return 0;
> +        }
> +        i++;
> +    }
> +    spin_unlock(&dev->vlpi_lock);
> +
> +    return 1;
> +}
> +
> +static struct its_device *vgic_its_check_device(struct vcpu *v, int dev_id)
> +{
> +    struct domain *d = v->domain;
> +    struct its_device *dev = NULL, *tmp;
> +
> +    spin_lock(&d->arch.vits_devs.lock);
> +    list_for_each_entry(tmp, &d->arch.vits_devs.dev_list, entry)
> +    {
> +        if ( tmp->device_id == dev_id )
> +        {
> +            DPRINTK("vITS: Found device 0x%x\n", device_id);
> +            dev = tmp;
> +            break;
> +        }
> +    }
> +    spin_unlock(&d->arch.vits_devs.lock);
> +
> +    return dev;
> +}
> +
> +static int vgic_its_check_cid(struct vcpu *v,
> +                              struct vgic_its *vits,
> +                              uint8_t vcid, uint32_t *pcid)
> +{
> +    uint32_t nmap = vits->cid_map.nr_cid;
> +    int i;

nmap is uint32_t so i should be too.

> +
> +    for ( i = 0; i < nmap; i++ )
> +    {
> +        if ( vcid == vits->cid_map.vcid[i] )
> +        {
> +            *pcid = vits->cid_map.pcid[i];
> +            DPRINTK("vITS: Found vcid %d for vcid %d\n", *pcid,
> +                     vits->cid_map.vcid[i]);
> +            return 0;
> +        }
> +    }
> +
> +    return 1;
> +}
> +
> +static uint64_t vgic_its_get_pta(struct vcpu *v, struct vgic_its *vits,
> +                                 uint64_t vta)
> +{
> +
> +    uint32_t nmap = vits->cid_map.nr_cid;
> +    int i;

ditto

> +    uint8_t pcid;
> +    uint64_t pta;
> +
> +    for ( i = 0; i < nmap; i++ )
> +    {
> +        if ( vta == vits->cid_map.vta[i] )
> +        {
> +            pcid = vits->cid_map.pcid[i];
> +            DPRINTK("vITS: Found vcid %d for vta 0x%lx\n", pcid,
> +                     vits->cid_map.vta[i]);
> +            if ( its_get_target(pcid, &pta) )
> +                BUG_ON(1);

No BUG_ON, please handle the error correctly.

> +            return pta;
> +        }
> +    }
> +
> +    BUG_ON(1);

Ditto

> +    return 1;
> +}
> +
> +static int vgic_its_build_mapd_cmd(struct vcpu *v,

Why this function take a vCPU in a parameter. Shouldn't it take a domain?

> +                                   struct its_cmd_block *virt_cmd,
> +                                   struct its_cmd_block *phys_cmd)
> +{
> +    unsigned long itt_addr;

its_decode_itt return an uint64_t.

> +
> +    itt_addr = its_decode_itt(virt_cmd);
> +    /* Get ITT PA from ITT IPA */
> +    itt_addr = p2m_lookup(v->domain, itt_addr, NULL);

Multiple problems:

1) If the 'V' bit is not set, "ITT Address" will likely be 0.
But IPA 0 != PA 0 so you will end up to use a wrong address. So I would 
do a specific case for 'V' = 0
2) p2m_lookup may return INVALD_PADDR if the page is not mapped
3) You should validate that the guest is using a RAM page and belongs to 
him. Otherwise it may use an MMIO region or a page from another guest
4) Depending of the size, the ITT may cross multiple pages. But the 
physical address may not be contiguous
5) The guest, Xen may remove under our feat the pages which belong to 
the ITT. That would result to the ITS using a wrong page. I think you 
have to take a reference on those pages

> +    its_encode_cmd(phys_cmd, GITS_CMD_MAPD);
> +    its_encode_devid(phys_cmd, its_decode_devid(virt_cmd));

If "Device ID" exceeds the maximum value support by ITS,  a command 
error will be issued.

Also, are we sure to always have vdevid == pdevid? If so, this should be 
written somewhere.

> +    its_encode_size(phys_cmd, its_decode_size(virt_cmd));

If "Size" exceeds the value permitted by GITS_TYPER.IDbits
the physical ITS will issue a command error.

If the platform support System Error Interrupts (GITS_TYPER.SEIS set to 
1), this will generate a system error and hang the hypervisor.

As the generation of the system error is platform specific you have to 
validate all the data given by the guest in order to avoid a such thing 
happening.

My remark here is valid everywhere a command error can be issued in this 
patch.

> +    its_encode_itt(phys_cmd, itt_addr);
> +    its_encode_valid(phys_cmd, its_decode_valid(virt_cmd));
> +
> +    DPRINTK("vITS: Build MAPD with itt_addr 0x%lx devId %d\n",itt_addr,

Space after the comma and devid is an uint32_t so please use %u

> +            its_decode_devid(virt_cmd));
> +
> +    return 0;
> +}

It's late here, I will finish to review this patch next week. For now I 
hit send with my first comments.

Regards,


-- 
Julien Grall