Re: [Xen-devel] [PATCH RFC 12/15] xen/arm: generate vpl011 node on device tree for domU
On Fri, 15 Jun 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 06/13/2018 11:15 PM, Stefano Stabellini wrote:
> > Introduce vpl011 support to guests started from Xen: it provides a
> > simple way to print output from a guest, as most guests come with a
> > pl011 driver. It is also able to provide a working console with
> > interrupt support.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > xen/arch/arm/domain_build.c | 70
> > +
> > 1 file changed, 70 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b4f560f..ff65057 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1470,6 +1470,70 @@ static int make_timer_domU_node(const struct domain
> > *d, void *fdt)
> > return res;
> > }
> > +static void set_interrupt(gic_interrupt_t *interrupt, unsigned int irq,
>
> The definition of interrupt looks suspicious. gic_interrupt_t is defined as
> be32[3]. Here you pass a pointer, so interrupt type would be __be32 **, that
> you crudely cast to __be32* below.
>
> Most likely you don't want to pass a pointer here and just use the type
> gic_interrupt_t. Because it is an array, then there will be no issue.
Right, I'll fix
> > + unsigned int cpumask, unsigned int level)
> > +{
> > +__be32 *cells = (__be32 *) interrupt;
>
> Explicit cast are always a bad idea. If you need one, then mostly likely you
> did something wrong :). In that case interrupt type is __be32** and you cast
> to __be32*. If you change the type as suggested above, then the cast will not
> be necessary here.
Yeah
> > +int is_ppi = (irq < 32);
> > +
> > +irq -= (is_ppi) ? 16: 32; /* PPIs start at 16, SPIs at 32 */
> > +
> > +/* See linux Documentation/devictree/bindings/arm/gic.txt */
> > +dt_set_cell(, 1, is_ppi); /* is a PPI? */
> > +dt_set_cell(, 1, irq);
> > +dt_set_cell(, 1, (cpumask << 8) | level);
> > +}
>
> We already have a function to generate PPI interrupt (set_interrupt_ppi).
> Would it be possible to extend it to support interrupt?
>
> Most likely, you will want to use set_interrupt(...) everywhere and just drop
> set_interrupt_ppi.
Good idea, I'll remove set_interrupt_ppi
> > +
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > +static int make_vpl011_uart_node(const struct domain *d, void *fdt,
> > + int addrcells, int sizecells)
> > +{
> > +int res;
> > +gic_interrupt_t intr;
> > +int reg_size = addrcells + sizecells;
> > +int nr_cells = reg_size;
> > +__be32 reg[nr_cells];
> > +__be32 *cells;
> > +
> > +res = fdt_begin_node(fdt, "sbsa-pl011");
> > +if (res)
>
> Coding style:
>
> if ( ... )
>
> > +return res;
> > +
> > +res = fdt_property_string(fdt, "compatible", "arm,sbsa-uart");
>
> To make clear, you are exposing a SBSA compatible UART and not a PL011. SBSA
> UART is a subset of PL011 r1p5. A full PL011 implementation in Xen would just
> be too difficult, so your guest may require some changes in their driver.
>
> I think this is a small price to pay, but I wanted to make sure you don't
> expect the guest to drive the UART the same way a PL011.
I'll add a note to the commit message
> > +if (res)
>
> Coding style
>
> > +return res;
> > +
> > +cells = [0];
> > +dt_child_set_range(, addrcells, sizecells, GUEST_PL011_BASE,
> > +GUEST_PL011_SIZE);
>
> The indentation looks wrong here.
>
> > +if (res)
>
> Coding style
>
> > +return res;
> > +res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > +if (res)
>
> Coding style
>
> > +return res;
> > +
> > +set_interrupt(, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> > +
> > +res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
> > +if (res)
>
> Coding style
>
> > +return res;
> > +
> > +res = fdt_property_cell(fdt, "interrupt-parent",
> > +PHANDLE_GIC);
> > +if (res)
>
> Coding style
>
> > +return res;
> > +
> > +/* Use a default baud rate of 115200. */
> > +fdt_property_u32(fdt, "current-speed", 115200);
> > +
> > +res = fdt_end_node(fdt);
> > +if (res)
>
> Coding style
Fixed them all
> > +return res;
> > +
> > +return 0;
> > +}
> > +#endif
> > +
> > #define DOMU_DTB_SIZE 4096
> > static int prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
> > {
> > @@ -1531,6 +1595,12 @@ static int prepare_dtb_domU(struct domain *d, struct
> > kernel_info *kinfo)
> > if ( ret )
> > goto err;
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE.
> > +ret = make_vpl011_uart_node(d, kinfo->fdt, addrcells, sizecells);
>
> I would prefer if don't expose the pl011 by default to a guest and provide a
> way to enable it for a given guest
I'll add a vpl011 option
> > +if ( ret )
> > +goto err;
> > +#endif
> > +
Re: [Xen-devel] [PATCH RFC 12/15] xen/arm: generate vpl011 node on device tree for domU
Hi Stefano,
On 06/13/2018 11:15 PM, Stefano Stabellini wrote:
Introduce vpl011 support to guests started from Xen: it provides a
simple way to print output from a guest, as most guests come with a
pl011 driver. It is also able to provide a working console with
interrupt support.
Signed-off-by: Stefano Stabellini
---
xen/arch/arm/domain_build.c | 70 +
1 file changed, 70 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b4f560f..ff65057 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1470,6 +1470,70 @@ static int make_timer_domU_node(const struct domain *d,
void *fdt)
return res;
}
+static void set_interrupt(gic_interrupt_t *interrupt, unsigned int irq,
The definition of interrupt looks suspicious. gic_interrupt_t is defined
as be32[3]. Here you pass a pointer, so interrupt type would be __be32
**, that you crudely cast to __be32* below.
Most likely you don't want to pass a pointer here and just use the type
gic_interrupt_t. Because it is an array, then there will be no issue.
+ unsigned int cpumask, unsigned int level)
+{
+__be32 *cells = (__be32 *) interrupt;
Explicit cast are always a bad idea. If you need one, then mostly likely
you did something wrong :). In that case interrupt type is __be32** and
you cast to __be32*. If you change the type as suggested above, then the
cast will not be necessary here.
+int is_ppi = (irq < 32);
+
+irq -= (is_ppi) ? 16: 32; /* PPIs start at 16, SPIs at 32 */
+
+/* See linux Documentation/devictree/bindings/arm/gic.txt */
+dt_set_cell(, 1, is_ppi); /* is a PPI? */
+dt_set_cell(, 1, irq);
+dt_set_cell(, 1, (cpumask << 8) | level);
+}
We already have a function to generate PPI interrupt
(set_interrupt_ppi). Would it be possible to extend it to support interrupt?
Most likely, you will want to use set_interrupt(...) everywhere and just
drop set_interrupt_ppi.
+
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+static int make_vpl011_uart_node(const struct domain *d, void *fdt,
+ int addrcells, int sizecells)
+{
+int res;
+gic_interrupt_t intr;
+int reg_size = addrcells + sizecells;
+int nr_cells = reg_size;
+__be32 reg[nr_cells];
+__be32 *cells;
+
+res = fdt_begin_node(fdt, "sbsa-pl011");
+if (res)
Coding style:
if ( ... )
+return res;
+
+res = fdt_property_string(fdt, "compatible", "arm,sbsa-uart");
To make clear, you are exposing a SBSA compatible UART and not a PL011.
SBSA UART is a subset of PL011 r1p5. A full PL011 implementation in Xen
would just be too difficult, so your guest may require some changes in
their driver.
I think this is a small price to pay, but I wanted to make sure you
don't expect the guest to drive the UART the same way a PL011.
+if (res)
Coding style
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, GUEST_PL011_BASE,
+GUEST_PL011_SIZE);
The indentation looks wrong here.
+if (res)
Coding style
+return res;
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+if (res)
Coding style
+return res;
+
+set_interrupt(, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
+if (res)
Coding style
+return res;
+
+res = fdt_property_cell(fdt, "interrupt-parent",
+PHANDLE_GIC);
+if (res)
Coding style
+return res;
+
+/* Use a default baud rate of 115200. */
+fdt_property_u32(fdt, "current-speed", 115200);
+
+res = fdt_end_node(fdt);
+if (res)
Coding style
+return res;
+
+return 0;
+}
+#endif
+
#define DOMU_DTB_SIZE 4096
static int prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
{
@@ -1531,6 +1595,12 @@ static int prepare_dtb_domU(struct domain *d, struct
kernel_info *kinfo)
if ( ret )
goto err;
+#ifdef CONFIG_SBSA_VUART_CONSOLE.
+ret = make_vpl011_uart_node(d, kinfo->fdt, addrcells, sizecells);
I would prefer if don't expose the pl011 by default to a guest and
provide a way to enable it for a given guest
+if ( ret )
+goto err;
+#endif
+
ret = fdt_end_node(kinfo->fdt);
if ( ret < 0 )
goto err;
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH RFC 12/15] xen/arm: generate vpl011 node on device tree for domU
Introduce vpl011 support to guests started from Xen: it provides a
simple way to print output from a guest, as most guests come with a
pl011 driver. It is also able to provide a working console with
interrupt support.
Signed-off-by: Stefano Stabellini
---
xen/arch/arm/domain_build.c | 70 +
1 file changed, 70 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b4f560f..ff65057 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1470,6 +1470,70 @@ static int make_timer_domU_node(const struct domain *d,
void *fdt)
return res;
}
+static void set_interrupt(gic_interrupt_t *interrupt, unsigned int irq,
+ unsigned int cpumask, unsigned int level)
+{
+__be32 *cells = (__be32 *) interrupt;
+int is_ppi = (irq < 32);
+
+irq -= (is_ppi) ? 16: 32; /* PPIs start at 16, SPIs at 32 */
+
+/* See linux Documentation/devictree/bindings/arm/gic.txt */
+dt_set_cell(, 1, is_ppi); /* is a PPI? */
+dt_set_cell(, 1, irq);
+dt_set_cell(, 1, (cpumask << 8) | level);
+}
+
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+static int make_vpl011_uart_node(const struct domain *d, void *fdt,
+ int addrcells, int sizecells)
+{
+int res;
+gic_interrupt_t intr;
+int reg_size = addrcells + sizecells;
+int nr_cells = reg_size;
+__be32 reg[nr_cells];
+__be32 *cells;
+
+res = fdt_begin_node(fdt, "sbsa-pl011");
+if (res)
+return res;
+
+res = fdt_property_string(fdt, "compatible", "arm,sbsa-uart");
+if (res)
+return res;
+
+cells = [0];
+dt_child_set_range(, addrcells, sizecells, GUEST_PL011_BASE,
+GUEST_PL011_SIZE);
+if (res)
+return res;
+res = fdt_property(fdt, "reg", reg, sizeof(reg));
+if (res)
+return res;
+
+set_interrupt(, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+
+res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
+if (res)
+return res;
+
+res = fdt_property_cell(fdt, "interrupt-parent",
+PHANDLE_GIC);
+if (res)
+return res;
+
+/* Use a default baud rate of 115200. */
+fdt_property_u32(fdt, "current-speed", 115200);
+
+res = fdt_end_node(fdt);
+if (res)
+return res;
+
+return 0;
+}
+#endif
+
#define DOMU_DTB_SIZE 4096
static int prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
{
@@ -1531,6 +1595,12 @@ static int prepare_dtb_domU(struct domain *d, struct
kernel_info *kinfo)
if ( ret )
goto err;
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+ret = make_vpl011_uart_node(d, kinfo->fdt, addrcells, sizecells);
+if ( ret )
+goto err;
+#endif
+
ret = fdt_end_node(kinfo->fdt);
if ( ret < 0 )
goto err;
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
