| Message ID | 20211208084745.31082-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/helpers: PVH xenstore-stubdom console fixes | expand |
On 08/12/2021 08:47, Juergen Gross wrote: > The HVM parameters for pre-allocated event channels should be set in > libxenguest, like it is done for PV guests and for the pre-allocated > ring pages. > > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> I'm not sure that we have the concept of pre-allocated ring pages. For PV, we have: dom->console_pfn = xc_dom_alloc_page(dom, "console"); if ( dom->console_pfn == INVALID_PFN ) return -1; xc_clear_domain_page(dom->xch, dom->guest_domid, xc_dom_p2m(dom, dom->console_pfn)); and for HVM, we have: dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn); With a suitable tweak to the commit message (probably just deleting the final clause), Reivewed-by: Andrew Cooper <andrew.cooper3@citrix.com> That said... > diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c > index fe9f760f71..c9c24666cd 100644 > --- a/tools/libs/light/libxl_dom.c > +++ b/tools/libs/light/libxl_dom.c > @@ -723,9 +723,8 @@ out: > > static int hvm_build_set_params(xc_interface *handle, uint32_t domid, > libxl_domain_build_info *info, > - int store_evtchn, unsigned long *store_mfn, > - int console_evtchn, unsigned long *console_mfn, > - domid_t store_domid, domid_t console_domid) > + unsigned long *store_mfn, > + unsigned long *console_mfn) > { > struct hvm_info_table *va_hvm; > uint8_t *va_map, sum; > @@ -752,8 +751,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid, > > xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn); > xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn); ... these are junk too. I'm dismayed at how much of the toolstack tries passing function parameters via HVM_PARAMS. libxl's HVM path ought to mirror the PV path and, after libxl__build_dom() is called, just read the values back out: state->console_mfn = dom->console_pfn; state->store_mfn = dom->xenstore_pfn; That then leaves hvm_build_set_params() doing nothing but adjusting the HVM info table for real HVM guests. dom->max_vcpus is already present which covers 2 of the 3 fields, leaving only the ACPI boolean left to pass. So by passing the ACPI boolean down, we get rid of hvm_build_set_params() entirely, which seems like a very good move. ~Andrew
On 08.12.21 14:43, Andrew Cooper wrote: > On 08/12/2021 08:47, Juergen Gross wrote: >> The HVM parameters for pre-allocated event channels should be set in >> libxenguest, like it is done for PV guests and for the pre-allocated >> ring pages. >> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > I'm not sure that we have the concept of pre-allocated ring pages. > > For PV, we have: > > dom->console_pfn = xc_dom_alloc_page(dom, "console"); > if ( dom->console_pfn == INVALID_PFN ) > return -1; > xc_clear_domain_page(dom->xch, dom->guest_domid, > xc_dom_p2m(dom, dom->console_pfn)); > > and for HVM, we have: > > dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); > xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn); Isn't that a pre-allocation? The PFNs are fixed at boot time of the guest. > > With a suitable tweak to the commit message (probably just deleting the > final clause), Reivewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > That said... > >> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c >> index fe9f760f71..c9c24666cd 100644 >> --- a/tools/libs/light/libxl_dom.c >> +++ b/tools/libs/light/libxl_dom.c >> @@ -723,9 +723,8 @@ out: >> >> static int hvm_build_set_params(xc_interface *handle, uint32_t domid, >> libxl_domain_build_info *info, >> - int store_evtchn, unsigned long *store_mfn, >> - int console_evtchn, unsigned long *console_mfn, >> - domid_t store_domid, domid_t console_domid) >> + unsigned long *store_mfn, >> + unsigned long *console_mfn) >> { >> struct hvm_info_table *va_hvm; >> uint8_t *va_map, sum; >> @@ -752,8 +751,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid, >> >> xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn); >> xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn); > > ... these are junk too. I'm dismayed at how much of the toolstack tries > passing function parameters via HVM_PARAMS. > > libxl's HVM path ought to mirror the PV path and, after > libxl__build_dom() is called, just read the values back out: > > state->console_mfn = dom->console_pfn; > state->store_mfn = dom->xenstore_pfn; > > > That then leaves hvm_build_set_params() doing nothing but adjusting the > HVM info table for real HVM guests. dom->max_vcpus is already present > which covers 2 of the 3 fields, leaving only the ACPI boolean left to pass. > > So by passing the ACPI boolean down, we get rid of > hvm_build_set_params() entirely, which seems like a very good move. Yes, this should be in another patch, though. Juergen
On 08/12/2021 14:22, Juergen Gross wrote: > On 08.12.21 14:43, Andrew Cooper wrote: >> On 08/12/2021 08:47, Juergen Gross wrote: >>> The HVM parameters for pre-allocated event channels should be set in >>> libxenguest, like it is done for PV guests and for the pre-allocated >>> ring pages. >>> >>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> >> I'm not sure that we have the concept of pre-allocated ring pages. >> >> For PV, we have: >> >> dom->console_pfn = xc_dom_alloc_page(dom, "console"); >> if ( dom->console_pfn == INVALID_PFN ) >> return -1; >> xc_clear_domain_page(dom->xch, dom->guest_domid, >> xc_dom_p2m(dom, dom->console_pfn)); >> >> and for HVM, we have: >> >> dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn); > > Isn't that a pre-allocation? The PFNs are fixed at boot time of the > guest. Yeah, but "allocated in the library call we're making" is not the same as "caller has to allocate and pass details in". I would not class the frames as "pre-allocated" in this context. "allocated" sure, so perhaps "just like it is done for PV guests, and the ring pages that libxenguest allocates" ? > >> >> With a suitable tweak to the commit message (probably just deleting the >> final clause), Reivewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> That said... >> >>> diff --git a/tools/libs/light/libxl_dom.c >>> b/tools/libs/light/libxl_dom.c >>> index fe9f760f71..c9c24666cd 100644 >>> --- a/tools/libs/light/libxl_dom.c >>> +++ b/tools/libs/light/libxl_dom.c >>> @@ -723,9 +723,8 @@ out: >>> static int hvm_build_set_params(xc_interface *handle, uint32_t >>> domid, >>> libxl_domain_build_info *info, >>> - int store_evtchn, unsigned long >>> *store_mfn, >>> - int console_evtchn, unsigned long >>> *console_mfn, >>> - domid_t store_domid, domid_t >>> console_domid) >>> + unsigned long *store_mfn, >>> + unsigned long *console_mfn) >>> { >>> struct hvm_info_table *va_hvm; >>> uint8_t *va_map, sum; >>> @@ -752,8 +751,6 @@ static int hvm_build_set_params(xc_interface >>> *handle, uint32_t domid, >>> xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn); >>> xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, >>> &cons_mfn); >> >> ... these are junk too. I'm dismayed at how much of the toolstack tries >> passing function parameters via HVM_PARAMS. >> >> libxl's HVM path ought to mirror the PV path and, after >> libxl__build_dom() is called, just read the values back out: >> >> state->console_mfn = dom->console_pfn; >> state->store_mfn = dom->xenstore_pfn; >> >> >> That then leaves hvm_build_set_params() doing nothing but adjusting the >> HVM info table for real HVM guests. dom->max_vcpus is already present >> which covers 2 of the 3 fields, leaving only the ACPI boolean left to >> pass. >> >> So by passing the ACPI boolean down, we get rid of >> hvm_build_set_params() entirely, which seems like a very good move. > > Yes, this should be in another patch, though. Absolutely. This wasn't a request to merge more changes into this patch. ~Andrew
On 08.12.21 16:54, Andrew Cooper wrote: > On 08/12/2021 14:22, Juergen Gross wrote: >> On 08.12.21 14:43, Andrew Cooper wrote: >>> On 08/12/2021 08:47, Juergen Gross wrote: >>>> The HVM parameters for pre-allocated event channels should be set in >>>> libxenguest, like it is done for PV guests and for the pre-allocated >>>> ring pages. >>>> >>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> I'm not sure that we have the concept of pre-allocated ring pages. >>> >>> For PV, we have: >>> >>> dom->console_pfn = xc_dom_alloc_page(dom, "console"); >>> if ( dom->console_pfn == INVALID_PFN ) >>> return -1; >>> xc_clear_domain_page(dom->xch, dom->guest_domid, >>> xc_dom_p2m(dom, dom->console_pfn)); >>> >>> and for HVM, we have: >>> >>> dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); >>> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn); >> >> Isn't that a pre-allocation? The PFNs are fixed at boot time of the >> guest. > > Yeah, but "allocated in the library call we're making" is not the same > as "caller has to allocate and pass details in". > > I would not class the frames as "pre-allocated" in this context. > "allocated" sure, so perhaps "just like it is done for PV guests, and > the ring pages that libxenguest allocates" ? Fine with me. Should I send another round, or can this be changed when committing? Juergen
On 08/12/2021 15:57, Juergen Gross wrote: > On 08.12.21 16:54, Andrew Cooper wrote: >> On 08/12/2021 14:22, Juergen Gross wrote: >>> On 08.12.21 14:43, Andrew Cooper wrote: >>>> On 08/12/2021 08:47, Juergen Gross wrote: >>>>> The HVM parameters for pre-allocated event channels should be set in >>>>> libxenguest, like it is done for PV guests and for the pre-allocated >>>>> ring pages. >>>>> >>>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> >>>> I'm not sure that we have the concept of pre-allocated ring pages. >>>> >>>> For PV, we have: >>>> >>>> dom->console_pfn = xc_dom_alloc_page(dom, "console"); >>>> if ( dom->console_pfn == INVALID_PFN ) >>>> return -1; >>>> xc_clear_domain_page(dom->xch, dom->guest_domid, >>>> xc_dom_p2m(dom, dom->console_pfn)); >>>> >>>> and for HVM, we have: >>>> >>>> dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); >>>> xc_clear_domain_page(dom->xch, dom->guest_domid, >>>> dom->console_pfn); >>> >>> Isn't that a pre-allocation? The PFNs are fixed at boot time of the >>> guest. >> >> Yeah, but "allocated in the library call we're making" is not the same >> as "caller has to allocate and pass details in". >> >> I would not class the frames as "pre-allocated" in this context. >> "allocated" sure, so perhaps "just like it is done for PV guests, and >> the ring pages that libxenguest allocates" ? > > Fine with me. > > Should I send another round, or can this be changed when committing? Fixed on commit. No need to resend just for this. Question is whether Anthony has any view, or whether my R-by is good enough? ~Andrew
On Wed, Dec 08, 2021 at 04:02:17PM +0000, Andrew Cooper wrote: > On 08/12/2021 15:57, Juergen Gross wrote: > > On 08.12.21 16:54, Andrew Cooper wrote: > >> On 08/12/2021 14:22, Juergen Gross wrote: > >>> On 08.12.21 14:43, Andrew Cooper wrote: > >>>> On 08/12/2021 08:47, Juergen Gross wrote: > >>>>> The HVM parameters for pre-allocated event channels should be set in > >>>>> libxenguest, like it is done for PV guests and for the pre-allocated > >>>>> ring pages. > >>>>> > >>>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> > >>>> > >>>> I'm not sure that we have the concept of pre-allocated ring pages. > >>>> > >>>> For PV, we have: > >>>> > >>>> dom->console_pfn = xc_dom_alloc_page(dom, "console"); > >>>> if ( dom->console_pfn == INVALID_PFN ) > >>>> return -1; > >>>> xc_clear_domain_page(dom->xch, dom->guest_domid, > >>>> xc_dom_p2m(dom, dom->console_pfn)); > >>>> > >>>> and for HVM, we have: > >>>> > >>>> dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE); > >>>> xc_clear_domain_page(dom->xch, dom->guest_domid, > >>>> dom->console_pfn); > >>> > >>> Isn't that a pre-allocation? The PFNs are fixed at boot time of the > >>> guest. > >> > >> Yeah, but "allocated in the library call we're making" is not the same > >> as "caller has to allocate and pass details in". > >> > >> I would not class the frames as "pre-allocated" in this context. > >> "allocated" sure, so perhaps "just like it is done for PV guests, and > >> the ring pages that libxenguest allocates" ? > > > > Fine with me. > > > > Should I send another round, or can this be changed when committing? > > Fixed on commit. No need to resend just for this. > > Question is whether Anthony has any view, or whether my R-by is good enough? Patch looks good, so: Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c index b6e75afba2..9328fbf804 100644 --- a/tools/libs/guest/xg_dom_x86.c +++ b/tools/libs/guest/xg_dom_x86.c @@ -1866,6 +1866,12 @@ static int bootlate_hvm(struct xc_dom_image *dom) munmap(hvm_info_page, PAGE_SIZE); } + if ( xc_hvm_param_set(xch, domid, HVM_PARAM_CONSOLE_EVTCHN, + dom->console_evtchn) || + xc_hvm_param_set(xch, domid, HVM_PARAM_STORE_EVTCHN, + dom->xenstore_evtchn) ) + return -1; + return 0; } diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c index fe9f760f71..c9c24666cd 100644 --- a/tools/libs/light/libxl_dom.c +++ b/tools/libs/light/libxl_dom.c @@ -723,9 +723,8 @@ out: static int hvm_build_set_params(xc_interface *handle, uint32_t domid, libxl_domain_build_info *info, - int store_evtchn, unsigned long *store_mfn, - int console_evtchn, unsigned long *console_mfn, - domid_t store_domid, domid_t console_domid) + unsigned long *store_mfn, + unsigned long *console_mfn) { struct hvm_info_table *va_hvm; uint8_t *va_map, sum; @@ -752,8 +751,6 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid, xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn); xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn); - xc_hvm_param_set(handle, domid, HVM_PARAM_STORE_EVTCHN, store_evtchn); - xc_hvm_param_set(handle, domid, HVM_PARAM_CONSOLE_EVTCHN, console_evtchn); *store_mfn = str_mfn; *console_mfn = cons_mfn; @@ -1123,7 +1120,9 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid, dom->vga_hole_size = device_model ? LIBXL_VGA_HOLE_SIZE : 0; dom->device_model = device_model; dom->max_vcpus = info->max_vcpus; + dom->console_evtchn = state->console_port; dom->console_domid = state->console_domid; + dom->xenstore_evtchn = state->store_port; dom->xenstore_domid = state->store_domid; rc = libxl__domain_device_construct_rdm(gc, d_config, @@ -1169,10 +1168,8 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid, if (rc != 0) goto out; - rc = hvm_build_set_params(ctx->xch, domid, info, state->store_port, - &state->store_mfn, state->console_port, - &state->console_mfn, state->store_domid, - state->console_domid); + rc = hvm_build_set_params(ctx->xch, domid, info, &state->store_mfn, + &state->console_mfn); if (rc != 0) { LOG(ERROR, "hvm build set params failed"); goto out;
The HVM parameters for pre-allocated event channels should be set in libxenguest, like it is done for PV guests and for the pre-allocated ring pages. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V3: - replacement for former patch 2 (Andrew Cooper) --- tools/libs/guest/xg_dom_x86.c | 6 ++++++ tools/libs/light/libxl_dom.c | 15 ++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-)