Re: [PATCH v2 15/27] ARM: vITS: introduce translation table walks

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

Hi Andre,

On 03/16/2017 11:20 AM, Andre Przywara wrote:
> The ITS stores the target (v)CPU and the (virtual) LPI number in tables.
> Introduce functions to walk those tables and translate an device ID -
> event ID pair into a pair of virtual LPI and vCPU.
> Since the final interrupt translation tables can be smaller than a page,
> we map them on demand (which is cheap on arm64). Also we take care of
> the locking on the way, since we can't easily protect those ITTs from
> being altered by the guest.
>
> To allow compiling without warnings, we declare two functions as
> non-static for the moment.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v3-its.c | 135 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 135 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 5337638..267a573 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -62,6 +62,141 @@ struct vits_itte
>      uint16_t collection;
>  };
>
> +#define UNMAPPED_COLLECTION      ((uint16_t)~0)
> +
> +/* Must be called with the ITS lock held. */

This is a call for an ASSERT in the function.

> +static struct vcpu *get_vcpu_from_collection(struct virt_its *its, int collid)

s/int/unsigned int/

> +{
> +    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)))

The layout of dev_table[...] really needs to be explained. It took me 
quite a while to understand how it works. For instance why you skip the 
first 8 bits for the address...

> +
> +static paddr_t get_itte_address(struct virt_its *its,
> +                                uint32_t devid, uint32_t evid)
> +{

I was expected to see the support of two-level page table for the vITS. 
Any plan for that?

> +    paddr_t addr;
> +
> +    if ( devid >= its->max_devices )
> +        return ~0;

Please don't hardcode invalid address and use INVALID_PADDR.

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

Ditto.

> +
> +    addr = DEV_TABLE_ITT_ADDR(its->dev_table[devid]);

You read dev_table[...] multiple time. What prevents someone to modify 
dev_table after you did someone sanity check?

It would be safer to do a read_atomic(..) at the beginning and use a 
temporary variable.

> +
> +    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.

This is a call for an ASSERT in the code.

> + */
> +static struct vits_itte *get_devid_evid(struct virt_its *its,
> +                                        uint32_t devid, uint32_t evid)

The naming of the function is confusing. It doesn't look up a device 
ID/event ID but an IIT. So I would rename it to find_itte.

> +{
> +    paddr_t addr = get_itte_address(its, devid, evid);
> +
> +    if ( addr == ~0 )


Please use INVALID_PADDR.

> +        return NULL;
> +
> +    return map_guest_pages(its->d, addr, 1);
> +}
> +
> +/* 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);
> +}
> +
> +/*
> + * Queries the collection and device tables to get the vCPU and virtual
> + * LPI number for a given guest event. This takes care of mapping the
> + * respective tables and validating the values, since we can't efficiently
> + * protect the ITTs with their less-than-page-size granularity.
> + * Takes and drops the its_lock.

I am not sure to understand the usefulness of "takes and drops the 
its_lock".

> + */
> +bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
> +               struct vcpu **vcpu, uint32_t *vlpi)
> +{
> +    struct vits_itte *itte;
> +    int collid;
> +    uint32_t _vlpi;
> +    struct vcpu *_vcpu;
> +
> +    spin_lock(&its->its_lock);

Do we expect multiple vCPU calling read_itte at the same time? This 
needs to be clarify in the comments because this current function is not 
scalable.

> +    itte = get_devid_evid(its, devid, evid);
> +    if ( !itte )
> +    {
> +        spin_unlock(&its->its_lock);
> +        return false;
> +    }
> +    collid = itte->collection;
> +    _vlpi = itte->vlpi;
> +    put_devid_evid(its, itte);
> +
> +    _vcpu = get_vcpu_from_collection(its, collid);
> +    spin_unlock(&its->its_lock);
> +
> +    if ( !_vcpu )
> +        return false;
> +
> +    if ( collid >= its->max_collections )
> +        return false;

No need for this check. It is already done in get_vcpu_from_collection.

> +
> +    *vcpu = _vcpu;
> +    *vlpi = _vlpi;
> +
> +    return true;
> +}
> +
> +#define SKIP_LPI_UPDATE 1
> +bool write_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
> +                uint32_t collid, uint32_t vlpi, struct vcpu **vcpu)
> +{
> +    struct vits_itte *itte;
> +
> +    if ( collid >= its->max_collections )
> +        return false;
> +
> +    /* TODO: validate vlpi */

This TODO needs to be fixed as soon as possible.

> +
> +    spin_lock(&its->its_lock);
> +    itte = get_devid_evid(its, devid, evid);
> +    if ( !itte )
> +    {
> +        spin_unlock(&its->its_lock);
> +        return false;
> +    }
> +
> +    itte->collection = collid;
> +    if ( vlpi != SKIP_LPI_UPDATE )
> +        itte->vlpi = vlpi;
> +
> +    if ( vcpu )
> +        *vcpu = get_vcpu_from_collection(its, collid);
> +
> +    put_devid_evid(its, itte);
> +    spin_unlock(&its->its_lock);
> +
> +    return true;
> +}
> +
>  /**************************************
>   * Functions that handle ITS commands *
>   **************************************/
>

Cheers,

-- 
Julien Grall

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