Re: [RFC PATCH 13/24] ARM: vITS: handle CLEAR command

[Julien Grall <julien.grall@arm.com>]

Hi,

On 09/11/16 00:39, Stefano Stabellini wrote:
> On Wed, 28 Sep 2016, Andre Przywara wrote:
>> This introduces the ITS command handler for the CLEAR command, which
>> clears the pending state of an LPI.
>> This removes a not-yet injected, but already queued IRQ from a VCPU.
>>
>> In addition this patch introduces the lookup function which translates
>> a given DeviceID/EventID pair into a pointer to our vITTE structure.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/vgic-its.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 115 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic-its.c b/xen/arch/arm/vgic-its.c
>> index 875b992..99d9e9c 100644
>> --- a/xen/arch/arm/vgic-its.c
>> +++ b/xen/arch/arm/vgic-its.c
>> @@ -61,6 +61,73 @@ struct vits_itte
>>      uint64_t collection:16;
>>  };
>>
>> +#define UNMAPPED_COLLECTION      ((uint16_t)~0)
>> +
>> +/* Must be called with the ITS lock held. */
>> +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid)
>> +{
>> +    uint16_t vcpu_id;
>> +
>> +    if ( collid >= its->max_collections )
>> +        return NULL;
>> +
>> +    vcpu_id = its->coll_table[collid];
>> +    if ( vcpu_id == UNMAPPED_COLLECTION || vcpu_id >= its->d->max_vcpus )
>> +        return NULL;
>> +
>> +    return its->d->vcpu[vcpu_id];
>> +}
>> +
>> +#define DEV_TABLE_ITT_ADDR(x) ((x) & GENMASK(51, 8))
>> +#define DEV_TABLE_ITT_SIZE(x) (BIT(((x) & GENMASK(7, 0)) + 1))
>> +#define DEV_TABLE_ENTRY(addr, bits)                     \
>> +        (((addr) & GENMASK(51, 8)) | (((bits) - 1) & GENMASK(7, 0)))
>> +
>> +static paddr_t get_itte_address(struct virt_its *its,
>> +                                uint32_t devid, uint32_t evid)
>> +{
>> +    paddr_t addr;
>> +
>> +    if ( devid >= its->max_devices )
>> +        return ~0;
>
> Please #define the error

Technically this should be INVALID_PADDR here.

>
>> +    if ( evid >= DEV_TABLE_ITT_SIZE(its->dev_table[devid]) )
>> +        return ~0;
>
> same here

Ditto.

>
>
>> +    addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]);
>> +
>> +    return addr + evid * sizeof(struct vits_itte);
>> +}
>> +
>> +/* Looks up a given deviceID/eventID pair on an ITS and returns a pointer to
>> + * the corresponding ITTE. This maps the respective guest page into Xen.
>> + * Once finished with handling the ITTE, call put_devid_evid() to unmap
>> + * the page again.
>> + * Must be called with the ITS lock held.
>> + */
>> +static struct vits_itte *get_devid_evid(struct virt_its *its,
>> +                                        uint32_t devid, uint32_t evid)
>> +{
>> +    paddr_t addr = get_itte_address(its, devid, evid);
>> +    struct vits_itte *itte;
>> +
>> +    if (addr == ~0)
>> +        return NULL;
>> +
>> +    /* TODO: check locking for map_guest_pages() */
>> +    itte = map_guest_pages(its->d, addr & PAGE_MASK, 1);
>> +    if ( !itte )
>> +        return NULL;
>
> No need to use the vmap to map 1 page

But you do have to translate the IPA to a PA, so you cannot directly use 
__pa on it.

>
>> +    return itte + (addr & ~PAGE_MASK) / sizeof(struct vits_itte);
>
> Please use () around the div operation for clarity
>
>
>> +}
>> +
>> +/* Must be called with the ITS lock held. */
>> +static void put_devid_evid(struct virt_its *its, struct vits_itte *itte)
>> +{
>> +    unmap_guest_pages(itte, 1);
>
> No need for this, once you use __pa instead of the vmap

Well, we should at least use map_domain_page/unmap_domain_page even if 
they are a nop on ARM64. And not directly __pa.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel