Re: [Xen-devel] [RFC PATCH 12/24] ARM: vGICv3: introduce basic ITS emulation bits

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 28/09/16 19:24, Andre Przywara wrote:
> Create a new file to hold the emulation code for the ITS widget.
> For now we 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).
The ITS is a complex piece so I think it would be good to describe more 
in the commit message how this will work. Also a documentation in the 
tree would be very good to help understanding the code.
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/vgic-its.c           | 378 ++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c            |   9 -
>  xen/include/asm-arm/gic_v3_defs.h |  19 ++
>  4 files changed, 398 insertions(+), 9 deletions(-)
>  create mode 100644 xen/arch/arm/vgic-its.c
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index c2c4daa..cb0201f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -44,6 +44,7 @@ obj-y += traps.o
>  obj-y += vgic.o
>  obj-y += vgic-v2.o
>  obj-$(CONFIG_ARM_64) += vgic-v3.o
> +obj-$(CONFIG_HAS_ITS) += vgic-its.o
>  obj-y += vm_event.o
>  obj-y += vtimer.o
>  obj-y += vpsci.o
> diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c
> new file mode 100644
> index 0000000..875b992
> --- /dev/null
> +++ b/xen/arch/arm/vgic-its.c
> @@ -0,0 +1,378 @@
> +/*
> + * xen/arch/arm/vgic-its.c
> + *
> + * ARM Interrupt Translation Service (ITS) emulation
> + *
> + * Andre Przywara <andre.przywara@xxxxxxx>
> + * Copyright (c) 2016 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include <xen/bitops.h>
> +#include <xen/config.h>
> +#include <xen/domain_page.h>
> +#include <xen/lib.h>
> +#include <xen/init.h>
> +#include <xen/softirq.h>
> +#include <xen/irq.h>
> +#include <xen/sched.h>
> +#include <xen/sizes.h>
> +#include <asm/current.h>
> +#include <asm/mmio.h>
> +#include <asm/gic_v3_defs.h>
> +#include <asm/gic-its.h>
> +#include <asm/vgic.h>
> +#include <asm/vgic-emul.h>
> +
> +/* Data structure to describe a virtual ITS */
> +struct virt_its {
> +    struct domain *d;
> +    struct host_its *hw_its;
> +    spinlock_t vcmd_lock;       /* protects the virtual command buffer */
> +    uint64_t cbaser;
> +    uint64_t *cmdbuf;
> +    int cwriter;
> +    int creadr;
CWRITER and CREADR are registers so they need to be described in term of 
number of bits. Also, while the top word of CREADR/CWRITER is RES0. I 
would much prefer to see uint64_t rather than uint32_t as this is the 
real size of the register.
> +    spinlock_t its_lock;        /* protects the collection and device tables */
> +    uint64_t baser0, baser1;
Please describe what contains baser0 and baser1. If I understand 
correctly the code, baser0 will be store Device information whilst 
baser1 the collection.
> +    uint16_t *coll_table;
What is the layout of the device table?

> +    int max_collections;
unsigned int

> +    uint64_t *dev_table;
What is the layout of the device table?

> +    int max_devices;
unsigned int.

> +    bool enabled;
> +};
> +
> +/* An Interrupt Translation Table Entry: this is indexed by a
Coding style:

/*
 * Foo

> + * DeviceID/EventID pair and is located in guest memory.
> + */
> +struct vits_itte
> +{
> +    uint64_t hlpi:24;
> +    uint64_t vlpi:24;
> +    uint64_t collection:16;
> +};
> +
> +/**************************************
> + * Functions that handle ITS commands *
> + **************************************/
> +
> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd,
Please make this function inline.

> +                                   int word, int shift, int size)
unsigned for all those parameters.

> +{
> +    return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
It is probably better to use BIT_ULL (see my explanation on previous 
patches).
> +}
> +
> +#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_BUFFER_SIZE(baser)      ((((baser) & 0xff) + 1) << 12)
> +
> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its,
> +                                uint32_t writer)
uint64_t here.

> +{
> +    uint64_t *cmdptr;
> +
> +    if ( !its->cmdbuf )
> +        return -1;
> +
> +    if ( writer >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +        return -1;
You return an error value but the caller does not check it. Should not 
the caller do a different action when the return -1? If not, it should 
be documented.
> +
> +    spin_lock(&its->vcmd_lock);
I am quite concerned about this locking.

> +
> +    while ( its->creadr != writer )
> +    {
> +        cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf));
> +        switch (its_cmd_get_command(cmdptr))
Coding style: switch ( ... )

> +        {
> +        case GITS_CMD_SYNC:
> +            /* We handle ITS commands synchronously, so we ignore SYNC. */
> +           break;
The indentation is wrong.

> +        default:
> +            gdprintk(XENLOG_G_WARNING, "ITS: unhandled ITS command %ld\n",
gdprintk will happen XENLOG_GUEST, so you can use XENLOG_WARNING here.

Also s/%ld/%lu/

> +                   its_cmd_get_command(cmdptr));
Should not we report the error to the default, or crash it? We tend to 
do the latter on Xen for constrained unpredictable behavior.
> +            break;
> +        }
> +
> +        its->creadr += ITS_CMD_SIZE;
> +        if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
> +            its->creadr = 0;
> +    }
> +    its->cwriter = writer;
I think its->cwriter should be updated before the loop. So another vCPU 
could read the correct CWRITER whilst this vCPU is executing the commands.
> +
> +    spin_unlock(&its->vcmd_lock);
> +
> +    return 0;
> +}
> +
> +/*****************************
> + * ITS registers read access *
> + *****************************/
> +
> +/* The physical address is encoded slightly differently depending on
Coding style:

/*
 * foo

> + * the used page size: the highest four bits are stored in the lowest
> + * four bits of the field for 64K pages.
> + */
> +static paddr_t get_baser_phys_addr(uint64_t reg)
> +{
> +    if ( reg & BIT(9) )
Please document what is bit 9.

> +        return (reg & GENMASK(47, 16)) | ((reg & GENMASK(15, 12)) << 36);
> +    else
> +        return reg & GENMASK(47, 12);
> +}
> +
> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
> +                                 register_t *r, void *priv)
> +{
> +    struct virt_its *its = priv;
> +
> +    switch ( info->gpa & 0xffff )
> +    {
> +    case VREG32(GITS_CTLR):
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +        *r = vgic_reg32_extract(its->enabled | BIT(31), info);
Please use a define for BIT(31). Also, technically the ITS is not 
quiescent when command are executed (GITS_CTLR could be read from 
another vCPU).
> +       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 ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(0x1eff1, info);
Please document the value and add defines. Vijay's mentioned about the 
number of device IDs, but the number of collection likely needs to be 
dynamic as it depends on the number of vCPUs.
> +        break;
> +    case VREG64(GITS_CBASER):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(its->cbaser, info);
> +        break;
> +    case VREG64(GITS_CWRITER):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(its->cwriter, info);
> +        break;
> +    case VREG64(GITS_CREADR):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(its->creadr, info);
> +        break;
> +    case VREG64(GITS_BASER0):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(its->baser0, info);
> +        break;
> +    case VREG64(GITS_BASER1):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(its->baser1, info);
> +        break;
> +    case VRANGE64(GITS_BASER2, GITS_BASER7):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +        *r = vgic_reg64_extract(0, info);
Please introduce a label read_as_zero_64 at the end and do the 
implementation of RAZ there. It will acts as a documentation too (see an 
example in vgic-v3.c).
Also, vgic_reg64_extract(0, info) will ... always return 0. So you can 
optimize it ;).
> +        break;
> +    case VREG32(GICD_PIDR2):
This feels odd to use GICD_PIDR2 here. Please define GITS_PIDR2 to avoid 
any confusion.
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +        *r = vgic_reg32_extract(GICV3_GICD_PIDR2, info);
Ditto.

> +        break;
Please add all the registers even implementation defined and reserved 
one. Ignoring registers without any warning is usually a bad idea as it 
makes very difficult to debug it. You can look at vgic-v3.c for an example.

> +    }
> +
> +    return 1;
> +
> +bad_width:
Please print an error here (see vgic-v3.c).

> +    domain_crash_synchronous();
> +
> +    return 0;
> +}
> +
> +/******************************
> + * ITS registers write access *
> + ******************************/
> +
> +static int its_baser_table_size(uint64_t baser)
unsigned int for the return and the function would probably benefit to 
be inlined.
> +{
> +    int page_size = 0;
unsigned int.

> +
> +    switch ( (baser >> 8) & 3 )
Please define 8 and 3.

> +    {
> +    case 0: page_size = SZ_4K; break;
> +    case 1: page_size = SZ_16K; break;
> +    case 2:
> +    case 3: page_size = SZ_64K; break;
> +    }
It looks like to me that the switch could be turned into an array:

unsigned page_size[] = {SZ_4K, SZ_16K, SZ_64K, SZ_64K};

This woudl make the code simpler.

> +
> +    return page_size * ((baser & GENMASK(7, 0)) + 1);
> +}
> +
> +static int its_baser_nr_entries(uint64_t baser)
unsigned int for the return and the function would probably benefit to 
be inlined.
> +{
> +    int entry_size = ((baser & GENMASK(52, 48)) >> 48) + 1;
unsigned int for the type. Also please use a define for 48.

> +
> +    return its_baser_table_size(baser) / entry_size;
> +}
> +
> +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 ctlr;
ctlr could be defined in the case...

> +
> +    switch ( info->gpa & 0xffff )
> +    {
> +    case VREG32(GITS_CTLR):
here. I tend to prefer to restrict the scope whenever it is possible.

> +        ctlr = its->enabled ? GITS_CTLR_ENABLE : 0;
> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
> +       vgic_reg32_update(&ctlr, r, info);
> +       its->enabled = ctlr & GITS_CTLR_ENABLE;
> +       /* TODO: trigger something ... */
The indentation is wrong.

> +        return 1;
> +    case VREG32(GITS_IIDR):
> +        goto write_ignore_32;
> +    case VREG32(GITS_TYPER):
> +        goto write_ignore_32;
> +    case VREG64(GITS_CBASER):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access.

> +
> +        /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
> +        if ( its->enabled )
> +            return 1;
> +
There may have concurrent access to GITS_BASER, so you want to have some 
lock here.
> +        reg = its->cbaser;
> +        vgic_reg64_update(&reg, r, info);
> +        /* TODO: sanitise! */
Please fix this todo as soon as possible.

> +        its->cbaser = reg;
Also, I am not sure to understand why you need a temporary variable. 
Whilst you could directly update its->cbaser:
vgic_regs64_update(&its->cbaser, r, info);

Also, from the spec (8.19.2 in ARM IHI 0069C), GITS_CREADR (i.e 
its->creadr) should be reset to 0.
> +
> +        if ( reg & BIT(63) )
Please define bit 63.

> +        {
> +            its->cmdbuf = map_guest_pages(d, reg & GENMASK(51, 12), 1);
> +        }
> +        else
> +        {
> +            unmap_guest_pages(its->cmdbuf, 1);
> +            its->cmdbuf = NULL;
> +        }
> +
> +       return 1;
> +    case VREG64(GITS_CWRITER):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access.

> +        reg = its->cwriter;
> +        vgic_reg64_update(&reg, r, info);
vgic_its_handle_cmds expect CWRITER to the bit 0 (Retry) masked and bit 
[32:20], [4:1] should be RES0 (i.e masked).
> +        vgic_its_handle_cmds(d, its, reg);
Should not you check the return value?

> +        return 1;
> +    case VREG64(GITS_CREADR):
> +        goto write_ignore_64;
> +    case VREG64(GITS_BASER0):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +
> +        /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
> +        if ( its->enabled )
> +            return 1;
> +
> +        reg = its->baser0;
> +        vgic_reg64_update(&reg, r, info);
> +
> +        reg &= ~GITS_BASER_RO_MASK;
> +        reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
Where does this sizeof(uint64_t) come from?

> +        reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
> +        /* TODO: sanitise! */
> +        /* TODO: locking(?) */
Yes, some locking is needed.

> +
> +        if ( reg & GITS_BASER_VALID )
> +        {
> +            its->dev_table = map_guest_pages(d,
> +                                             get_baser_phys_addr(reg),
> +                                             its_baser_table_size(reg) >> 
> PAGE_SHIFT);
> +            its->max_devices = its_baser_nr_entries(reg);
> +            memset(its->dev_table, 0, its->max_devices * sizeof(uint64_t));
I am not sure to understand why we need to memset and what the value 
corresponds to.
> +        }
> +        else
> +        {
> +            unmap_guest_pages(its->dev_table,
> +                              its_baser_table_size(reg) >> PAGE_SHIFT);
> +            its->max_devices = 0;
> +        }
> +
> +        its->baser0 = reg;
Why don't you update baser0 directly (with vgic_reg64_update)?

> +        return 1;
> +    case VREG64(GITS_BASER1):
> +        if ( info->dabt.size < DABT_WORD ) goto bad_width;
Please use vgic_reg64_check_access

> +
> +        /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
> +        if ( its->enabled )
> +            return 1;
> +
> +        reg = its->baser1;
> +        vgic_reg64_update(&reg, r, info);
> +        reg &= ~GITS_BASER_RO_MASK;
> +        reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> +        reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
> +        /* TODO: sanitise! */
> +
> +        /* TODO: sort out locking */
I am expecting this to be fixed in the next version.

> +        /* TODO: repeated calls: free old mapping */
> +        if ( reg & GITS_BASER_VALID )
> +        {
> +            its->coll_table = map_guest_pages(d, get_baser_phys_addr(reg),
> +                                              its_baser_table_size(reg) >> 
> PAGE_SHIFT);
> +            its->max_collections = its_baser_nr_entries(reg);
> +            memset(its->coll_table, 0xff,
> +                   its->max_collections * sizeof(uint16_t));
I am not sure to understand why we need to memset and what the value 
corresponds to.
> +        }
> +        else
> +        {
> +            unmap_guest_pages(its->coll_table,
> +                              its_baser_table_size(reg) >> PAGE_SHIFT);
> +            its->max_collections = 0;
> +        }
> +        its->baser1 = reg;
Why don't you update baser1 directly (with vgic_reg64_update)?

> +        return 1;
> +    case VRANGE64(GITS_BASER2, GITS_BASER7):
> +        goto write_ignore_64;
From the ITS register map, we would have to emulate more register (at 
least reserved, implementation defined and RAZ).
> +    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;
> +
> +bad_width:
> +    printk(XENLOG_G_ERR "%pv vGICR: bad read width %d r%d offset %#08lx\n",
> +           v, info->dabt.size, info->dabt.reg, info->gpa & 0xffff);
> +
> +    domain_crash_synchronous();
> +
> +    return 0;
> +}
> +
The Makefile already includes the vi
> +static const struct mmio_handler_ops vgic_its_mmio_handler = {
> +    .read  = vgic_v3_its_mmio_read,
> +    .write = vgic_v3_its_mmio_write,
> +};
This will break compilation with randconfig as the ITS is selectable. 
Please make sure that every patch built one by one. A good approach 
would be allowing the selection of the ITS at the end of this series.
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 8fe8386..aa53a1e 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -158,15 +158,6 @@ static void vgic_store_irouter(struct domain *d, struct 
> vgic_irq_rank *rank,
>      rank->vcpu[offset] = new_vcpu->vcpu_id;
>  }
>
> -static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
> -{
> -    /*
> -     * 64 bits registers can be accessible using 32-bit and 64-bit unless
> -     * stated otherwise (See 8.1.3 ARM IHI 0069A).
> -     */
> -    return ( dabt.size == DABT_DOUBLE_WORD || dabt.size == DABT_WORD );
> -}
> -
>  static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>                                           uint32_t gicr_reg,
>                                           register_t *r)
> diff --git a/xen/include/asm-arm/gic_v3_defs.h 
> b/xen/include/asm-arm/gic_v3_defs.h
> index da5fb77..6a91f5b 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -147,6 +147,16 @@
>  #define LPI_PROP_RES1                (1 << 1)
>  #define LPI_PROP_ENABLED             (1 << 0)
>
> +/*
> + * PIDR2: Only bits[7:4] are not implementation defined. We are
> + * emulating a GICv3 ([7:4] = 0x3).
> + *
> + * We don't emulate a specific registers scheme so implement the others
> + * bits as RES0 as recommended by the spec (see 8.1.13 in ARM IHI 0069A).
> + */
> +#define GICV3_GICD_PIDR2  0x30
> +#define GICV3_GICR_PIDR2  GICV3_GICD_PIDR2
Those values should not be defined in gic_v3_defs.h but a vgic headers. 
My rationale is, those value are implementation defined (e.g depends on 
the emulation).
> +
>  #define GICH_VMCR_EOI                (1 << 9)
>  #define GICH_VMCR_VENG1              (1 << 1)
>
> @@ -190,6 +200,15 @@ struct rdist_region {
>      bool single_rdist;
>  };
>
> +/*
> + * 64 bits registers can be accessible using 32-bit and 64-bit unless
> + * stated otherwise (See 8.1.3 ARM IHI 0069A).
> + */
> +static inline bool vgic_reg64_check_access(struct hsr_dabt dabt)
> +{
> +    return ( dabt.size == DABT_DOUBLE_WORD || dabt.size == DABT_WORD );
> +}
> +
This function should be defined in vgic.h and not gic_v3_defs.h

>  #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>
>  /*
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel