| Message ID | 1487676368-22356-7-git-send-email-bhupinder.thakur@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pl011 emulation support in Xen | expand |
> @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, > val |= GUEST_EVTCHN_PPI; > rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, > val); > + > if (rc) > return rc; > > + Please do be careful. There is no need for these changes.
On Tue, Feb 21, 2017 at 04:56:03PM +0530, Bhupinder Thakur wrote: > Add a new pl011 uart node > - Get the pl011 spi virq from Xen using a hvm call > - Add a new device tree node in the guest DT for SBSA pl011 uart containing the IRQ > (read above) and the MMIO address range to be used by the guest > > The format for the node is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. Why don't you just copy-n-paste it in? It is only 10 linues. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index d842d88..34c7e39 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -130,9 +130,10 @@ static struct arch_info { > const char *guest_type; > const char *timer_compat; > const char *cpu_compat; > + const char *uart_compat; > } arch_info[] = { > - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" }, > - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, > + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" }, > + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" }, > }; > > /* > @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt, > return 0; > } > > +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt, > + const struct arch_info *ainfo, > + struct xc_dom_image *dom, uint64_t irq) Hm, mayube my editor is wrong but these arguments don't appear to be right.. > +{ > + int res; > + gic_interrupt intr; > + > + res = fdt_begin_node(fdt, "sbsa-pl011"); > + if (res) return res; > + > + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat); > + if (res) return res; > + > + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, > + 1, > + GUEST_PL011_BASE, GUEST_PL011_SIZE); > + if (res) > + return res; Shouldn't we free them? > + > + set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH); 0xF looks like a good candidate for #define. > + > + res = fdt_property_interrupts(gc, fdt, &intr, 1); > + if (res) return res; Again, shouldn't we free it if we things go south? > + > + fdt_property_u32(fdt, "current-speed", 115200); So perhaps a #define? > + > + res = fdt_end_node(fdt); > + if (res) return res; > + > + return 0; > +} > + > static const struct arch_info *get_arch_info(libxl__gc *gc, > const struct xc_dom_image *dom) > { > @@ -790,6 +823,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info, > int rc, res; > size_t fdt_size = 0; > int pfdt_size = 0; > + uint64_t vpl011_irq=0; Does it have to be =0 ? > > const libxl_version_info *vers; > const struct arch_info *ainfo; > @@ -889,6 +923,13 @@ next_resize: > FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) ); > FDT( make_hypervisor_node(gc, fdt, vers) ); > > + /* > + * get the vpl011 VIRQ and use it for creating a vpl011 node entry > + */ > + if ( !xc_hvm_param_get(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_VIRQ, > + &vpl011_irq) ) Why is the &vpl011 so far right? It should be right after the ( > + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) ); Ah, so it will just fail with an goto out and leak. > + > if (pfdt) > FDT( copy_partial_fdt(gc, fdt, pfdt) ); > > @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, > val |= GUEST_EVTCHN_PPI; > rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, > val); > + Ahem? > if (rc) > return rc; > > + AHEM?? > rc = libxl__prepare_dtb(gc, info, state, dom); > if (rc) goto out; > > -- > 2.7.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
Hi Konrad, On 03/03/2017 09:03 PM, Konrad Rzeszutek Wilk wrote: > On Tue, Feb 21, 2017 at 04:56:03PM +0530, Bhupinder Thakur wrote: >> Add a new pl011 uart node >> - Get the pl011 spi virq from Xen using a hvm call >> - Add a new device tree node in the guest DT for SBSA pl011 uart containing the IRQ >> (read above) and the MMIO address range to be used by the guest >> >> The format for the node is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. > > Why don't you just copy-n-paste it in? It is only 10 linues. > >> >> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> >> --- >> tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 45 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index d842d88..34c7e39 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -130,9 +130,10 @@ static struct arch_info { >> const char *guest_type; >> const char *timer_compat; >> const char *cpu_compat; >> + const char *uart_compat; >> } arch_info[] = { >> - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" }, >> - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, >> + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" }, >> + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" }, >> }; >> >> /* >> @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt, >> return 0; >> } >> >> +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt, >> + const struct arch_info *ainfo, >> + struct xc_dom_image *dom, uint64_t irq) > > Hm, mayube my editor is wrong but these arguments don't appear > to be right.. > >> +{ >> + int res; >> + gic_interrupt intr; >> + >> + res = fdt_begin_node(fdt, "sbsa-pl011"); >> + if (res) return res; >> + >> + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat); >> + if (res) return res; >> + >> + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, >> + 1, >> + GUEST_PL011_BASE, GUEST_PL011_SIZE); >> + if (res) >> + return res; > > Shouldn't we free them? libxl is allocating a big chunk of memory for the FDT blob (see libxl__prepare_dtb) and those helpers will only fill the buffer. If they fail is means the buffer is not big enough and the process will be restarted from the beginning. So there is not need to worry to free anything here. > >> + >> + set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH); > > 0xF looks like a good candidate for #define. >> + >> + res = fdt_property_interrupts(gc, fdt, &intr, 1); >> + if (res) return res; > > Again, shouldn't we free it if we things go south? >> + >> + fdt_property_u32(fdt, "current-speed", 115200); > > So perhaps a #define? +1 and explaining why 115200. If it is a random value it should be explained. Cheers,
Hi Bhupinder, CC toolstack maintainers. On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: > Add a new pl011 uart node > - Get the pl011 spi virq from Xen using a hvm call See my comment on previous patches. > - Add a new device tree node in the guest DT for SBSA pl011 uart containing the IRQ > (read above) and the MMIO address range to be used by the guest > > The format for the node is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. The ordering of the patches in this series is a bit weird. You are exposing the pl011 to the guest before all the code to handle it is there. This patch should likely be at the end of this series. Also, you want a libxl option to allow the user enabling/disable pl011 emulation. We likely want this emulation disable by default. > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > --- > tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > index d842d88..34c7e39 100644 > --- a/tools/libxl/libxl_arm.c > +++ b/tools/libxl/libxl_arm.c > @@ -130,9 +130,10 @@ static struct arch_info { > const char *guest_type; > const char *timer_compat; > const char *cpu_compat; > + const char *uart_compat; > } arch_info[] = { > - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" }, > - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, > + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" }, > + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" }, > }; > > /* > @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt, > return 0; > } > > +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt, > + const struct arch_info *ainfo, > + struct xc_dom_image *dom, uint64_t irq) Why uint64_t for irq? > +{ > + int res; > + gic_interrupt intr; > + > + res = fdt_begin_node(fdt, "sbsa-pl011"); > + if (res) return res; > + > + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat); > + if (res) return res; > + > + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, > + 1, > + GUEST_PL011_BASE, GUEST_PL011_SIZE); > + if (res) > + return res; > + > + set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH); > + > + res = fdt_property_interrupts(gc, fdt, &intr, 1); > + if (res) return res; > + > + fdt_property_u32(fdt, "current-speed", 115200); > + > + res = fdt_end_node(fdt); > + if (res) return res; > + > + return 0; > +} > + > static const struct arch_info *get_arch_info(libxl__gc *gc, > const struct xc_dom_image *dom) > { > @@ -790,6 +823,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info, > int rc, res; > size_t fdt_size = 0; > int pfdt_size = 0; > + uint64_t vpl011_irq=0; > > const libxl_version_info *vers; > const struct arch_info *ainfo; > @@ -889,6 +923,13 @@ next_resize: > FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) ); > FDT( make_hypervisor_node(gc, fdt, vers) ); > > + /* > + * get the vpl011 VIRQ and use it for creating a vpl011 node entry > + */ > + if ( !xc_hvm_param_get(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_VIRQ, > + &vpl011_irq) ) > + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) ); > + > if (pfdt) > FDT( copy_partial_fdt(gc, fdt, pfdt) ); > > @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, > val |= GUEST_EVTCHN_PPI; > rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, > val); > + > if (rc) > return rc; > > + > rc = libxl__prepare_dtb(gc, info, state, dom); > if (rc) goto out; > > Cheers,
On Sun, Mar 05, 2017 at 01:04:07PM +0000, Julien Grall wrote: > Hi Bhupinder, > > CC toolstack maintainers. > > On 02/21/2017 11:26 AM, Bhupinder Thakur wrote: > > Add a new pl011 uart node > > - Get the pl011 spi virq from Xen using a hvm call > > See my comment on previous patches. > > > - Add a new device tree node in the guest DT for SBSA pl011 uart containing the IRQ > > (read above) and the MMIO address range to be used by the guest > > > > The format for the node is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. > > The ordering of the patches in this series is a bit weird. You are exposing > the pl011 to the guest before all the code to handle it is there. This patch > should likely be at the end of this series. > > Also, you want a libxl option to allow the user enabling/disable pl011 > emulation. We likely want this emulation disable by default. > Yes, I concur. > > > > Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> > > --- > > tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > > index d842d88..34c7e39 100644 > > --- a/tools/libxl/libxl_arm.c > > +++ b/tools/libxl/libxl_arm.c > > @@ -130,9 +130,10 @@ static struct arch_info { > > const char *guest_type; > > const char *timer_compat; > > const char *cpu_compat; > > + const char *uart_compat; > > } arch_info[] = { > > - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" }, > > - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, > > + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" }, > > + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" }, > > }; > > > > /* > > @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt, > > return 0; > > } > > > > +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt, > > + const struct arch_info *ainfo, > > + struct xc_dom_image *dom, uint64_t irq) > > Why uint64_t for irq? > > > +{ > > + int res; > > + gic_interrupt intr; > > + > > + res = fdt_begin_node(fdt, "sbsa-pl011"); > > + if (res) return res; > > + > > + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat); > > + if (res) return res; > > + > > + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, > > + 1, > > + GUEST_PL011_BASE, GUEST_PL011_SIZE); > > + if (res) > > + return res; > > + > > + set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH); > > + > > + res = fdt_property_interrupts(gc, fdt, &intr, 1); > > + if (res) return res; > > + > > + fdt_property_u32(fdt, "current-speed", 115200); > > + > > + res = fdt_end_node(fdt); > > + if (res) return res; > > + > > + return 0; > > +} > > + > > static const struct arch_info *get_arch_info(libxl__gc *gc, > > const struct xc_dom_image *dom) > > { > > @@ -790,6 +823,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info, > > int rc, res; > > size_t fdt_size = 0; > > int pfdt_size = 0; > > + uint64_t vpl011_irq=0; Spaces around "=" please. > > > > const libxl_version_info *vers; > > const struct arch_info *ainfo; > > @@ -889,6 +923,13 @@ next_resize: > > FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) ); > > FDT( make_hypervisor_node(gc, fdt, vers) ); > > > > + /* > > + * get the vpl011 VIRQ and use it for creating a vpl011 node entry > > + */ > > + if ( !xc_hvm_param_get(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_VIRQ, > > + &vpl011_irq) ) > > + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) ); > > + Coding style is wrong. > > if (pfdt) > > FDT( copy_partial_fdt(gc, fdt, pfdt) ); > > > > @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, > > val |= GUEST_EVTCHN_PPI; > > rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, > > val); > > + Stray blank line. > > if (rc) > > return rc; > > > > + Ditto. Please check libxl/CODING_STYLE and avoid unrelated changes. Wei. > > rc = libxl__prepare_dtb(gc, info, state, dom); > > if (rc) goto out; > > > > > > Cheers, > > -- > Julien Grall
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index d842d88..34c7e39 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -130,9 +130,10 @@ static struct arch_info { const char *guest_type; const char *timer_compat; const char *cpu_compat; + const char *uart_compat; } arch_info[] = { - {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15" }, - {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" }, + {"xen-3.0-armv7l", "arm,armv7-timer", "arm,cortex-a15", "arm,sbsa-uart" }, + {"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8", "arm,sbsa-uart" }, }; /* @@ -590,6 +591,38 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt, return 0; } +static int make_vpl011_uart_node(libxl__gc *gc, void *fdt, + const struct arch_info *ainfo, + struct xc_dom_image *dom, uint64_t irq) +{ + int res; + gic_interrupt intr; + + res = fdt_begin_node(fdt, "sbsa-pl011"); + if (res) return res; + + res = fdt_property_compat(gc, fdt, 1, ainfo->uart_compat); + if (res) return res; + + res = fdt_property_regs(gc, fdt, ROOT_ADDRESS_CELLS, ROOT_SIZE_CELLS, + 1, + GUEST_PL011_BASE, GUEST_PL011_SIZE); + if (res) + return res; + + set_interrupt(intr, irq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH); + + res = fdt_property_interrupts(gc, fdt, &intr, 1); + if (res) return res; + + fdt_property_u32(fdt, "current-speed", 115200); + + res = fdt_end_node(fdt); + if (res) return res; + + return 0; +} + static const struct arch_info *get_arch_info(libxl__gc *gc, const struct xc_dom_image *dom) { @@ -790,6 +823,7 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_build_info *info, int rc, res; size_t fdt_size = 0; int pfdt_size = 0; + uint64_t vpl011_irq=0; const libxl_version_info *vers; const struct arch_info *ainfo; @@ -889,6 +923,13 @@ next_resize: FDT( make_timer_node(gc, fdt, ainfo, xc_config->clock_frequency) ); FDT( make_hypervisor_node(gc, fdt, vers) ); + /* + * get the vpl011 VIRQ and use it for creating a vpl011 node entry + */ + if ( !xc_hvm_param_get(dom->xch, dom->guest_domid, HVM_PARAM_VPL011_VIRQ, + &vpl011_irq) ) + FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom, vpl011_irq) ); + if (pfdt) FDT( copy_partial_fdt(gc, fdt, pfdt) ); @@ -933,9 +974,11 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc, val |= GUEST_EVTCHN_PPI; rc = xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CALLBACK_IRQ, val); + if (rc) return rc; + rc = libxl__prepare_dtb(gc, info, state, dom); if (rc) goto out;
Add a new pl011 uart node - Get the pl011 spi virq from Xen using a hvm call - Add a new device tree node in the guest DT for SBSA pl011 uart containing the IRQ (read above) and the MMIO address range to be used by the guest The format for the node is specified in Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt. Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org> --- tools/libxl/libxl_arm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)