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 12:29:14PM +0200, Greg Kurz wrote:
> On Mon, 18 Jun 2018 16:36:03 +1000
> David Gibson <address@hidden> wrote:
[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]);
> 
> s/SPAPR_CAP_HPT_MPS/SPAPR_CAP_HPT_MAXPAGESIZE

Fixed.

> > +
> > +    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);
> 
> Even if in this precise case QEMU will always exit gracefully since
> errp == &error_fatal, passing errp several times is a fragile pattern.
> It may cause a crash if *errp was already allocated.
> 
> Maybe use a local_err variable and error_propagate() or at least return
> in the (val < 12) block above.

Actually, just a return; after the first error should be sufficient.
I've put that in.

> 
> Rest looks good. With the two issues addressed:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> >  }
> >  
> >  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