Re: [PATCH v2 20/27] ARM: vITS: handle MAPTI command

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 24/03/17 14:54, Julien Grall wrote:
> Hi Andre,
> 
> On 03/16/2017 11:20 AM, Andre Przywara wrote:
>> The MAPTI commands associates a DeviceID/EventID pair with a LPI/CPU
>> pair and actually instantiates LPI interrupts.
>> We connect the already allocated host LPI to this virtual LPI, so that
>> any triggering IRQ on the host can be quickly forwarded to a guest.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  xen/arch/arm/gic-v3-its.c        | 63
>> ++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/gic-v3-lpi.c        | 18 ++++++++++++
>>  xen/arch/arm/vgic-v3-its.c       | 27 +++++++++++++++--
>>  xen/include/asm-arm/gic_v3_its.h |  6 ++++
>>  4 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 5a2dbec..e2fcf50 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -724,6 +724,69 @@ restart:
>>      spin_unlock(&d->arch.vgic.its_devices_lock);
>>  }
>>
>> +/*
>> + * Translates an event for a given guest device ID into the
>> associated host
>> + * LPI number. This can be used to look up the mapped guest LPI.
>> + */
>> +static uint32_t translate_event(struct domain *d, paddr_t doorbell,
>> +                                uint32_t devid, uint32_t eventid)
>> +{
>> +    struct rb_node *node;
>> +    struct its_devices *dev;
>> +    uint32_t host_lpi = 0;
>> +    int cmp;
>> +
>> +    spin_lock(&d->arch.vgic.its_devices_lock);
>> +    node = d->arch.vgic.its_devices.rb_node;
>> +    while (node)
>> +    {
>> +        dev = rb_entry(node, struct its_devices, rbnode);
>> +        cmp = compare_its_guest_devices(dev, doorbell, devid);
>> +
>> +        if ( !cmp )
>> +        {
>> +            if ( eventid >= dev->eventids )
>> +                goto out;
>> +
>> +            host_lpi = dev->host_lpis[eventid / LPI_BLOCK] +
>> +                                (eventid % LPI_BLOCK);
>> +            if ( !is_lpi(host_lpi) )
> 
> Hmmm, I don't understand this check. host_lpi should always be an LPI. No?

Looks like. Dropped in v4.

>> +                host_lpi = 0;
>> +            goto out;
>> +        }
>> +
>> +        if ( cmp > 0 )
>> +            node = node->rb_left;
>> +        else
>> +            node = node->rb_right;
>> +    }
>> +
>> +out:
>> +    spin_unlock(&d->arch.vgic.its_devices_lock);
>> +
>> +    return host_lpi;
>> +}
>> +
>> +/*
>> + * Connects the event ID for an already assigned device to the given
>> VCPU/vLPI
>> + * pair. The corresponding physical LPI is already mapped on the host
>> side
>> + * (when assigning the physical device to the guest), so we just
>> connect the
>> + * target VCPU/vLPI pair to that interrupt to inject it properly if
>> it fires.
>> + */
>> +int gicv3_assign_guest_event(struct domain *d, paddr_t doorbell_address,
>> +                             uint32_t devid, uint32_t eventid,
> 
> It looks like to me that devid is the virtual deviceID. If so, please
> prefix with 'v', otherwise 'p'.

Fixed in v4.

>> +                             struct vcpu *v, uint32_t virt_lpi)
>> +{
>> +    uint32_t host_lpi = translate_event(d, doorbell_address, devid,
>> eventid);
>> +
>> +    if ( !host_lpi )
>> +        return -ENOENT;
>> +
>> +    gicv3_lpi_update_host_entry(host_lpi, d->domain_id, v->vcpu_id,
>> virt_lpi);
>> +
>> +    return 0;
>> +}
>> +
>>  /* Scan the DT for any ITS nodes and create a list of host ITSes out
>> of it. */
>>  void gicv3_its_dt_init(const struct dt_device_node *node)
>>  {
>> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
>> index 994698e..c110ec9 100644
>> --- a/xen/arch/arm/gic-v3-lpi.c
>> +++ b/xen/arch/arm/gic-v3-lpi.c
>> @@ -153,6 +153,24 @@ void do_LPI(unsigned int lpi)
>>      vgic_vcpu_inject_irq(vcpu, hlpi.virt_lpi);
>>  }
>>
>> +int gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id,
>> +                                unsigned int vcpu_id, uint32_t virt_lpi)
>> +{
>> +    union host_lpi *hlpip, hlpi;
>> +
>> +    host_lpi -= LPI_OFFSET;
> 
> I would add an ASSERT(host_lpi > LPI_OFFSET);

Fixed in v4.

>> +
>> +    hlpip = &lpi_data.host_lpis[host_lpi /
>> HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE];
>> +
>> +    hlpi.virt_lpi = virt_lpi;
>> +    hlpi.dom_id = domain_id;
>> +    hlpi.vcpu_id = vcpu_id;
>> +
>> +    write_u64_atomic(&hlpip->data, hlpi.data);
>> +
>> +    return 0;
>> +}
>> +
>>  static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
>>  {
>>      uint64_t val;
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index c26d5d4..600ff69 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -167,8 +167,8 @@ static bool read_itte(struct virt_its *its,
>> uint32_t devid, uint32_t evid,
>>  }
>>
>>  #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)
>> +static bool write_itte(struct virt_its *its, uint32_t devid, uint32_t
>> evid,
>> +                       uint32_t collid, uint32_t vlpi, struct vcpu
>> **vcpu)
> 
> Please explain in the commit message why you move to static.

Fixed in v4.

> 
>>  {
>>      struct vits_itte *itte;
>>
>> @@ -332,6 +332,25 @@ static int its_handle_mapd(struct virt_its *its,
>> uint64_t *cmdptr)
>>      return 0;
>>  }
>>
>> +static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr)
>> +{
>> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
>> +    uint32_t eventid = its_cmd_get_id(cmdptr);
>> +    uint32_t intid = its_cmd_get_physical_id(cmdptr);
>> +    int collid = its_cmd_get_collection(cmdptr);
> 
> This should be unsigned.

Fixed in v4.

>> +    struct vcpu *vcpu;
>> +
>> +    if ( its_cmd_get_command(cmdptr) == GITS_CMD_MAPI )
>> +        intid = eventid;
>> +
>> +    if ( !write_itte(its, devid, eventid, collid, intid, &vcpu) )
>> +        return -1;
>> +
>> +    gicv3_assign_guest_event(its->d, its->doorbell_address, devid,
>> eventid, vcpu, intid);
> 
> If you have a function returning an error, then you should check it and
> not ignoring it.

Fixed in v3.

Cheers,
Andre.

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