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

[Julien Grall <julien.grall@xxxxxxx>]

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