Re: [Xen-devel] [PATCH v3 18/25] xen/arm: generate a simple device tree for domUs
On Mon, 13 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 01/08/18 00:28, Stefano Stabellini wrote:
> > Introduce functions to generate a basic domU device tree, similar to the
> > existing functions in tools/libxl/libxl_arm.c.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > Changes in v3:
> > - remove CONFIG_ACPI for make_chosen_node
> > - remove make_hypervisor_node until all Xen related functionalities
> >(evtchns, grant table, etc.) work correctly
> >
> > Changes in v2:
> > - move prepare_dtb rename to previous patch
> > - use switch for the gic version
> > - use arm,gic-400 instead of arm,cortex-a15-gic
> > - add @unit-address in the gic node name
> > - add comment on DOMU_DTB_SIZE
> > ---
> > xen/arch/arm/domain_build.c | 211
> > +++-
> > 1 file changed, 209 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index dfa74e4..167a56e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1057,7 +1057,6 @@ static int __init make_timer_node(const struct domain
> > *d, void *fdt,
> > return res;
> > }
> > -#ifdef CONFIG_ACPI
> > /*
> >* This function is used as part of the device tree generation for Dom0
> >* on ACPI systems, and DomUs started directly from Xen based on device
> > @@ -1103,7 +1102,6 @@ static int __init make_chosen_node(const struct
> > kernel_info *kinfo)
> > return res;
> > }
> > -#endif
> > static int __init map_irq_to_domain(struct domain *d, unsigned int irq,
> > bool need_mapping, const char
> > *devname)
> > @@ -1496,6 +1494,211 @@ static int __init handle_node(struct domain *d,
> > struct kernel_info *kinfo,
> > return res;
> > }
> > +static int __init make_gic_domU_node(const struct domain *d, void *fdt,
> > int addrcells, int sizecells)
> > +{
> > +int res = 0;
> > +int reg_size = addrcells + sizecells;
> > +int nr_cells = reg_size * 2;
> > +__be32 reg[nr_cells];
> > +__be32 *cells;
>
> You are trying to be save a few lines by handling both GICv2 and GICv3 node in
> a single function. But this only thing it adds is confusion when reading the
> code.
>
> Please have two functions (one for each GIC version) to generate the DT. This
> would also help to add ITS/GICv2m support in the future.
OK
> > +
> > +switch ( gic_hw_version() )
> > +{
> > +case GIC_V3:
> > +res = fdt_begin_node(fdt,
> > "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
> > +break;
> > +case GIC_V2:
> > +res = fdt_begin_node(fdt,
> > "interrupt-controller@"__stringify(GUEST_GICD_BASE));
> > +break;
> > +default:
> > +panic("Unsupported GIC version");
> > +}
> > +
> > +if ( res )
> > +return res;
> > +
> > +res = fdt_property_cell(fdt, "#address-cells", 0);
> > +if ( res )
> > +return res;
> > +
> > +res = fdt_property_cell(fdt, "#interrupt-cells", 3);
> > +if ( res )
> > +return res;
> > +
> > +res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> > +if ( res )
> > +return res;
> > +
> > +switch ( gic_hw_version() )
> > +{
> > +case GIC_V3:
> > +{
> > +const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
> > +const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
> > +const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
> > +const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
>
> I am not entirely convinced of the usefulness of those variables.
I can remove
> > +
> > +res = fdt_property_string(fdt, "compatible", "arm,gic-v3");
> > +if ( res )
> > +return res;
> > +
> > +cells = [0];
> > +dt_child_set_range(, addrcells, sizecells, gicd_base,
> > gicd_size);
> > +dt_child_set_range(, addrcells, sizecells, gicr0_base,
> > gicr0_size);
> > +res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +break;
> > +}
> > +case GIC_V2:
> > +{
> > +const uint64_t gicd_base = GUEST_GICD_BASE;
> > +const uint64_t gicd_size = GUEST_GICD_SIZE;
> > +const uint64_t gicc_base = GUEST_GICC_BASE;
> > +const uint64_t gicc_size = GUEST_GICC_SIZE;
>
> Same here.
>
> > +
> > +res = fdt_property_string(fdt, "compatible", "arm,gic-400");
> > +if ( res )
> > +return res;
> > +
> > +cells = [0];
> > +dt_child_set_range(, addrcells, sizecells, gicd_base,
> > gicd_size);
> > +dt_child_set_range(, addrcells, sizecells, gicc_base,
> > gicc_size);
> > +break;
> > +}
> > +default:
> > +break;
> > +}
> > +
> > +res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +if (res)
> > +return res;
> > +
> > +res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
> > +if (res)
> > +
Re: [Xen-devel] [PATCH v3 18/25] xen/arm: generate a simple device tree for domUs
Hi Stefano,
On 01/08/18 00:28, Stefano Stabellini wrote:
Introduce functions to generate a basic domU device tree, similar to the
existing functions in tools/libxl/libxl_arm.c.
Signed-off-by: Stefano Stabellini
---
Changes in v3:
- remove CONFIG_ACPI for make_chosen_node
- remove make_hypervisor_node until all Xen related functionalities
(evtchns, grant table, etc.) work correctly
Changes in v2:
- move prepare_dtb rename to previous patch
- use switch for the gic version
- use arm,gic-400 instead of arm,cortex-a15-gic
- add @unit-address in the gic node name
- add comment on DOMU_DTB_SIZE
---
xen/arch/arm/domain_build.c | 211 +++-
1 file changed, 209 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index dfa74e4..167a56e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1057,7 +1057,6 @@ static int __init make_timer_node(const struct domain *d,
void *fdt,
return res;
}
-#ifdef CONFIG_ACPI
/*
* This function is used as part of the device tree generation for Dom0
* on ACPI systems, and DomUs started directly from Xen based on device
@@ -1103,7 +1102,6 @@ static int __init make_chosen_node(const struct
kernel_info *kinfo)
return res;
}
-#endif
static int __init map_irq_to_domain(struct domain *d, unsigned int irq,
bool need_mapping, const char *devname)
@@ -1496,6 +1494,211 @@ static int __init handle_node(struct domain *d, struct
kernel_info *kinfo,
return res;
}
+static int __init make_gic_domU_node(const struct domain *d, void *fdt, int addrcells, int sizecells)
+{
+int res = 0;
+int reg_size = addrcells + sizecells;
+int nr_cells = reg_size * 2;
+__be32 reg[nr_cells];
+__be32 *cells;
You are trying to be save a few lines by handling both GICv2 and GICv3
node in a single function. But this only thing it adds is confusion when
reading the code.
Please have two functions (one for each GIC version) to generate the DT.
This would also help to add ITS/GICv2m support in the future.
+
+switch ( gic_hw_version() )
+{
+case GIC_V3:
+res = fdt_begin_node(fdt,
"interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
+break;
+case GIC_V2:
+res = fdt_begin_node(fdt,
"interrupt-controller@"__stringify(GUEST_GICD_BASE));
+break;
+default:
+panic("Unsupported GIC version");
+}
+
+if ( res )
+return res;
+
+res = fdt_property_cell(fdt, "#address-cells", 0);
+if ( res )
+return res;
+
+res = fdt_property_cell(fdt, "#interrupt-cells", 3);
+if ( res )
+return res;
+
+res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+if ( res )
+return res;
+
+switch ( gic_hw_version() )
+{
+case GIC_V3:
+{
+const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
+const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
+const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
+const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
I am not entirely convinced of the usefulness of those variables.
+
+res = fdt_property_string(fdt, "compatible", "arm,gic-v3");
+if ( res )
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, gicd_base, gicd_size);
+dt_child_set_range(, addrcells, sizecells, gicr0_base,
gicr0_size);
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+break;
+}
+case GIC_V2:
+{
+const uint64_t gicd_base = GUEST_GICD_BASE;
+const uint64_t gicd_size = GUEST_GICD_SIZE;
+const uint64_t gicc_base = GUEST_GICC_BASE;
+const uint64_t gicc_size = GUEST_GICC_SIZE;
Same here.
+
+res = fdt_property_string(fdt, "compatible", "arm,gic-400");
+if ( res )
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, gicd_base, gicd_size);
+dt_child_set_range(, addrcells, sizecells, gicc_base, gicc_size);
+break;
+}
+default:
+break;
+}
+
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+if (res)
+return res;
+
+res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
+if (res)
+return res;
+
+res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC);
+if (res)
+return res;
+
+res = fdt_end_node(fdt);
+
+return res;
+}
+
+static int __init make_timer_domU_node(const struct domain *d, void *fdt)
+{
+int res;
+gic_interrupt_t intrs[3];
+
+res = fdt_begin_node(fdt, "timer");
+if ( res )
+return res;
+
+if (!is_64bit_domain(d))
Coding style:
if ( ... )
+{
+res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
+if ( res )
+return res;
+} else {
Coding style:
}
[Xen-devel] [PATCH v3 18/25] xen/arm: generate a simple device tree for domUs
Introduce functions to generate a basic domU device tree, similar to the
existing functions in tools/libxl/libxl_arm.c.
Signed-off-by: Stefano Stabellini
---
Changes in v3:
- remove CONFIG_ACPI for make_chosen_node
- remove make_hypervisor_node until all Xen related functionalities
(evtchns, grant table, etc.) work correctly
Changes in v2:
- move prepare_dtb rename to previous patch
- use switch for the gic version
- use arm,gic-400 instead of arm,cortex-a15-gic
- add @unit-address in the gic node name
- add comment on DOMU_DTB_SIZE
---
xen/arch/arm/domain_build.c | 211 +++-
1 file changed, 209 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index dfa74e4..167a56e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1057,7 +1057,6 @@ static int __init make_timer_node(const struct domain *d,
void *fdt,
return res;
}
-#ifdef CONFIG_ACPI
/*
* This function is used as part of the device tree generation for Dom0
* on ACPI systems, and DomUs started directly from Xen based on device
@@ -1103,7 +1102,6 @@ static int __init make_chosen_node(const struct
kernel_info *kinfo)
return res;
}
-#endif
static int __init map_irq_to_domain(struct domain *d, unsigned int irq,
bool need_mapping, const char *devname)
@@ -1496,6 +1494,211 @@ static int __init handle_node(struct domain *d, struct
kernel_info *kinfo,
return res;
}
+static int __init make_gic_domU_node(const struct domain *d, void *fdt, int
addrcells, int sizecells)
+{
+int res = 0;
+int reg_size = addrcells + sizecells;
+int nr_cells = reg_size * 2;
+__be32 reg[nr_cells];
+__be32 *cells;
+
+switch ( gic_hw_version() )
+{
+case GIC_V3:
+res = fdt_begin_node(fdt,
"interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
+break;
+case GIC_V2:
+res = fdt_begin_node(fdt,
"interrupt-controller@"__stringify(GUEST_GICD_BASE));
+break;
+default:
+panic("Unsupported GIC version");
+}
+
+if ( res )
+return res;
+
+res = fdt_property_cell(fdt, "#address-cells", 0);
+if ( res )
+return res;
+
+res = fdt_property_cell(fdt, "#interrupt-cells", 3);
+if ( res )
+return res;
+
+res = fdt_property(fdt, "interrupt-controller", NULL, 0);
+if ( res )
+return res;
+
+switch ( gic_hw_version() )
+{
+case GIC_V3:
+{
+const uint64_t gicd_base = GUEST_GICV3_GICD_BASE;
+const uint64_t gicd_size = GUEST_GICV3_GICD_SIZE;
+const uint64_t gicr0_base = GUEST_GICV3_GICR0_BASE;
+const uint64_t gicr0_size = GUEST_GICV3_GICR0_SIZE;
+
+res = fdt_property_string(fdt, "compatible", "arm,gic-v3");
+if ( res )
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, gicd_base, gicd_size);
+dt_child_set_range(, addrcells, sizecells, gicr0_base,
gicr0_size);
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+break;
+}
+case GIC_V2:
+{
+const uint64_t gicd_base = GUEST_GICD_BASE;
+const uint64_t gicd_size = GUEST_GICD_SIZE;
+const uint64_t gicc_base = GUEST_GICC_BASE;
+const uint64_t gicc_size = GUEST_GICC_SIZE;
+
+res = fdt_property_string(fdt, "compatible", "arm,gic-400");
+if ( res )
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, gicd_base, gicd_size);
+dt_child_set_range(, addrcells, sizecells, gicc_base, gicc_size);
+break;
+}
+default:
+break;
+}
+
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+if (res)
+return res;
+
+res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
+if (res)
+return res;
+
+res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC);
+if (res)
+return res;
+
+res = fdt_end_node(fdt);
+
+return res;
+}
+
+static int __init make_timer_domU_node(const struct domain *d, void *fdt)
+{
+int res;
+gic_interrupt_t intrs[3];
+
+res = fdt_begin_node(fdt, "timer");
+if ( res )
+return res;
+
+if (!is_64bit_domain(d))
+{
+res = fdt_property_string(fdt, "compatible", "arm,armv7-timer");
+if ( res )
+return res;
+} else {
+res = fdt_property_string(fdt, "compatible", "arm,armv8-timer");
+if ( res )
+return res;
+}
+
+set_interrupt_ppi(intrs[0], GUEST_TIMER_PHYS_S_PPI, 0xf,
DT_IRQ_TYPE_LEVEL_LOW);
+set_interrupt_ppi(intrs[1], GUEST_TIMER_PHYS_NS_PPI, 0xf,
DT_IRQ_TYPE_LEVEL_LOW);
+set_interrupt_ppi(intrs[2], GUEST_TIMER_VIRT_PPI, 0xf,
DT_IRQ_TYPE_LEVEL_LOW);
+
+res = fdt_property(fdt, "interrupts", intrs, sizeof (intrs[0]) * 3);
+if ( res )
+return res;
+
+res =
