Re: [PATCH v2 08/27] ARM: GICv3 ITS: introduce host LPI array

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 23 Mar 2017, Andre Przywara wrote:
> Hi,
> 
> On 22/03/17 23:38, Stefano Stabellini wrote:
> > On Thu, 16 Mar 2017, Andre Przywara wrote:
> >> The number of LPIs on a host can be potentially huge (millions),
> >> although in practise will be mostly reasonable. So prematurely allocating
> >> an array of struct irq_desc's for each LPI is not an option.
> >> However Xen itself does not care about LPIs, as every LPI will be injected
> >> into a guest (Dom0 for now).
> >> Create a dense data structure (8 Bytes) for each LPI which holds just
> >> enough information to determine the virtual IRQ number and the VCPU into
> >> which the LPI needs to be injected.
> >> Also to not artificially limit the number of LPIs, we create a 2-level
> >> table for holding those structures.
> >> This patch introduces functions to initialize these tables and to
> >> create, lookup and destroy entries for a given LPI.
> >> By using the naturally atomic access guarantee the native uint64_t data
> >> type gives us, we allocate and access LPI information in a way that does
> >> not require a lock.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  xen/arch/arm/gic-v3-its.c        |  90 ++++++++++++++++++-
> >>  xen/arch/arm/gic-v3-lpi.c        | 188 +++++++++++++++++++++++++++++++++++++++
> >>  xen/include/asm-arm/gic.h        |   5 ++
> >>  xen/include/asm-arm/gic_v3_its.h |  11 +++
> >>  4 files changed, 292 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> >> index 60b15b5..ed14d95 100644
> >> --- a/xen/arch/arm/gic-v3-its.c
> >> +++ b/xen/arch/arm/gic-v3-its.c
> >> @@ -148,6 +148,20 @@ static int its_send_cmd_sync(struct host_its *its, unsigned int cpu)
> >>      return its_send_command(its, cmd);
> >>  }
> >>  
> >> +static int its_send_cmd_mapti(struct host_its *its,
> >> +                              uint32_t deviceid, uint32_t eventid,
> >> +                              uint32_t pintid, uint16_t icid)
> >> +{
> >> +    uint64_t cmd[4];
> >> +
> >> +    cmd[0] = GITS_CMD_MAPTI | ((uint64_t)deviceid << 32);
> >> +    cmd[1] = eventid | ((uint64_t)pintid << 32);
> >> +    cmd[2] = icid;
> >> +    cmd[3] = 0x00;
> >> +
> >> +    return its_send_command(its, cmd);
> >> +}
> >> +
> >>  static int its_send_cmd_mapc(struct host_its *its, uint32_t collection_id,
> >>                               unsigned int cpu)
> >>  {
> >> @@ -180,6 +194,19 @@ static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid,
> >>      return its_send_command(its, cmd);
> >>  }
> >>  
> >> +static int its_send_cmd_inv(struct host_its *its,
> >> +                            uint32_t deviceid, uint32_t eventid)
> >> +{
> >> +    uint64_t cmd[4];
> >> +
> >> +    cmd[0] = GITS_CMD_INV | ((uint64_t)deviceid << 32);
> >> +    cmd[1] = eventid;
> >> +    cmd[2] = 0x00;
> >> +    cmd[3] = 0x00;
> >> +
> >> +    return its_send_command(its, cmd);
> >> +}
> >> +
> >>  /* Set up the (1:1) collection mapping for the given host CPU. */
> >>  int gicv3_its_setup_collection(unsigned int cpu)
> >>  {
> >> @@ -462,7 +489,7 @@ int gicv3_its_init(void)
> >>  
> >>  static int remove_mapped_guest_device(struct its_devices *dev)
> >>  {
> >> -    int ret;
> >> +    int ret, i;
> >>  
> >>      if ( dev->hw_its )
> >>      {
> >> @@ -471,11 +498,19 @@ static int remove_mapped_guest_device(struct its_devices *dev)
> >>              return ret;
> >>      }
> >>  
> >> +    /*
> >> +     * The only error the function below would return is -ENOENT, in which
> >> +     * case there is nothing to free here. So we just ignore it.
> >> +     */
> >> +    for ( i = 0; i < DIV_ROUND_UP(dev->eventids, LPI_BLOCK); i++ )
> >> +        gicv3_free_host_lpi_block(dev->hw_its, dev->host_lpis[i]);
> >> +
> >>      ret = gicv3_its_wait_commands(dev->hw_its);
> >>      if ( ret )
> >>          return ret;
> >>  
> >>      xfree(dev->itt_addr);
> >> +    xfree(dev->host_lpis);
> >>      xfree(dev);
> >>  
> >>      return 0;
> >> @@ -513,6 +548,36 @@ static int compare_its_guest_devices(struct its_devices *dev,
> >>  }
> >>  
> >>  /*
> >> + * On the host ITS @its, map @nr_events consecutive LPIs.
> >> + * The mapping connects a device @devid and event @eventid pair to LPI @lpi,
> >> + * increasing both @eventid and @lpi to cover the number of requested LPIs.
> >> + */
> >> +int gicv3_its_map_host_events(struct host_its *its,
> >> +                              uint32_t devid, uint32_t eventid, uint32_t lpi,
> >> +                              uint32_t nr_events)
> >> +{
> >> +    uint32_t i;
> >> +    int ret;
> >> +
> >> +    for ( i = 0; i < nr_events; i++ )
> >> +    {
> >> +        /* For now we map every host LPI to host CPU 0 */
> >> +        ret = its_send_cmd_mapti(its, devid, eventid + i, lpi + i, 0);
> >> +        if ( ret )
> >> +            return ret;
> >> +        ret = its_send_cmd_inv(its, devid, eventid + i);
> >> +        if ( ret )
> >> +            return ret;
> >> +    }
> >> +
> >> +    ret = its_send_cmd_sync(its, 0);
> >> +    if ( ret )
> >> +        return ret;
> >> +
> >> +    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.
> >> @@ -528,7 +593,7 @@ int gicv3_its_map_guest_device(struct domain *d,
> >>      struct host_its *hw_its;
> >>      struct its_devices *dev = NULL, *temp;
> >>      struct rb_node **new = &d->arch.vgic.its_devices.rb_node, *parent = NULL;
> >> -    int ret = -ENOENT;
> >> +    int ret = -ENOENT, i;
> >>  
> >>      hw_its = gicv3_its_find_by_doorbell(host_doorbell);
> >>      if ( !hw_its )
> >> @@ -574,6 +639,11 @@ int gicv3_its_map_guest_device(struct domain *d,
> >>      if ( !dev )
> >>          goto out_unlock;
> >>  
> >> +    dev->host_lpis = xzalloc_array(uint32_t,
> >> +                                   DIV_ROUND_UP(nr_events, LPI_BLOCK));
> >> +    if ( !dev->host_lpis )
> >> +        goto out_unlock;
> > 
> > The expectation is that we are going to allocate far more than 32 lpis
> > for one device, possibly thousands, right?
> 
> Mmh, what makes you think so?
> My understanding is that you have one MSI per interrupt "reason" (TX,
> RX, error, for instance). And even if you have multiple queues, each
> connected to separate MSIs, that won't go into the thousands. MSI-X for
> instance is *limited* to 2048 MSIs.
> 
> But in reality I dont' see nearly that many MSIs, mostly less than a
> dozen per device. And since 32 is the old PCI MSI limit, I thought this
> would be a good starting point. So most of the time we just need one block.
> 
> I believe this could add up with virtual functions, but those are
> separate devices, right?

Yes, you are right about that, but even 2048 can make a difference when
taken 32 at a time: an array of 64 entries vs. 1 entry (if the
allocation is fully contiguous), and that for each device, in the case
of SR-IOV they can be hundreds.


On Thu, 23 Mar 2017, Julien Grall wrote:
> Not answering directly to the question. But I think this is an improvement and
> not necessary for a first version.
> 
> I would like to see this series merged in Xen 4.9 as a tech preview, so I
> would suggest to gather this list of improvement (maybe on Jira?) and defer
> them for Xen 4.10. They would need to be addressed before making ITS stable.
> Stefano, does it sound good to you?

Yes, I am OK with not having this in 4.9, but we need to keep track
(Jira or otherwise) of these things.

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