Hi,
On 09/04/17 21:16, Julien Grall wrote:
> Hi Andre,
>
> On 04/07/2017 06:32 PM, 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.przyw...@arm.com>
>> ---
>> xen/arch/arm/vgic-v3-its.c | 512
>> +++++++++++++++++++++++++++++++++++++++
>> xen/include/asm-arm/gic_v3_its.h | 3 +
>> 2 files changed, 515 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 065ffe2..a171a3b 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -67,6 +67,9 @@ struct vits_itte
>> uint16_t pad;
>> };
>>
>> +#define GITS_BASER_RO_MASK (GITS_BASER_TYPE_MASK | \
>> + (31UL << GITS_BASER_ENTRY_SIZE_SHIFT))
>> +
>> int vgic_v3_its_init_domain(struct domain *d)
>> {
>> spin_lock_init(&d->arch.vgic.its_devices_lock);
>> @@ -80,6 +83,515 @@ 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(51, 12);
>> + uint64_t command[4];
>> + uint64_t creadr = its->creadr;
>> +
>> + ASSERT(spin_is_locked(&its->vcmd_lock));
>> +
>> + if ( its->cwriter >= ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> + return -1;
>> +
>> + while ( creadr != its->cwriter )
>> + {
>> + int ret;
>> +
>> + ret = vgic_access_guest_memory(d, addr + 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;
>> + }
>> +
>> + creadr += ITS_CMD_SIZE;
>> + if ( creadr == ITS_CMD_BUFFER_SIZE(its->cbaser) )
>> + creadr = 0;
>> + its->creadr = creadr; /* allow the guest to see the
>> progress */
>
> I hope you know that the compiler can decide to drop the temporary
> variable for optimization? ;) So it may decide to write-back everytime
> in its->creadr.
I don't think it can do it, because creadr is different from its->creadr
here (on purpose!). So doing this optimization would violate the program
semantic (because the end-of-buffer value would never be visible).
But just to be sure I replaced this check with a modulo operation over
the ITS_CMD_BUFFER_SIZE.
Not sure everyone likes *that* now, though ;-)
Cheers,
Andre.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
