BUG: libxl vuart build order

BUG: libxl vuart build order

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

+ xen-devel and libxl maintainers

In short, there is a regression in libxl with the ARM vuart introduced
by moving ARM guests to the PVH build.


On Thu, 29 Oct 2020, Takahiro Akashi wrote:
> On Wed, Oct 28, 2020 at 02:44:16PM -0700, Stefano Stabellini wrote:
> > On Wed, 28 Oct 2020, Takahiro Akashi wrote:
> > > On Tue, Oct 27, 2020 at 09:02:14AM +0900, Takahiro Akashi wrote:
> > > > On Mon, Oct 26, 2020 at 04:37:30PM -0700, Stefano Stabellini wrote:
> > > > > 
> > > > > On Mon, 26 Oct 2020, Takahiro Akashi wrote:
> > > > > > Stefano,
> > > > > > 
> > > > > > # I'm afraid that I have already bothered you with a lot of questions.
> > > > > > 
> > > > > > When I looked at Xen's vpl011 implementation, I found
> > > > > > CR (and LCHR) register is not supported. (trap may cause a data abort).
> > > > > > On the other hand, for example, linux's pl011 driver surely
> > > > > > accesses CR (and LCHR) register.
> > > > > > So I guess that linux won't be able to use pl011 on a Xen guest vm
> > > > > > if vuart = "sbsa_uart".
> > > > > > 
> > > > > > Is this a known issue or do I miss anything?
> > > > > 
> > > > > Linux should definitely be able to use it, and in fact, I am using it
> > > > > with Linux in my test environment.
> > > > > 
> > > > > I think the confusion comes from the name "vpl011": it is in fact not a
> > > > > full PL011 UART, but an SBSA UART.
> > > > 
> > > > Yeah, I have noticed it.
> > > > 
> > > > > SBSA UART only implements a subset of
> > > > > the PL011 registers. The compatible string is "arm,sbsa-uart", also see
> > > > > drivers/tty/serial/amba-pl011.c:sbsa_uart_probe.
> > > > 
> > > > Looking closely into the details of implementation, I found
> > > > that all the accesses to unimplemented registers, including
> > > > CR, are deliberately avoided in sbsa part of linux driver.
> > > 
> > > So I'm now trying to implement "sbsa-uart" driver on U-Boot
> > > by modifying the existing pl011 driver.
> > > (Please note the current xen'ized U-Boot utilises a para-virtualized
> > > console, i.e. with HVM_PARAM_CONSOLE_PFN.)
> > > 
> > > So far my all attempts have failed.
> > > 
> > > There are a couple of problems, and one of them is how we can
> > > access vpl011 port (from dom0).
> > > What I did is:
> > > - modify U-Boot's pl011 driver
> > >   (I'm sure that the driver correctly handle a vpl011 device
> > >   with regard of accessing a proper set of registers.)
> > > - start U-Boot guest with "vuart=sbsa_uart" by
> > >     xl create uboot.cfg -c
> > > 
> > > Then I have seen almost nothing on the screen.
> > > Digging into vpl011 implementation, I found that all the characters
> > > written DR register will be directed to a "backend domain" if a guest
> > > vm is launched by xl command.
> > > (In case of dom0less, the backend seems to be Xen itself.)
> > > 
> > > As a silly experiment, I modified domain_vpl011_init() to always create
> > > a vpl011 device with "backend_in_domain == false".
> > > Then, I could see more boot messages from U-Boot, but still fails
> > > to use the device as a console, I mean, we will lose all the outputs
> > > after at some point and won't be able to type any keys (at a command prompt).
> > > (This will be another problem on U-Boot side.)
> > > 
> > > My first question here is how we can configure and connect a console
> > > in this case?
> > > Should "xl create -c" or "xl console -t vuart" simply work?
> > 
> > "xl create -c" creates a guest and connect to the primary console which
> > is the PV console (i.e. HVM_PARAM_CONSOLE_PFN.)
> 
> So in case of vuart, it (console) doesn't work?
> (Apparently, "xl create" doesn't take '-t' option.)
> 
> > To connect to the emulated sbsa uart you need to pass -t vuart. So yes,
> > "xl console -t vuart domain_name" should get you access to the emulated
> > sbsa uart. The sbsa uart can also be exposed to dom0less guests; you get
> > their output by using CTRL-AAA to switch to right domU console.
> > 
> > You can add printks to xen/arch/arm/vpl011.c in Xen to see what's
> > happening on the Xen side. vpl011.c is the emulator.
> 
> I'm sure that write to "REG_DR" register is caught by Xen.
> What I don't understand is
> if back_in_domain -> no outputs
> if !back_in_domain -> can see outputs
> 
> (As you know, if a guest is created by xl command, back_in_domain
> is forcedly set to true.)
> 
> I looked into xenstore and found that "vuart/0/tty" does not exist,
> but "console/tty" does.
> How can this happen for vuart?
> (I clearly specified, vuart = "sbsa_uart" in Xen config.)

It looks like we have a bug :-(

I managed to reproduce the issue. The problem is a race in libxl.

tools/libxc/xc_dom_arm.c:alloc_magic_pages is called first, setting
dom->vuart_gfn.  Then, libxl__build_hvm should be setting
state->vuart_gfn to dom->vuart_gfn (like libxl__build_pv does) but it
doesn't.


---

libxl: set vuart_gfn in libxl__build_hvm

Setting vuart_gfn was missed when switching ARM guests to the PVH build.
Like libxl__build_pv, libxl__build_hvm should set state->vuart_gfn to
dom->vuart_gfn.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f8661e90d4..36fe8915e7 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1184,6 +1184,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
         LOG(ERROR, "hvm build set params failed");
         goto out;
     }
+    state->vuart_gfn = dom->vuart_gfn;
 
     rc = hvm_build_set_xs_values(gc, domid, dom, info);
     if (rc != 0) {

Re: BUG: libxl vuart build order

[Takahiro Akashi]

Hi Stefano,

On Thu, Oct 29, 2020 at 07:03:28PM -0700, Stefano Stabellini wrote:
> + xen-devel and libxl maintainers
> 
> In short, there is a regression in libxl with the ARM vuart introduced
> by moving ARM guests to the PVH build.
> 
> 
> On Thu, 29 Oct 2020, Takahiro Akashi wrote:
> > On Wed, Oct 28, 2020 at 02:44:16PM -0700, Stefano Stabellini wrote:
> > > On Wed, 28 Oct 2020, Takahiro Akashi wrote:
> > > > On Tue, Oct 27, 2020 at 09:02:14AM +0900, Takahiro Akashi wrote:
> > > > > On Mon, Oct 26, 2020 at 04:37:30PM -0700, Stefano Stabellini wrote:
> > > > > > 
> > > > > > On Mon, 26 Oct 2020, Takahiro Akashi wrote:
> > > > > > > Stefano,
> > > > > > > 
> > > > > > > # I'm afraid that I have already bothered you with a lot of questions.
> > > > > > > 
> > > > > > > When I looked at Xen's vpl011 implementation, I found
> > > > > > > CR (and LCHR) register is not supported. (trap may cause a data abort).
> > > > > > > On the other hand, for example, linux's pl011 driver surely
> > > > > > > accesses CR (and LCHR) register.
> > > > > > > So I guess that linux won't be able to use pl011 on a Xen guest vm
> > > > > > > if vuart = "sbsa_uart".
> > > > > > > 
> > > > > > > Is this a known issue or do I miss anything?
> > > > > > 
> > > > > > Linux should definitely be able to use it, and in fact, I am using it
> > > > > > with Linux in my test environment.
> > > > > > 
> > > > > > I think the confusion comes from the name "vpl011": it is in fact not a
> > > > > > full PL011 UART, but an SBSA UART.
> > > > > 
> > > > > Yeah, I have noticed it.
> > > > > 
> > > > > > SBSA UART only implements a subset of
> > > > > > the PL011 registers. The compatible string is "arm,sbsa-uart", also see
> > > > > > drivers/tty/serial/amba-pl011.c:sbsa_uart_probe.
> > > > > 
> > > > > Looking closely into the details of implementation, I found
> > > > > that all the accesses to unimplemented registers, including
> > > > > CR, are deliberately avoided in sbsa part of linux driver.
> > > > 
> > > > So I'm now trying to implement "sbsa-uart" driver on U-Boot
> > > > by modifying the existing pl011 driver.
> > > > (Please note the current xen'ized U-Boot utilises a para-virtualized
> > > > console, i.e. with HVM_PARAM_CONSOLE_PFN.)
> > > > 
> > > > So far my all attempts have failed.
> > > > 
> > > > There are a couple of problems, and one of them is how we can
> > > > access vpl011 port (from dom0).
> > > > What I did is:
> > > > - modify U-Boot's pl011 driver
> > > >   (I'm sure that the driver correctly handle a vpl011 device
> > > >   with regard of accessing a proper set of registers.)
> > > > - start U-Boot guest with "vuart=sbsa_uart" by
> > > >     xl create uboot.cfg -c
> > > > 
> > > > Then I have seen almost nothing on the screen.
> > > > Digging into vpl011 implementation, I found that all the characters
> > > > written DR register will be directed to a "backend domain" if a guest
> > > > vm is launched by xl command.
> > > > (In case of dom0less, the backend seems to be Xen itself.)
> > > > 
> > > > As a silly experiment, I modified domain_vpl011_init() to always create
> > > > a vpl011 device with "backend_in_domain == false".
> > > > Then, I could see more boot messages from U-Boot, but still fails
> > > > to use the device as a console, I mean, we will lose all the outputs
> > > > after at some point and won't be able to type any keys (at a command prompt).
> > > > (This will be another problem on U-Boot side.)
> > > > 
> > > > My first question here is how we can configure and connect a console
> > > > in this case?
> > > > Should "xl create -c" or "xl console -t vuart" simply work?
> > > 
> > > "xl create -c" creates a guest and connect to the primary console which
> > > is the PV console (i.e. HVM_PARAM_CONSOLE_PFN.)
> > 
> > So in case of vuart, it (console) doesn't work?
> > (Apparently, "xl create" doesn't take '-t' option.)
> > 
> > > To connect to the emulated sbsa uart you need to pass -t vuart. So yes,
> > > "xl console -t vuart domain_name" should get you access to the emulated
> > > sbsa uart. The sbsa uart can also be exposed to dom0less guests; you get
> > > their output by using CTRL-AAA to switch to right domU console.
> > > 
> > > You can add printks to xen/arch/arm/vpl011.c in Xen to see what's
> > > happening on the Xen side. vpl011.c is the emulator.
> > 
> > I'm sure that write to "REG_DR" register is caught by Xen.
> > What I don't understand is
> > if back_in_domain -> no outputs
> > if !back_in_domain -> can see outputs
> > 
> > (As you know, if a guest is created by xl command, back_in_domain
> > is forcedly set to true.)
> > 
> > I looked into xenstore and found that "vuart/0/tty" does not exist,
> > but "console/tty" does.
> > How can this happen for vuart?
> > (I clearly specified, vuart = "sbsa_uart" in Xen config.)
> 
> It looks like we have a bug :-(
> 
> I managed to reproduce the issue. The problem is a race in libxl.
> 
> tools/libxc/xc_dom_arm.c:alloc_magic_pages is called first, setting
> dom->vuart_gfn.  Then, libxl__build_hvm should be setting
> state->vuart_gfn to dom->vuart_gfn (like libxl__build_pv does) but it
> doesn't.

Thank you for the patch.
I confirmed that sbsa-uart driver on U-Boot now works.

=== after "xl console -t vuart" ===
U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest

Xen virtual CPU
Model: XENVM-4.15
DRAM:  128 MiB

In:    sbsa-pl011
Out:   sbsa-pl011
Err:   sbsa-pl011
xenguest# dm tree
 Class     Index  Probed  Driver                Name
-----------------------------------------------------------
 root          0  [ + ]   root_driver           root_driver
 firmware      0  [   ]   psci                  |-- psci
 serial        0  [ + ]   serial_pl01x          |-- sbsa-pl011
 pvblock       0  [   ]   pvblock               `-- pvblock
===

If possible, I hope that "xl create -c" command would accept "-t vuart"
option (or it should automatically selects uart type from the config).

-Takahiro Akashi


> 
> ---
> 
> libxl: set vuart_gfn in libxl__build_hvm
> 
> Setting vuart_gfn was missed when switching ARM guests to the PVH build.
> Like libxl__build_pv, libxl__build_hvm should set state->vuart_gfn to
> dom->vuart_gfn.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index f8661e90d4..36fe8915e7 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1184,6 +1184,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>          LOG(ERROR, "hvm build set params failed");
>          goto out;
>      }
> +    state->vuart_gfn = dom->vuart_gfn;
>  
>      rc = hvm_build_set_xs_values(gc, domid, dom, info);
>      if (rc != 0) {

Re: BUG: libxl vuart build order

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7189 bytes --]

On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> Hi Stefano,
> 
> On Thu, Oct 29, 2020 at 07:03:28PM -0700, Stefano Stabellini wrote:
> > + xen-devel and libxl maintainers
> > 
> > In short, there is a regression in libxl with the ARM vuart introduced
> > by moving ARM guests to the PVH build.
> > 
> > 
> > On Thu, 29 Oct 2020, Takahiro Akashi wrote:
> > > On Wed, Oct 28, 2020 at 02:44:16PM -0700, Stefano Stabellini wrote:
> > > > On Wed, 28 Oct 2020, Takahiro Akashi wrote:
> > > > > On Tue, Oct 27, 2020 at 09:02:14AM +0900, Takahiro Akashi wrote:
> > > > > > On Mon, Oct 26, 2020 at 04:37:30PM -0700, Stefano Stabellini wrote:
> > > > > > > 
> > > > > > > On Mon, 26 Oct 2020, Takahiro Akashi wrote:
> > > > > > > > Stefano,
> > > > > > > > 
> > > > > > > > # I'm afraid that I have already bothered you with a lot of questions.
> > > > > > > > 
> > > > > > > > When I looked at Xen's vpl011 implementation, I found
> > > > > > > > CR (and LCHR) register is not supported. (trap may cause a data abort).
> > > > > > > > On the other hand, for example, linux's pl011 driver surely
> > > > > > > > accesses CR (and LCHR) register.
> > > > > > > > So I guess that linux won't be able to use pl011 on a Xen guest vm
> > > > > > > > if vuart = "sbsa_uart".
> > > > > > > > 
> > > > > > > > Is this a known issue or do I miss anything?
> > > > > > > 
> > > > > > > Linux should definitely be able to use it, and in fact, I am using it
> > > > > > > with Linux in my test environment.
> > > > > > > 
> > > > > > > I think the confusion comes from the name "vpl011": it is in fact not a
> > > > > > > full PL011 UART, but an SBSA UART.
> > > > > > 
> > > > > > Yeah, I have noticed it.
> > > > > > 
> > > > > > > SBSA UART only implements a subset of
> > > > > > > the PL011 registers. The compatible string is "arm,sbsa-uart", also see
> > > > > > > drivers/tty/serial/amba-pl011.c:sbsa_uart_probe.
> > > > > > 
> > > > > > Looking closely into the details of implementation, I found
> > > > > > that all the accesses to unimplemented registers, including
> > > > > > CR, are deliberately avoided in sbsa part of linux driver.
> > > > > 
> > > > > So I'm now trying to implement "sbsa-uart" driver on U-Boot
> > > > > by modifying the existing pl011 driver.
> > > > > (Please note the current xen'ized U-Boot utilises a para-virtualized
> > > > > console, i.e. with HVM_PARAM_CONSOLE_PFN.)
> > > > > 
> > > > > So far my all attempts have failed.
> > > > > 
> > > > > There are a couple of problems, and one of them is how we can
> > > > > access vpl011 port (from dom0).
> > > > > What I did is:
> > > > > - modify U-Boot's pl011 driver
> > > > >   (I'm sure that the driver correctly handle a vpl011 device
> > > > >   with regard of accessing a proper set of registers.)
> > > > > - start U-Boot guest with "vuart=sbsa_uart" by
> > > > >     xl create uboot.cfg -c
> > > > > 
> > > > > Then I have seen almost nothing on the screen.
> > > > > Digging into vpl011 implementation, I found that all the characters
> > > > > written DR register will be directed to a "backend domain" if a guest
> > > > > vm is launched by xl command.
> > > > > (In case of dom0less, the backend seems to be Xen itself.)
> > > > > 
> > > > > As a silly experiment, I modified domain_vpl011_init() to always create
> > > > > a vpl011 device with "backend_in_domain == false".
> > > > > Then, I could see more boot messages from U-Boot, but still fails
> > > > > to use the device as a console, I mean, we will lose all the outputs
> > > > > after at some point and won't be able to type any keys (at a command prompt).
> > > > > (This will be another problem on U-Boot side.)
> > > > > 
> > > > > My first question here is how we can configure and connect a console
> > > > > in this case?
> > > > > Should "xl create -c" or "xl console -t vuart" simply work?
> > > > 
> > > > "xl create -c" creates a guest and connect to the primary console which
> > > > is the PV console (i.e. HVM_PARAM_CONSOLE_PFN.)
> > > 
> > > So in case of vuart, it (console) doesn't work?
> > > (Apparently, "xl create" doesn't take '-t' option.)
> > > 
> > > > To connect to the emulated sbsa uart you need to pass -t vuart. So yes,
> > > > "xl console -t vuart domain_name" should get you access to the emulated
> > > > sbsa uart. The sbsa uart can also be exposed to dom0less guests; you get
> > > > their output by using CTRL-AAA to switch to right domU console.
> > > > 
> > > > You can add printks to xen/arch/arm/vpl011.c in Xen to see what's
> > > > happening on the Xen side. vpl011.c is the emulator.
> > > 
> > > I'm sure that write to "REG_DR" register is caught by Xen.
> > > What I don't understand is
> > > if back_in_domain -> no outputs
> > > if !back_in_domain -> can see outputs
> > > 
> > > (As you know, if a guest is created by xl command, back_in_domain
> > > is forcedly set to true.)
> > > 
> > > I looked into xenstore and found that "vuart/0/tty" does not exist,
> > > but "console/tty" does.
> > > How can this happen for vuart?
> > > (I clearly specified, vuart = "sbsa_uart" in Xen config.)
> > 
> > It looks like we have a bug :-(
> > 
> > I managed to reproduce the issue. The problem is a race in libxl.
> > 
> > tools/libxc/xc_dom_arm.c:alloc_magic_pages is called first, setting
> > dom->vuart_gfn.  Then, libxl__build_hvm should be setting
> > state->vuart_gfn to dom->vuart_gfn (like libxl__build_pv does) but it
> > doesn't.
> 
> Thank you for the patch.
> I confirmed that sbsa-uart driver on U-Boot now works.

Excellent!


> === after "xl console -t vuart" ===
> U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> 
> Xen virtual CPU
> Model: XENVM-4.15
> DRAM:  128 MiB
> 
> In:    sbsa-pl011
> Out:   sbsa-pl011
> Err:   sbsa-pl011
> xenguest# dm tree
>  Class     Index  Probed  Driver                Name
> -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
>  firmware      0  [   ]   psci                  |-- psci
>  serial        0  [ + ]   serial_pl01x          |-- sbsa-pl011
>  pvblock       0  [   ]   pvblock               `-- pvblock
> ===
> 
> If possible, I hope that "xl create -c" command would accept "-t vuart"
> option (or it should automatically selects uart type from the config).

I think a patch to add the "-t" option to "xl create" would be
acceptable, right Anthony?


> > 
> > ---
> > 
> > libxl: set vuart_gfn in libxl__build_hvm
> > 
> > Setting vuart_gfn was missed when switching ARM guests to the PVH build.
> > Like libxl__build_pv, libxl__build_hvm should set state->vuart_gfn to
> > dom->vuart_gfn.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index f8661e90d4..36fe8915e7 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -1184,6 +1184,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> >          LOG(ERROR, "hvm build set params failed");
> >          goto out;
> >      }
> > +    state->vuart_gfn = dom->vuart_gfn;
> >  
> >      rc = hvm_build_set_xs_values(gc, domid, dom, info);
> >      if (rc != 0) {
> 

Re: BUG: libxl vuart build order

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7586 bytes --]

ping?


On Fri, 30 Oct 2020, Stefano Stabellini wrote:
> On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> > Hi Stefano,
> > 
> > On Thu, Oct 29, 2020 at 07:03:28PM -0700, Stefano Stabellini wrote:
> > > + xen-devel and libxl maintainers
> > > 
> > > In short, there is a regression in libxl with the ARM vuart introduced
> > > by moving ARM guests to the PVH build.
> > > 
> > > 
> > > On Thu, 29 Oct 2020, Takahiro Akashi wrote:
> > > > On Wed, Oct 28, 2020 at 02:44:16PM -0700, Stefano Stabellini wrote:
> > > > > On Wed, 28 Oct 2020, Takahiro Akashi wrote:
> > > > > > On Tue, Oct 27, 2020 at 09:02:14AM +0900, Takahiro Akashi wrote:
> > > > > > > On Mon, Oct 26, 2020 at 04:37:30PM -0700, Stefano Stabellini wrote:
> > > > > > > > 
> > > > > > > > On Mon, 26 Oct 2020, Takahiro Akashi wrote:
> > > > > > > > > Stefano,
> > > > > > > > > 
> > > > > > > > > # I'm afraid that I have already bothered you with a lot of questions.
> > > > > > > > > 
> > > > > > > > > When I looked at Xen's vpl011 implementation, I found
> > > > > > > > > CR (and LCHR) register is not supported. (trap may cause a data abort).
> > > > > > > > > On the other hand, for example, linux's pl011 driver surely
> > > > > > > > > accesses CR (and LCHR) register.
> > > > > > > > > So I guess that linux won't be able to use pl011 on a Xen guest vm
> > > > > > > > > if vuart = "sbsa_uart".
> > > > > > > > > 
> > > > > > > > > Is this a known issue or do I miss anything?
> > > > > > > > 
> > > > > > > > Linux should definitely be able to use it, and in fact, I am using it
> > > > > > > > with Linux in my test environment.
> > > > > > > > 
> > > > > > > > I think the confusion comes from the name "vpl011": it is in fact not a
> > > > > > > > full PL011 UART, but an SBSA UART.
> > > > > > > 
> > > > > > > Yeah, I have noticed it.
> > > > > > > 
> > > > > > > > SBSA UART only implements a subset of
> > > > > > > > the PL011 registers. The compatible string is "arm,sbsa-uart", also see
> > > > > > > > drivers/tty/serial/amba-pl011.c:sbsa_uart_probe.
> > > > > > > 
> > > > > > > Looking closely into the details of implementation, I found
> > > > > > > that all the accesses to unimplemented registers, including
> > > > > > > CR, are deliberately avoided in sbsa part of linux driver.
> > > > > > 
> > > > > > So I'm now trying to implement "sbsa-uart" driver on U-Boot
> > > > > > by modifying the existing pl011 driver.
> > > > > > (Please note the current xen'ized U-Boot utilises a para-virtualized
> > > > > > console, i.e. with HVM_PARAM_CONSOLE_PFN.)
> > > > > > 
> > > > > > So far my all attempts have failed.
> > > > > > 
> > > > > > There are a couple of problems, and one of them is how we can
> > > > > > access vpl011 port (from dom0).
> > > > > > What I did is:
> > > > > > - modify U-Boot's pl011 driver
> > > > > >   (I'm sure that the driver correctly handle a vpl011 device
> > > > > >   with regard of accessing a proper set of registers.)
> > > > > > - start U-Boot guest with "vuart=sbsa_uart" by
> > > > > >     xl create uboot.cfg -c
> > > > > > 
> > > > > > Then I have seen almost nothing on the screen.
> > > > > > Digging into vpl011 implementation, I found that all the characters
> > > > > > written DR register will be directed to a "backend domain" if a guest
> > > > > > vm is launched by xl command.
> > > > > > (In case of dom0less, the backend seems to be Xen itself.)
> > > > > > 
> > > > > > As a silly experiment, I modified domain_vpl011_init() to always create
> > > > > > a vpl011 device with "backend_in_domain == false".
> > > > > > Then, I could see more boot messages from U-Boot, but still fails
> > > > > > to use the device as a console, I mean, we will lose all the outputs
> > > > > > after at some point and won't be able to type any keys (at a command prompt).
> > > > > > (This will be another problem on U-Boot side.)
> > > > > > 
> > > > > > My first question here is how we can configure and connect a console
> > > > > > in this case?
> > > > > > Should "xl create -c" or "xl console -t vuart" simply work?
> > > > > 
> > > > > "xl create -c" creates a guest and connect to the primary console which
> > > > > is the PV console (i.e. HVM_PARAM_CONSOLE_PFN.)
> > > > 
> > > > So in case of vuart, it (console) doesn't work?
> > > > (Apparently, "xl create" doesn't take '-t' option.)
> > > > 
> > > > > To connect to the emulated sbsa uart you need to pass -t vuart. So yes,
> > > > > "xl console -t vuart domain_name" should get you access to the emulated
> > > > > sbsa uart. The sbsa uart can also be exposed to dom0less guests; you get
> > > > > their output by using CTRL-AAA to switch to right domU console.
> > > > > 
> > > > > You can add printks to xen/arch/arm/vpl011.c in Xen to see what's
> > > > > happening on the Xen side. vpl011.c is the emulator.
> > > > 
> > > > I'm sure that write to "REG_DR" register is caught by Xen.
> > > > What I don't understand is
> > > > if back_in_domain -> no outputs
> > > > if !back_in_domain -> can see outputs
> > > > 
> > > > (As you know, if a guest is created by xl command, back_in_domain
> > > > is forcedly set to true.)
> > > > 
> > > > I looked into xenstore and found that "vuart/0/tty" does not exist,
> > > > but "console/tty" does.
> > > > How can this happen for vuart?
> > > > (I clearly specified, vuart = "sbsa_uart" in Xen config.)
> > > 
> > > It looks like we have a bug :-(
> > > 
> > > I managed to reproduce the issue. The problem is a race in libxl.
> > > 
> > > tools/libxc/xc_dom_arm.c:alloc_magic_pages is called first, setting
> > > dom->vuart_gfn.  Then, libxl__build_hvm should be setting
> > > state->vuart_gfn to dom->vuart_gfn (like libxl__build_pv does) but it
> > > doesn't.
> > 
> > Thank you for the patch.
> > I confirmed that sbsa-uart driver on U-Boot now works.
> 
> Excellent!
> 
> 
> > === after "xl console -t vuart" ===
> > U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> > 
> > Xen virtual CPU
> > Model: XENVM-4.15
> > DRAM:  128 MiB
> > 
> > In:    sbsa-pl011
> > Out:   sbsa-pl011
> > Err:   sbsa-pl011
> > xenguest# dm tree
> >  Class     Index  Probed  Driver                Name
> > -----------------------------------------------------------
> >  root          0  [ + ]   root_driver           root_driver
> >  firmware      0  [   ]   psci                  |-- psci
> >  serial        0  [ + ]   serial_pl01x          |-- sbsa-pl011
> >  pvblock       0  [   ]   pvblock               `-- pvblock
> > ===
> > 
> > If possible, I hope that "xl create -c" command would accept "-t vuart"
> > option (or it should automatically selects uart type from the config).
> 
> I think a patch to add the "-t" option to "xl create" would be
> acceptable, right Anthony?
> 
> 
> > > 
> > > ---
> > > 
> > > libxl: set vuart_gfn in libxl__build_hvm
> > > 
> > > Setting vuart_gfn was missed when switching ARM guests to the PVH build.
> > > Like libxl__build_pv, libxl__build_hvm should set state->vuart_gfn to
> > > dom->vuart_gfn.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > 
> > > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > > index f8661e90d4..36fe8915e7 100644
> > > --- a/tools/libxl/libxl_dom.c
> > > +++ b/tools/libxl/libxl_dom.c
> > > @@ -1184,6 +1184,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
> > >          LOG(ERROR, "hvm build set params failed");
> > >          goto out;
> > >      }
> > > +    state->vuart_gfn = dom->vuart_gfn;
> > >  
> > >      rc = hvm_build_set_xs_values(gc, domid, dom, info);
> > >      if (rc != 0) {
> > 

Re: BUG: libxl vuart build order

[Anthony PERARD]

On Fri, Oct 30, 2020 at 10:46:37AM -0700, Stefano Stabellini wrote:
> On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> > === after "xl console -t vuart" ===
> > U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> > 
> > Xen virtual CPU
> > Model: XENVM-4.15
> > DRAM:  128 MiB
> > 
> > In:    sbsa-pl011
> > Out:   sbsa-pl011
> > Err:   sbsa-pl011
> > ===
> > 
> > If possible, I hope that "xl create -c" command would accept "-t vuart"
> > option (or it should automatically selects uart type from the config).
> 
> I think a patch to add the "-t" option to "xl create" would be
> acceptable, right Anthony?

I don't know. Why `xl' isn't able to select the vuart as the default one?

Maybe a long option would be better in the cases where we would like to
connect to a "secondary" console? I could see `xl create --console=vuart'
been fine, I don't know if that's possible.

-- 
Anthony PERARD

Re: BUG: libxl vuart build order

[Julien Grall]

On 05/11/2020 15:41, Anthony PERARD wrote:
> On Fri, Oct 30, 2020 at 10:46:37AM -0700, Stefano Stabellini wrote:
>> On Fri, 30 Oct 2020, Takahiro Akashi wrote:
>>> === after "xl console -t vuart" ===
>>> U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
>>>
>>> Xen virtual CPU
>>> Model: XENVM-4.15
>>> DRAM:  128 MiB
>>>
>>> In:    sbsa-pl011
>>> Out:   sbsa-pl011
>>> Err:   sbsa-pl011
>>> ===
>>>
>>> If possible, I hope that "xl create -c" command would accept "-t vuart"
>>> option (or it should automatically selects uart type from the config).
>>
>> I think a patch to add the "-t" option to "xl create" would be
>> acceptable, right Anthony?
> 
> I don't know. Why `xl' isn't able to select the vuart as the default one?

The SBSA UART was originally introduced mostly for debugging purpose and 
to be compliant with the SBSA specification.

The default console so for on Arm is the PV console.

Cheers,

-- 
Julien Grall

Re: BUG: libxl vuart build order

[Stefano Stabellini]

On Thu, 5 Nov 2020, Anthony PERARD wrote:
> On Fri, Oct 30, 2020 at 10:46:37AM -0700, Stefano Stabellini wrote:
> > On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> > > === after "xl console -t vuart" ===
> > > U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> > > 
> > > Xen virtual CPU
> > > Model: XENVM-4.15
> > > DRAM:  128 MiB
> > > 
> > > In:    sbsa-pl011
> > > Out:   sbsa-pl011
> > > Err:   sbsa-pl011
> > > ===
> > > 
> > > If possible, I hope that "xl create -c" command would accept "-t vuart"
> > > option (or it should automatically selects uart type from the config).
> > 
> > I think a patch to add the "-t" option to "xl create" would be
> > acceptable, right Anthony?
> 
> I don't know. Why `xl' isn't able to select the vuart as the default one?

Because both consoles are still valid: when the emulated uart is
enabled, the normal PV console is also enabled.


> Maybe a long option would be better in the cases where we would like to
> connect to a "secondary" console? I could see `xl create --console=vuart'
> been fine, I don't know if that's possible.

That's OK for me but keep in mind that xl console already takes -t
vuart. In other words:

1) xl console -t vuart    -> WORKS
2) xl create -c -t vuart  -> DOESN'T WORK


P.S.

Could you also take a quick look at the patch I appended to the previous
email? Or would you prefer me to send it out separately as its own
patch?

Re: BUG: libxl vuart build order

[Anthony PERARD]

On Thu, Nov 05, 2020 at 08:29:20AM -0800, Stefano Stabellini wrote:
> On Thu, 5 Nov 2020, Anthony PERARD wrote:
> > On Fri, Oct 30, 2020 at 10:46:37AM -0700, Stefano Stabellini wrote:
> > > On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> > > > === after "xl console -t vuart" ===
> > > > U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> > > > 
> > > > Xen virtual CPU
> > > > Model: XENVM-4.15
> > > > DRAM:  128 MiB
> > > > 
> > > > In:    sbsa-pl011
> > > > Out:   sbsa-pl011
> > > > Err:   sbsa-pl011
> > > > ===
> > > > 
> > > > If possible, I hope that "xl create -c" command would accept "-t vuart"
> > > > option (or it should automatically selects uart type from the config).
> > > 
> > > I think a patch to add the "-t" option to "xl create" would be
> > > acceptable, right Anthony?
> > 
> > I don't know. Why `xl' isn't able to select the vuart as the default one?
> 
> Because both consoles are still valid: when the emulated uart is
> enabled, the normal PV console is also enabled.
> 
> 
> > Maybe a long option would be better in the cases where we would like to
> > connect to a "secondary" console? I could see `xl create --console=vuart'
> > been fine, I don't know if that's possible.
> 
> That's OK for me but keep in mind that xl console already takes -t
> vuart. In other words:

I don't know why we would need the same exact option, `xl console` and
`xl create` are two different commands. Also, I usually prefer long
option for less used options as it makes it a bit easier to figure out
what a command is supposed to do (without checking the man; and when
there is both long and short options available).

> 1) xl console -t vuart    -> WORKS

-t for `xl console` kind of works well, -t could be a shortcut of "type"
of console".

> 2) xl create -c -t vuart  -> DOESN'T WORK

But here, -t would not be a "type" of console since we are creating a
VM. Also `xl create -t vuart` without -c would do nothing, right?
(create a vm but ignoring the -t).

Anyway, an option to auto-connect to a different console would be
useful.

> P.S.
> 
> Could you also take a quick look at the patch I appended to the previous
> email? Or would you prefer me to send it out separately as its own
> patch?

It's probably better to have a patch on its own when it's ready for
review rather that been embedded in an email in a long
discussion/debugging thread. That leave a better chance for others to
spot that a patch exist and review.

Cheers,

-- 
Anthony PERARD

Re: BUG: libxl vuart build order

[Stefano Stabellini]

On Thu, 5 Nov 2020, Anthony PERARD wrote:
> On Thu, Nov 05, 2020 at 08:29:20AM -0800, Stefano Stabellini wrote:
> > On Thu, 5 Nov 2020, Anthony PERARD wrote:
> > > On Fri, Oct 30, 2020 at 10:46:37AM -0700, Stefano Stabellini wrote:
> > > > On Fri, 30 Oct 2020, Takahiro Akashi wrote:
> > > > > === after "xl console -t vuart" ===
> > > > > U-Boot 2020.10-00777-g10cf956a26ba (Oct 29 2020 - 19:31:29 +0900) xenguest
> > > > > 
> > > > > Xen virtual CPU
> > > > > Model: XENVM-4.15
> > > > > DRAM:  128 MiB
> > > > > 
> > > > > In:    sbsa-pl011
> > > > > Out:   sbsa-pl011
> > > > > Err:   sbsa-pl011
> > > > > ===
> > > > > 
> > > > > If possible, I hope that "xl create -c" command would accept "-t vuart"
> > > > > option (or it should automatically selects uart type from the config).
> > > > 
> > > > I think a patch to add the "-t" option to "xl create" would be
> > > > acceptable, right Anthony?
> > > 
> > > I don't know. Why `xl' isn't able to select the vuart as the default one?
> > 
> > Because both consoles are still valid: when the emulated uart is
> > enabled, the normal PV console is also enabled.
> > 
> > 
> > > Maybe a long option would be better in the cases where we would like to
> > > connect to a "secondary" console? I could see `xl create --console=vuart'
> > > been fine, I don't know if that's possible.
> > 
> > That's OK for me but keep in mind that xl console already takes -t
> > vuart. In other words:
> 
> I don't know why we would need the same exact option, `xl console` and
> `xl create` are two different commands. Also, I usually prefer long
> option for less used options as it makes it a bit easier to figure out
> what a command is supposed to do (without checking the man; and when
> there is both long and short options available).

That is true. I don't have a strong opinion if it should be "-t" or
something longer like "--console". Either one works. I tend to like "-t"
a bit more because it would make it more consident, but I don't think it
matters much.


> > 1) xl console -t vuart    -> WORKS
> 
> -t for `xl console` kind of works well, -t could be a shortcut of "type"
> of console".
> 
> > 2) xl create -c -t vuart  -> DOESN'T WORK
> 
> But here, -t would not be a "type" of console since we are creating a
> VM. Also `xl create -t vuart` without -c would do nothing, right?
> (create a vm but ignoring the -t).

Yes, it would do nothing


> Anyway, an option to auto-connect to a different console would be
> useful.

Yeah


> > P.S.
> > 
> > Could you also take a quick look at the patch I appended to the previous
> > email? Or would you prefer me to send it out separately as its own
> > patch?
> 
> It's probably better to have a patch on its own when it's ready for
> review rather that been embedded in an email in a long
> discussion/debugging thread. That leave a better chance for others to
> spot that a patch exist and review.

All right, I'll resend reparately.