|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 13/28] ARM: vITS: add command handling stub and MMIO emulation
On Thu, 11 May 2017, Andre Przywara wrote:
> + case VREG64(GITS_CWRITER):
> + if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> + reg = its->cwriter;
> + *r = vgic_reg64_extract(reg, info);
Why is this not protected by a lock? Also from the comment above I
cannot tell if it should be protected by its_lock or by vcmd_lock.
> + 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;
Same here
> + case VRANGE64(0x0098, 0x00F8):
> + 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 VRANGE32(0x0140, 0xBFFC):
> + goto read_reserved;
> + case VRANGE32(0xC000, 0xFFCC):
> + goto read_impl_defined;
> + case VRANGE32(0xFFD0, 0xFFE4):
> + goto read_impl_defined;
> + case VREG32(GITS_PIDR2):
> + if ( info->dabt.size != DABT_WORD ) goto bad_width;
> + *r = vgic_reg32_extract(GIC_PIDR2_ARCH_GICv3, info);
> + break;
> + case VRANGE32(0xFFEC, 0xFFFC):
> + goto read_impl_defined;
> + default:
> + printk(XENLOG_G_ERR
> + "%pv: vGITS: unhandled read r%d offset %#04lx\n",
> + v, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
> + return 0;
> + }
> +
> + 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 "vGITS: 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)
> +{
> + unsigned 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 &= ~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 &= ~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 &= ~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(®32, 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 VRANGE32(0x0018, 0x001C):
> + goto write_reserved;
> + case VRANGE32(0x0020, 0x003C):
> + goto write_impl_defined;
> + case VRANGE32(0x0040, 0x007C):
> + goto write_reserved;
> + case VREG64(GITS_CBASER):
> + if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> + spin_lock(&its->its_lock);
> + /* Changing base registers with the ITS enabled is UNPREDICTABLE. */
> + if ( its->enabled )
> + {
> + spin_unlock(&its->its_lock);
> + gdprintk(XENLOG_WARNING,
> + "vGITS: tried to change CBASER with the ITS
> enabled.\n");
> + return 1;
> + }
> +
> + reg = its->cbaser;
> + vgic_reg64_update(®, r, info);
> + sanitize_its_base_reg(®);
> +
> + its->cbaser = reg;
> + its->creadr = 0;
> + spin_unlock(&its->its_lock);
> +
> + return 1;
> +
> + case VREG64(GITS_CWRITER):
> + if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width;
> +
> + spin_lock(&its->vcmd_lock);
> + reg = ITS_CMD_OFFSET(its->cwriter);
> + vgic_reg64_update(®, r, info);
> + its->cwriter = ITS_CMD_OFFSET(reg);
> +
> + if ( its->enabled )
> + if ( vgic_its_handle_cmds(d, its) )
> + gdprintk(XENLOG_WARNING, "error handling ITS commands\n");
> +
> + spin_unlock(&its->vcmd_lock);
OK, so it looks like the reads should be protected by vcmd_lock
> + return 1;
> +
> + case VREG64(GITS_CREADR):
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |