[RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Manish Jaggi]

This patch supports ITS in hardware domain, supports ITS in Xen
when booting with ACPI.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
Changes since v1:
- Moved its specific code to gic-v3-its.c
- fixed macros

  xen/arch/arm/domain_build.c      |  6 ++--
  xen/arch/arm/gic-v3-its.c        | 75 
+++++++++++++++++++++++++++++++++++++++-
  xen/arch/arm/gic-v3.c            | 10 ++++--
  xen/include/asm-arm/gic_v3_its.h |  6 ++++
  4 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3abacc0..d6d6c94 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -20,7 +20,7 @@
  #include <asm/psci.h>
  #include <asm/setup.h>
  #include <asm/cpufeature.h>
-
+#include <asm-arm/gic_v3_its.h>
  #include <asm/gic.h>
  #include <xen/irq.h>
  #include <xen/grant_table.h>
@@ -1804,7 +1804,9 @@ static int estimate_acpi_efi_size(struct domain 
*d, struct kernel_info *kinfo)

      madt_size = sizeof(struct acpi_table_madt)
                  + sizeof(struct acpi_madt_generic_interrupt) * 
d->max_vcpus
-                + sizeof(struct acpi_madt_generic_distributor);
+                + sizeof(struct acpi_madt_generic_distributor)
+                + gicv3_its_madt_generic_translator_size();
+
      if ( d->arch.vgic.version == GIC_V3 )
          madt_size += sizeof(struct acpi_madt_generic_redistributor)
                       * d->arch.vgic.nr_regions;
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 1fb06ca..937b970 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -25,14 +25,18 @@
  #include <xen/rbtree.h>
  #include <xen/sched.h>
  #include <xen/sizes.h>
+#include <xen/iocap.h>
  #include <asm/gic.h>
  #include <asm/gic_v3_defs.h>
  #include <asm/gic_v3_its.h>
  #include <asm/io.h>
  #include <asm/page.h>
+#include <xen/acpi.h>
+#include <acpi/actables.h>
+#include <xen/pfn.h>

  #define ITS_CMD_QUEUE_SZ                SZ_1M
-
+#define ACPI_GICV3_ITS_MEM_SIZE (SZ_64K)
  /*
   * No lock here, as this list gets only populated upon boot while scanning
   * firmware tables for all host ITSes, and only gets iterated afterwards.
@@ -920,6 +924,55 @@ int gicv3_lpi_change_vcpu(struct domain *d, paddr_t 
vdoorbell,
      return 0;
  }

+int gicv3_its_deny_access(const struct domain *d)
+{
+    int rc = 0;
+    unsigned long mfn, nr;
+    const struct host_its *its_data;
+
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        mfn = paddr_to_pfn(its_data->addr);
+        nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
+        rc = iomem_deny_access(d, mfn, mfn + nr);
+        if ( rc )
+            goto end;
+    }
+end:
+    return rc;
+}
+
+u32 gicv3_its_madt_generic_translator_size(void)
+{
+    const struct host_its *its_data;
+    u32 size = 0;
+
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        size += sizeof(struct acpi_madt_generic_translator);
+    }
+    return size;
+}
+
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
+{
+    struct acpi_madt_generic_translator *gic_its;
+    const struct host_its *its_data;
+    u32 table_len = offset, size;
+
+    /* Update GIC ITS information in hardware domain's MADT */
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        size = sizeof(struct acpi_madt_generic_translator);
+        gic_its = (struct acpi_madt_generic_translator *)(base_ptr + 
table_len);
+        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
+        gic_its->header.length = size;
+        gic_its->base_address = its_data->addr;
+        table_len +=  size;
+    }
+    return table_len;
+}
+
  /*
   * Create the respective guest DT nodes from a list of host ITSes.
   * This copies the reg property, so the guest sees the ITS at the same 
address
@@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct 
domain *d,
      return res;
  }

+int gicv3_its_acpi_init(struct acpi_subtable_header *header, const 
unsigned long end)
+{
+    struct acpi_madt_generic_translator *its_entry;
+    struct host_its *its_data;
+
+    its_data = xzalloc(struct host_its);
+    if (!its_data)
+        return -1;
+
+    its_entry = (struct acpi_madt_generic_translator *)header;
+    its_data->addr  = its_entry->base_address;
+    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
+
+    spin_lock_init(&its_data->cmd_lock);
+
+    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
+
+    list_add_tail(&its_data->entry, &host_its_list);
+    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.c b/xen/arch/arm/gic-v3.c
index c927306..f0f6d12 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct 
domain *d)
          return iomem_deny_access(d, mfn, mfn + nr);
      }

-    return 0;
+    return gicv3_its_deny_access(d);
  }
-
  #ifdef CONFIG_ACPI
  static void __init
  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool single_rdist)
@@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct 
domain *d, u32 offset)
      for ( i = 0; i < d->max_vcpus; i++ )
      {
          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr + 
table_len);
+
          ACPI_MEMCPY(gicc, host_gicc, size);
          gicc->cpu_interface_number = i;
          gicc->uid = i;
@@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct 
domain *d, u32 offset)
          gicr->length = d->arch.vgic.rdist_regions[i].size;
          table_len += size;
      }
-
+    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
      return table_len;
  }

@@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)

      gicv3.rdist_stride = 0;

+    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+                          gicv3_its_acpi_init, 0);
+
      /*
       * In ACPI, 0 is considered as the invalid address. However the rest
       * of the initialization rely on the invalid address to be
@@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
      else
          vsize = GUEST_GICC_SIZE;

+
  }
  #else
  static void __init gicv3_acpi_init(void) { }
diff --git a/xen/include/asm-arm/gic_v3_its.h 
b/xen/include/asm-arm/gic_v3_its.h
index d2a3e53..b72aec2 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -105,6 +105,7 @@

  #include <xen/device_tree.h>
  #include <xen/rbtree.h>
+#include <xen/acpi.h>

  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
  #define HOST_ITS_USES_PTA               (1U << 1)
@@ -134,6 +135,7 @@ extern struct list_head host_its_list;

  /* Parse the host DT and pick up all host ITSes. */
  void gicv3_its_dt_init(const struct dt_device_node *node);
+int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const 
unsigned long end);

  bool gicv3_its_host_has_its(void);

@@ -167,6 +169,10 @@ int gicv3_its_make_hwdom_dt_nodes(const struct 
domain *d,
                                    const struct dt_device_node *gic,
                                    void *fdt);

+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
+u32 gicv3_its_madt_generic_translator_size(void);
+/* Deny iomem access for its */
+int gicv3_its_deny_access(const struct domain *d);
  /*
   * 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.
-- 
2.7.4



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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Julien Grall]

Hi,

Please CC all relevant maintainers.

On 08/06/17 14:03, Manish Jaggi wrote:
>

Spurious newline

> This patch supports ITS in hardware domain, supports ITS in Xen
> when booting with ACPI.
>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
> Changes since v1:
> - Moved its specific code to gic-v3-its.c
> - fixed macros

It sounds like you haven't addressed all my comments. I will repeat them 
for this time. But next time, I will not bother reviewing your patch.

>
>  xen/arch/arm/domain_build.c      |  6 ++--
>  xen/arch/arm/gic-v3-its.c        | 75
> +++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/gic-v3.c            | 10 ++++--
>  xen/include/asm-arm/gic_v3_its.h |  6 ++++
>  4 files changed, 91 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3abacc0..d6d6c94 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -20,7 +20,7 @@
>  #include <asm/psci.h>
>  #include <asm/setup.h>
>  #include <asm/cpufeature.h>
> -

Why did you drop this newline?

> +#include <asm-arm/gic_v3_its.h>

Nack. I asked on v1 to separate code between GICv3 and ITS, it is not 
for directly calling gicv3 code directly in the common code.

If you need to call GICv3 specific code, then introduce a callback in 
gic_hw_operations.

>  #include <asm/gic.h>
>  #include <xen/irq.h>
>  #include <xen/grant_table.h>
> @@ -1804,7 +1804,9 @@ static int estimate_acpi_efi_size(struct domain
> *d, struct kernel_info *kinfo)
>
>      madt_size = sizeof(struct acpi_table_madt)
>                  + sizeof(struct acpi_madt_generic_interrupt) *
> d->max_vcpus
> -                + sizeof(struct acpi_madt_generic_distributor);
> +                + sizeof(struct acpi_madt_generic_distributor)
> +                + gicv3_its_madt_generic_translator_size();

See my comment above.

> +
>      if ( d->arch.vgic.version == GIC_V3 )
>          madt_size += sizeof(struct acpi_madt_generic_redistributor)
>                       * d->arch.vgic.nr_regions;
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 1fb06ca..937b970 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -25,14 +25,18 @@
>  #include <xen/rbtree.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/iocap.h>

The include are ordered alphabetically, please respect it.

>  #include <asm/gic.h>
>  #include <asm/gic_v3_defs.h>
>  #include <asm/gic_v3_its.h>
>  #include <asm/io.h>
>  #include <asm/page.h>
> +#include <xen/acpi.h>
> +#include <acpi/actables.h>
> +#include <xen/pfn.h>

Ditto.

>
>  #define ITS_CMD_QUEUE_SZ                SZ_1M
> -

Again, we don't drop newline for no reason.

> +#define ACPI_GICV3_ITS_MEM_SIZE (SZ_64K)
>  /*
>   * No lock here, as this list gets only populated upon boot while scanning
>   * firmware tables for all host ITSes, and only gets iterated afterwards.
> @@ -920,6 +924,55 @@ int gicv3_lpi_change_vcpu(struct domain *d, paddr_t
> vdoorbell,
>      return 0;
>  }
>
> +int gicv3_its_deny_access(const struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned long mfn, nr;
> +    const struct host_its *its_data;
> +
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +    {
> +        mfn = paddr_to_pfn(its_data->addr);
> +        nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
> +        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        if ( rc )
> +            goto end;

Hmmm, why not using a break here rather than a goto?

> +    }
> +end:
> +    return rc;
> +}
> +
> +u32 gicv3_its_madt_generic_translator_size(void)
> +{
> +    const struct host_its *its_data;
> +    u32 size = 0;
> +
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +    {

Pointless {

> +        size += sizeof(struct acpi_madt_generic_translator);
> +    }

Same here + add a newline.

> +    return size;
> +}
> +
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
> +{
> +    struct acpi_madt_generic_translator *gic_its;
> +    const struct host_its *its_data;
> +    u32 table_len = offset, size;
> +
> +    /* Update GIC ITS information in hardware domain's MADT */
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +    {
> +        size = sizeof(struct acpi_madt_generic_translator);
> +        gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
> table_len);

This line is likely too long.

> +        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
> +        gic_its->header.length = size;
> +        gic_its->base_address = its_data->addr;

On the previous patch you had:

gic_its->translation_id = its_data->translation_id;

I asked to explain why you need to have the same ID as the host. And now 
you dropped it. This does not match the spec (Table 5-67 in ACPI 6.1):

"GIC ITS ID. In a system with multiple GIC ITS units, this value must
be unique to each one."

But here, the ITS ID will not be unique. So why did you dropped it?

> +        table_len +=  size;
> +    }
> +    return table_len;
> +}
> +
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
>   * This copies the reg property, so the guest sees the ITS at the same
> address
> @@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
> domain *d,
>      return res;
>  }
>
> +int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
> unsigned long end)

ACPI is an option and is not able by default. Please make sure that this 
code build without ACPI. Likely this means surrounding with #ifdef 
CONFIG_ACPI.

> +{
> +    struct acpi_madt_generic_translator *its_entry;
> +    struct host_its *its_data;
> +
> +    its_data = xzalloc(struct host_its);
> +    if (!its_data)

Coding style.

> +        return -1;
> +
> +    its_entry = (struct acpi_madt_generic_translator *)header;
> +    its_data->addr  = its_entry->base_address;
> +    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
> +
> +    spin_lock_init(&its_data->cmd_lock);
> +
> +    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
> +
> +    list_add_tail(&its_data->entry, &host_its_list);

As said on v1, likely you could re-use factorize a part of 
gicv3_its_dt_init to avoid implementing twice the initialization.

Also newline.

> +    return 0;
> +}

Newline here.

>  /* 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.c b/xen/arch/arm/gic-v3.c
> index c927306..f0f6d12 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
> domain *d)
>          return iomem_deny_access(d, mfn, mfn + nr);
>      }
>
> -    return 0;
> +    return gicv3_its_deny_access(d);

Copying my answer from v1 for convenience:

     if ( vbase != INVALID_PADDR )
     {
         mfn = vbase >> PAGE_SHIFT;
         nr = DIV_ROUND_UP(csize, PAGE_SIZE);
         return iomem_deny_access(d, mfn, mfn + nr);
     }

When GICv3 is able to support GICv2, vbase will be valid and the code 
will bail out after denying access to the GICV. So the ITS regions will 
not be denied.


>  }
> -

Again, why did you drop this newline?

>  #ifdef CONFIG_ACPI
>  static void __init
>  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool single_rdist)
> @@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
> domain *d, u32 offset)
>      for ( i = 0; i < d->max_vcpus; i++ )
>      {
>          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
> table_len);
> +

As said on v1, spurious change.

>          ACPI_MEMCPY(gicc, host_gicc, size);
>          gicc->cpu_interface_number = i;
>          gicc->uid = i;
> @@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
> domain *d, u32 offset)
>          gicr->length = d->arch.vgic.rdist_regions[i].size;
>          table_len += size;
>      }
> -

Again why did you drop the newline?

> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
>      return table_len;
>  }
>
> @@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
>
>      gicv3.rdist_stride = 0;
>
> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> +                          gicv3_its_acpi_init, 0);

As said on v1, acpi_table_parse_madt may return an error. Why this is 
not checked?

> +
>      /*
>       * In ACPI, 0 is considered as the invalid address. However the rest
>       * of the initialization rely on the invalid address to be
> @@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
>      else
>          vsize = GUEST_GICC_SIZE;
>
> +

As said on v1, this is a spurious newline.

>  }
>  #else
>  static void __init gicv3_acpi_init(void) { }
> diff --git a/xen/include/asm-arm/gic_v3_its.h
> b/xen/include/asm-arm/gic_v3_its.h
> index d2a3e53..b72aec2 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -105,6 +105,7 @@
>
>  #include <xen/device_tree.h>
>  #include <xen/rbtree.h>
> +#include <xen/acpi.h>
>
>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>  #define HOST_ITS_USES_PTA               (1U << 1)
> @@ -134,6 +135,7 @@ extern struct list_head host_its_list;
>
>  /* Parse the host DT and pick up all host ITSes. */
>  void gicv3_its_dt_init(const struct dt_device_node *node);
> +int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const
> unsigned long end);

This will likely need an #ifdef CONFIG_ACPI. And also a stub would be 
required if ITS is disabled.

>
>  bool gicv3_its_host_has_its(void);
>
> @@ -167,6 +169,10 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
> domain *d,
>                                    const struct dt_device_node *gic,
>                                    void *fdt);
>
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
> +u32 gicv3_its_madt_generic_translator_size(void);
> +/* Deny iomem access for its */
> +int gicv3_its_deny_access(const struct domain *d);

Same here.

>  /*
>   * 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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Manish Jaggi]

On 6/8/2017 7:28 PM, Julien Grall wrote:
> Hi,
Hello Julien,
>
> Please CC all relevant maintainers.
Sure. Will do in the next patch rev.
>
> On 08/06/17 14:03, Manish Jaggi wrote:
>>
>
> Spurious newline
>
>> This patch supports ITS in hardware domain, supports ITS in Xen
>> when booting with ACPI.
>>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>> Changes since v1:
>> - Moved its specific code to gic-v3-its.c
>> - fixed macros
>
> It sounds like you haven't addressed all my comments. I will repeat 
> them for this time. But next time, I will not bother reviewing your 
> patch.
*Thanks* for reviewing the patch, I will try to address _all_ the comments
>
>>
>>  xen/arch/arm/domain_build.c      |  6 ++--
>>  xen/arch/arm/gic-v3-its.c        | 75
>> +++++++++++++++++++++++++++++++++++++++-
>>  xen/arch/arm/gic-v3.c            | 10 ++++--
>>  xen/include/asm-arm/gic_v3_its.h |  6 ++++
>>  4 files changed, 91 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 3abacc0..d6d6c94 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -20,7 +20,7 @@
>>  #include <asm/psci.h>
>>  #include <asm/setup.h>
>>  #include <asm/cpufeature.h>
>> -
>
> Why did you drop this newline?
I will fix it.
>
>> +#include <asm-arm/gic_v3_its.h>
>
> Nack. I asked on v1 to separate code between GICv3 and ITS, it is not 
> for directly calling gicv3 code directly in the common code.
>
> If you need to call GICv3 specific code, then introduce a callback in 
> gic_hw_operations.
>
Good point, I will add it.
>>  #include <asm/gic.h>
>>  #include <xen/irq.h>
>>  #include <xen/grant_table.h>
>> @@ -1804,7 +1804,9 @@ static int estimate_acpi_efi_size(struct domain
>> *d, struct kernel_info *kinfo)
>>
>>      madt_size = sizeof(struct acpi_table_madt)
>>                  + sizeof(struct acpi_madt_generic_interrupt) *
>> d->max_vcpus
>> -                + sizeof(struct acpi_madt_generic_distributor);
>> +                + sizeof(struct acpi_madt_generic_distributor)
>> +                + gicv3_its_madt_generic_translator_size();
>
> See my comment above.
Will address it.
>
>> +
>>      if ( d->arch.vgic.version == GIC_V3 )
>>          madt_size += sizeof(struct acpi_madt_generic_redistributor)
>>                       * d->arch.vgic.nr_regions;
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 1fb06ca..937b970 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -25,14 +25,18 @@
>>  #include <xen/rbtree.h>
>>  #include <xen/sched.h>
>>  #include <xen/sizes.h>
>> +#include <xen/iocap.h>
>
> The include are ordered alphabetically, please respect it.
>
Sure. I will fix it.
>>  #include <asm/gic.h>
>>  #include <asm/gic_v3_defs.h>
>>  #include <asm/gic_v3_its.h>
>>  #include <asm/io.h>
>>  #include <asm/page.h>
>> +#include <xen/acpi.h>
>> +#include <acpi/actables.h>
>> +#include <xen/pfn.h>
>
> Ditto.
>
Sure. I will fix it.
>>
>>  #define ITS_CMD_QUEUE_SZ                SZ_1M
>> -
>
> Again, we don't drop newline for no reason.
I will fix it.
>
>> +#define ACPI_GICV3_ITS_MEM_SIZE (SZ_64K)
>>  /*
>>   * No lock here, as this list gets only populated upon boot while 
>> scanning
>>   * firmware tables for all host ITSes, and only gets iterated 
>> afterwards.
>> @@ -920,6 +924,55 @@ int gicv3_lpi_change_vcpu(struct domain *d, paddr_t
>> vdoorbell,
>>      return 0;
>>  }
>>
>> +int gicv3_its_deny_access(const struct domain *d)
>> +{
>> +    int rc = 0;
>> +    unsigned long mfn, nr;
>> +    const struct host_its *its_data;
>> +
>> +    list_for_each_entry(its_data, &host_its_list, entry)
>> +    {
>> +        mfn = paddr_to_pfn(its_data->addr);
>> +        nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
>> +        rc = iomem_deny_access(d, mfn, mfn + nr);
>> +        if ( rc )
>> +            goto end;
>
> Hmmm, why not using a break here rather than a goto?
I can use break, np.
>
>> +    }
>> +end:
>> +    return rc;
>> +}
>> +
>> +u32 gicv3_its_madt_generic_translator_size(void)
>> +{
>> +    const struct host_its *its_data;
>> +    u32 size = 0;
>> +
>> +    list_for_each_entry(its_data, &host_its_list, entry)
>> +    {
>
> Pointless {
>
>> +        size += sizeof(struct acpi_madt_generic_translator);
>> +    }
Just for readability of code.
>
> Same here + add a newline.
>
Sure.
>> +    return size;
>> +}
>> +
>> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
>> +{
>> +    struct acpi_madt_generic_translator *gic_its;
>> +    const struct host_its *its_data;
>> +    u32 table_len = offset, size;
>> +
>> +    /* Update GIC ITS information in hardware domain's MADT */
>> +    list_for_each_entry(its_data, &host_its_list, entry)
>> +    {
>> +        size = sizeof(struct acpi_madt_generic_translator);
>> +        gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
>> table_len);
>
> This line is likely too long.
>
I will check it.
>> +        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
>> +        gic_its->header.length = size;
>> +        gic_its->base_address = its_data->addr;
>
> On the previous patch you had:
>
> gic_its->translation_id = its_data->translation_id;
>
> I asked to explain why you need to have the same ID as the host. And 
> now you dropped it. This does not match the spec (Table 5-67 in ACPI 
> 6.1):
>
> "GIC ITS ID. In a system with multiple GIC ITS units, this value must
> be unique to each one."
>
> But here, the ITS ID will not be unique. So why did you dropped it?
>
The reason I dropped it from its_data as I was not setting it. So it 
doesn't belong there.
Will the below code be ok?
+ int tras_id = 0;
+ list_for_each_entry(its_data, &host_its_list, entry)
+ {
+    gic_its->translation_id = ++trans_id;

>> +        table_len +=  size;
>> +    }
>> +    return table_len;
>> +}
>> +
>>  /*
>>   * Create the respective guest DT nodes from a list of host ITSes.
>>   * This copies the reg property, so the guest sees the ITS at the same
>> address
>> @@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
>> domain *d,
>>      return res;
>>  }
>>
>> +int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>> unsigned long end)
>
> ACPI is an option and is not able by default. Please make sure that 
> this code build without ACPI. Likely this means surrounding with 
> #ifdef CONFIG_ACPI.
I will get compiled but not called. Do you still want to put ifdef, i 
can add that.
>
>> +{
>> +    struct acpi_madt_generic_translator *its_entry;
>> +    struct host_its *its_data;
>> +
>> +    its_data = xzalloc(struct host_its);
>> +    if (!its_data)
>
> Coding style.
>
Sure.
>> +        return -1;
>> +
>> +    its_entry = (struct acpi_madt_generic_translator *)header;
>> +    its_data->addr  = its_entry->base_address;
>> +    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
>> +
>> +    spin_lock_init(&its_data->cmd_lock);
>> +
>> +    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
>> +
>> +    list_add_tail(&its_data->entry, &host_its_list);
>
> As said on v1, likely you could re-use factorize a part of 
> gicv3_its_dt_init to avoid implementing twice the initialization.
>
For this I have a different opinion.
gicv3_its_dt_init has a loop  dt_for_each_child_node(node, its) while 
gicv3_its_acpi_init is a callback.
Moreover,  apart from xzalloc and list_add_tail most of the code is 
different. so IMHO keeping them separate is better.

> Also newline.
>
>> +    return 0;
>> +}
>
> Newline here.
Sure.
>
>>  /* 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.c b/xen/arch/arm/gic-v3.c
>> index c927306..f0f6d12 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
>> domain *d)
>>          return iomem_deny_access(d, mfn, mfn + nr);
>>      }
>>
>> -    return 0;
>> +    return gicv3_its_deny_access(d);
>
> Copying my answer from v1 for convenience:
>
>     if ( vbase != INVALID_PADDR )
>     {
>         mfn = vbase >> PAGE_SHIFT;
>         nr = DIV_ROUND_UP(csize, PAGE_SIZE);
>         return iomem_deny_access(d, mfn, mfn + nr);
>     }
>
> When GICv3 is able to support GICv2, vbase will be valid and the code 
> will bail out after denying access to the GICV. So the ITS regions 
> will not be denied.
>
Ok, got your point. Would the below code be ok?
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c927306..a3d1eff 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct 
domain *d)
      if ( rc )
          return rc;

+    if ( gicv3_its_host_has_its() )
+    {
+        rc = gicv3_its_deny_access(d);
+        if ( rc )
+            return rc;
+    }
+
      for ( i = 0; i < gicv3.rdist_count; i++ )
      {
          mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
>>  }
>> -
>
> Again, why did you drop this newline?
I will fix it
>
>>  #ifdef CONFIG_ACPI
>>  static void __init
>>  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool 
>> single_rdist)
>> @@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
>> domain *d, u32 offset)
>>      for ( i = 0; i < d->max_vcpus; i++ )
>>      {
>>          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
>> table_len);
>> +
>
> As said on v1, spurious change.
No. I wanted to add it intentionally. for a better code readability.
>
>>          ACPI_MEMCPY(gicc, host_gicc, size);
>>          gicc->cpu_interface_number = i;
>>          gicc->uid = i;
>> @@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
>> domain *d, u32 offset)
>>          gicr->length = d->arch.vgic.rdist_regions[i].size;
>>          table_len += size;
>>      }
>> -
>
> Again why did you drop the newline?
I will fix it
>
>> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
>>      return table_len;
>>  }
>>
>> @@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
>>
>>      gicv3.rdist_stride = 0;
>>
>> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> +                          gicv3_its_acpi_init, 0);
>
> As said on v1, acpi_table_parse_madt may return an error. Why this is 
> not checked?
>
I missed that, I will add it,
>> +
>>      /*
>>       * In ACPI, 0 is considered as the invalid address. However the 
>> rest
>>       * of the initialization rely on the invalid address to be
>> @@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
>>      else
>>          vsize = GUEST_GICC_SIZE;
>>
>> +
>
> As said on v1, this is a spurious newline.
ok
>
>>  }
>>  #else
>>  static void __init gicv3_acpi_init(void) { }
>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>> b/xen/include/asm-arm/gic_v3_its.h
>> index d2a3e53..b72aec2 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -105,6 +105,7 @@
>>
>>  #include <xen/device_tree.h>
>>  #include <xen/rbtree.h>
>> +#include <xen/acpi.h>
>>
>>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>>  #define HOST_ITS_USES_PTA               (1U << 1)
>> @@ -134,6 +135,7 @@ extern struct list_head host_its_list;
>>
>>  /* Parse the host DT and pick up all host ITSes. */
>>  void gicv3_its_dt_init(const struct dt_device_node *node);
>> +int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>> unsigned long end);
>
> This will likely need an #ifdef CONFIG_ACPI. And also a stub would be 
> required if ITS is disabled.
>
Sorry didnt got your point. on ifdef.
I can add a check gicv3_its_host_has_its() before calling 
gicv3_its_acpi_init, would that be ok?
>>
>>  bool gicv3_its_host_has_its(void);
>>
>> @@ -167,6 +169,10 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
>> domain *d,
>>                                    const struct dt_device_node *gic,
>>                                    void *fdt);
>>
>> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
>> +u32 gicv3_its_madt_generic_translator_size(void);
>> +/* Deny iomem access for its */
>> +int gicv3_its_deny_access(const struct domain *d);
>
> Same here.
>
Please see my response above.
>>  /*
>>   * 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,
>


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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Julien Grall]

On 09/06/2017 07:48, Manish Jaggi wrote:
>
> On 6/8/2017 7:28 PM, Julien Grall wrote:
>> Hi,
> Hello Julien,

Hello,

>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>> +    {
>>
>> Pointless {
>>
>>> +        size += sizeof(struct acpi_madt_generic_translator);
>>> +    }
> Just for readability of code.

You have indentation for that. So I don't think it helps.

>>
>> Same here + add a newline.
>>
> Sure.
>>> +    return size;
>>> +}
>>> +
>>> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
>>> +{
>>> +    struct acpi_madt_generic_translator *gic_its;
>>> +    const struct host_its *its_data;
>>> +    u32 table_len = offset, size;
>>> +
>>> +    /* Update GIC ITS information in hardware domain's MADT */
>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>> +    {
>>> +        size = sizeof(struct acpi_madt_generic_translator);
>>> +        gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
>>> table_len);
>>
>> This line is likely too long.
>>
> I will check it.
>>> +        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
>>> +        gic_its->header.length = size;
>>> +        gic_its->base_address = its_data->addr;
>>
>> On the previous patch you had:
>>
>> gic_its->translation_id = its_data->translation_id;
>>
>> I asked to explain why you need to have the same ID as the host. And
>> now you dropped it. This does not match the spec (Table 5-67 in ACPI
>> 6.1):
>>
>> "GIC ITS ID. In a system with multiple GIC ITS units, this value must
>> be unique to each one."
>>
>> But here, the ITS ID will not be unique. So why did you dropped it?
>>
> The reason I dropped it from its_data as I was not setting it. So it
> doesn't belong there.

Where would it belong then?

This function is used to generate ACPI tables for the hardware domain.


> Will the below code be ok?

If you noticed, I didn't say this code is wrong. Instead I asked why you 
use the same ID. Meaning, is there anything in the DSDT requiring this 
value?

> + int tras_id = 0;

unsigned.

> + list_for_each_entry(its_data, &host_its_list, entry)
> + {
> +    gic_its->translation_id = ++trans_id;

You start the translation ID at 1. Why?

>
>>> +        table_len +=  size;
>>> +    }
>>> +    return table_len;
>>> +}
>>> +
>>>  /*
>>>   * Create the respective guest DT nodes from a list of host ITSes.
>>>   * This copies the reg property, so the guest sees the ITS at the same
>>> address
>>> @@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
>>> domain *d,
>>>      return res;
>>>  }
>>>
>>> +int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>> unsigned long end)
>>
>> ACPI is an option and is not able by default. Please make sure that
>> this code build without ACPI. Likely this means surrounding with
>> #ifdef CONFIG_ACPI.
> I will get compiled but not called. Do you still want to put ifdef, i
> can add that.

All ACPIs functions are protected by ifdef. So this one should be as well.

>>
>>> +{
>>> +    struct acpi_madt_generic_translator *its_entry;
>>> +    struct host_its *its_data;
>>> +
>>> +    its_data = xzalloc(struct host_its);
>>> +    if (!its_data)
>>
>> Coding style.
>>
> Sure.
>>> +        return -1;
>>> +
>>> +    its_entry = (struct acpi_madt_generic_translator *)header;
>>> +    its_data->addr  = its_entry->base_address;
>>> +    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
>>> +
>>> +    spin_lock_init(&its_data->cmd_lock);
>>> +
>>> +    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
>>> +
>>> +    list_add_tail(&its_data->entry, &host_its_list);
>>
>> As said on v1, likely you could re-use factorize a part of
>> gicv3_its_dt_init to avoid implementing twice the initialization.
>>
> For this I have a different opinion.

Why didn't you state it on the previous version? I usually interpret a 
non-answer as an acknowledgment.

> gicv3_its_dt_init has a loop  dt_for_each_child_node(node, its) while
> gicv3_its_acpi_init is a callback.
> Moreover,  apart from xzalloc and list_add_tail most of the code is
> different. so IMHO keeping them separate is better.

You still set addr and size as in the DT counterpart. Also, this is a 
call to forget to initialize a field if we decided to extend the 
structure host_its. So I still don't see any reason to open-code it and 
take the risk to introduce bug in the future...

>> Also newline.
>>
>>> +    return 0;
>>> +}
>>
>> Newline here.
> Sure.
>>
>>>  /* 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.c b/xen/arch/arm/gic-v3.c
>>> index c927306..f0f6d12 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
>>> domain *d)
>>>          return iomem_deny_access(d, mfn, mfn + nr);
>>>      }
>>>
>>> -    return 0;
>>> +    return gicv3_its_deny_access(d);
>>
>> Copying my answer from v1 for convenience:
>>
>>     if ( vbase != INVALID_PADDR )
>>     {
>>         mfn = vbase >> PAGE_SHIFT;
>>         nr = DIV_ROUND_UP(csize, PAGE_SIZE);
>>         return iomem_deny_access(d, mfn, mfn + nr);
>>     }
>>
>> When GICv3 is able to support GICv2, vbase will be valid and the code
>> will bail out after denying access to the GICV. So the ITS regions
>> will not be denied.
>>
> Ok, got your point. Would the below code be ok?

LGTM.

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index c927306..a3d1eff 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct
> domain *d)
>      if ( rc )
>          return rc;
>
> +    if ( gicv3_its_host_has_its() )
> +    {
> +        rc = gicv3_its_deny_access(d);
> +        if ( rc )
> +            return rc;
> +    }
> +
>      for ( i = 0; i < gicv3.rdist_count; i++ )
>      {
>          mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
>>>  }
>>> -
>>
>> Again, why did you drop this newline?
> I will fix it
>>
>>>  #ifdef CONFIG_ACPI
>>>  static void __init
>>>  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool
>>> single_rdist)
>>> @@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
>>> domain *d, u32 offset)
>>>      for ( i = 0; i < d->max_vcpus; i++ )
>>>      {
>>>          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
>>> table_len);
>>> +
>>
>> As said on v1, spurious change.
> No. I wanted to add it intentionally. for a better code readability.

A general and simple rule is to separate code clean-up with new 
functionality. This is not a new functionality and therefore should not 
be there.

>>
>>>          ACPI_MEMCPY(gicc, host_gicc, size);
>>>          gicc->cpu_interface_number = i;
>>>          gicc->uid = i;
>>> @@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
>>> domain *d, u32 offset)
>>>          gicr->length = d->arch.vgic.rdist_regions[i].size;
>>>          table_len += size;
>>>      }
>>> -
>>
>> Again why did you drop the newline?
> I will fix it
>>
>>> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
>>>      return table_len;
>>>  }
>>>
>>> @@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
>>>
>>>      gicv3.rdist_stride = 0;
>>>
>>> +    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>> +                          gicv3_its_acpi_init, 0);
>>
>> As said on v1, acpi_table_parse_madt may return an error. Why this is
>> not checked?
>>
> I missed that, I will add it,
>>> +
>>>      /*
>>>       * In ACPI, 0 is considered as the invalid address. However the
>>> rest
>>>       * of the initialization rely on the invalid address to be
>>> @@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
>>>      else
>>>          vsize = GUEST_GICC_SIZE;
>>>
>>> +
>>
>> As said on v1, this is a spurious newline.
> ok
>>
>>>  }
>>>  #else
>>>  static void __init gicv3_acpi_init(void) { }
>>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>>> b/xen/include/asm-arm/gic_v3_its.h
>>> index d2a3e53..b72aec2 100644
>>> --- a/xen/include/asm-arm/gic_v3_its.h
>>> +++ b/xen/include/asm-arm/gic_v3_its.h
>>> @@ -105,6 +105,7 @@
>>>
>>>  #include <xen/device_tree.h>
>>>  #include <xen/rbtree.h>
>>> +#include <xen/acpi.h>
>>>
>>>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>>>  #define HOST_ITS_USES_PTA               (1U << 1)
>>> @@ -134,6 +135,7 @@ extern struct list_head host_its_list;
>>>
>>>  /* Parse the host DT and pick up all host ITSes. */
>>>  void gicv3_its_dt_init(const struct dt_device_node *node);
>>> +int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>> unsigned long end);
>>
>> This will likely need an #ifdef CONFIG_ACPI. And also a stub would be
>> required if ITS is disabled.
>>
> Sorry didnt got your point. on ifdef.

Look how gicv3_its_dt_init has been introduced. There is a stub for when 
ITS is not built-in. Furthermore, the function is ACPI specific are 
therefore should be protected with #ifdef CONFIG_ACPI.

> I can add a check gicv3_its_host_has_its() before calling
> gicv3_its_acpi_init, would that be ok?

gicv3_its_host_has_its will always return false as it is rely on the 
list of ITS to be populated. However, this will be populated by 
gicv3_its_acpi_init...

Regards,

-- 
Julien Grall

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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Manish Jaggi]

Hi julien,

On 6/9/2017 2:09 PM, Julien Grall wrote:
>
>
> On 09/06/2017 07:48, Manish Jaggi wrote:
>>
>> On 6/8/2017 7:28 PM, Julien Grall wrote:
>>> Hi,
>> Hello Julien,
>
> Hello,
>
>>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>>> +    {
>>>
>>> Pointless {
>>>
>>>> +        size += sizeof(struct acpi_madt_generic_translator);
>>>> +    }
>> Just for readability of code.
>
> You have indentation for that. So I don't think it helps.
ok i will fix it.
>
>>>
>>> Same here + add a newline.
>>>
>> Sure.
>>>> +    return size;
>>>> +}
>>>> +
>>>> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
>>>> +{
>>>> +    struct acpi_madt_generic_translator *gic_its;
>>>> +    const struct host_its *its_data;
>>>> +    u32 table_len = offset, size;
>>>> +
>>>> +    /* Update GIC ITS information in hardware domain's MADT */
>>>> +    list_for_each_entry(its_data, &host_its_list, entry)
>>>> +    {
>>>> +        size = sizeof(struct acpi_madt_generic_translator);
>>>> +        gic_its = (struct acpi_madt_generic_translator *)(base_ptr +
>>>> table_len);
>>>
>>> This line is likely too long.
>>>
>> I will check it.
>>>> +        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
>>>> +        gic_its->header.length = size;
>>>> +        gic_its->base_address = its_data->addr;
>>>
>>> On the previous patch you had:
>>>
>>> gic_its->translation_id = its_data->translation_id;
>>>
>>> I asked to explain why you need to have the same ID as the host. And
>>> now you dropped it. This does not match the spec (Table 5-67 in ACPI
>>> 6.1):
>>>
>>> "GIC ITS ID. In a system with multiple GIC ITS units, this value must
>>> be unique to each one."
>>>
>>> But here, the ITS ID will not be unique. So why did you dropped it?
>>>
>> The reason I dropped it from its_data as I was not setting it. So it
>> doesn't belong there.
>
> Where would it belong then?
>
> This function is used to generate ACPI tables for the hardware domain.
>
>
>> Will the below code be ok?
>
> If you noticed, I didn't say this code is wrong. Instead I asked why 
> you use the same ID. Meaning, is there anything in the DSDT requiring 
> this value?
>
>> + int tras_id = 0;
>
> unsigned.
>
>> + list_for_each_entry(its_data, &host_its_list, entry)
>> + {
>> +    gic_its->translation_id = ++trans_id;
>
> You start the translation ID at 1. Why?
>
as per the ACPI spec the value should be unique to each GIC ITS unit.
Does starting with 1 break anything? Or should I start with a magic number?
>>
>>>> +        table_len +=  size;
>>>> +    }
>>>> +    return table_len;
>>>> +}
>>>> +
>>>>  /*
>>>>   * Create the respective guest DT nodes from a list of host ITSes.
>>>>   * This copies the reg property, so the guest sees the ITS at the 
>>>> same
>>>> address
>>>> @@ -992,6 +1045,26 @@ int gicv3_its_make_hwdom_dt_nodes(const struct
>>>> domain *d,
>>>>      return res;
>>>>  }
>>>>
>>>> +int gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>>> unsigned long end)
>>>
>>> ACPI is an option and is not able by default. Please make sure that
>>> this code build without ACPI. Likely this means surrounding with
>>> #ifdef CONFIG_ACPI.
>> I will get compiled but not called. Do you still want to put ifdef, i
>> can add that.
>
> All ACPIs functions are protected by ifdef. So this one should be as 
> well.
ok will do.
>
>>>
>>>> +{
>>>> +    struct acpi_madt_generic_translator *its_entry;
>>>> +    struct host_its *its_data;
>>>> +
>>>> +    its_data = xzalloc(struct host_its);
>>>> +    if (!its_data)
>>>
>>> Coding style.
>>>
>> Sure.
>>>> +        return -1;
>>>> +
>>>> +    its_entry = (struct acpi_madt_generic_translator *)header;
>>>> +    its_data->addr  = its_entry->base_address;
>>>> +    its_data->size = ACPI_GICV3_ITS_MEM_SIZE;
>>>> +
>>>> +    spin_lock_init(&its_data->cmd_lock);
>>>> +
>>>> +    printk("GICv3: Found ITS @0x%lx\n", its_data->addr);
>>>> +
>>>> +    list_add_tail(&its_data->entry, &host_its_list);
>>>
>>> As said on v1, likely you could re-use factorize a part of
>>> gicv3_its_dt_init to avoid implementing twice the initialization.
>>>
>> For this I have a different opinion.
>
> Why didn't you state it on the previous version? I usually interpret a 
> non-answer as an acknowledgment.
>
>> gicv3_its_dt_init has a loop dt_for_each_child_node(node, its) while
>> gicv3_its_acpi_init is a callback.
>> Moreover,  apart from xzalloc and list_add_tail most of the code is
>> different. so IMHO keeping them separate is better.
>
> You still set addr and size as in the DT counterpart. Also, this is a 
> call to forget to initialize a field if we decided to extend the 
> structure host_its. So I still don't see any reason to open-code it 
> and take the risk to introduce bug in the future...
ok Added.
>
>>> Also newline.
>>>
>>>> +    return 0;
>>>> +}
>>>
>>> Newline here.
>> Sure.
>>>
>>>>  /* 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.c b/xen/arch/arm/gic-v3.c
>>>> index c927306..f0f6d12 100644
>>>> --- a/xen/arch/arm/gic-v3.c
>>>> +++ b/xen/arch/arm/gic-v3.c
>>>> @@ -1333,9 +1333,8 @@ static int gicv3_iomem_deny_access(const struct
>>>> domain *d)
>>>>          return iomem_deny_access(d, mfn, mfn + nr);
>>>>      }
>>>>
>>>> -    return 0;
>>>> +    return gicv3_its_deny_access(d);
>>>
>>> Copying my answer from v1 for convenience:
>>>
>>>     if ( vbase != INVALID_PADDR )
>>>     {
>>>         mfn = vbase >> PAGE_SHIFT;
>>>         nr = DIV_ROUND_UP(csize, PAGE_SIZE);
>>>         return iomem_deny_access(d, mfn, mfn + nr);
>>>     }
>>>
>>> When GICv3 is able to support GICv2, vbase will be valid and the code
>>> will bail out after denying access to the GICV. So the ITS regions
>>> will not be denied.
>>>
>> Ok, got your point. Would the below code be ok?
>
> LGTM.
>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index c927306..a3d1eff 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct
>> domain *d)
>>      if ( rc )
>>          return rc;
>>
>> +    if ( gicv3_its_host_has_its() )
>> +    {
>> +        rc = gicv3_its_deny_access(d);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>>      for ( i = 0; i < gicv3.rdist_count; i++ )
>>      {
>>          mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
>>>>  }
>>>> -
>>>
>>> Again, why did you drop this newline?
>> I will fix it
>>>
>>>>  #ifdef CONFIG_ACPI
>>>>  static void __init
>>>>  gic_acpi_add_rdist_region(paddr_t base, paddr_t size, bool
>>>> single_rdist)
>>>> @@ -1374,6 +1373,7 @@ static int gicv3_make_hwdom_madt(const struct
>>>> domain *d, u32 offset)
>>>>      for ( i = 0; i < d->max_vcpus; i++ )
>>>>      {
>>>>          gicc = (struct acpi_madt_generic_interrupt *)(base_ptr +
>>>> table_len);
>>>> +
>>>
>>> As said on v1, spurious change.
>> No. I wanted to add it intentionally. for a better code readability.
>
> A general and simple rule is to separate code clean-up with new 
> functionality. This is not a new functionality and therefore should 
> not be there.
ok
>
>>>
>>>>          ACPI_MEMCPY(gicc, host_gicc, size);
>>>>          gicc->cpu_interface_number = i;
>>>>          gicc->uid = i;
>>>> @@ -1399,7 +1399,7 @@ static int gicv3_make_hwdom_madt(const struct
>>>> domain *d, u32 offset)
>>>>          gicr->length = d->arch.vgic.rdist_regions[i].size;
>>>>          table_len += size;
>>>>      }
>>>> -
>>>
>>> Again why did you drop the newline?
>> I will fix it
>>>
>>>> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
>>>>      return table_len;
>>>>  }
>>>>
>>>> @@ -1567,6 +1567,9 @@ static void __init gicv3_acpi_init(void)
>>>>
>>>>      gicv3.rdist_stride = 0;
>>>>
>>>> + acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>>> +                          gicv3_its_acpi_init, 0);
>>>
>>> As said on v1, acpi_table_parse_madt may return an error. Why this is
>>> not checked?
>>>
>> I missed that, I will add it,
>>>> +
>>>>      /*
>>>>       * In ACPI, 0 is considered as the invalid address. However the
>>>> rest
>>>>       * of the initialization rely on the invalid address to be
>>>> @@ -1585,6 +1588,7 @@ static void __init gicv3_acpi_init(void)
>>>>      else
>>>>          vsize = GUEST_GICC_SIZE;
>>>>
>>>> +
>>>
>>> As said on v1, this is a spurious newline.
>> ok
>>>
>>>>  }
>>>>  #else
>>>>  static void __init gicv3_acpi_init(void) { }
>>>> diff --git a/xen/include/asm-arm/gic_v3_its.h
>>>> b/xen/include/asm-arm/gic_v3_its.h
>>>> index d2a3e53..b72aec2 100644
>>>> --- a/xen/include/asm-arm/gic_v3_its.h
>>>> +++ b/xen/include/asm-arm/gic_v3_its.h
>>>> @@ -105,6 +105,7 @@
>>>>
>>>>  #include <xen/device_tree.h>
>>>>  #include <xen/rbtree.h>
>>>> +#include <xen/acpi.h>
>>>>
>>>>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>>>>  #define HOST_ITS_USES_PTA               (1U << 1)
>>>> @@ -134,6 +135,7 @@ extern struct list_head host_its_list;
>>>>
>>>>  /* Parse the host DT and pick up all host ITSes. */
>>>>  void gicv3_its_dt_init(const struct dt_device_node *node);
>>>> +int  gicv3_its_acpi_init(struct acpi_subtable_header *header, const
>>>> unsigned long end);
>>>
>>> This will likely need an #ifdef CONFIG_ACPI. And also a stub would be
>>> required if ITS is disabled.
>>>
>> Sorry didnt got your point. on ifdef.
>
> Look how gicv3_its_dt_init has been introduced. There is a stub for 
> when ITS is not built-in. Furthermore, the function is ACPI specific 
> are therefore should be protected with #ifdef CONFIG_ACPI.
>
>> I can add a check gicv3_its_host_has_its() before calling
>> gicv3_its_acpi_init, would that be ok?
>
> gicv3_its_host_has_its will always return false as it is rely on the 
> list of ITS to be populated. However, this will be populated by 
> gicv3_its_acpi_init...
>
ok will add
> Regards,
>


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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Julien Grall]

On 13/06/17 12:02, Manish Jaggi wrote:
>>> Will the below code be ok?
>>
>> If you noticed, I didn't say this code is wrong. Instead I asked why
>> you use the same ID. Meaning, is there anything in the DSDT requiring
>> this value?
>>
>>> + int tras_id = 0;
>>
>> unsigned.
>>
>>> + list_for_each_entry(its_data, &host_its_list, entry)
>>> + {
>>> +    gic_its->translation_id = ++trans_id;
>>
>> You start the translation ID at 1. Why?
>>
> as per the ACPI spec the value should be unique to each GIC ITS unit.
> Does starting with 1 break anything? Or should I start with a magic number?

Rather than arguing on the start value here, you should have first 
answer to the question regarding the usage of translation_id.

I understand that nobody is using it today. However, when I asked around 
me nobody ruled out to any future usage of GIC ITS ID and request this 
to be kept as it is.

This means that you can simply copy over the ACPI tables. Rather 
regenerating them.

Cheers,

-- 
Julien Grall

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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Manish Jaggi]

On 6/13/2017 4:58 PM, Julien Grall wrote:
> On 13/06/17 12:02, Manish Jaggi wrote:
>>>> Will the below code be ok?
>>>
>>> If you noticed, I didn't say this code is wrong. Instead I asked why
>>> you use the same ID. Meaning, is there anything in the DSDT requiring
>>> this value?
>>>
>>>> + int tras_id = 0;
>>>
>>> unsigned.
>>>
>>>> + list_for_each_entry(its_data, &host_its_list, entry)
>>>> + {
>>>> +    gic_its->translation_id = ++trans_id;
>>>
>>> You start the translation ID at 1. Why?
>>>
>> as per the ACPI spec the value should be unique to each GIC ITS unit.
>> Does starting with 1 break anything? Or should I start with a magic 
>> number?
>
> Rather than arguing on the start value here, you should have first 
> answer to the question regarding the usage of translation_id.
in v1 I assumed that it would be the same as read from host its tables, 
so it would have a unique value as programmed by host firmware.
>
> I understand that nobody is using it today. However, when I asked 
> around me nobody ruled out to any future usage of GIC ITS ID and 
> request this to be kept as it is.
>
> This means that you can simply copy over the ACPI tables. Rather 
> regenerating them.
>
I dont follow your comment, a bit confused
In v1 you mentioned that "Please explain why you need to have the same 
ID as the host."
now when you say we copy over the translation_id would be same as that 
of host?

> Cheers,
>


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

Re: [RFC v2][PATCH] arm-acpi: Add ITS Support for Dom0

[Julien Grall]

On 13/06/17 12:44, Manish Jaggi wrote:
> On 6/13/2017 4:58 PM, Julien Grall wrote:
>> On 13/06/17 12:02, Manish Jaggi wrote:
>>>>> Will the below code be ok?
>>>>
>>>> If you noticed, I didn't say this code is wrong. Instead I asked why
>>>> you use the same ID. Meaning, is there anything in the DSDT requiring
>>>> this value?
>>>>
>>>>> + int tras_id = 0;
>>>>
>>>> unsigned.
>>>>
>>>>> + list_for_each_entry(its_data, &host_its_list, entry)
>>>>> + {
>>>>> +    gic_its->translation_id = ++trans_id;
>>>>
>>>> You start the translation ID at 1. Why?
>>>>
>>> as per the ACPI spec the value should be unique to each GIC ITS unit.
>>> Does starting with 1 break anything? Or should I start with a magic
>>> number?
>>
>> Rather than arguing on the start value here, you should have first
>> answer to the question regarding the usage of translation_id.
> in v1 I assumed that it would be the same as read from host its tables,
> so it would have a unique value as programmed by host firmware.
>>
>> I understand that nobody is using it today. However, when I asked
>> around me nobody ruled out to any future usage of GIC ITS ID and
>> request this to be kept as it is.
>>
>> This means that you can simply copy over the ACPI tables. Rather
>> regenerating them.
>>
> I dont follow your comment, a bit confused
> In v1 you mentioned that "Please explain why you need to have the same
> ID as the host."
> now when you say we copy over the translation_id would be same as that
> of host?

Usually when I say: "Please explain..." it means I want more 
documentation in the code because I am not sure to follow why it is 
necessary. It does not mean "The code is wrong". If it was, I would have 
clearly wrote it and give justification on it.

Furthermore, this kind of documentation will help a reader to understand 
your code and avoid spending hours to find a justification.

The contributor should be able to justify any code he wrote and help the 
reviewers to understand the patch quickly.

Cheers,

-- 
Julien Grall

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