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

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

Hi Andre,

On 03/16/2017 11:20 AM, 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.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/vgic-v3-its.c       | 29 +++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v3.c           | 12 ++++++++++++
>  xen/include/asm-arm/domain.h     |  1 +
>  xen/include/asm-arm/gic_v3_its.h | 12 ++++++++++++
>  4 files changed, 54 insertions(+)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index cc12c1c..caa7354 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -914,6 +914,35 @@ 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)

*All* initialization function should have a counterpart in the same 
patch to free the memory.

> +{
> +    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;

Please explain the 0ULL.

> +    its->baser0  = GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT;
> +    its->baser0 |= (7ULL << GITS_BASER_ENTRY_SIZE_SHIFT) | base_attr;

Please explain 7ULL. I suspect you can use a sizeof of the device structure.

> +    its->baser1  = GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT;
> +    its->baser1 |= (1ULL << GITS_BASER_ENTRY_SIZE_SHIFT) | base_attr;

Please explain 1ULL. I suspect you can use a sizeof of the collection 
structure.

> +    its->d = d;
> +    its->doorbell_address = guest_addr + ITS_DOORBELL_OFFSET;
> +    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);
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index d1382be..8faec95 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -31,6 +31,7 @@
>  #include <asm/current.h>
>  #include <asm/mmio.h>
>  #include <asm/gic_v3_defs.h>
> +#include <asm/gic_v3_its.h>
>  #include <asm/vgic.h>
>  #include <asm/vgic-emul.h>
>  #include <asm/vreg.h>
> @@ -1651,6 +1652,7 @@ static int vgic_v3_domain_init(struct domain *d)
>       */
>      if ( is_hardware_domain(d) )
>      {
> +        struct host_its *hw_its;
>          unsigned int first_cpu = 0;
>
>          d->arch.vgic.dbase = vgic_v3_hw.dbase;
> @@ -1676,6 +1678,16 @@ static int vgic_v3_domain_init(struct domain *d)
>
>              first_cpu += size / d->arch.vgic.rdist_stride;
>          }
> +        d->arch.vgic.nr_regions = vgic_v3_hw.nr_rdist_regions;

Why did you add that?

> +
> +        list_for_each_entry(hw_its, &host_its_list, entry)

This could be done in vgic-v3-its.c. Also, I would prefer to see 
something similar to vgic_v3_setup_hw for ITS to make the code agnostic.

> +        {
> +            /* Emulate the control registers frame (lower 64K). */
> +            vgic_v3_its_init_virtual(d, hw_its->addr);

You likely need to pass more information to the vITS such as the number 
of deviceID, EventID...

Also, please check the return value.

> +
> +            d->arch.vgic.has_its = true;
> +        }
> +
>      }
>      else
>      {
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 33c1851..27cc310 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -115,6 +115,7 @@ struct arch_domain
>          uint8_t *proptable;
>          struct rb_root its_devices;         /* devices mapped to an ITS */
>          spinlock_t its_devices_lock;        /* protects the its_devices tree */
> +        bool has_its;
>  #endif
>      } vgic;
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 25b6b3c..55ef143 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -148,6 +148,13 @@ uint64_t gicv3_get_redist_address(unsigned int cpu, bool use_pta);
>  int gicv3_its_setup_collection(unsigned int cpu);
>
>  /*
> + * Create and register a virtual ITS at the given guest address.
> + * If a host ITS is specified, a hardware domain can reach out to that host
> + * ITS to deal with devices and LPI mappings and can enable/disable LPIs.
> + */
> +int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr);
> +
> +/*
>   * 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.
>   * Setting "valid" to false deallocates the device.
> @@ -211,6 +218,11 @@ static inline int gicv3_its_setup_collection(unsigned int cpu)
>      return 0;
>  }
>
> +static inline int vgic_v3_its_init_virtual(struct domain *d, paddr_t guest_addr)
> +{
> +    return 0;
> +}
> +
>  #endif /* CONFIG_HAS_ITS */
>
>  #endif
>

Cheers,

-- 
Julien Grall

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