Re: [PATCH v5 11/30] ARM: GICv3 ITS: introduce device mapping

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

Hi Andre,

A couple of more comments.

On 04/06/2017 12:19 AM, Andre Przywara wrote:
> +static int remove_mapped_guest_device(struct its_devices *dev)
> +{
> +    int ret = 0;
> +    unsigned int i;
> +

I would add a TODO here to think about the interaction with the guest 
vITS when removing a device whilst it is still in-use. Such as when the 
vLPI is still inflight or the ITT has not been unmapped.

It still possible to do a MAPD (V=0) before whilst vLPI associated to 
this device are inflight. This would lead to issue when freeing 
pending_irq as they would become NULL.

Maybe for this version we can think to never remove a device from DOM0 
(i.e MAPD(V=0) will not call gicv3_its_map_guest_device).

This would prevent all the issue possible with pending_irq. But this 
would need to be fixed before guest support (*hint* this should be added 
in the TODO in the cover letter ;)).

> +    if ( dev->hw_its )
> +        /* MAPD also discards all events with this device ID. */
> +        ret = its_send_cmd_mapd(dev->hw_its, dev->host_devid, 0, 0, false);
> +
> +    for ( i = 0; i < dev->eventids / LPI_BLOCK; i++ )
> +        gicv3_free_host_lpi_block(dev->host_lpi_blocks[i]);
> +
> +    /* Make sure the MAPD command above is really executed. */
> +    if ( !ret )
> +        ret = gicv3_its_wait_commands(dev->hw_its);
> +
> +    /* This should never happen, but just in case ... */
> +    if ( ret )
> +        printk(XENLOG_WARNING "Can't unmap host ITS device 0x%x\n",
> +               dev->host_devid);
> +
> +    xfree(dev->itt_addr);
> +    xfree(dev->pend_irqs);
> +    xfree(dev->host_lpi_blocks);
> +    xfree(dev);
> +
> +    return 0;

[...]

>  /*
>   * On the host ITS @its, map @nr_events consecutive LPIs.
>   * The mapping connects a device @devid and event @eventid pair to LPI @lpi,
> @@ -510,6 +616,163 @@ static int gicv3_its_map_host_events(struct host_its *its,
>      return gicv3_its_wait_commands(its);
>  }
>
> +/*
> + * Map a hardware device, identified by a certain host ITS and its device ID
> + * to domain d, a guest ITS (identified by its doorbell address) and device ID.
> + * Also provide the number of events (MSIs) needed for that device.
> + * This does not check if this particular hardware device is already mapped
> + * at another domain, it is expected that this would be done by the caller.
> + */
> +int gicv3_its_map_guest_device(struct domain *d,
> +                               paddr_t host_doorbell, uint32_t host_devid,
> +                               paddr_t guest_doorbell, uint32_t guest_devid,
> +                               uint32_t nr_events, bool valid)

My understanding if the number of event cannot be 0. You would start 
from 1 and it is possible to have up to 2^32 events which will overflow.

So I think we should use uint64_t for nr_events.

Cheers,

-- 
Julien Grall

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