diff mbox

[v6,24/36] ARM: vITS: add command handling stub and MMIO emulation

Message ID 20170407173307.9788-25-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andre Przywara April 7, 2017, 5:32 p.m. UTC
Emulate the memory mapped ITS registers and provide a stub to introduce
the ITS command handling framework (but without actually emulating any
commands at this time).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/vgic-v3-its.c       | 512 +++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/gic_v3_its.h |   3 +
 2 files changed, 515 insertions(+)

Comments

Julien Grall April 9, 2017, 8:16 p.m. UTC | #1
Hi Andre,

On 04/07/2017 06:32 PM, Andre Przywara wrote:
> Emulate the memory mapped ITS registers and provide a stub to introduce
> the ITS command handling framework (but without actually emulating any
> commands at this time).
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v3-its.c       | 512 +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic_v3_its.h |   3 +
>  2 files changed, 515 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 065ffe2..a171a3b 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -67,6 +67,9 @@ struct vits_itte
>      uint16_t pad;
>  };
>
> +#define GITS_BASER_RO_MASK       (GITS_BASER_TYPE_MASK | \
> +                                  (31UL << GITS_BASER_ENTRY_SIZE_SHIFT))
> +
>  int vgic_v3_its_init_domain(struct domain *d)
>  {
>      spin_lock_init(&d->arch.vgic.its_devices_lock);
> @@ -80,6 +83,515 @@ void vgic_v3_its_free_domain(struct domain *d)
>      ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
>  }
>
> +/**************************************
> + * Functions that handle ITS commands *
> + **************************************/
> +
> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
> +                                   unsigned int shift, unsigned int size)
> +{
> +    return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
> +}
> +
> +#define its_cmd_get_command(cmd)        its_cmd_mask_field(cmd, 0,  0,  8)
> +#define its_cmd_get_deviceid(cmd)       its_cmd_mask_field(cmd, 0, 32, 32)
> +#define its_cmd_get_size(cmd)           its_cmd_mask_field(cmd, 1,  0,  5)
> +#define its_cmd_get_id(cmd)             its_cmd_mask_field(cmd, 1,  0, 32)
> +#define its_cmd_get_physical_id(cmd)    its_cmd_mask_field(cmd, 1, 32, 32)
> +#define its_cmd_get_collection(cmd)     its_cmd_mask_field(cmd, 2,  0, 16)
> +#define its_cmd_get_target_addr(cmd)    its_cmd_mask_field(cmd, 2, 16, 32)
> +#define its_cmd_get_validbit(cmd)       its_cmd_mask_field(cmd, 2, 63,  1)
> +#define its_cmd_get_ittaddr(cmd)        (its_cmd_mask_field(cmd, 2, 8, 44) << 8)
> +
> +#define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
> +
> +/*
> + * Requires the vcmd_lock to be held.
> + * TODO: Investigate whether we can be smarter here and don't need to hold
> + * the lock all of the time.
> + */
> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
> +{
> +    paddr_t addr = its->cbaser & GENMASK(51, 12);
> +    uint64_t command[4];
> +    uint64_t creadr = its->creadr;
> +
> +    ASSERT(spin_is_locked(&its->vcmd_lock));
> +
> +    if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +        return -1;
> +
> +    while ( creadr != its->cwriter )
> +    {
> +        int ret;
> +
> +        ret = vgic_access_guest_memory(d, addr + creadr,
> +                                       command, sizeof(command), false);
> +        if ( ret )
> +            return ret;
> +
> +        switch ( its_cmd_get_command(command) )
> +        {
> +        case GITS_CMD_SYNC:
> +            /* We handle ITS commands synchronously, so we ignore SYNC. */
> +            break;
> +        default:
> +            gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
> +                     its_cmd_get_command(command));
> +            break;
> +        }
> +
> +        creadr += ITS_CMD_SIZE;
> +        if ( creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +            creadr = 0;
> +        its->creadr = creadr;   /* allow the guest to see the progress */

I hope you know that the compiler can decide to drop the temporary 
variable for optimization? ;) So it may decide to write-back everytime 
in its->creadr.

> +
> +        if ( ret )
> +            gdprintk(XENLOG_WARNING,
> +                     "ITS: ITS command error %d while handling command %lu\n",
> +                     ret, its_cmd_get_command(command));
> +    }
> +
> +    return 0;
> +}
> +
> +/*****************************
> + * ITS registers read access *
> + *****************************/
> +
> +/* Identifying as an ARM IP, using "X" as the product ID. */
> +#define GITS_IIDR_VALUE                 0x5800034c

Do we need to request ARM to register this value? Preventing someone to 
re-use it for another purpose in the future.

> +
> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                                 register_t *r, void *priv)
> +{
> +    struct virt_its *its = priv;
> +    uint64_t reg;
> +
> +    switch ( info->gpa & 0xffff )
> +    {
> +    case VREG32(GITS_CTLR):
> +    {
> +        /*
> +         * We try to avoid waiting for the command queue lock and report
> +         * non-quiescent if that lock is already taken.
> +         */
> +        bool have_cmd_lock;
> +
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +
> +        have_cmd_lock = spin_trylock(&its->vcmd_lock);
> +        spin_lock(&its->its_lock);
> +        if ( its->enabled )
> +            reg = GITS_CTLR_ENABLE;
> +        else
> +            reg = 0;
> +
> +        if ( have_cmd_lock && its->cwriter == its->creadr )
> +            reg |= GITS_CTLR_QUIESCENT;
> +
> +        spin_unlock(&its->its_lock);
> +        if ( have_cmd_lock )
> +            spin_unlock(&its->vcmd_lock);
> +
> +        *r = vgic_reg32_extract(reg, info);
> +        break;
> +    }
> +    case VREG32(GITS_IIDR):
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +        *r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
> +        break;
> +    case VREG64(GITS_TYPER):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        reg = GITS_TYPER_PHYSICAL;
> +        reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
> +        reg |= (its->intid_bits - 1) << GITS_TYPER_IDBITS_SHIFT;
> +        reg |= (its->devid_bits - 1) << GITS_TYPER_DEVIDS_SHIFT;
> +        *r = vgic_reg64_extract(reg, info);
> +        break;
> +    case 0x0018 ... 0x001c:
> +        goto read_reserved;
> +    case 0x0020 ... 0x003c:
> +        goto read_impl_defined;
> +    case 0x0040 ... 0x007c:
> +        goto read_reserved;
> +    case VREG64(GITS_CBASER):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +        spin_lock(&its->its_lock);
> +        *r = vgic_reg64_extract(its->cbaser, info);
> +        spin_unlock(&its->its_lock);
> +        break;
> +    case VREG64(GITS_CWRITER):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        reg = its->cwriter;
> +        *r = vgic_reg64_extract(reg, info);
> +        break;
> +    case VREG64(GITS_CREADR):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        reg = its->creadr;
> +        *r = vgic_reg64_extract(reg, info);
> +        break;
> +    case 0x0098 ... 0x00fc:
> +        goto read_reserved;
> +    case VREG64(GITS_BASER0):           /* device table */
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +        spin_lock(&its->its_lock);
> +        *r = vgic_reg64_extract(its->baser_dev, info);
> +        spin_unlock(&its->its_lock);
> +        break;
> +    case VREG64(GITS_BASER1):           /* collection table */
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +        spin_lock(&its->its_lock);
> +        *r = vgic_reg64_extract(its->baser_coll, info);
> +        spin_unlock(&its->its_lock);
> +        break;
> +    case VRANGE64(GITS_BASER2, GITS_BASER7):
> +        goto read_as_zero_64;
> +    case 0x0140 ... 0xbffc:
> +        goto read_reserved;
> +    case 0xc000 ... 0xffcc:
> +        goto read_impl_defined;
> +    case 0xffd0 ... 0xffe4:
> +        goto read_as_zero_64;
> +    case VREG32(GITS_PIDR2):
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +        *r = vgic_reg32_extract(GIC_PIDR2_ARCH_GICv3, info);
> +        break;
> +    case 0xffec ... 0xfffc:
> +        goto read_as_zero_64;

Why don't you have a default here? You don't cover all the ranges 
(basically all the end of reserved regions such as 0xfffc - 0xfffe ...). 
For those accesses you will return 1 as it was handled.

So please add a default and switch all s ... e to VRANGE*.

> +    }
> +
> +    return 1;
> +
> +read_as_zero_64:
> +    if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +    *r = 0;
> +
> +    return 1;
> +
> +read_impl_defined:
> +    printk(XENLOG_G_DEBUG
> +           "%pv: vGITS: RAZ on implementation defined register offset %#04lx\n",
> +           v, info->gpa & 0xffff);
> +    *r = 0;
> +    return 1;
> +
> +read_reserved:
> +    printk(XENLOG_G_DEBUG
> +           "%pv: vGITS: RAZ on reserved register offset %#04lx\n",
> +           v, info->gpa & 0xffff);
> +    *r = 0;
> +    return 1;
> +
> +bad_width:
> +    printk(XENLOG_G_ERR "vGIIS: bad read width %d r%d offset %#04lx\n",
> +           info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
> +    domain_crash_synchronous();
> +
> +    return 0;
> +}
> +
> +/******************************
> + * ITS registers write access *
> + ******************************/
> +
> +static unsigned int its_baser_table_size(uint64_t baser)
> +{
> +    unsigned int ret, page_size[4] = {SZ_4K, SZ_16K, SZ_64K, SZ_64K};
> +
> +    ret = page_size[(baser >> GITS_BASER_PAGE_SIZE_SHIFT) & 3];
> +
> +    return ret * ((baser & GITS_BASER_SIZE_MASK) + 1);
> +}
> +
> +static unsigned int its_baser_nr_entries(uint64_t baser)
> +{
> +    int entry_size = GITS_BASER_ENTRY_SIZE(baser);

You said you fixed this .... but it looks like not. So please 
s/int/unsigned int/

> +
> +    return its_baser_table_size(baser) / entry_size;
> +}
> +
> +/* Must be called with the ITS lock held. */
> +static bool vgic_v3_verify_its_status(struct virt_its *its, bool status)
> +{
> +    ASSERT(spin_is_locked(&its->its_lock));
> +
> +    if ( !status )
> +        return false;
> +
> +    if ( !(its->cbaser & GITS_VALID_BIT) ||
> +         !(its->baser_dev & GITS_VALID_BIT) ||
> +         !(its->baser_coll & GITS_VALID_BIT) )
> +    {
> +        printk(XENLOG_G_WARNING "d%d tried to enable ITS without having the tables configured.\n",
> +               its->d->domain_id);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void sanitize_its_base_reg(uint64_t *reg)
> +{
> +    uint64_t r = *reg;
> +
> +    /* Avoid outer shareable. */
> +    switch ( (r >> GITS_BASER_SHAREABILITY_SHIFT) & 0x03 )
> +    {
> +    case GIC_BASER_OuterShareable:
> +        r = r & ~GITS_BASER_SHAREABILITY_MASK;

NIT r &= ~GITS...;


> +        r |= GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    /* Avoid any inner non-cacheable mapping. */
> +    switch ( (r >> GITS_BASER_INNER_CACHEABILITY_SHIFT) & 0x07 )
> +    {
> +    case GIC_BASER_CACHE_nCnB:
> +    case GIC_BASER_CACHE_nC:
> +        r = r & ~GITS_BASER_INNER_CACHEABILITY_MASK;

Ditto.

> +        r |= GIC_BASER_CACHE_RaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    /* Only allow non-cacheable or same-as-inner. */
> +    switch ( (r >> GITS_BASER_OUTER_CACHEABILITY_SHIFT) & 0x07 )
> +    {
> +    case GIC_BASER_CACHE_SameAsInner:
> +    case GIC_BASER_CACHE_nC:
> +        break;
> +    default:
> +        r = r & ~GITS_BASER_OUTER_CACHEABILITY_MASK;

Ditto.

> +        r |= GIC_BASER_CACHE_nC << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> +        break;
> +    }
> +
> +    *reg = r;
> +}
> +
> +static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
> +                                  register_t r, void *priv)
> +{
> +    struct domain *d = v->domain;
> +    struct virt_its *its = priv;
> +    uint64_t reg;
> +    uint32_t reg32;
> +
> +    switch ( info->gpa & 0xffff )
> +    {
> +    case VREG32(GITS_CTLR):
> +    {
> +        uint32_t ctlr;
> +
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +
> +        /*
> +         * We need to take the vcmd_lock to prevent a guest from disabling
> +         * the ITS while commands are still processed.
> +         */
> +        spin_lock(&its->vcmd_lock);
> +        spin_lock(&its->its_lock);
> +        ctlr = its->enabled ? GITS_CTLR_ENABLE : 0;
> +        reg32 = ctlr;
> +        vgic_reg32_update(&reg32, r, info);
> +
> +        if ( ctlr ^ reg32 )
> +            its->enabled = vgic_v3_verify_its_status(its,
> +                                                     reg32 & GITS_CTLR_ENABLE);
> +        spin_unlock(&its->its_lock);
> +        spin_unlock(&its->vcmd_lock);
> +        return 1;
> +    }
> +
> +    case VREG32(GITS_IIDR):
> +        goto write_ignore_32;
> +    case VREG32(GITS_TYPER):
> +        goto write_ignore_32;
> +    case 0x0018 ... 0x001c:

Please correctly implement the range use VRANGE*.

> +        goto write_reserved;
> +    case 0x0020 ... 0x003c:
> +        goto write_impl_defined;
> +    case 0x0040 ... 0x007c:
> +        goto write_reserved;
> +    case VREG64(GITS_CBASER):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        spin_lock(&its->vcmd_lock);

Why do you need to take the command lock here? its->enabled will prevent 
to modify cbaser to be overwritten.

> +        spin_lock(&its->its_lock);
> +        /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
> +        if ( its->enabled )
> +        {
> +            spin_unlock(&its->its_lock);
> +            spin_unlock(&its->vcmd_lock);
> +            gdprintk(XENLOG_WARNING,
> +                     "ITS: tried to change CBASER with the ITS enabled.\n");
> +            return 1;
> +        }
> +
> +        reg = its->cbaser;
> +        vgic_reg64_update(&reg, r, info);
> +        sanitize_its_base_reg(&reg);
> +
> +        its->cbaser = reg;
> +        its->creadr = 0;
> +        spin_unlock(&its->its_lock);
> +        spin_unlock(&its->vcmd_lock);
> +
> +        return 1;
> +
> +    case VREG64(GITS_CWRITER):
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        spin_lock(&its->vcmd_lock);
> +        reg = its->cwriter & 0xfffe0;

Please explain this mask.


> +        vgic_reg64_update(&reg, r, info);
> +        its->cwriter = reg & 0xfffe0;

Ditto. You likely need a define for that.

> +
> +        if ( its->enabled )

So its->enabled is in this case protected by vcmd_lock and not its->lock 
as other place, correct? If so, please document it.

> +        {
> +            int ret = vgic_its_handle_cmds(d, its);

I am not convinced of the usefulness of the temporary variable ret. You 
could directly do:

if ( vgic_its_handle_cmds(...) )
   printk(....)

> +
> +            if ( ret )
> +                printk(XENLOG_G_WARNING "error handling ITS commands\n");

Again you likely want to print the domain id here. So I would it to 
gdprintk.

> +        }
> +        spin_unlock(&its->vcmd_lock);
> +
> +        return 1;
> +
> +    case VREG64(GITS_CREADR):
> +        goto write_ignore_64;
> +
> +    case 0x0098 ... 0x00fc:
> +        goto write_reserved;
> +    case VREG64(GITS_BASER0):           /* device table */
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        spin_lock(&its->its_lock);
> +
> +        /*
> +         * Changing base registers with the ITS enabled is UNPREDICTABLE,
> +         * we choose to ignore it, but warn.
> +         */
> +        if ( its->enabled )
> +        {
> +            spin_unlock(&its->its_lock);
> +            gdprintk(XENLOG_WARNING, "ITS: tried to change BASER with the ITS enabled.\n");
> +
> +            return 1;
> +        }
> +
> +        reg = its->baser_dev;
> +        vgic_reg64_update(&reg, r, info);
> +
> +        /* We don't support indirect tables for now. */
> +        reg &= ~(GITS_BASER_RO_MASK | GITS_BASER_INDIRECT);
> +        reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> +        reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
> +        sanitize_its_base_reg(&reg);
> +
> +        if ( reg & GITS_VALID_BIT )
> +        {
> +            its->max_devices = its_baser_nr_entries(reg);
> +            if ( its->max_devices > BIT(its->devid_bits) )
> +                its->max_devices = BIT(its->devid_bits);
> +        }
> +        else
> +            its->max_devices = 0;
> +
> +        its->baser_dev = reg;
> +        spin_unlock(&its->its_lock);
> +        return 1;
> +    case VREG64(GITS_BASER1):           /* collection table */
> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> +        spin_lock(&its->its_lock);
> +        /*
> +         * Changing base registers with the ITS enabled is UNPREDICTABLE,
> +         * we choose to ignore it, but warn.
> +         */
> +        if ( its->enabled )
> +        {
> +            spin_unlock(&its->its_lock);
> +            gdprintk(XENLOG_INFO, "ITS: tried to change BASER with the ITS enabled.\n");
> +            return 1;
> +        }
> +
> +        reg = its->baser_coll;
> +        vgic_reg64_update(&reg, r, info);
> +        /* No indirect tables for the collection table. */
> +        reg &= ~(GITS_BASER_RO_MASK | GITS_BASER_INDIRECT);
> +        reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> +        reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
> +        sanitize_its_base_reg(&reg);
> +
> +        if ( reg & GITS_VALID_BIT )
> +            its->max_collections = its_baser_nr_entries(reg);
> +        else
> +            its->max_collections = 0;
> +        its->baser_coll = reg;
> +        spin_unlock(&its->its_lock);
> +        return 1;
> +    case VRANGE64(GITS_BASER2, GITS_BASER7):
> +        goto write_ignore_64;
> +    case 0x0140 ... 0xbffc:
> +        goto write_reserved;
> +    case 0xc000 ... 0xffcc:
> +        goto write_impl_defined;
> +    case 0xffd0 ... 0xffe4:             /* IMPDEF identification registers */
> +        goto write_impl_defined;
> +    case VREG32(GITS_PIDR2):
> +        goto write_ignore_32;
> +    case 0xffec ... 0xfffc:             /* IMPDEF identification registers */
> +        goto write_impl_defined;
> +    default:
> +        gdprintk(XENLOG_G_WARNING, "ITS: unhandled ITS register 0x%lx\n",
> +                 info->gpa & 0xffff);
> +        return 0;
> +    }
> +
> +    return 1;
> +
> +write_ignore_64:
> +    if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +    return 1;
> +
> +write_ignore_32:
> +    if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +    return 1;
> +
> +write_impl_defined:
> +    printk(XENLOG_G_DEBUG
> +           "%pv: vGITS: WI on implementation defined register offset %#04lx\n",
> +           v, info->gpa & 0xffff);
> +    return 1;
> +
> +write_reserved:
> +    printk(XENLOG_G_DEBUG
> +           "%pv: vGITS: WI on implementation defined register offset %#04lx\n",
> +           v, info->gpa & 0xffff);
> +    return 1;
> +
> +bad_width:
> +    printk(XENLOG_G_ERR "vGITS: bad write width %d r%d offset %#08lx\n",
> +           info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
> +
> +    domain_crash_synchronous();
> +
> +    return 0;
> +}
> +
> +static const struct mmio_handler_ops vgic_its_mmio_handler = {
> +    .read  = vgic_v3_its_mmio_read,
> +    .write = vgic_v3_its_mmio_write,
> +};
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 09c7117..ea574c4 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -35,6 +35,7 @@
>  #define GITS_BASER5                     0x128
>  #define GITS_BASER6                     0x130
>  #define GITS_BASER7                     0x138
> +#define GITS_PIDR2                      GICR_PIDR2
>
>  /* Register bits */
>  #define GITS_VALID_BIT                  BIT(63)
> @@ -57,6 +58,7 @@
>  #define GITS_TYPER_ITT_SIZE_MASK        (0xfUL << GITS_TYPER_ITT_SIZE_SHIFT)
>  #define GITS_TYPER_ITT_SIZE(r)          ((((r) & GITS_TYPER_ITT_SIZE_MASK) >> \
>                                                   GITS_TYPER_ITT_SIZE_SHIFT) + 1)
> +#define GITS_TYPER_PHYSICAL             (1U << 0)
>
>  #define GITS_BASER_INDIRECT             BIT(62)
>  #define GITS_BASER_INNER_CACHEABILITY_SHIFT        59
> @@ -76,6 +78,7 @@
>                          (((reg >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
>  #define GITS_BASER_SHAREABILITY_SHIFT   10
>  #define GITS_BASER_PAGE_SIZE_SHIFT      8
> +#define GITS_BASER_SIZE_MASK            0xff
>  #define GITS_BASER_SHAREABILITY_MASK   (0x3ULL << GITS_BASER_SHAREABILITY_SHIFT)
>  #define GITS_BASER_OUTER_CACHEABILITY_MASK   (0x7ULL << GITS_BASER_OUTER_CACHEABILITY_SHIFT)
>  #define GITS_BASER_INNER_CACHEABILITY_MASK   (0x7ULL << GITS_BASER_INNER_CACHEABILITY_SHIFT)
>
Andre Przywara April 11, 2017, 3:49 p.m. UTC | #2
Hi,

On 09/04/17 21:16, Julien Grall wrote:
> Hi Andre,
> 
> On 04/07/2017 06:32 PM, Andre Przywara wrote:
>> Emulate the memory mapped ITS registers and provide a stub to introduce
>> the ITS command handling framework (but without actually emulating any
>> commands at this time).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/vgic-v3-its.c       | 512
>> +++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/gic_v3_its.h |   3 +
>>  2 files changed, 515 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 065ffe2..a171a3b 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -67,6 +67,9 @@ struct vits_itte
>>      uint16_t pad;
>>  };
>>
>> +#define GITS_BASER_RO_MASK       (GITS_BASER_TYPE_MASK | \
>> +                                  (31UL << GITS_BASER_ENTRY_SIZE_SHIFT))
>> +
>>  int vgic_v3_its_init_domain(struct domain *d)
>>  {
>>      spin_lock_init(&d->arch.vgic.its_devices_lock);
>> @@ -80,6 +83,515 @@ void vgic_v3_its_free_domain(struct domain *d)
>>      ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
>>  }
>>
>> +/**************************************
>> + * Functions that handle ITS commands *
>> + **************************************/
>> +
>> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
>> +                                   unsigned int shift, unsigned int
>> size)
>> +{
>> +    return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
>> +}
>> +
>> +#define its_cmd_get_command(cmd)        its_cmd_mask_field(cmd, 0, 
>> 0,  8)
>> +#define its_cmd_get_deviceid(cmd)       its_cmd_mask_field(cmd, 0,
>> 32, 32)
>> +#define its_cmd_get_size(cmd)           its_cmd_mask_field(cmd, 1, 
>> 0,  5)
>> +#define its_cmd_get_id(cmd)             its_cmd_mask_field(cmd, 1, 
>> 0, 32)
>> +#define its_cmd_get_physical_id(cmd)    its_cmd_mask_field(cmd, 1,
>> 32, 32)
>> +#define its_cmd_get_collection(cmd)     its_cmd_mask_field(cmd, 2, 
>> 0, 16)
>> +#define its_cmd_get_target_addr(cmd)    its_cmd_mask_field(cmd, 2,
>> 16, 32)
>> +#define its_cmd_get_validbit(cmd)       its_cmd_mask_field(cmd, 2,
>> 63,  1)
>> +#define its_cmd_get_ittaddr(cmd)        (its_cmd_mask_field(cmd, 2,
>> 8, 44) << 8)
>> +
>> +#define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
>> +
>> +/*
>> + * Requires the vcmd_lock to be held.
>> + * TODO: Investigate whether we can be smarter here and don't need to
>> hold
>> + * the lock all of the time.
>> + */
>> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
>> +{
>> +    paddr_t addr = its->cbaser & GENMASK(51, 12);
>> +    uint64_t command[4];
>> +    uint64_t creadr = its->creadr;
>> +
>> +    ASSERT(spin_is_locked(&its->vcmd_lock));
>> +
>> +    if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +        return -1;
>> +
>> +    while ( creadr != its->cwriter )
>> +    {
>> +        int ret;
>> +
>> +        ret = vgic_access_guest_memory(d, addr + creadr,
>> +                                       command, sizeof(command), false);
>> +        if ( ret )
>> +            return ret;
>> +
>> +        switch ( its_cmd_get_command(command) )
>> +        {
>> +        case GITS_CMD_SYNC:
>> +            /* We handle ITS commands synchronously, so we ignore
>> SYNC. */
>> +            break;
>> +        default:
>> +            gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
>> +                     its_cmd_get_command(command));
>> +            break;
>> +        }
>> +
>> +        creadr += ITS_CMD_SIZE;
>> +        if ( creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +            creadr = 0;
>> +        its->creadr = creadr;   /* allow the guest to see the
>> progress */
> 
> I hope you know that the compiler can decide to drop the temporary
> variable for optimization? ;) So it may decide to write-back everytime
> in its->creadr.

I don't think it can do it, because creadr is different from its->creadr
here (on purpose!). So doing this optimization would violate the program
semantic (because the end-of-buffer value would never be visible).

But just to be sure I replaced this check with a modulo operation over
the ITS_CMD_BUFFER_SIZE.
Not sure everyone likes *that* now, though ;-)

Cheers,
Andre.
diff mbox

Patch

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 065ffe2..a171a3b 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -67,6 +67,9 @@  struct vits_itte
     uint16_t pad;
 };
 
+#define GITS_BASER_RO_MASK       (GITS_BASER_TYPE_MASK | \
+                                  (31UL << GITS_BASER_ENTRY_SIZE_SHIFT))
+
 int vgic_v3_its_init_domain(struct domain *d)
 {
     spin_lock_init(&d->arch.vgic.its_devices_lock);
@@ -80,6 +83,515 @@  void vgic_v3_its_free_domain(struct domain *d)
     ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
 }
 
+/**************************************
+ * Functions that handle ITS commands *
+ **************************************/
+
+static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
+                                   unsigned int shift, unsigned int size)
+{
+    return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
+}
+
+#define its_cmd_get_command(cmd)        its_cmd_mask_field(cmd, 0,  0,  8)
+#define its_cmd_get_deviceid(cmd)       its_cmd_mask_field(cmd, 0, 32, 32)
+#define its_cmd_get_size(cmd)           its_cmd_mask_field(cmd, 1,  0,  5)
+#define its_cmd_get_id(cmd)             its_cmd_mask_field(cmd, 1,  0, 32)
+#define its_cmd_get_physical_id(cmd)    its_cmd_mask_field(cmd, 1, 32, 32)
+#define its_cmd_get_collection(cmd)     its_cmd_mask_field(cmd, 2,  0, 16)
+#define its_cmd_get_target_addr(cmd)    its_cmd_mask_field(cmd, 2, 16, 32)
+#define its_cmd_get_validbit(cmd)       its_cmd_mask_field(cmd, 2, 63,  1)
+#define its_cmd_get_ittaddr(cmd)        (its_cmd_mask_field(cmd, 2, 8, 44) << 8)
+
+#define ITS_CMD_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
+
+/*
+ * Requires the vcmd_lock to be held.
+ * TODO: Investigate whether we can be smarter here and don't need to hold
+ * the lock all of the time.
+ */
+static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
+{
+    paddr_t addr = its->cbaser & GENMASK(51, 12);
+    uint64_t command[4];
+    uint64_t creadr = its->creadr;
+
+    ASSERT(spin_is_locked(&its->vcmd_lock));
+
+    if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
+        return -1;
+
+    while ( creadr != its->cwriter )
+    {
+        int ret;
+
+        ret = vgic_access_guest_memory(d, addr + creadr,
+                                       command, sizeof(command), false);
+        if ( ret )
+            return ret;
+
+        switch ( its_cmd_get_command(command) )
+        {
+        case GITS_CMD_SYNC:
+            /* We handle ITS commands synchronously, so we ignore SYNC. */
+            break;
+        default:
+            gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
+                     its_cmd_get_command(command));
+            break;
+        }
+
+        creadr += ITS_CMD_SIZE;
+        if ( creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
+            creadr = 0;
+        its->creadr = creadr;   /* allow the guest to see the progress */
+
+        if ( ret )
+            gdprintk(XENLOG_WARNING,
+                     "ITS: ITS command error %d while handling command %lu\n",
+                     ret, its_cmd_get_command(command));
+    }
+
+    return 0;
+}
+
+/*****************************
+ * ITS registers read access *
+ *****************************/
+
+/* Identifying as an ARM IP, using "X" as the product ID. */
+#define GITS_IIDR_VALUE                 0x5800034c
+
+static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
+                                 register_t *r, void *priv)
+{
+    struct virt_its *its = priv;
+    uint64_t reg;
+
+    switch ( info->gpa & 0xffff )
+    {
+    case VREG32(GITS_CTLR):
+    {
+        /*
+         * We try to avoid waiting for the command queue lock and report
+         * non-quiescent if that lock is already taken.
+         */
+        bool have_cmd_lock;
+
+        if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+        have_cmd_lock = spin_trylock(&its->vcmd_lock);
+        spin_lock(&its->its_lock);
+        if ( its->enabled )
+            reg = GITS_CTLR_ENABLE;
+        else
+            reg = 0;
+
+        if ( have_cmd_lock && its->cwriter == its->creadr )
+            reg |= GITS_CTLR_QUIESCENT;
+
+        spin_unlock(&its->its_lock);
+        if ( have_cmd_lock )
+            spin_unlock(&its->vcmd_lock);
+
+        *r = vgic_reg32_extract(reg, info);
+        break;
+    }
+    case VREG32(GITS_IIDR):
+        if ( info->dabt.size != DABT_WORD ) goto bad_width;
+        *r = vgic_reg32_extract(GITS_IIDR_VALUE, info);
+        break;
+    case VREG64(GITS_TYPER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        reg = GITS_TYPER_PHYSICAL;
+        reg |= (sizeof(struct vits_itte) - 1) << GITS_TYPER_ITT_SIZE_SHIFT;
+        reg |= (its->intid_bits - 1) << GITS_TYPER_IDBITS_SHIFT;
+        reg |= (its->devid_bits - 1) << GITS_TYPER_DEVIDS_SHIFT;
+        *r = vgic_reg64_extract(reg, info);
+        break;
+    case 0x0018 ... 0x001c:
+        goto read_reserved;
+    case 0x0020 ... 0x003c:
+        goto read_impl_defined;
+    case 0x0040 ... 0x007c:
+        goto read_reserved;
+    case VREG64(GITS_CBASER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+        spin_lock(&its->its_lock);
+        *r = vgic_reg64_extract(its->cbaser, info);
+        spin_unlock(&its->its_lock);
+        break;
+    case VREG64(GITS_CWRITER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        reg = its->cwriter;
+        *r = vgic_reg64_extract(reg, info);
+        break;
+    case VREG64(GITS_CREADR):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        reg = its->creadr;
+        *r = vgic_reg64_extract(reg, info);
+        break;
+    case 0x0098 ... 0x00fc:
+        goto read_reserved;
+    case VREG64(GITS_BASER0):           /* device table */
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+        spin_lock(&its->its_lock);
+        *r = vgic_reg64_extract(its->baser_dev, info);
+        spin_unlock(&its->its_lock);
+        break;
+    case VREG64(GITS_BASER1):           /* collection table */
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+        spin_lock(&its->its_lock);
+        *r = vgic_reg64_extract(its->baser_coll, info);
+        spin_unlock(&its->its_lock);
+        break;
+    case VRANGE64(GITS_BASER2, GITS_BASER7):
+        goto read_as_zero_64;
+    case 0x0140 ... 0xbffc:
+        goto read_reserved;
+    case 0xc000 ... 0xffcc:
+        goto read_impl_defined;
+    case 0xffd0 ... 0xffe4:
+        goto read_as_zero_64;
+    case VREG32(GITS_PIDR2):
+        if ( info->dabt.size != DABT_WORD ) goto bad_width;
+        *r = vgic_reg32_extract(GIC_PIDR2_ARCH_GICv3, info);
+        break;
+    case 0xffec ... 0xfffc:
+        goto read_as_zero_64;
+    }
+
+    return 1;
+
+read_as_zero_64:
+    if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+    *r = 0;
+
+    return 1;
+
+read_impl_defined:
+    printk(XENLOG_G_DEBUG
+           "%pv: vGITS: RAZ on implementation defined register offset %#04lx\n",
+           v, info->gpa & 0xffff);
+    *r = 0;
+    return 1;
+
+read_reserved:
+    printk(XENLOG_G_DEBUG
+           "%pv: vGITS: RAZ on reserved register offset %#04lx\n",
+           v, info->gpa & 0xffff);
+    *r = 0;
+    return 1;
+
+bad_width:
+    printk(XENLOG_G_ERR "vGIIS: bad read width %d r%d offset %#04lx\n",
+           info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
+    domain_crash_synchronous();
+
+    return 0;
+}
+
+/******************************
+ * ITS registers write access *
+ ******************************/
+
+static unsigned int its_baser_table_size(uint64_t baser)
+{
+    unsigned int ret, page_size[4] = {SZ_4K, SZ_16K, SZ_64K, SZ_64K};
+
+    ret = page_size[(baser >> GITS_BASER_PAGE_SIZE_SHIFT) & 3];
+
+    return ret * ((baser & GITS_BASER_SIZE_MASK) + 1);
+}
+
+static unsigned int its_baser_nr_entries(uint64_t baser)
+{
+    int entry_size = GITS_BASER_ENTRY_SIZE(baser);
+
+    return its_baser_table_size(baser) / entry_size;
+}
+
+/* Must be called with the ITS lock held. */
+static bool vgic_v3_verify_its_status(struct virt_its *its, bool status)
+{
+    ASSERT(spin_is_locked(&its->its_lock));
+
+    if ( !status )
+        return false;
+
+    if ( !(its->cbaser & GITS_VALID_BIT) ||
+         !(its->baser_dev & GITS_VALID_BIT) ||
+         !(its->baser_coll & GITS_VALID_BIT) )
+    {
+        printk(XENLOG_G_WARNING "d%d tried to enable ITS without having the tables configured.\n",
+               its->d->domain_id);
+        return false;
+    }
+
+    return true;
+}
+
+static void sanitize_its_base_reg(uint64_t *reg)
+{
+    uint64_t r = *reg;
+
+    /* Avoid outer shareable. */
+    switch ( (r >> GITS_BASER_SHAREABILITY_SHIFT) & 0x03 )
+    {
+    case GIC_BASER_OuterShareable:
+        r = r & ~GITS_BASER_SHAREABILITY_MASK;
+        r |= GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
+        break;
+    default:
+        break;
+    }
+
+    /* Avoid any inner non-cacheable mapping. */
+    switch ( (r >> GITS_BASER_INNER_CACHEABILITY_SHIFT) & 0x07 )
+    {
+    case GIC_BASER_CACHE_nCnB:
+    case GIC_BASER_CACHE_nC:
+        r = r & ~GITS_BASER_INNER_CACHEABILITY_MASK;
+        r |= GIC_BASER_CACHE_RaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
+        break;
+    default:
+        break;
+    }
+
+    /* Only allow non-cacheable or same-as-inner. */
+    switch ( (r >> GITS_BASER_OUTER_CACHEABILITY_SHIFT) & 0x07 )
+    {
+    case GIC_BASER_CACHE_SameAsInner:
+    case GIC_BASER_CACHE_nC:
+        break;
+    default:
+        r = r & ~GITS_BASER_OUTER_CACHEABILITY_MASK;
+        r |= GIC_BASER_CACHE_nC << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
+        break;
+    }
+
+    *reg = r;
+}
+
+static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info,
+                                  register_t r, void *priv)
+{
+    struct domain *d = v->domain;
+    struct virt_its *its = priv;
+    uint64_t reg;
+    uint32_t reg32;
+
+    switch ( info->gpa & 0xffff )
+    {
+    case VREG32(GITS_CTLR):
+    {
+        uint32_t ctlr;
+
+        if ( info->dabt.size != DABT_WORD ) goto bad_width;
+
+        /*
+         * We need to take the vcmd_lock to prevent a guest from disabling
+         * the ITS while commands are still processed.
+         */
+        spin_lock(&its->vcmd_lock);
+        spin_lock(&its->its_lock);
+        ctlr = its->enabled ? GITS_CTLR_ENABLE : 0;
+        reg32 = ctlr;
+        vgic_reg32_update(&reg32, r, info);
+
+        if ( ctlr ^ reg32 )
+            its->enabled = vgic_v3_verify_its_status(its,
+                                                     reg32 & GITS_CTLR_ENABLE);
+        spin_unlock(&its->its_lock);
+        spin_unlock(&its->vcmd_lock);
+        return 1;
+    }
+
+    case VREG32(GITS_IIDR):
+        goto write_ignore_32;
+    case VREG32(GITS_TYPER):
+        goto write_ignore_32;
+    case 0x0018 ... 0x001c:
+        goto write_reserved;
+    case 0x0020 ... 0x003c:
+        goto write_impl_defined;
+    case 0x0040 ... 0x007c:
+        goto write_reserved;
+    case VREG64(GITS_CBASER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        spin_lock(&its->vcmd_lock);
+        spin_lock(&its->its_lock);
+        /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
+        if ( its->enabled )
+        {
+            spin_unlock(&its->its_lock);
+            spin_unlock(&its->vcmd_lock);
+            gdprintk(XENLOG_WARNING,
+                     "ITS: tried to change CBASER with the ITS enabled.\n");
+            return 1;
+        }
+
+        reg = its->cbaser;
+        vgic_reg64_update(&reg, r, info);
+        sanitize_its_base_reg(&reg);
+
+        its->cbaser = reg;
+        its->creadr = 0;
+        spin_unlock(&its->its_lock);
+        spin_unlock(&its->vcmd_lock);
+
+        return 1;
+
+    case VREG64(GITS_CWRITER):
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        spin_lock(&its->vcmd_lock);
+        reg = its->cwriter & 0xfffe0;
+        vgic_reg64_update(&reg, r, info);
+        its->cwriter = reg & 0xfffe0;
+
+        if ( its->enabled )
+        {
+            int ret = vgic_its_handle_cmds(d, its);
+
+            if ( ret )
+                printk(XENLOG_G_WARNING "error handling ITS commands\n");
+        }
+        spin_unlock(&its->vcmd_lock);
+
+        return 1;
+
+    case VREG64(GITS_CREADR):
+        goto write_ignore_64;
+
+    case 0x0098 ... 0x00fc:
+        goto write_reserved;
+    case VREG64(GITS_BASER0):           /* device table */
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        spin_lock(&its->its_lock);
+
+        /*
+         * Changing base registers with the ITS enabled is UNPREDICTABLE,
+         * we choose to ignore it, but warn.
+         */
+        if ( its->enabled )
+        {
+            spin_unlock(&its->its_lock);
+            gdprintk(XENLOG_WARNING, "ITS: tried to change BASER with the ITS enabled.\n");
+
+            return 1;
+        }
+
+        reg = its->baser_dev;
+        vgic_reg64_update(&reg, r, info);
+
+        /* We don't support indirect tables for now. */
+        reg &= ~(GITS_BASER_RO_MASK | GITS_BASER_INDIRECT);
+        reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+        reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
+        sanitize_its_base_reg(&reg);
+
+        if ( reg & GITS_VALID_BIT )
+        {
+            its->max_devices = its_baser_nr_entries(reg);
+            if ( its->max_devices > BIT(its->devid_bits) )
+                its->max_devices = BIT(its->devid_bits);
+        }
+        else
+            its->max_devices = 0;
+
+        its->baser_dev = reg;
+        spin_unlock(&its->its_lock);
+        return 1;
+    case VREG64(GITS_BASER1):           /* collection table */
+        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+
+        spin_lock(&its->its_lock);
+        /*
+         * Changing base registers with the ITS enabled is UNPREDICTABLE,
+         * we choose to ignore it, but warn.
+         */
+        if ( its->enabled )
+        {
+            spin_unlock(&its->its_lock);
+            gdprintk(XENLOG_INFO, "ITS: tried to change BASER with the ITS enabled.\n");
+            return 1;
+        }
+
+        reg = its->baser_coll;
+        vgic_reg64_update(&reg, r, info);
+        /* No indirect tables for the collection table. */
+        reg &= ~(GITS_BASER_RO_MASK | GITS_BASER_INDIRECT);
+        reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
+        reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
+        sanitize_its_base_reg(&reg);
+
+        if ( reg & GITS_VALID_BIT )
+            its->max_collections = its_baser_nr_entries(reg);
+        else
+            its->max_collections = 0;
+        its->baser_coll = reg;
+        spin_unlock(&its->its_lock);
+        return 1;
+    case VRANGE64(GITS_BASER2, GITS_BASER7):
+        goto write_ignore_64;
+    case 0x0140 ... 0xbffc:
+        goto write_reserved;
+    case 0xc000 ... 0xffcc:
+        goto write_impl_defined;
+    case 0xffd0 ... 0xffe4:             /* IMPDEF identification registers */
+        goto write_impl_defined;
+    case VREG32(GITS_PIDR2):
+        goto write_ignore_32;
+    case 0xffec ... 0xfffc:             /* IMPDEF identification registers */
+        goto write_impl_defined;
+    default:
+        gdprintk(XENLOG_G_WARNING, "ITS: unhandled ITS register 0x%lx\n",
+                 info->gpa & 0xffff);
+        return 0;
+    }
+
+    return 1;
+
+write_ignore_64:
+    if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
+    return 1;
+
+write_ignore_32:
+    if ( info->dabt.size != DABT_WORD ) goto bad_width;
+    return 1;
+
+write_impl_defined:
+    printk(XENLOG_G_DEBUG
+           "%pv: vGITS: WI on implementation defined register offset %#04lx\n",
+           v, info->gpa & 0xffff);
+    return 1;
+
+write_reserved:
+    printk(XENLOG_G_DEBUG
+           "%pv: vGITS: WI on implementation defined register offset %#04lx\n",
+           v, info->gpa & 0xffff);
+    return 1;
+
+bad_width:
+    printk(XENLOG_G_ERR "vGITS: bad write width %d r%d offset %#08lx\n",
+           info->dabt.size, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
+
+    domain_crash_synchronous();
+
+    return 0;
+}
+
+static const struct mmio_handler_ops vgic_its_mmio_handler = {
+    .read  = vgic_v3_its_mmio_read,
+    .write = vgic_v3_its_mmio_write,
+};
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 09c7117..ea574c4 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -35,6 +35,7 @@ 
 #define GITS_BASER5                     0x128
 #define GITS_BASER6                     0x130
 #define GITS_BASER7                     0x138
+#define GITS_PIDR2                      GICR_PIDR2
 
 /* Register bits */
 #define GITS_VALID_BIT                  BIT(63)
@@ -57,6 +58,7 @@ 
 #define GITS_TYPER_ITT_SIZE_MASK        (0xfUL << GITS_TYPER_ITT_SIZE_SHIFT)
 #define GITS_TYPER_ITT_SIZE(r)          ((((r) & GITS_TYPER_ITT_SIZE_MASK) >> \
                                                  GITS_TYPER_ITT_SIZE_SHIFT) + 1)
+#define GITS_TYPER_PHYSICAL             (1U << 0)
 
 #define GITS_BASER_INDIRECT             BIT(62)
 #define GITS_BASER_INNER_CACHEABILITY_SHIFT        59
@@ -76,6 +78,7 @@ 
                         (((reg >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
 #define GITS_BASER_SHAREABILITY_SHIFT   10
 #define GITS_BASER_PAGE_SIZE_SHIFT      8
+#define GITS_BASER_SIZE_MASK            0xff
 #define GITS_BASER_SHAREABILITY_MASK   (0x3ULL << GITS_BASER_SHAREABILITY_SHIFT)
 #define GITS_BASER_OUTER_CACHEABILITY_MASK   (0x7ULL << GITS_BASER_OUTER_CACHEABILITY_SHIFT)
 #define GITS_BASER_INNER_CACHEABILITY_MASK   (0x7ULL << GITS_BASER_INNER_CACHEABILITY_SHIFT)