Re: [Qemu-devel] [PATCH 8/9] spapr: Limit available pagesizes to provide a consistent guest environment

[Cédric Le Goater]

On 06/21/2018 01:52 PM, David Gibson wrote:
> On Thu, Jun 21, 2018 at 09:01:27AM +0200, Cédric Le Goater wrote:
>> On 06/18/2018 08:36 AM, David Gibson wrote:
>>> KVM HV has some limitations (deriving from the hardware) that mean not all
>>> host-cpu supported pagesizes may be usable in the guest.  At present this
>>> means that KVM guests and TCG guests may see different available page sizes
>>> even if they notionally have the same vcpu model.  This is confusing and
>>> also prevents migration between TCG and KVM.
>>>
>>> This patch makes the environment consistent by always allowing the same set
>>> of pagesizes.  Since we can't remove the KVM limitations, we do this by
>>> always applying the same limitations it has, even to TCG guests.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>>
>>> ---
>>>  hw/ppc/spapr_caps.c | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index 9fc739b3f5..0584c7c6ab 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -334,6 +334,38 @@ static void 
>>> cap_hpt_maxpagesize_apply(sPAPRMachineState *spapr,
>>>      spapr_check_pagesize(spapr, qemu_getrampagesize(), errp);
>>>  }
>>>  
>>> +static bool spapr_pagesize_cb(void *opaque, uint32_t seg_pshift, uint32_t 
>>> pshift)
>>> +{
>>> +    unsigned maxshift = *((unsigned *)opaque);
>>> +
>>> +    assert(pshift >= seg_pshift);
>>
>> you could check that elsewhere.
> 
> Um.. I'm not sure what you're getting at.

you could put the assert in ppc_hash64_filter_pagesizes(), that is where
the parameters are coming from.

>>> +    /* Don't allow the guest to use pages bigger than the configured
>>> +     * maximum size */
>>> +    if (pshift > maxshift) {
>>> +        return false;
>>> +    }
>>> +
>>> +    /* For whatever reason, KVM doesn't allow multiple pagesizes
>>> +     * within a segment, *except* for the case of 16M pages in a 4k or
>>> +     * 64k segment.  Always exclude other cases, so that TCG and KVM
>>> +     * guests see a consistent environment */
>>> +    if ((pshift != seg_pshift) && (pshift != 24)) {
>>> +        return false;
>>> +    }
> 
> Note the stanza above, I'll refer to it below.

ok.

> 
>>> +
>>> +    return true;
>>> +}
>>
>> So, do we really need ppc_hash64_filter_pagesizes() to have a callback ? 
> 
> I agree that it seems overly involved, but it was the best way I could
> see to logically separate the TCG / softmmu specific logic from the
> spapr specific logic.

ok. I agree then.

Reviewed-by: Cédric Le Goater <address@hidden>

Thanks,

C.

>> It seems that we only use the routine once in the patchset and that the
>> only thing we need to check is 'maxshift'.
> 
> Not quite.  An earlier draft had this routine just take a max page
> size and clamp accordingly.  But that failed when I wrote the code to
> check against the KVM capabilities, because KVM also excludes some
> other pagesize combinations.  That's what the stanza I point out above
> is about
> 
>> Do you envision other usage of the routine ?
> 
> Not really, no.
> 
>>
>> Thanks,
>>
>> C.
>>
>>> +static void cap_hpt_maxpagesize_cpu_apply(sPAPRMachineState *spapr,
>>> +                                          PowerPCCPU *cpu,
>>> +                                          uint8_t val, Error **errp)
>>> +{
>>> +    unsigned maxshift = val;
>>> +
>>> +    ppc_hash64_filter_pagesizes(cpu, spapr_pagesize_cb, &maxshift);
>>> +}
>>> +
>>>  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>      [SPAPR_CAP_HTM] = {
>>>          .name = "htm",
>>> @@ -401,6 +433,7 @@ sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>>>          .set = spapr_cap_set_pagesize,
>>>          .type = "int",
>>>          .apply = cap_hpt_maxpagesize_apply,
>>> +        .cpu_apply = cap_hpt_maxpagesize_cpu_apply,
>>>      },
>>>  };
>>>  
>>>
>>
>