Re: [Qemu-devel] [for-2.9 2/5] pseries: Stubs for HPT resizing

[Thomas Huth]

On 09.12.2016 10:09, David Gibson wrote:
> On Fri, Dec 09, 2016 at 09:18:51AM +0100, Thomas Huth wrote:
>> On 09.12.2016 03:23, David Gibson wrote:
>>> This introduces stub implementations of the H_RESIZE_HPT_PREPARE and
>>> H_RESIZE_HPT_COMMIT hypercalls which we hope to add in a PAPR
>>> extension to allow run time resizing of a guest's hash page table.  It
>>> also adds a new machine property for controlling whether this new
>>> facility is available, and logic to check that against availability
>>> with KVM (only supported with KVM PR for now).
>>>
>>> Finally, it adds a new string to the hypertas property in the device
>>> tree, advertising to the guest the availability of the HPT resizing
>>> hypercalls.  This is a tentative suggested value, and would need to be
>>> standardized by PAPR before being merged.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>>  hw/ppc/spapr.c         | 66 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/ppc/spapr_hcall.c   | 36 +++++++++++++++++++++++++++
>>>  hw/ppc/trace-events    |  2 ++
>>>  include/hw/ppc/spapr.h | 11 +++++++++
>>>  target-ppc/kvm.c       | 26 ++++++++++++++++++++
>>>  target-ppc/kvm_ppc.h   |  5 ++++
>>>  6 files changed, 146 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 0f25e83..ecb0822 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -760,6 +760,11 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, 
>>> void *fdt)
>>>      if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
>>>          add_str(hypertas, "hcall-multi-tce");
>>>      }
>>> +
>>> +    if (spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) {
>>> +        add_str(hypertas, "hcall-hpt-resize");
>>> +    }
>>> +
>>>      _FDT(fdt_setprop(fdt, rtas, "ibm,hypertas-functions",
>>>                       hypertas->str, hypertas->len));
>>>      g_string_free(hypertas, TRUE);
>>> @@ -1839,11 +1844,31 @@ static void ppc_spapr_init(MachineState *machine)
>>>      long load_limit, fw_size;
>>>      char *filename;
>>>      int smt = kvmppc_smt_threads();
>>> +    Error *resize_hpt_err = NULL;
>>>  
>>>      msi_nonbroken = true;
>>>  
>>>      QLIST_INIT(&spapr->phbs);
>>>  
>>> +    /* Check HPT resizing availability */
>>> +    kvmppc_check_papr_resize_hpt(&resize_hpt_err);
>>> +    if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
>>> +        if (resize_hpt_err) {
>>> +            spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
>>> +            error_free(resize_hpt_err);
>>> +            resize_hpt_err = NULL;
>>> +        } else {
>>> +            spapr->resize_hpt = smc->resize_hpt_default;
>>> +        }
>>> +    }
>>> +
>>> +    assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
>>> +
>>> +    if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && 
>>> resize_hpt_err) {
>>> +        error_report_err(resize_hpt_err);
>>> +        exit(1);
>>> +    }
>>
>> The logic here is IMHO a little bit hard to understand. Why do you need
>> the SPAPR_RESIZE_HPT_DEFAULT state here at all? Wouldn't it be
>> sufficient to start with SPAPR_RESIZE_HPT_DISABLED by default and to
>> only enable it when the extension is available?
> 
> Because if the user explicitly sets something, we should never
> override itin the logic here.  In order to determine whether the user
> has set something explicitly, we need another value that means "not
> set explicitly".

Hmm, ok, I think I slowly get it. Another question: Does this also take
care of the handling if the user migrates from a machine that supports
HPT resizing to a machine that does not support this feature?

 Thomas

signature.asc
Description: OpenPGP digital signature