| Message ID | 1491434362-30310-18-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > --- > 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. */ > + 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(®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 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(®, r, info); > + sanitize_its_base_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(®, 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(®, 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(®); > + > + 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(®, 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(®); > + > + 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 >
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@arm.com> >> --- >> 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(®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 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(®, r, info); >> + sanitize_its_base_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(®, 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(®, 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(®); >> + >> + 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(®, 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(®); >> + >> + 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 >>
Hi Andre, On 04/06/2017 12:19 AM, 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 | 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 ) I think it would be better if you can read atomically cwriter and creadr. This would avoid to take the vcmd_lock here, as this would prevent a guest vCPU to access this register while you are processing the command queue. > + 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); > + 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); Same remark here. > + *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); Ditto. > + *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) As said on v4, this should return unsigned int. > +{ > + int entry_size = GITS_BASER_ENTRY_SIZE(baser); Ditto. > + > + 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); I am not sure to understand why taking the vcmd_lock here. > + 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); Should not you print a warning to help a user debugging if you don't enable ITS as expected? Also, how do you disable it? > + 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(®, r, info); > + sanitize_its_base_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(®, 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"); You likely want to print the domain id here. So I would turn to gdprintk. > + } > + 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(®, 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(®); > + > + 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(®, 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(®); > + > + 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 ); > +} Again, this should be in vgic-emul.h and not gic_v3_defs.h > + > #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) > Cheers,
On 06/04/17 22:43, Julien Grall wrote: Salut Julien, > On 04/06/2017 12:19 AM, 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 | 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 ) > > I think it would be better if you can read atomically cwriter and > creadr. What do you mean by "atomically" here? For reading and comparing *both* registers I have to hold a lock, isn't it? > This would avoid to take the vcmd_lock here, as this would > prevent a guest vCPU to access this register while you are processing > the command queue. That's a noble goal, but I don't know if this is easily feasible. I wonder if one can use spin_trylock(&its->vcmd_lock) here, and assume not quiescent if that fails? My understanding is that an OS would poll CTLR until it becomes quiescent (Linux does so), so it would try again, returning here until the lock becomes actually free. Would that work? But looking at the Linux code again I see that this is only done once at probe time (before the ITS is enabled), so at least from that side it's not really worth optimizing. >> + 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); >> + 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); > > Same remark here. Indeed it looks like I don't need the lock here. That other open source hypervisor certainly doesn't ;-) >> + *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); > > Ditto. Currently this does not work because it could read it when creadr reaches the end of the buffer (before the code above resets it to 0). But by rewriting that code using a local variable this should be doable. >> + *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) > > As said on v4, this should return unsigned int. > >> +{ >> + int entry_size = GITS_BASER_ENTRY_SIZE(baser); > > Ditto. I found this myself today and fixed that already. Sorry for missing that. > >> + >> + 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); > > I am not sure to understand why taking the vcmd_lock here. To prevent a nasty guest from turning off the ITS while commands are still handled. >> + 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); > > Should not you print a warning to help a user debugging if you don't > enable ITS as expected? Good idea. > Also, how do you disable it? Not sure what you mean? vgic_v3_verify_its_status() returns false if the ENABLE_BIT has been cleared, so its->enabled becomes false. Or what do I miss? >> + 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(®, r, info); >> + sanitize_its_base_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(®, 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"); > > You likely want to print the domain id here. So I would turn to gdprintk. Indeed. >> + } >> + 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(®, 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(®); >> + >> + 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(®, 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(®); >> + >> + 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 ); >> +} > > Again, this should be in vgic-emul.h and not gic_v3_defs.h Fixed in my tree already. Sorry for missing that again. Cheers, Andre. >> + >> #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) >> > > Cheers, >
Hi Andre, On 04/06/2017 12:19 AM, Andre Przywara wrote: > + 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(®, r, info); > + > + reg &= ~GITS_BASER_RO_MASK; It took me a bit of time to understand how you ensure the guest will not hand over two-level table. But this is hidden in the GITS_BASER_RO_MASK as you always mask the indirect bit. I would prefer if you make clear that two-level table are not currently supported with a comment. > + reg |= (sizeof(uint64_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT; > + reg |= GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT; > + sanitize_its_base_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(®, r, info); > + reg &= ~GITS_BASER_RO_MASK; Ditto. > + reg |= (sizeof(uint16_t) - 1) << GITS_BASER_ENTRY_SIZE_SHIFT; > + reg |= GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT; > + sanitize_its_base_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; > + } Cheers,
Hi Andre, On 04/06/2017 11:22 PM, AndrĂ© Przywara wrote: > On 06/04/17 22:43, Julien Grall wrote: >> On 04/06/2017 12:19 AM, Andre Przywara wrote: >>> +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 ) >> >> I think it would be better if you can read atomically cwriter and >> creadr. > > What do you mean by "atomically" here? For reading and comparing *both* > registers I have to hold a lock, isn't it? > >> This would avoid to take the vcmd_lock here, as this would >> prevent a guest vCPU to access this register while you are processing >> the command queue. > > That's a noble goal, but I don't know if this is easily feasible. > I wonder if one can use spin_trylock(&its->vcmd_lock) here, and assume > not quiescent if that fails? My understanding is that an OS would poll > CTLR until it becomes quiescent (Linux does so), so it would try again, > returning here until the lock becomes actually free. > Would that work? > But looking at the Linux code again I see that this is only done once at > probe time (before the ITS is enabled), so at least from that side it's > not really worth optimizing. As you may know, I don't buy the reason: "Linux is only doing it once". In this case my worry is not about optimizing because a guest could call it often but the fact the you might end up in all the vCPUs but one reading GITS_CTLR and one handling the command queue. So formers will wait on the lock which will monopolize the pCPUs. The command queue handling is not ideal (it can take a bit of time), but this is going to be worst. And I really don't want to see that. >>> + *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); >> >> Ditto. > > Currently this does not work because it could read it when creadr > reaches the end of the buffer (before the code above resets it to 0). > But by rewriting that code using a local variable this should be doable. Please do it. >>> + >>> +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); >> >> I am not sure to understand why taking the vcmd_lock here. > > To prevent a nasty guest from turning off the ITS while commands are > still handled. Please document it. > >>> + 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); >> >> Should not you print a warning to help a user debugging if you don't >> enable ITS as expected? > > Good idea. > >> Also, how do you disable it? > > Not sure what you mean? vgic_v3_verify_its_status() returns false if the > ENABLE_BIT has been cleared, so its->enabled becomes false. > Or what do I miss? I think I wrote the question and answered myself before sending the e-mail. Though I forgot to drop it. Cheers,
Hi Andre, Another thing I missed. On 04/06/2017 12:19 AM, Andre Przywara wrote: > + 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(®, 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(®); > + > + if ( reg & GITS_VALID_BIT ) > + its->max_devices = its_baser_nr_entries(reg); Should not you validate this against the maximum number of device supported by the ITS (i.e devid)? > + 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(®, 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(®); > + > + if ( reg & GITS_VALID_BIT ) > + its->max_collections = its_baser_nr_entries(reg); Similar question for the collection. Although, I am not sure against what. Cheers,
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); + 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(®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 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(®, r, info); + sanitize_its_base_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(®, 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(®, 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(®); + + 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(®, 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(®); + + 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)
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 | 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(-)