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

[Andrew Cooper <amc96@xxxxxxxx>]

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

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

Absolutely.  This wasn't a request to merge more changes into this patch.

~Andrew