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

[PATCH v3 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: set event channel HVM parameters in libxenguest

 tools/helpers/init-xenstore-domain.c |  8 +++++---
 tools/libs/guest/xg_dom_x86.c        |  6 ++++++
 tools/libs/light/libxl_dom.c         | 15 ++++++---------
 3 files changed, 17 insertions(+), 12 deletions(-)

-- 
2.26.2

[PATCH v3 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 v3 2/2] tools: set event channel HVM parameters in libxenguest

[Juergen Gross]

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

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Andrew Cooper]

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

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3044 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Andrew Cooper]

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

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1587 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Andrew Cooper]

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

Re: [PATCH v3 2/2] tools: set event channel HVM parameters in libxenguest

[Anthony PERARD]

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,

-- 
Anthony PERARD