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(-)
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;
--
2.26.2
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
© 2016 - 2026 Red Hat, Inc.