Re: [Xen-devel] [PATCH v3 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

2017-09-14 Thread Julien Grall 

Hi,

On 05/09/17 18:14, mja...@caviumnetworks.com wrote:

From: Manish Jaggi 

estimate_acpi_efi_size needs to be updated to provide correct size of
hardware domains MADT, which now adds ITS information as well.

Introducing gic_get_hwdom_madt_size.

Signed-off-by: Manish Jaggi 
---
  xen/arch/arm/domain_build.c |  7 +--
  xen/arch/arm/gic-v2.c   |  6 ++
  xen/arch/arm/gic-v3.c   | 18 ++
  xen/arch/arm/gic.c  | 11 +++
  xen/include/asm-arm/gic.h   |  3 +++
  5 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1bec4fa..5739ea4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, 
struct kernel_info *kinfo)
  acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
  acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
  
-madt_size = sizeof(struct acpi_table_madt)

-+ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
-+ sizeof(struct acpi_madt_generic_distributor);
-if ( d->arch.vgic.version == GIC_V3 )
-madt_size += sizeof(struct acpi_madt_generic_redistributor)
- * d->arch.vgic.nr_regions;
+madt_size = gic_get_hwdom_madt_size(d);
  acpi_size += ROUNDUP(madt_size, 8);
  
  addr = acpi_os_get_root_pointer();

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cbe71a9..737c50a 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain 
*d)
  return iomem_deny_access(d, mfn, mfn + nr);
  }
  
+static unsigned long gicv2_get_hwdom_madt_size(const struct domain *d)

+{
+return 0;
+}
+
  #ifdef CONFIG_ACPI
  static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
  {
@@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
  .read_apr= gicv2_read_apr,
  .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
  .make_hwdom_madt = gicv2_make_hwdom_madt,
+.get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
  .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
  .iomem_deny_access   = gicv2_iomem_deny_access,
  .do_LPI  = gicv2_do_LPI,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b3d605d..3eb67f2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain *d, 
u32 offset)
  return table_len;
  }
  
+static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)

+{
+unsigned long size;


As already requested on v2, you need to add a newline here.


+size  = sizeof(struct acpi_madt_generic_redistributor)
+* d->arch.vgic.nr_regions;
+
+size  += vgic_v3_its_count(d)
+* sizeof(struct acpi_madt_generic_translator);
+
+return size;
+}
+
  static int __init
  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
  const unsigned long end)
@@ -1597,6 +1609,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, 
u32 offset)
  {
  return 0;
  }
+
+static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+return 0;
+}
  #endif
  
  /* Set up the GIC */

@@ -1698,6 +1715,7 @@ static const struct gic_hw_operations gicv3_ops = {
  .secondary_init  = gicv3_secondary_cpu_init,
  .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
  .make_hwdom_madt = gicv3_make_hwdom_madt,
+.get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
  .iomem_deny_access   = gicv3_iomem_deny_access,
  .do_LPI  = gicv3_do_LPI,
  };
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6c803bf..9ffd33a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.cMissing newline here.
@@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
  return gic_hw_ops->make_hwdom_madt(d, offset);
  }
  
+unsigned long gic_get_hwdom_madt_size(const struct domain *d)

+{
+unsigned long madt_size;


Same here.


+madt_size = sizeof(struct acpi_table_madt)
++ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
++ sizeof(struct acpi_madt_generic_distributor)
++ gic_hw_ops->get_hwdom_madt_size(d);
+
+return madt_size;
+}
+
  int gic_iomem_deny_access(const struct domain *d)
  {
  return gic_hw_ops->iomem_deny_access(d);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6203dc5..3acdd6d 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -365,6 +365,8 @@ struct gic_hw_operations {
  int (*make_hwdom_madt)(const struct domain *d, u32 offset);
  /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
  int

Re: [Xen-devel] [PATCH v3 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

2017-09-07 Thread Andre Przywara 
Hi,

On 05/09/17 18:14, mja...@caviumnetworks.com wrote:
> From: Manish Jaggi 
> 
> estimate_acpi_efi_size needs to be updated to provide correct size of
> hardware domains MADT, which now adds ITS information as well.
> 
> Introducing gic_get_hwdom_madt_size.
> 
> Signed-off-by: Manish Jaggi 
> ---
>  xen/arch/arm/domain_build.c |  7 +--
>  xen/arch/arm/gic-v2.c   |  6 ++
>  xen/arch/arm/gic-v3.c   | 18 ++
>  xen/arch/arm/gic.c  | 11 +++
>  xen/include/asm-arm/gic.h   |  3 +++
>  5 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1bec4fa..5739ea4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, 
> struct kernel_info *kinfo)
>  acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>  acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
>  
> -madt_size = sizeof(struct acpi_table_madt)
> -+ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> -+ sizeof(struct acpi_madt_generic_distributor);
> -if ( d->arch.vgic.version == GIC_V3 )
> -madt_size += sizeof(struct acpi_madt_generic_redistributor)
> - * d->arch.vgic.nr_regions;
> +madt_size = gic_get_hwdom_madt_size(d);
>  acpi_size += ROUNDUP(madt_size, 8);
>  
>  addr = acpi_os_get_root_pointer();
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index cbe71a9..737c50a 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain 
> *d)
>  return iomem_deny_access(d, mfn, mfn + nr);
>  }
>  
> +static unsigned long gicv2_get_hwdom_madt_size(const struct domain *d)
> +{
> +return 0;
> +}

Nothing too critical, but this looks a bit confusing, as the size of the
GIC part of the MADT isn't 0 even for GICv2. So either you rename it to
something containing "additional" or the like or you do what it says on
the tin and return the per-VCPU size and the size for the distributor
here (at the cost of copying this to the GICv3 code).

Cheers,
Andre.

> +
>  #ifdef CONFIG_ACPI
>  static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
>  {
> @@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
>  .read_apr= gicv2_read_apr,
>  .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
>  .make_hwdom_madt = gicv2_make_hwdom_madt,
> +.get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
>  .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>  .iomem_deny_access   = gicv2_iomem_deny_access,
>  .do_LPI  = gicv2_do_LPI,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index b3d605d..3eb67f2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain 
> *d, u32 offset)
>  return table_len;
>  }
>  
> +static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +unsigned long size;
> +size  = sizeof(struct acpi_madt_generic_redistributor)
> +* d->arch.vgic.nr_regions;
> +
> +size  += vgic_v3_its_count(d)
> +* sizeof(struct acpi_madt_generic_translator);
> +
> +return size;
> +}
> +
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>  const unsigned long end)
> @@ -1597,6 +1609,11 @@ static int gicv3_make_hwdom_madt(const struct domain 
> *d, u32 offset)
>  {
>  return 0;
>  }
> +
> +static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +return 0;
> +}
>  #endif
>  
>  /* Set up the GIC */
> @@ -1698,6 +1715,7 @@ static const struct gic_hw_operations gicv3_ops = {
>  .secondary_init  = gicv3_secondary_cpu_init,
>  .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
>  .make_hwdom_madt = gicv3_make_hwdom_madt,
> +.get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
>  .iomem_deny_access   = gicv3_iomem_deny_access,
>  .do_LPI  = gicv3_do_LPI,
>  };
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6c803bf..9ffd33a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 
> offset)
>  return gic_hw_ops->make_hwdom_madt(d, offset);
>  }
>  
> +unsigned long gic_get_hwdom_madt_size(const struct domain *d)
> +{
> +unsigned long madt_size;
> +madt_size = sizeof(struct acpi_table_madt)
> ++ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> ++ sizeof(struct acpi_madt_generic_distributor)
> ++ gic_hw_ops->get_hwdom_madt_size(d);
> +
> +return madt_size;
> +}
> +
>  int

[Xen-devel] [PATCH v3 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

2017-09-05 Thread mjaggi 
From: Manish Jaggi 

estimate_acpi_efi_size needs to be updated to provide correct size of
hardware domains MADT, which now adds ITS information as well.

Introducing gic_get_hwdom_madt_size.

Signed-off-by: Manish Jaggi 
---
 xen/arch/arm/domain_build.c |  7 +--
 xen/arch/arm/gic-v2.c   |  6 ++
 xen/arch/arm/gic-v3.c   | 18 ++
 xen/arch/arm/gic.c  | 11 +++
 xen/include/asm-arm/gic.h   |  3 +++
 5 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1bec4fa..5739ea4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, 
struct kernel_info *kinfo)
 acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
 acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
 
-madt_size = sizeof(struct acpi_table_madt)
-+ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
-+ sizeof(struct acpi_madt_generic_distributor);
-if ( d->arch.vgic.version == GIC_V3 )
-madt_size += sizeof(struct acpi_madt_generic_redistributor)
- * d->arch.vgic.nr_regions;
+madt_size = gic_get_hwdom_madt_size(d);
 acpi_size += ROUNDUP(madt_size, 8);
 
 addr = acpi_os_get_root_pointer();
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cbe71a9..737c50a 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain 
*d)
 return iomem_deny_access(d, mfn, mfn + nr);
 }
 
+static unsigned long gicv2_get_hwdom_madt_size(const struct domain *d)
+{
+return 0;
+}
+
 #ifdef CONFIG_ACPI
 static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
 {
@@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
 .read_apr= gicv2_read_apr,
 .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
 .make_hwdom_madt = gicv2_make_hwdom_madt,
+.get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
 .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
 .iomem_deny_access   = gicv2_iomem_deny_access,
 .do_LPI  = gicv2_do_LPI,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b3d605d..3eb67f2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain *d, 
u32 offset)
 return table_len;
 }
 
+static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+unsigned long size;
+size  = sizeof(struct acpi_madt_generic_redistributor)
+* d->arch.vgic.nr_regions;
+
+size  += vgic_v3_its_count(d)
+* sizeof(struct acpi_madt_generic_translator);
+
+return size;
+}
+
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 const unsigned long end)
@@ -1597,6 +1609,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, 
u32 offset)
 {
 return 0;
 }
+
+static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+return 0;
+}
 #endif
 
 /* Set up the GIC */
@@ -1698,6 +1715,7 @@ static const struct gic_hw_operations gicv3_ops = {
 .secondary_init  = gicv3_secondary_cpu_init,
 .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
 .make_hwdom_madt = gicv3_make_hwdom_madt,
+.get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
 .iomem_deny_access   = gicv3_iomem_deny_access,
 .do_LPI  = gicv3_do_LPI,
 };
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6c803bf..9ffd33a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
 return gic_hw_ops->make_hwdom_madt(d, offset);
 }
 
+unsigned long gic_get_hwdom_madt_size(const struct domain *d)
+{
+unsigned long madt_size;
+madt_size = sizeof(struct acpi_table_madt)
++ sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
++ sizeof(struct acpi_madt_generic_distributor)
++ gic_hw_ops->get_hwdom_madt_size(d);
+
+return madt_size;
+}
+
 int gic_iomem_deny_access(const struct domain *d)
 {
 return gic_hw_ops->iomem_deny_access(d);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6203dc5..3acdd6d 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -365,6 +365,8 @@ struct gic_hw_operations {
 int (*make_hwdom_madt)(const struct domain *d, u32 offset);
 /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
 int (*map_hwdom_extra_mappings)(struct domain *d);
+/* Query the size of hardware domain madt table */
+unsigned long (*get_hwdom_madt_size)(const struct domain *d);
 /* Deny access to GIC regions */
 int