Re: [PATCH v9 27/28] ARM: vITS: create and initialize virtual ITSes for Dom0

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

Hi Andre,

On 11/05/17 18:53, Andre Przywara wrote:
> For each hardware ITS create and initialize a virtual ITS for Dom0.
> We use the same memory mapped address to keep the doorbell working.
> This introduces a function to initialize a virtual ITS.
> We maintain a list of virtual ITSes, at the moment for the only
> purpose of later being able to free them again.
> We configure the virtual ITSes to match the hardware ones, that is we
> keep the number of device ID bits and event ID bits the same as the host
> ITS.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v3-its.c       | 75 ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c           |  4 +++
>  xen/include/asm-arm/domain.h     |  1 +
>  xen/include/asm-arm/gic_v3_its.h |  4 +++
>  4 files changed, 84 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 8f6ff11..ca35aca 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -52,6 +52,7 @@
>   */
>  struct virt_its {
>      struct domain *d;
> +    struct list_head vits_list;
>      paddr_t doorbell_address;
>      unsigned int devid_bits;
>      unsigned int evid_bits;
> @@ -103,14 +104,49 @@ unsigned int vgic_v3_its_count(const struct domain *d)
>
>  int vgic_v3_its_init_domain(struct domain *d)
>  {
> +    int ret;
> +
> +    INIT_LIST_HEAD(&d->arch.vgic.vits_list);
>      spin_lock_init(&d->arch.vgic.its_devices_lock);
>      d->arch.vgic.its_devices = RB_ROOT;
>
> +    if ( is_hardware_domain(d) )
> +    {
> +        struct host_its *hw_its;
> +
> +        list_for_each_entry(hw_its, &host_its_list, entry)
> +        {
> +            /*
> +             * For each host ITS create a virtual ITS using the same
> +             * base and thus doorbell address.
> +             * Use the same number of device ID and event ID bits as the host.
> +             */
> +            ret = vgic_v3_its_init_virtual(d, hw_its->addr,
> +                                           hw_its->devid_bits,
> +                                           hw_its->evid_bits);
> +            if ( ret )
> +            {
> +                vgic_v3_its_free_domain(d);

You don't need this call. vgic_v3_free_domain will always be called when 
a domain is destroyed even if it has not finished to be built.

> +                return ret;
> +            }
> +            else
> +                d->arch.vgic.has_its = true;
> +        }
> +    }
> +
>      return 0;
>  }
>
>  void vgic_v3_its_free_domain(struct domain *d)
>  {
> +    struct virt_its *pos, *temp;
> +
> +    list_for_each_entry_safe( pos, temp, &d->arch.vgic.vits_list, vits_list )
> +    {
> +        list_del(&pos->vits_list);
> +        xfree(pos);
> +    }
> +
>      ASSERT(RB_EMPTY_ROOT(&d->arch.vgic.its_devices));
>  }
>
> @@ -1407,6 +1443,45 @@ static const struct mmio_handler_ops vgic_its_mmio_handler = {
>      .write = vgic_v3_its_mmio_write,
>  };
>
> +int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
> +                             unsigned int devid_bits, unsigned int evid_bits)

Why this is exported? The only caller in the same file.

> +{
> +    struct virt_its *its;
> +    uint64_t base_attr;
> +
> +    its = xzalloc(struct virt_its);
> +    if ( !its )
> +        return -ENOMEM;
> +
> +    base_attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> +    base_attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> +    base_attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
> +
> +    its->cbaser  = base_attr;
> +    base_attr |= 0ULL << GITS_BASER_PAGE_SIZE_SHIFT;    /* 4K pages */
> +    its->baser_dev = GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
> +    its->baser_dev |= (sizeof(dev_table_entry_t) - 1) <<
> +                      GITS_BASER_ENTRY_SIZE_SHIFT;
> +    its->baser_dev |= base_attr;
> +    its->baser_coll  = GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
> +    its->baser_coll |= (sizeof(coll_table_entry_t) - 1) <<
> +                       GITS_BASER_ENTRY_SIZE_SHIFT;
> +    its->baser_coll |= base_attr;
> +    its->d = d;
> +    its->doorbell_address = guest_addr + ITS_DOORBELL_OFFSET;
> +    its->devid_bits = devid_bits;
> +    its->evid_bits = evid_bits;
> +    spin_lock_init(&its->vcmd_lock);
> +    spin_lock_init(&its->its_lock);
> +
> +    register_mmio_handler(d, &vgic_its_mmio_handler, guest_addr, SZ_64K, its);
> +
> +    /* Register the virtual ITSes to be able to clean them up later. */

There is only one virtual ITS registered here. So s/ITSes/ITS/

> +    list_add_tail(&its->vits_list, &d->arch.vgic.vits_list);
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 41cda78..fd4b5f4 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1700,6 +1700,10 @@ static int vgic_v3_domain_init(struct domain *d)
>          d->arch.vgic.intid_bits = GUEST_GICV3_GICD_INTID_BITS;
>      }
>
> +    /*
> +     * For a hardware domain, this will iterate over the host ITSes
> +     * and maps  one virtual ITS per host ITS at the same address.
> +     */

This kind of comment will get easily rotten if you put it on the caller. 
It would be better to move it on the top of the declaration.

>      ret = vgic_v3_its_init_domain(d);
>      if ( ret )
>          return ret;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index b2d98bb..92f4ce5 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -115,6 +115,7 @@ struct arch_domain
>          spinlock_t its_devices_lock;        /* Protects the its_devices tree */
>          struct radix_tree_root pend_lpi_tree; /* Stores struct pending_irq's */
>          rwlock_t pend_lpi_tree_lock;        /* Protects the pend_lpi_tree */
> +        struct list_head vits_list;         /* List of virtual ITSes */
>          unsigned int intid_bits;
>          bool rdists_enabled;                /* Is any redistributor enabled? */
>          bool has_its;
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 927568f..e41f8fd 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -158,6 +158,10 @@ int gicv3_its_setup_collection(unsigned int cpu);
>  int vgic_v3_its_init_domain(struct domain *d);
>  void vgic_v3_its_free_domain(struct domain *d);
>
> +/* Create and register a virtual ITS at the given guest address. */
> +int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr,
> +			     unsigned int devid_bits, unsigned int evid_bits);
> +
>  /*
>   * Map a device on the host by allocating an ITT on the host (ITS).
>   * "nr_event" specifies how many events (interrupts) this device will need.
>

Cheers,

-- 
Julien Grall

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