Re: BUG: libxl vuart build order

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > 
> > > 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) {
> >