|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 17/30] ARM: vITS: add command handling stub and MMIO emulation
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@xxxxxxx>
>> ---
>> xen/arch/arm/vgic-v3-its.c | 416
>> ++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/vgic-v3.c | 9 -
>> xen/include/asm-arm/gic_v3_defs.h | 9 +
>> xen/include/asm-arm/gic_v3_its.h | 3 +
>> 4 files changed, 428 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 9dfda59..f6bf1ee 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -78,6 +78,422 @@ void vgic_v3_its_free_domain(struct domain *d)
>> ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
>> }
>>
>> +/**************************************
>> + * Functions that handle ITS commands *
>> + **************************************/
>> +
>> +static uint64_t its_cmd_mask_field(uint64_t *its_cmd, unsigned int word,
>> + unsigned int shift, unsigned int
>> size)
>> +{
>> + return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT(size) - 1);
>> +}
>> +
>> +#define its_cmd_get_command(cmd) its_cmd_mask_field(cmd, 0,
>> 0, 8)
>> +#define its_cmd_get_deviceid(cmd) its_cmd_mask_field(cmd, 0,
>> 32, 32)
>> +#define its_cmd_get_size(cmd) its_cmd_mask_field(cmd, 1,
>> 0, 5)
>> +#define its_cmd_get_id(cmd) its_cmd_mask_field(cmd, 1,
>> 0, 32)
>> +#define its_cmd_get_physical_id(cmd) its_cmd_mask_field(cmd, 1,
>> 32, 32)
>> +#define its_cmd_get_collection(cmd) its_cmd_mask_field(cmd, 2,
>> 0, 16)
>> +#define its_cmd_get_target_addr(cmd) its_cmd_mask_field(cmd, 2,
>> 16, 32)
>> +#define its_cmd_get_validbit(cmd) its_cmd_mask_field(cmd, 2,
>> 63, 1)
>> +#define its_cmd_get_ittaddr(cmd) (its_cmd_mask_field(cmd, 2,
>> 8, 44) << 8)
>> +
>> +#define ITS_CMD_BUFFER_SIZE(baser) ((((baser) & 0xff) + 1) << 12)
>> +
>> +/*
>> + * Requires the vcmd_lock to be held.
>> + * TODO: Investigate whether we can be smarter here and don't need to
>> hold
>> + * the lock all of the time.
>> + */
>> +static int vgic_its_handle_cmds(struct domain *d, struct virt_its *its)
>> +{
>> + paddr_t addr = its->cbaser & GENMASK_ULL(51, 12);
>> + uint64_t command[4];
>> +
>> + ASSERT(spin_is_locked(&its->vcmd_lock));
>> +
>> + if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> + return -1;
>> +
>> + while ( its->creadr != its->cwriter )
>> + {
>> + int ret;
>> +
>> + ret = vgic_access_guest_memory(d, addr + its->creadr,
>> + command, sizeof(command), false);
>> + if ( ret )
>> + return ret;
>> +
>> + switch ( its_cmd_get_command(command) )
>> + {
>> + case GITS_CMD_SYNC:
>> + /* We handle ITS commands synchronously, so we ignore
>> SYNC. */
>> + break;
>> + default:
>> + gdprintk(XENLOG_WARNING, "ITS: unhandled ITS command %lu\n",
>> + its_cmd_get_command(command));
>> + break;
>> + }
>> +
>> + its->creadr += ITS_CMD_SIZE;
>> + if ( its->creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> + its->creadr = 0;
>> +
>> + if ( ret )
>> + gdprintk(XENLOG_WARNING,
>> + "ITS: ITS command error %d while handling
>> command %lu\n",
>> + ret, its_cmd_get_command(command));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*****************************
>> + * ITS registers read access *
>> + *****************************/
>> +
>> +static int vgic_v3_its_mmio_read(struct vcpu *v, mmio_info_t *info,
>> + register_t *r, void *priv)
>> +{
>> + struct virt_its *its = priv;
>> + uint64_t reg;
>> +
>> + switch ( info->gpa & 0xffff )
>> + {
>> + case VREG32(GITS_CTLR):
>> + if ( info->dabt.size != DABT_WORD ) goto bad_width;
>> +
>> + spin_lock(&its->vcmd_lock);
>> + spin_lock(&its->its_lock);
>> + if ( its->enabled )
>> + reg = GITS_CTLR_ENABLE;
>> + else
>> + reg = 0;
>> +
>> + if ( its->cwriter == its->creadr )
>
> 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,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |