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 <da...@gibson.dropbear.id.au>
>>>
>>> ---
>>> 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 <c...@kaod.org>
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,
>>> },
>>> };
>>>
>>>
>>
>
