Re: [Qemu-devel] [PATCH 6/9] spapr: Use maximum page size capability to simplify memory backend checking

[David Gibson]

On Thu, Jun 21, 2018 at 08:29:36AM +0200, Cédric Le Goater wrote:
> On 06/18/2018 08:36 AM, David Gibson wrote:
> > The way we used to handle KVM allowable guest pagesizes for PAPR guests
> > required some convoluted checking of memory attached to the guest.
> > 
> > The allowable pagesizes advertised to the guest cpus depended on the memory
> > which was attached at boot, but then we needed to ensure that any memory
> > later hotplugged didn't change which pagesizes were allowed.
> > 
> > Now that we have an explicit machine option to control the allowable
> > maximum pagesize we can simplify this.  We just check all memory backends
> > against that declared pagesize.  We check base and cold-plugged memory at
> > reset time, and hotplugged memory at pre_plug() time.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> 
> One minor question below.
> 
> Nevertheless,
> 
> Reviewed-by: Cédric Le Goater <address@hidden>

[snip]
> > @@ -304,6 +305,23 @@ static void 
> > cap_safe_indirect_branch_apply(sPAPRMachineState *spapr,
> >  
> >  #define VALUE_DESC_TRISTATE     " (broken, workaround, fixed)"
> >  
> > +void spapr_check_pagesize(sPAPRMachineState *spapr, hwaddr pagesize,
> > +                          Error **errp)
> > +{
> > +    hwaddr maxpagesize = (1ULL << spapr->eff.caps[SPAPR_CAP_HPT_MPS]);
> 
> I suppose this is SPAPR_CAP_HPT_MAXPAGESIZE now ? 

Oops.  I thought I'd compile tested all the intermediate patches,
which would have caught this, but apparently not.

Fixed now.

> > +
> > +    if (!kvmppc_hpt_needs_host_contiguous_pages()) {
> > +        return;
> > +    }
> > +
> > +    if (maxpagesize > pagesize) {
> > +        error_setg(errp,
> > +                   "Can't support %"HWADDR_PRIu" kiB guest pages with %"
> > +                   HWADDR_PRIu" kiB host pages with this KVM 
> > implementation",
> > +                   maxpagesize >> 10, pagesize >> 10);
> > +    }
> > +}
> > +
> >  static void cap_hpt_maxpagesize_apply(sPAPRMachineState *spapr,
> >                                        uint8_t val, Error **errp)
> >  {
> > @@ -312,6 +330,8 @@ static void cap_hpt_maxpagesize_apply(sPAPRMachineState 
> > *spapr,
> >      } else if (val < 16) {
> >          warn_report("Many guests require at least 64kiB 
> > hpt-max-page-size");
> >      }
> > +
> > +    spapr_check_pagesize(spapr, qemu_getrampagesize(), errp);
> >  }
> >  
> >  sPAPRCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c97593d032..75e2cf2687 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -806,4 +806,7 @@ void spapr_caps_cpu_apply(sPAPRMachineState *spapr, 
> > PowerPCCPU *cpu);
> >  void spapr_caps_add_properties(sPAPRMachineClass *smc, Error **errp);
> >  int spapr_caps_post_migration(sPAPRMachineState *spapr);
> >  
> > +void spapr_check_pagesize(sPAPRMachineState *spapr, hwaddr pagesize,
> > +                          Error **errp);
> > +
> >  #endif /* HW_SPAPR_H */
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 50b5d01432..9cfbd388ad 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -500,26 +500,12 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >          cpu->hash64_opts->flags &= ~PPC_HASH64_1TSEG;
> >      }
> >  }
> > -
> > -bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
> > -{
> > -    Object *mem_obj = object_resolve_path(obj_path, NULL);
> > -    long pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(mem_obj));
> > -
> > -    return pagesize >= max_cpu_page_size;
> > -}
> > -
> >  #else /* defined (TARGET_PPC64) */
> >  
> >  static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >  {
> >  }
> >  
> > -bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
> > -{
> > -    return true;
> > -}
> > -
> >  #endif /* !defined (TARGET_PPC64) */
> >  
> >  unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index a7ddb8a5d6..443fca0a4e 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -71,7 +71,6 @@ int kvmppc_resize_hpt_commit(PowerPCCPU *cpu, 
> > target_ulong flags, int shift);
> >  bool kvmppc_pvr_workaround_required(PowerPCCPU *cpu);
> >  
> >  bool kvmppc_hpt_needs_host_contiguous_pages(void);
> > -bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path);
> >  
> >  #else
> >  
> > @@ -228,11 +227,6 @@ static inline bool 
> > kvmppc_hpt_needs_host_contiguous_pages(void)
> >      return false;
> >  }
> >  
> > -static inline bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
> > -{
> > -    return true;
> > -}
> > -
> >  static inline bool kvmppc_has_cap_spapr_vfio(void)
> >  {
> >      return false;
> > 
> 

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

signature.asc
Description: PGP signature