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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4614 bytes --]

On Thu, Jun 21, 2018 at 02:50:32PM +0200, Cédric Le Goater wrote:
> 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 <david@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.

Yes.. but it's here that we're relying on that fact.  That's kind of
the point with assert()s.

> 
> >>> +    /* 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 <clg@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,
> >>>      },
> >>>  };
> >>>  
> >>>
> >>
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]