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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 06/04/17 16:34, Julien Grall wrote:
> Hi Andre,
> 
> On 06/04/17 00:19, Andre Przywara wrote:
>> The ITS uses device IDs to map LPIs to a device. Dom0 will later use
>> those IDs, which we directly pass on to the host.
>> For this we have to map each device that Dom0 may request to a host
>> ITS device with the same identifier.
>> Allocate the respective memory and enter each device into an rbtree to
>> later be able to iterate over it or to easily teardown guests.
>> Because device IDs are per ITS, we need to identify a virtual ITS. We
>> use the doorbell address for that purpose, as it is a nice architectural
>> MSI property and spares us handling with opaque pointer or break
>> the VGIC abstraction.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/gic-v3-its.c        | 263
>> +++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/vgic-v3-its.c       |   3 +
>>  xen/include/asm-arm/domain.h     |   3 +
>>  xen/include/asm-arm/gic_v3_its.h |  13 ++
>>  4 files changed, 282 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index eb47c9d..45bbfa7 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -21,6 +21,8 @@
>>  #include <xen/lib.h>
>>  #include <xen/delay.h>
>>  #include <xen/mm.h>
>> +#include <xen/rbtree.h>
>> +#include <xen/sched.h>
>>  #include <xen/sizes.h>
>>  #include <asm/gic.h>
>>  #include <asm/gic_v3_defs.h>
>> @@ -36,6 +38,26 @@
>>   */
>>  LIST_HEAD(host_its_list);
>>
>> +/*
>> + * Describes a device which is using the ITS and is used by a guest.
>> + * Since device IDs are per ITS (in contrast to vLPIs, which are per
>> + * guest), we have to differentiate between different virtual ITSes.
>> + * We use the doorbell address here, since this is a nice architectural
>> + * property of MSIs in general and we can easily get to the base address
>> + * of the ITS and look that up.
>> + */
>> +struct its_devices {
> 
> Again, why its_devices? You only store the information for one device.

You are right, and actually I fixed that already an hour ago.

>> +    struct rb_node rbnode;
>> +    struct host_its *hw_its;
>> +    void *itt_addr;
>> +    paddr_t guest_doorbell;             /* Identifies the virtual ITS */
>> +    uint32_t host_devid;
>> +    uint32_t guest_devid;
>> +    uint32_t eventids;                  /* Number of event IDs (MSIs) */
>> +    uint32_t *host_lpi_blocks;          /* Which LPIs are used on the
>> host */
>> +    struct pending_irq *pend_irqs;      /* One struct per event */
>> +};
> 
> [...]
> 
>> +static int remove_mapped_guest_device(struct its_devices *dev)
>> +{
>> +    int ret = 0;
>> +    unsigned int i;
>> +
>> +    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);
> 
> I think we want to ratelimit this.

OK.

>> +
>> +    xfree(dev->itt_addr);
>> +    xfree(dev->pend_irqs);
>> +    xfree(dev->host_lpi_blocks);
>> +    xfree(dev);
>> +
>> +    return 0;
>> +}
> 
> [...]
> 
>> +/*
>> + * 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)
>> +{
>> +    void *itt_addr = NULL;
>> +    struct host_its *hw_its;
>> +    struct its_devices *dev = NULL;
>> +    struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent
>> = NULL;
>> +    unsigned int i;
>> +    int ret = -ENOENT;
>> +
>> +    hw_its = gicv3_its_find_by_doorbell(host_doorbell);
>> +    if ( !hw_its )
>> +        return ret;
>> +
>> +    /* Sanitise the provided hardware values against the host ITS. */
>> +    if ( host_devid >= BIT(hw_its->devid_bits) )
>> +        return -EINVAL;
>> +
>> +    /* We allocate events and LPIs in chunks of LPI_BLOCK (=32). */
>> +    nr_events = ROUNDUP(nr_events, LPI_BLOCK);
> 
> I think it is still not enough. The MAPD command will use the number of
> bits, so you want to make sure you cover all the events in the table.
> 
> For instance, if nr_events = 66, you will round up to 96. My
> understanding, is you will call mapd with 7 bits rather 8 bits.
> 
> So you have to round up to the next power of two. Whether you want to
> allocate a power of two of LPIs is another question. But the ITT should
> be configured correctly.

Again good catch.

> 
> [...]
> 
>> +    if ( ret )
>> +    {
>> +        do {
>> +            i--;
>> +            gicv3_free_host_lpi_block(dev->host_lpi_blocks[i]);
>> +            if ( i == 0 )
>> +                break;
>> +        } while (1);
> 
> This could be  } while ( i > 0 ) saving 3 lines.

Argh, I rewrote this sucker three times because "i" is unsigned and I
apparently missed the easiest solution ;-)
And still there is a bug in there (if the first allocation fails already
above). Do you mind if I revert "i" back to a signed int? That would
allow me to do write "while ( i >= 0 )" here or a for loop.

>> +
>> +        /* Unmapping the device will discard all LPIs mapped so far. */
>> +        its_send_cmd_mapd(hw_its, host_devid, 1, 0, false);
> 
> You likely need to check the return of its_send_mapd...
> 
> Also why 1 for the 3rd argument?

Good catch, I thought I changed this to 0 after I applied your changes
to the wrapper function ...

>> +
>> +        goto out;
>> +    }
>> +
>> +    return 0;
>> +
>> +out_unlock:
>> +    spin_unlock(&d->arch.vgic.its_devices_lock);
>> +
>> +out:
>> +    if ( dev )
>> +    {
>> +        xfree(dev->pend_irqs);
>> +        xfree(dev->host_lpi_blocks);
>> +    }
>> +    xfree(itt_addr);
>> +    xfree(dev);
>> +
>> +    return ret;
>> +}
>> +
> 
> Cheers,
> 
> 

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