[PATCH v2 0/2] tools/helpers: PVH xenstore-stubdom console fixes

[PATCH v2 0/2] tools/helpers: PVH xenstore-stubdom console fixes

[Juergen Gross]

The console parameters for a PVH Xenstore-stubdom have been missing
or were just wrong.

Juergen Gross (2):
  tools/helpers: fix PVH xenstore-stubdom console parameters
  tools/helpers: set event channel for PVH xenstore-stubdom console

 tools/helpers/init-xenstore-domain.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] tools/helpers: fix PVH xenstore-stubdom console parameters

[Juergen Gross]

When using a PVH mode xenstore-stubdom the frame number of the console
should be a PFN instead of a MFN.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
V2:
- rename variable (Andrew Cooper)
---
 tools/helpers/init-xenstore-domain.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index b205a79ee6..9457d0251b 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -30,7 +30,7 @@ static char *param;
 static char *name = "Xenstore";
 static int memory;
 static int maxmem;
-static xen_pfn_t console_mfn;
+static xen_pfn_t console_gfn;
 static xc_evtchn_port_or_error_t console_evtchn;
 
 static struct option options[] = {
@@ -283,7 +283,9 @@ static int build(xc_interface *xch)
     }
 
     rv = 0;
-    console_mfn = xc_dom_p2m(dom, dom->console_pfn);
+    console_gfn = (dom->container_type == XC_DOM_PV_CONTAINER)
+                  ? xc_dom_p2m(dom, dom->console_pfn)
+                  : dom->console_pfn;
 
 err:
     if ( dom )
@@ -528,7 +530,7 @@ int main(int argc, char** argv)
     do_xs_write_dir_node(xsh, fe_path, "tty", "");
     snprintf(buf, 16, "%d", console_evtchn);
     do_xs_write_dir_node(xsh, fe_path, "port", buf);
-    snprintf(buf, 16, "%ld", console_mfn);
+    snprintf(buf, 16, "%ld", console_gfn);
     do_xs_write_dir_node(xsh, fe_path, "ring-ref", buf);
     xs_close(xsh);
 
-- 
2.26.2

[PATCH v2 2/2] tools/helpers: set event channel for PVH xenstore-stubdom console

[Juergen Gross]

In contrast to the PFN of the console ring page the event channel of
the console isn't being set automatically by xc_dom_build_image().

Call xc_hvm_param_set() explicitly for that reason.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 tools/helpers/init-xenstore-domain.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 9457d0251b..3eff839827 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -249,6 +249,14 @@ static int build(xc_interface *xch)
             fprintf(stderr, "xc_domain_set_memory_map failed\n");
             goto err;
         }
+
+        rv = xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_EVTCHN,
+                              console_evtchn);
+        if ( rv )
+        {
+            fprintf(stderr, "xc_hvm_param_set failed\n");
+            goto err;
+        }
     }
     rv = xc_dom_build_image(dom);
     if ( rv )
-- 
2.26.2

Re: [PATCH v2 2/2] tools/helpers: set event channel for PVH xenstore-stubdom console

[Andrew Cooper]

On 06/12/2021 14:29, Juergen Gross wrote:
> In contrast to the PFN of the console ring page the event channel of
> the console isn't being set automatically by xc_dom_build_image().
>
> Call xc_hvm_param_set() explicitly for that reason.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

So, technically, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

However...

That said, there is a distinct lack of joined-up thinking in this interface.

It makes no sense whatsoever to for xc_dom_build_image() to build the
grant, but leave the evtchn to the caller.

And indeed,

xg_dom_x86.c: start_info->console.domU.evtchn = dom->console_evtchn;

we set it up on the PV side of things.  So I think the proper fix is to
wire up the HVM side and prevent the callers needing to do this.


Furthermore, I doubt we skip setting up the xenstore connection.

Really, the users of xc_dom_build_image() want a console Y/n, xenstore
Y/n type interface, and judging by the fields we've already got, that
can reasonably be done on the non-zero-ness of *_evtchn

(It is also weird that the caller is required to bind the evtchn, but
that's so baked into the API that I'd need to rearrange code between
Ocaml daemons to make use of a "library code allocates evtchn+grant
together" option.)

~Andrew