Re: [Xen-devel] [PATCH v6 24/36] ARM: vITS: add command handling stub and MMIO emulation

[Andre Przywara <andre.przywara@xxxxxxx>]

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.przywara@xxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel