[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation



Hi Stefano,

On 06/04/17 01:21, Stefano Stabellini wrote:
> On Thu, 6 Apr 2017, 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@xxxxxxx>
>> ---
>>  xen/arch/arm/vgic-v3-its.c        | 416 
>> ++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vgic-v3.c            |   9 -
>>  xen/include/asm-arm/gic_v3_defs.h |   9 +
>>  xen/include/asm-arm/gic_v3_its.h  |   3 +
>>  4 files changed, 428 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 9dfda59..f6bf1ee 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -78,6 +78,422 @@ 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_ULL(51, 12);
>> +    uint64_t command[4];
>> +
>> +    ASSERT(spin_is_locked(&its->vcmd_lock));
>> +
>> +    if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +        return -1;
>> +
>> +    while ( its->creadr != its->cwriter )
>> +    {
>> +        int ret;
>> +
>> +        ret = vgic_access_guest_memory(d, addr + its->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;
>> +        }
>> +
>> +        its->creadr += ITS_CMD_SIZE;
>> +        if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> +            its->creadr = 0;
>> +
>> +        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 *
>> + *****************************/
>> +
>> +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):
>> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +
>> +        spin_lock(&its->vcmd_lock);
>> +        spin_lock(&its->its_lock);
>> +        if ( its->enabled )
>> +            reg = GITS_CTLR_ENABLE;
>> +        else
>> +            reg = 0;
>> +
>> +        if ( its->cwriter == its->creadr )
>> +            reg |= GITS_CTLR_QUIESCENT;
>> +        spin_unlock(&its->its_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 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);
> 
> From the comment at the top of this file, it looks like cbaser is
> supposed to be protected by the vcmd_lock:
> 
>     spinlock_t vcmd_lock;       /* Protects the virtual command buffer, which 
> */
>     uint64_t cwriter;           /* consists of CBASER and CWRITER and those   
> */
>     uint64_t creadr;            /* shadow variables cwriter and creadr. */

Ah, good spot. I meant CREADR and not CBASER. So the comment is wrong,
CBASER is protected by the ITS lock.

Cheers,
Andre.

>> +        spin_unlock(&its->its_lock);
>> +        break;
>> +    case VREG64(GITS_CWRITER):
>> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
>> +        spin_lock(&its->vcmd_lock);
>> +        *r = vgic_reg64_extract(its->cwriter, info);
>> +        spin_unlock(&its->vcmd_lock);
>> +        break;
>> +    case VREG64(GITS_CREADR):
>> +        if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
>> +        spin_lock(&its->vcmd_lock);
>> +        *r = vgic_reg64_extract(its->creadr, info);
>> +        spin_unlock(&its->vcmd_lock);
>> +        break;
>> +    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 VREG32(GITS_PIDR2):
>> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +        *r = vgic_reg32_extract(GIC_PIDR2_ARCH_GICv3, info);
>> +        break;
>> +    }
>> +
>> +    return 1;
>> +
>> +read_as_zero_64:
>> +    if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
>> +    *r = 0;
>> +
>> +    return 1;
>> +
>> +bad_width:
>> +    printk(XENLOG_G_ERR "vGIIS: bad read width %d r%d offset %#08lx\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 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) )
>> +        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, ctlr;
>> +
>> +    switch ( info->gpa & 0xffff )
>> +    {
>> +    case VREG32(GITS_CTLR):
>> +        if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +
>> +        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 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 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);
>> +
>> +        reg &= ~GITS_BASER_RO_MASK;
>> +        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);
>> +        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);
>> +        reg &= ~GITS_BASER_RO_MASK;
>> +        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;
>> +    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 "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/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 0623803..e6a33d0 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,
>>      write_atomic(&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 b7e5a47..e60dea5 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -198,6 +198,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 );
>> +}
>> +
>>  #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>>  
>>  /*
>> diff --git a/xen/include/asm-arm/gic_v3_its.h 
>> b/xen/include/asm-arm/gic_v3_its.h
>> index eb71fbd..d3f393f 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_ULL(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_IIDR_VALUE                 0x34c
>>  
>> @@ -78,6 +80,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_RO_MASK              (GITS_BASER_TYPE_MASK | \
>>                                          (31UL << 
>> GITS_BASER_ENTRY_SIZE_SHIFT) |\
>>                                          GITS_BASER_INDIRECT)
>> -- 
>> 2.8.2
>>

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.