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

[Juergen Gross <jgross@xxxxxxxx>]

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@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);
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@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.
Yes, this should be in another patch, though.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature