Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Check for valid page size when hot plugging memory

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

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

On Wed, Feb 15, 2017 at 10:21:44AM +0100, Thomas Huth wrote:
> On POWER, the valid page sizes that the guest can use are bound
> to the CPU and not to the memory region. QEMU already has some
> fancy logic to find out the right maximum memory size to tell
> it to the guest during boot (see getrampagesize() in the file
> target/ppc/kvm.c for more information).
> However, once we're booted and the guest is using huge pages
> already, it is currently still possible to hot-plug memory regions
> that does not support huge pages - which of course does not work
> on POWER, since the guest thinks that it is possible to use huge
> pages everywhere. The KVM_RUN ioctl will then abort with -EFAULT,
> QEMU spills out a not very helpful error message together with
> a register dump and the user is annoyed that the VM unexpectedly
> died.
> To avoid this situation, we should check the page size of hot-plugged
> DIMMs to see whether it is possible to use it in the current VM.
> If it does not fit, we can print out a better error message and
> refuse to add it, so that the VM does not die unexpectely and the
> user has a second chance to plug a DIMM with a matching memory
> backend instead.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1419466
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Using the global is a bit yucky, but I can't see an easy way to remove
it, and it's not like there aren't already some ugly globals in the
KVM code.  In the meantime this fixes a real bug, so I've merged this
to ppc-for-2.9.

Thanks.

> ---
>  hw/ppc/spapr.c       |  8 ++++++++
>  target/ppc/kvm.c     | 32 ++++++++++++++++++++++++++++----
>  target/ppc/kvm_ppc.h |  7 +++++++
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e465d7a..1a90aae 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2357,6 +2357,7 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      uint64_t align = memory_region_get_alignment(mr);
>      uint64_t size = memory_region_size(mr);
>      uint64_t addr;
> +    char *mem_dev;
>  
>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>          error_setg(&local_err, "Hotplugged memory size must be a multiple of "
> @@ -2364,6 +2365,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> +    mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
> +    if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
> +        error_setg(&local_err, "Memory backend has bad page size. "
> +                   "Use 'memory-backend-file' with correct mem-path.");
> +        goto out;
> +    }
> +
>      pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
>      if (local_err) {
>          goto out;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 663d2e7..584546b 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -438,12 +438,13 @@ static bool kvm_valid_page_size(uint32_t flags, long rampgsize, uint32_t shift)
>      return (1ul << shift) <= rampgsize;
>  }
>  
> +static long max_cpu_page_size;
> +
>  static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  {
>      static struct kvm_ppc_smmu_info smmu_info;
>      static bool has_smmu_info;
>      CPUPPCState *env = &cpu->env;
> -    long rampagesize;
>      int iq, ik, jq, jk;
>      bool has_64k_pages = false;
>  
> @@ -458,7 +459,9 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>          has_smmu_info = true;
>      }
>  
> -    rampagesize = getrampagesize();
> +    if (!max_cpu_page_size) {
> +        max_cpu_page_size = getrampagesize();
> +    }
>  
>      /* Convert to QEMU form */
>      memset(&env->sps, 0, sizeof(env->sps));
> @@ -478,14 +481,14 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>          struct ppc_one_seg_page_size *qsps = &env->sps.sps[iq];
>          struct kvm_ppc_one_seg_page_size *ksps = &smmu_info.sps[ik];
>  
> -        if (!kvm_valid_page_size(smmu_info.flags, rampagesize,
> +        if (!kvm_valid_page_size(smmu_info.flags, max_cpu_page_size,
>                                   ksps->page_shift)) {
>              continue;
>          }
>          qsps->page_shift = ksps->page_shift;
>          qsps->slb_enc = ksps->slb_enc;
>          for (jk = jq = 0; jk < KVM_PPC_PAGE_SIZES_MAX_SZ; jk++) {
> -            if (!kvm_valid_page_size(smmu_info.flags, rampagesize,
> +            if (!kvm_valid_page_size(smmu_info.flags, max_cpu_page_size,
>                                       ksps->enc[jk].page_shift)) {
>                  continue;
>              }
> @@ -510,12 +513,33 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>          env->mmu_model &= ~POWERPC_MMU_64K;
>      }
>  }
> +
> +bool kvmppc_is_mem_backend_page_size_ok(char *obj_path)
> +{
> +    Object *mem_obj = object_resolve_path(obj_path, NULL);
> +    char *mempath = object_property_get_str(mem_obj, "mem-path", NULL);
> +    long pagesize;
> +
> +    if (mempath) {
> +        pagesize = gethugepagesize(mempath);
> +    } else {
> +        pagesize = getpagesize();
> +    }
> +
> +    return pagesize >= max_cpu_page_size;
> +}
> +
>  #else /* defined (TARGET_PPC64) */
>  
>  static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  {
>  }
>  
> +int kvmppc_is_mem_backend_page_size_ok(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 151c00b..8da2ee4 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -60,6 +60,8 @@ int kvmppc_enable_hwrng(void);
>  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
>  
> +bool kvmppc_is_mem_backend_page_size_ok(char *obj_path);
> +
>  #else
>  
>  static inline uint32_t kvmppc_get_tbfreq(void)
> @@ -192,6 +194,11 @@ static inline uint64_t kvmppc_rma_size(uint64_t current_size,
>      return ram_size;
>  }
>  
> +static inline bool kvmppc_is_mem_backend_page_size_ok(char *obj_path)
> +{
> +    return true;
> +}
> +
>  #endif /* !CONFIG_USER_ONLY */
>  
>  static inline bool kvmppc_has_cap_epr(void)

-- 
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: 819 bytes --]