| Message ID | 20230127055421.22945-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tools/helpers: don't log errors when trying to load PVH xenstore-stubdom | expand |
On 27.01.2023 06:54, Juergen Gross wrote: > When loading a Xenstore stubdom the loader doesn't know whether the > lo be loaded kernel is a PVH or a PV one. So it tries to load it as > a PVH one first, and if this fails it is loading it as a PV kernel. > > This results in errors being logged in case the stubdom is a PV kernel. > > Suppress those errors by setting the minimum logging level to > "critical" while trying to load the kernel as PVH. > > Fixes: f89955449c5a ("tools/init-xenstore-domain: support xenstore pvh stubdom") > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - retry PVH loading with logging if PV fails, too (Jan Beulich) I'm sorry to be picky, but shouldn't this be reflected in the description? Jan
On 27.01.23 08:40, Jan Beulich wrote: > On 27.01.2023 06:54, Juergen Gross wrote: >> When loading a Xenstore stubdom the loader doesn't know whether the >> lo be loaded kernel is a PVH or a PV one. So it tries to load it as >> a PVH one first, and if this fails it is loading it as a PV kernel. >> >> This results in errors being logged in case the stubdom is a PV kernel. >> >> Suppress those errors by setting the minimum logging level to >> "critical" while trying to load the kernel as PVH. >> >> Fixes: f89955449c5a ("tools/init-xenstore-domain: support xenstore pvh stubdom") >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - retry PVH loading with logging if PV fails, too (Jan Beulich) > > I'm sorry to be picky, but shouldn't this be reflected in the description? I can add that, but I think looking at the patch itself it is rather clear, especially with the added comments. If you still think it should be added, my suggestion would be: In case PVH mode and PV mode loading fails, retry PVH mode loading without changing the log level in order to get the error messages logged. I can resend, if you want me to, or I'd be fine with above addition added while committing (if needed). Juergen
On 27/01/2023 5:54 am, Juergen Gross wrote: > When loading a Xenstore stubdom the loader doesn't know whether the > lo be loaded kernel is a PVH or a PV one. So it tries to load it as > a PVH one first, and if this fails it is loading it as a PV kernel. Well, yes it does. What might be missing is libxenguest's ability to parse the provided kernel's ELF Notes ahead of trying to build the domain. This is the same kind of poor design which has left us with a dom0=pv|pvh cmdline option which must match the kernel the bootloader gave us, if we want to not panic() on boot. So while this might be an acceptable gross bodge in the short term, this... > > This results in errors being logged in case the stubdom is a PV kernel. > > Suppress those errors by setting the minimum logging level to > "critical" while trying to load the kernel as PVH. > > Fixes: f89955449c5a ("tools/init-xenstore-domain: support xenstore pvh stubdom") > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - retry PVH loading with logging if PV fails, too (Jan Beulich) > --- > tools/helpers/init-xenstore-domain.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c > index 04e351ca29..4e2f8d4da5 100644 > --- a/tools/helpers/init-xenstore-domain.c > +++ b/tools/helpers/init-xenstore-domain.c > @@ -31,6 +31,8 @@ static int memory; > static int maxmem; > static xen_pfn_t console_gfn; > static xc_evtchn_port_or_error_t console_evtchn; > +static xentoollog_level minmsglevel = XTL_PROGRESS; > +static void *logger; > > static struct option options[] = { > { "kernel", 1, NULL, 'k' }, > @@ -141,19 +143,29 @@ static int build(xc_interface *xch) > goto err; > } > > + /* Try PVH first, suppress errors by setting min level high. */ ... needs to make the position clear. /* This is a bodge. We can't currently inspect the kernel's ELF notes ahead of attempting to construct a domain, so try PVH first, suppressing errors by setting min level to high, and fall back to PV. */ ~Andrew > dom->container_type = XC_DOM_HVM_CONTAINER; > + xtl_stdiostream_set_minlevel(logger, XTL_CRITICAL); > rv = xc_dom_parse_image(dom); > + xtl_stdiostream_set_minlevel(logger, minmsglevel); > if ( rv ) > { > dom->container_type = XC_DOM_PV_CONTAINER; > rv = xc_dom_parse_image(dom); > if ( rv ) > { > - fprintf(stderr, "xc_dom_parse_image failed\n"); > - goto err; > + /* Retry PVH, now with normal logging level. */ > + dom->container_type = XC_DOM_HVM_CONTAINER; > + rv = xc_dom_parse_image(dom); > + if ( rv ) > + { > + fprintf(stderr, "xc_dom_parse_image failed\n"); > + goto err; > + } > } > } > - else > + > + if ( dom->container_type == XC_DOM_HVM_CONTAINER ) > { > config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap; > config.arch.emulation_flags = XEN_X86_EMU_LAPIC; > @@ -412,8 +424,6 @@ int main(int argc, char** argv) > char buf[16], be_path[64], fe_path[64]; > int rv, fd; > char *maxmem_str = NULL; > - xentoollog_level minmsglevel = XTL_PROGRESS; > - xentoollog_logger *logger = NULL; > > while ( (opt = getopt_long(argc, argv, "v", options, NULL)) != -1 ) > { > @@ -456,9 +466,7 @@ int main(int argc, char** argv) > return 2; > } > > - logger = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, > - minmsglevel, 0); > - > + logger = xtl_createlogger_stdiostream(stderr, minmsglevel, 0); > xch = xc_interface_open(logger, logger, 0); > if ( !xch ) > {
On 27.01.23 11:28, Andrew Cooper wrote: > On 27/01/2023 5:54 am, Juergen Gross wrote: >> When loading a Xenstore stubdom the loader doesn't know whether the >> lo be loaded kernel is a PVH or a PV one. So it tries to load it as >> a PVH one first, and if this fails it is loading it as a PV kernel. > > Well, yes it does. > > What might be missing is libxenguest's ability to parse the provided > kernel's ELF Notes ahead of trying to build the domain. Correct. > This is the same kind of poor design which has left us with a > dom0=pv|pvh cmdline option which must match the kernel the bootloader > gave us, if we want to not panic() on boot. Hmm, this is only partially true. I agree that without any dom0 option given it would be nice if the hypervisor could use the specified dom0 kernel as long as it is supporting either PV or PVH mode. OTOH nowadays it is easily possible to build a kernel being capable to support both variants, in which case the hypervisor needs to select which mode to use. This might need the help of the user in case the non-default mode is wanted. For xenstore-stubdom it is easier, as there is no reason to prefer the PV mode over PVH (in fact I'm still working on Xenstore LU for the PVH case, making the decision even easier). > > So while this might be an acceptable gross bodge in the short term, this... > >> >> This results in errors being logged in case the stubdom is a PV kernel. >> >> Suppress those errors by setting the minimum logging level to >> "critical" while trying to load the kernel as PVH. >> >> Fixes: f89955449c5a ("tools/init-xenstore-domain: support xenstore pvh stubdom") >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - retry PVH loading with logging if PV fails, too (Jan Beulich) >> --- >> tools/helpers/init-xenstore-domain.c | 24 ++++++++++++++++-------- >> 1 file changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c >> index 04e351ca29..4e2f8d4da5 100644 >> --- a/tools/helpers/init-xenstore-domain.c >> +++ b/tools/helpers/init-xenstore-domain.c >> @@ -31,6 +31,8 @@ static int memory; >> static int maxmem; >> static xen_pfn_t console_gfn; >> static xc_evtchn_port_or_error_t console_evtchn; >> +static xentoollog_level minmsglevel = XTL_PROGRESS; >> +static void *logger; >> >> static struct option options[] = { >> { "kernel", 1, NULL, 'k' }, >> @@ -141,19 +143,29 @@ static int build(xc_interface *xch) >> goto err; >> } >> >> + /* Try PVH first, suppress errors by setting min level high. */ > > ... needs to make the position clear. > > /* This is a bodge. We can't currently inspect the kernel's ELF notes > ahead of attempting to construct a domain, so try PVH first, suppressing > errors by setting min level to high, and fall back to PV. */ No problem with me. Juergen
On 27/01/2023 10:55 am, Juergen Gross wrote: > > For xenstore-stubdom it is easier, as there is no reason to prefer > the PV mode over PVH (in fact I'm still working on Xenstore LU for > the PVH case, making the decision even easier). I fully support having both PV and PVH options available. It helps maintain architecturally "clean" interfaces, and not decisions are about performance. But a PVH stub xenstored cannot outperform a equivalent PV stub xenstored, for x86 architectural reasons. PV stub xenstored doesn't do any of the usual things that causes PV guests to be generally slow (different address spaces, user/kernel context switching), and event/grants to PV guests is architecturally more efficient than the HVM/PVH equivalents. PV will never go away fully die, because we've got enough fastpaths where this matters, but I do expect that we'll eventually get to a point where we don't have any general purpose OSes running as PV - only single-task workloads such as stub xenstored. The great thing about having both PV and PVH available is that people will be able to see the concrete data confirming that PV is better, without having to simply trust me on the matter. ~Andrew
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c index 04e351ca29..4e2f8d4da5 100644 --- a/tools/helpers/init-xenstore-domain.c +++ b/tools/helpers/init-xenstore-domain.c @@ -31,6 +31,8 @@ static int memory; static int maxmem; static xen_pfn_t console_gfn; static xc_evtchn_port_or_error_t console_evtchn; +static xentoollog_level minmsglevel = XTL_PROGRESS; +static void *logger; static struct option options[] = { { "kernel", 1, NULL, 'k' }, @@ -141,19 +143,29 @@ static int build(xc_interface *xch) goto err; } + /* Try PVH first, suppress errors by setting min level high. */ dom->container_type = XC_DOM_HVM_CONTAINER; + xtl_stdiostream_set_minlevel(logger, XTL_CRITICAL); rv = xc_dom_parse_image(dom); + xtl_stdiostream_set_minlevel(logger, minmsglevel); if ( rv ) { dom->container_type = XC_DOM_PV_CONTAINER; rv = xc_dom_parse_image(dom); if ( rv ) { - fprintf(stderr, "xc_dom_parse_image failed\n"); - goto err; + /* Retry PVH, now with normal logging level. */ + dom->container_type = XC_DOM_HVM_CONTAINER; + rv = xc_dom_parse_image(dom); + if ( rv ) + { + fprintf(stderr, "xc_dom_parse_image failed\n"); + goto err; + } } } - else + + if ( dom->container_type == XC_DOM_HVM_CONTAINER ) { config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap; config.arch.emulation_flags = XEN_X86_EMU_LAPIC; @@ -412,8 +424,6 @@ int main(int argc, char** argv) char buf[16], be_path[64], fe_path[64]; int rv, fd; char *maxmem_str = NULL; - xentoollog_level minmsglevel = XTL_PROGRESS; - xentoollog_logger *logger = NULL; while ( (opt = getopt_long(argc, argv, "v", options, NULL)) != -1 ) { @@ -456,9 +466,7 @@ int main(int argc, char** argv) return 2; } - logger = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, - minmsglevel, 0); - + logger = xtl_createlogger_stdiostream(stderr, minmsglevel, 0); xch = xc_interface_open(logger, logger, 0); if ( !xch ) {
When loading a Xenstore stubdom the loader doesn't know whether the lo be loaded kernel is a PVH or a PV one. So it tries to load it as a PVH one first, and if this fails it is loading it as a PV kernel. This results in errors being logged in case the stubdom is a PV kernel. Suppress those errors by setting the minimum logging level to "critical" while trying to load the kernel as PVH. Fixes: f89955449c5a ("tools/init-xenstore-domain: support xenstore pvh stubdom") Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - retry PVH loading with logging if PV fails, too (Jan Beulich) --- tools/helpers/init-xenstore-domain.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)