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

[Andrew Cooper <amc96@xxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

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@xxxxxxxxxx>

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