[Qemu-devel] [PATCH 0/4] ppc/kvm: htab fd code improvements

[Qemu-devel] [PATCH 0/4] ppc/kvm: htab fd code improvements

[Greg Kurz]

This series drops unused code and consolidates some duplicated code that
deals with KVM's htab fd feature. It also fixes some potential bugs.

--
Greg

---

Greg Kurz (4):
      ppc/kvm: drop kvmppc_has_cap_htab_fd()
      spapr: introduce helpers to migrate HPT chunks and the end marker
      ppc/kvm: change kvmppc_get_htab_fd() to return -errno on error
      ppc/kvm: generalize the use of kvmppc_get_htab_fd()


 hw/ppc/spapr.c       |   54 +++++++++++++++++++++++++++-----------------------
 target/ppc/kvm.c     |   42 ++++++++++++++++-----------------------
 target/ppc/kvm_ppc.h |   10 ++-------
 3 files changed, 48 insertions(+), 58 deletions(-)

[Qemu-devel] [PATCH 1/4] ppc/kvm: drop kvmppc_has_cap_htab_fd()

[Greg Kurz]

It never got used since its introduction (commit 7c43bca004af).

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/kvm.c     |    5 -----
 target/ppc/kvm_ppc.h |    6 ------
 2 files changed, 11 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 1deaf106d2b9..00d7029b8d7a 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2448,11 +2448,6 @@ bool kvmppc_has_cap_epr(void)
     return cap_epr;
 }
 
-bool kvmppc_has_cap_htab_fd(void)
-{
-    return cap_htab_fd;
-}
-
 bool kvmppc_has_cap_fixup_hcalls(void)
 {
     return cap_fixup_hcalls;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index f780e6ec7b72..08aab46c5a56 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -51,7 +51,6 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
 #endif /* !CONFIG_USER_ONLY */
 bool kvmppc_has_cap_epr(void);
 int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
-bool kvmppc_has_cap_htab_fd(void);
 int kvmppc_get_htab_fd(bool write);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
@@ -246,11 +245,6 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
     return -1;
 }
 
-static inline bool kvmppc_has_cap_htab_fd(void)
-{
-    return false;
-}
-
 static inline int kvmppc_get_htab_fd(bool write)
 {
     return -1;

[Qemu-devel] [PATCH 2/4] spapr: introduce helpers to migrate HPT chunks and the end marker

[Greg Kurz]

This consolidates some duplicated code in a single helper.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f680f28a15ea..841117f6d185 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1708,6 +1708,23 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
     return 0;
 }
 
+static void htab_save_chunk(QEMUFile *f, sPAPRMachineState *spapr,
+                            int chunkstart, int n_valid, int n_invalid)
+{
+    qemu_put_be32(f, chunkstart);
+    qemu_put_be16(f, n_valid);
+    qemu_put_be16(f, n_invalid);
+    if (spapr) {
+        qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
+                        HASH_PTE_SIZE_64 * n_valid);
+    }
+}
+
+static void htab_save_end_marker(QEMUFile *f)
+{
+    htab_save_chunk(f, NULL, 0, 0, 0);
+}
+
 static void htab_save_first_pass(QEMUFile *f, sPAPRMachineState *spapr,
                                  int64_t max_ns)
 {
@@ -1739,11 +1756,7 @@ static void htab_save_first_pass(QEMUFile *f, sPAPRMachineState *spapr,
         if (index > chunkstart) {
             int n_valid = index - chunkstart;
 
-            qemu_put_be32(f, chunkstart);
-            qemu_put_be16(f, n_valid);
-            qemu_put_be16(f, 0);
-            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
-                            HASH_PTE_SIZE_64 * n_valid);
+            htab_save_chunk(f, spapr, chunkstart, n_valid, 0);
 
             if (has_timeout &&
                 (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns) {
@@ -1805,11 +1818,7 @@ static int htab_save_later_pass(QEMUFile *f, sPAPRMachineState *spapr,
             int n_valid = invalidstart - chunkstart;
             int n_invalid = index - invalidstart;
 
-            qemu_put_be32(f, chunkstart);
-            qemu_put_be16(f, n_valid);
-            qemu_put_be16(f, n_invalid);
-            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
-                            HASH_PTE_SIZE_64 * n_valid);
+            htab_save_chunk(f, spapr, chunkstart, n_valid, n_invalid);
             sent += index - chunkstart;
 
             if (!final && (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns) {
@@ -1872,10 +1881,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
         rc = htab_save_later_pass(f, spapr, MAX_ITERATION_NS);
     }
 
-    /* End marker */
-    qemu_put_be32(f, 0);
-    qemu_put_be16(f, 0);
-    qemu_put_be16(f, 0);
+    htab_save_end_marker(f);
 
     return rc;
 }
@@ -1915,9 +1921,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
     }
 
     /* End marker */
-    qemu_put_be32(f, 0);
-    qemu_put_be16(f, 0);
-    qemu_put_be16(f, 0);
+    htab_save_end_marker(f);
 
     return 0;
 }

[Qemu-devel] [PATCH 3/4] ppc/kvm: change kvmppc_get_htab_fd() to return -errno on error

[Greg Kurz]

When kvmppc_get_htab_fd() fails, its return value is propagated up to
qemu_savevm_state_iterate() or to qemu_savevm_state_complete_precopy().
All savevm handlers expect to receive a negative errno on error.

Let's patch kvmppc_get_htab_fd() accordingly.

While here, let's change htab_load() in the spapr code to also
propagate the error, since it doesn't make sense to abort() if
we couldn't get the htab fd from KVM.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c   |    5 +++--
 target/ppc/kvm.c |   10 ++++++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 841117f6d185..1ae79326d1ac 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1218,7 +1218,7 @@ static int get_htab_fd(sPAPRMachineState *spapr)
     spapr->htab_fd = kvmppc_get_htab_fd(false);
     if (spapr->htab_fd < 0) {
         error_report("Unable to open fd for reading hash table from KVM: %s",
-                     strerror(errno));
+                     strerror(spapr->htab_fd));
     }
 
     return spapr->htab_fd;
@@ -1962,7 +1962,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
         fd = kvmppc_get_htab_fd(true);
         if (fd < 0) {
             error_report("Unable to open fd to restore KVM hash table: %s",
-                         strerror(errno));
+                         strerror(fd));
+            return fd;
         }
     }
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 00d7029b8d7a..09d7dea79e2d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2556,13 +2556,19 @@ int kvmppc_get_htab_fd(bool write)
         .flags = write ? KVM_GET_HTAB_WRITE : 0,
         .start_index = 0,
     };
+    int ret;
 
     if (!cap_htab_fd) {
         fprintf(stderr, "KVM version doesn't support saving the hash table\n");
-        return -1;
+        return -ENOTSUP;
+    }
+
+    ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
+    if (ret < 0) {
+        return -errno;
     }
 
-    return kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
+    return ret;
 }
 
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)

[Qemu-devel] [PATCH 4/4] ppc/kvm: generalize the use of kvmppc_get_htab_fd()

[Greg Kurz]

The use of KVM_PPC_GET_HTAB_FD is open-coded in kvmppc_read_hptes()
and kvmppc_write_hpte().

This patch modifies kvmppc_get_htab_fd() so that it can be used
everywhere we need to access the in-kernel htab:
- add an index argument
  => only kvmppc_read_hptes() passes an actual index, all other users
     pass 0
- add an errp argument to propagate error messages to the caller.
  => spapr migration code prints the error
  => hpte helpers pass &error_abort to keep the current behavior
     of hw_error()

While here, this also fixes a bug in kvmppc_write_hpte() so that it
opens the htab fd for writing instead of reading as it currently does.
This never broke anything because we currently never call this code,
as explained in the changelog of commit c1385933804bb:

"This support updating htab managed by the hypervisor. Currently
 we don't have any user for this feature. This actually bring the
 store_hpte interface in-line with the load_hpte one. We may want
 to use this when we want to emulate henter hcall in qemu for HV
 kvm."

The above is still true today.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c       |   15 +++++++--------
 target/ppc/kvm.c     |   27 +++++++++------------------
 target/ppc/kvm_ppc.h |    4 ++--
 3 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1ae79326d1ac..eeef549fbc15 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1211,14 +1211,15 @@ static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp)
  */
 static int get_htab_fd(sPAPRMachineState *spapr)
 {
+    Error *local_err = NULL;
+
     if (spapr->htab_fd >= 0) {
         return spapr->htab_fd;
     }
 
-    spapr->htab_fd = kvmppc_get_htab_fd(false);
+    spapr->htab_fd = kvmppc_get_htab_fd(false, 0, &local_err);
     if (spapr->htab_fd < 0) {
-        error_report("Unable to open fd for reading hash table from KVM: %s",
-                     strerror(spapr->htab_fd));
+        error_report_err(local_err);
     }
 
     return spapr->htab_fd;
@@ -1931,6 +1932,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     sPAPRMachineState *spapr = opaque;
     uint32_t section_hdr;
     int fd = -1;
+    Error *local_err = NULL;
 
     if (version_id < 1 || version_id > 1) {
         error_report("htab_load() bad version");
@@ -1945,8 +1947,6 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     if (section_hdr) {
-        Error *local_err = NULL;
-
         /* First section gives the htab size */
         spapr_reallocate_hpt(spapr, section_hdr, &local_err);
         if (local_err) {
@@ -1959,10 +1959,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     if (!spapr->htab) {
         assert(kvm_enabled());
 
-        fd = kvmppc_get_htab_fd(true);
+        fd = kvmppc_get_htab_fd(true, 0, &local_err);
         if (fd < 0) {
-            error_report("Unable to open fd to restore KVM hash table: %s",
-                         strerror(fd));
+            error_report_err(local_err);
             return fd;
         }
     }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 09d7dea79e2d..c37d74941085 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2550,21 +2550,25 @@ int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function)
     return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN, &args);
 }
 
-int kvmppc_get_htab_fd(bool write)
+int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
 {
     struct kvm_get_htab_fd s = {
         .flags = write ? KVM_GET_HTAB_WRITE : 0,
-        .start_index = 0,
+        .start_index = index,
     };
     int ret;
 
     if (!cap_htab_fd) {
-        fprintf(stderr, "KVM version doesn't support saving the hash table\n");
+        error_setg(errp, "KVM version doesn't support %s the HPT",
+                   write ? "writing" : "reading");
         return -ENOTSUP;
     }
 
     ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
     if (ret < 0) {
+        error_setg(errp, "Unable to open fd for %s HPT %s KVM: %s",
+                   write ? "writing" : "reading", write ? "to" : "from",
+                   strerror(errno));
         return -errno;
     }
 
@@ -2648,17 +2652,10 @@ void kvm_arch_init_irq_routing(KVMState *s)
 
 void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
 {
-    struct kvm_get_htab_fd ghf = {
-        .flags = 0,
-        .start_index = ptex,
-    };
     int fd, rc;
     int i;
 
-    fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
-    if (fd < 0) {
-        hw_error("kvmppc_read_hptes: Unable to open HPT fd");
-    }
+    fd = kvmppc_get_htab_fd(false, ptex, &error_abort);
 
     i = 0;
     while (i < n) {
@@ -2700,19 +2697,13 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
 void kvmppc_write_hpte(hwaddr ptex, uint64_t pte0, uint64_t pte1)
 {
     int fd, rc;
-    struct kvm_get_htab_fd ghf;
     struct {
         struct kvm_get_htab_header hdr;
         uint64_t pte0;
         uint64_t pte1;
     } buf;
 
-    ghf.flags = 0;
-    ghf.start_index = 0;     /* Ignored */
-    fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
-    if (fd < 0) {
-        hw_error("kvmppc_write_hpte: Unable to open HPT fd");
-    }
+    fd = kvmppc_get_htab_fd(true, 0 /* Ignored */, &error_abort);
 
     buf.hdr.n_valid = 1;
     buf.hdr.n_invalid = 0;
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 08aab46c5a56..349f892631bf 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -51,7 +51,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
 #endif /* !CONFIG_USER_ONLY */
 bool kvmppc_has_cap_epr(void);
 int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
-int kvmppc_get_htab_fd(bool write);
+int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
                            uint16_t n_valid, uint16_t n_invalid);
@@ -245,7 +245,7 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
     return -1;
 }
 
-static inline int kvmppc_get_htab_fd(bool write)
+static inline int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
 {
     return -1;
 }

Re: [Qemu-devel] [PATCH 1/4] ppc/kvm: drop kvmppc_has_cap_htab_fd()

[Thomas Huth]

On 15.09.2017 15:15, Greg Kurz wrote:
> It never got used since its introduction (commit 7c43bca004af).
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  target/ppc/kvm.c     |    5 -----
>  target/ppc/kvm_ppc.h |    6 ------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 1deaf106d2b9..00d7029b8d7a 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2448,11 +2448,6 @@ bool kvmppc_has_cap_epr(void)
>      return cap_epr;
>  }
>  
> -bool kvmppc_has_cap_htab_fd(void)
> -{
> -    return cap_htab_fd;
> -}
> -
>  bool kvmppc_has_cap_fixup_hcalls(void)
>  {
>      return cap_fixup_hcalls;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index f780e6ec7b72..08aab46c5a56 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -51,7 +51,6 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> -bool kvmppc_has_cap_htab_fd(void);
>  int kvmppc_get_htab_fd(bool write);
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> @@ -246,11 +245,6 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
>      return -1;
>  }
>  
> -static inline bool kvmppc_has_cap_htab_fd(void)
> -{
> -    return false;
> -}
> -
>  static inline int kvmppc_get_htab_fd(bool write)
>  {
>      return -1;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] ppc/kvm: drop kvmppc_has_cap_htab_fd()

[David Gibson]

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

On Fri, Sep 15, 2017 at 03:15:51PM +0200, Greg Kurz wrote:
> It never got used since its introduction (commit 7c43bca004af).
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.11, thanks.

> ---
>  target/ppc/kvm.c     |    5 -----
>  target/ppc/kvm_ppc.h |    6 ------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 1deaf106d2b9..00d7029b8d7a 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2448,11 +2448,6 @@ bool kvmppc_has_cap_epr(void)
>      return cap_epr;
>  }
>  
> -bool kvmppc_has_cap_htab_fd(void)
> -{
> -    return cap_htab_fd;
> -}
> -
>  bool kvmppc_has_cap_fixup_hcalls(void)
>  {
>      return cap_fixup_hcalls;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index f780e6ec7b72..08aab46c5a56 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -51,7 +51,6 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> -bool kvmppc_has_cap_htab_fd(void);
>  int kvmppc_get_htab_fd(bool write);
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> @@ -246,11 +245,6 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
>      return -1;
>  }
>  
> -static inline bool kvmppc_has_cap_htab_fd(void)
> -{
> -    return false;
> -}
> -
>  static inline int kvmppc_get_htab_fd(bool write)
>  {
>      return -1;
> 

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

Re: [Qemu-devel] [PATCH 2/4] spapr: introduce helpers to migrate HPT chunks and the end marker

[David Gibson]

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

On Fri, Sep 15, 2017 at 03:16:01PM +0200, Greg Kurz wrote:
> This consolidates some duplicated code in a single helper.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c |   38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f680f28a15ea..841117f6d185 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1708,6 +1708,23 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
>      return 0;
>  }
>  
> +static void htab_save_chunk(QEMUFile *f, sPAPRMachineState *spapr,
> +                            int chunkstart, int n_valid, int n_invalid)
> +{
> +    qemu_put_be32(f, chunkstart);
> +    qemu_put_be16(f, n_valid);
> +    qemu_put_be16(f, n_invalid);
> +    if (spapr) {

I like the basic idea, but passing NULL for spapr is *only* valid for
the end marker case.  The general test here is misleading.  I'd prefer
to either see the writes opencoded for the end marker, or have an
assert here (spapr can only be NULL if n_valid == n_invalid == 0).

> +        qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> +                        HASH_PTE_SIZE_64 * n_valid);
> +    }
> +}
> +
> +static void htab_save_end_marker(QEMUFile *f)
> +{
> +    htab_save_chunk(f, NULL, 0, 0, 0);
> +}
> +
>  static void htab_save_first_pass(QEMUFile *f, sPAPRMachineState *spapr,
>                                   int64_t max_ns)
>  {
> @@ -1739,11 +1756,7 @@ static void htab_save_first_pass(QEMUFile *f, sPAPRMachineState *spapr,
>          if (index > chunkstart) {
>              int n_valid = index - chunkstart;
>  
> -            qemu_put_be32(f, chunkstart);
> -            qemu_put_be16(f, n_valid);
> -            qemu_put_be16(f, 0);
> -            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> -                            HASH_PTE_SIZE_64 * n_valid);
> +            htab_save_chunk(f, spapr, chunkstart, n_valid, 0);
>  
>              if (has_timeout &&
>                  (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns) {
> @@ -1805,11 +1818,7 @@ static int htab_save_later_pass(QEMUFile *f, sPAPRMachineState *spapr,
>              int n_valid = invalidstart - chunkstart;
>              int n_invalid = index - invalidstart;
>  
> -            qemu_put_be32(f, chunkstart);
> -            qemu_put_be16(f, n_valid);
> -            qemu_put_be16(f, n_invalid);
> -            qemu_put_buffer(f, HPTE(spapr->htab, chunkstart),
> -                            HASH_PTE_SIZE_64 * n_valid);
> +            htab_save_chunk(f, spapr, chunkstart, n_valid, n_invalid);
>              sent += index - chunkstart;
>  
>              if (!final && (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - starttime) > max_ns) {
> @@ -1872,10 +1881,7 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
>          rc = htab_save_later_pass(f, spapr, MAX_ITERATION_NS);
>      }
>  
> -    /* End marker */
> -    qemu_put_be32(f, 0);
> -    qemu_put_be16(f, 0);
> -    qemu_put_be16(f, 0);
> +    htab_save_end_marker(f);
>  
>      return rc;
>  }
> @@ -1915,9 +1921,7 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
>      }
>  
>      /* End marker */
> -    qemu_put_be32(f, 0);
> -    qemu_put_be16(f, 0);
> -    qemu_put_be16(f, 0);
> +    htab_save_end_marker(f);
>  
>      return 0;
>  }
> 

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

Re: [Qemu-devel] [PATCH 3/4] ppc/kvm: change kvmppc_get_htab_fd() to return -errno on error

[David Gibson]

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

On Fri, Sep 15, 2017 at 03:16:10PM +0200, Greg Kurz wrote:
> When kvmppc_get_htab_fd() fails, its return value is propagated up to
> qemu_savevm_state_iterate() or to qemu_savevm_state_complete_precopy().
> All savevm handlers expect to receive a negative errno on error.
> 
> Let's patch kvmppc_get_htab_fd() accordingly.
> 
> While here, let's change htab_load() in the spapr code to also
> propagate the error, since it doesn't make sense to abort() if
> we couldn't get the htab fd from KVM.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.11.

> ---
>  hw/ppc/spapr.c   |    5 +++--
>  target/ppc/kvm.c |   10 ++++++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 841117f6d185..1ae79326d1ac 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1218,7 +1218,7 @@ static int get_htab_fd(sPAPRMachineState *spapr)
>      spapr->htab_fd = kvmppc_get_htab_fd(false);
>      if (spapr->htab_fd < 0) {
>          error_report("Unable to open fd for reading hash table from KVM: %s",
> -                     strerror(errno));
> +                     strerror(spapr->htab_fd));
>      }
>  
>      return spapr->htab_fd;
> @@ -1962,7 +1962,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>          fd = kvmppc_get_htab_fd(true);
>          if (fd < 0) {
>              error_report("Unable to open fd to restore KVM hash table: %s",
> -                         strerror(errno));
> +                         strerror(fd));
> +            return fd;
>          }
>      }
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 00d7029b8d7a..09d7dea79e2d 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2556,13 +2556,19 @@ int kvmppc_get_htab_fd(bool write)
>          .flags = write ? KVM_GET_HTAB_WRITE : 0,
>          .start_index = 0,
>      };
> +    int ret;
>  
>      if (!cap_htab_fd) {
>          fprintf(stderr, "KVM version doesn't support saving the hash table\n");
> -        return -1;
> +        return -ENOTSUP;
> +    }
> +
> +    ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
> +    if (ret < 0) {
> +        return -errno;
>      }
>  
> -    return kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
> +    return ret;
>  }
>  
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
> 

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

Re: [Qemu-devel] [PATCH 4/4] ppc/kvm: generalize the use of kvmppc_get_htab_fd()

[David Gibson]

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

On Fri, Sep 15, 2017 at 03:16:20PM +0200, Greg Kurz wrote:
> The use of KVM_PPC_GET_HTAB_FD is open-coded in kvmppc_read_hptes()
> and kvmppc_write_hpte().
> 
> This patch modifies kvmppc_get_htab_fd() so that it can be used
> everywhere we need to access the in-kernel htab:
> - add an index argument
>   => only kvmppc_read_hptes() passes an actual index, all other users
>      pass 0
> - add an errp argument to propagate error messages to the caller.
>   => spapr migration code prints the error
>   => hpte helpers pass &error_abort to keep the current behavior
>      of hw_error()
> 
> While here, this also fixes a bug in kvmppc_write_hpte() so that it
> opens the htab fd for writing instead of reading as it currently does.
> This never broke anything because we currently never call this code,
> as explained in the changelog of commit c1385933804bb:
> 
> "This support updating htab managed by the hypervisor. Currently
>  we don't have any user for this feature. This actually bring the
>  store_hpte interface in-line with the load_hpte one. We may want
>  to use this when we want to emulate henter hcall in qemu for HV
>  kvm."
> 
> The above is still true today.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.11.

> ---
>  hw/ppc/spapr.c       |   15 +++++++--------
>  target/ppc/kvm.c     |   27 +++++++++------------------
>  target/ppc/kvm_ppc.h |    4 ++--
>  3 files changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1ae79326d1ac..eeef549fbc15 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1211,14 +1211,15 @@ static uint64_t spapr_get_patbe(PPCVirtualHypervisor *vhyp)
>   */
>  static int get_htab_fd(sPAPRMachineState *spapr)
>  {
> +    Error *local_err = NULL;
> +
>      if (spapr->htab_fd >= 0) {
>          return spapr->htab_fd;
>      }
>  
> -    spapr->htab_fd = kvmppc_get_htab_fd(false);
> +    spapr->htab_fd = kvmppc_get_htab_fd(false, 0, &local_err);
>      if (spapr->htab_fd < 0) {
> -        error_report("Unable to open fd for reading hash table from KVM: %s",
> -                     strerror(spapr->htab_fd));
> +        error_report_err(local_err);
>      }
>  
>      return spapr->htab_fd;
> @@ -1931,6 +1932,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      sPAPRMachineState *spapr = opaque;
>      uint32_t section_hdr;
>      int fd = -1;
> +    Error *local_err = NULL;
>  
>      if (version_id < 1 || version_id > 1) {
>          error_report("htab_load() bad version");
> @@ -1945,8 +1947,6 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      }
>  
>      if (section_hdr) {
> -        Error *local_err = NULL;
> -
>          /* First section gives the htab size */
>          spapr_reallocate_hpt(spapr, section_hdr, &local_err);
>          if (local_err) {
> @@ -1959,10 +1959,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
>      if (!spapr->htab) {
>          assert(kvm_enabled());
>  
> -        fd = kvmppc_get_htab_fd(true);
> +        fd = kvmppc_get_htab_fd(true, 0, &local_err);
>          if (fd < 0) {
> -            error_report("Unable to open fd to restore KVM hash table: %s",
> -                         strerror(fd));
> +            error_report_err(local_err);
>              return fd;
>          }
>      }
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 09d7dea79e2d..c37d74941085 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2550,21 +2550,25 @@ int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function)
>      return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN, &args);
>  }
>  
> -int kvmppc_get_htab_fd(bool write)
> +int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>  {
>      struct kvm_get_htab_fd s = {
>          .flags = write ? KVM_GET_HTAB_WRITE : 0,
> -        .start_index = 0,
> +        .start_index = index,
>      };
>      int ret;
>  
>      if (!cap_htab_fd) {
> -        fprintf(stderr, "KVM version doesn't support saving the hash table\n");
> +        error_setg(errp, "KVM version doesn't support %s the HPT",
> +                   write ? "writing" : "reading");
>          return -ENOTSUP;
>      }
>  
>      ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
>      if (ret < 0) {
> +        error_setg(errp, "Unable to open fd for %s HPT %s KVM: %s",
> +                   write ? "writing" : "reading", write ? "to" : "from",
> +                   strerror(errno));
>          return -errno;
>      }
>  
> @@ -2648,17 +2652,10 @@ void kvm_arch_init_irq_routing(KVMState *s)
>  
>  void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
>  {
> -    struct kvm_get_htab_fd ghf = {
> -        .flags = 0,
> -        .start_index = ptex,
> -    };
>      int fd, rc;
>      int i;
>  
> -    fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> -    if (fd < 0) {
> -        hw_error("kvmppc_read_hptes: Unable to open HPT fd");
> -    }
> +    fd = kvmppc_get_htab_fd(false, ptex, &error_abort);
>  
>      i = 0;
>      while (i < n) {
> @@ -2700,19 +2697,13 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
>  void kvmppc_write_hpte(hwaddr ptex, uint64_t pte0, uint64_t pte1)
>  {
>      int fd, rc;
> -    struct kvm_get_htab_fd ghf;
>      struct {
>          struct kvm_get_htab_header hdr;
>          uint64_t pte0;
>          uint64_t pte1;
>      } buf;
>  
> -    ghf.flags = 0;
> -    ghf.start_index = 0;     /* Ignored */
> -    fd = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &ghf);
> -    if (fd < 0) {
> -        hw_error("kvmppc_write_hpte: Unable to open HPT fd");
> -    }
> +    fd = kvmppc_get_htab_fd(true, 0 /* Ignored */, &error_abort);
>  
>      buf.hdr.n_valid = 1;
>      buf.hdr.n_invalid = 0;
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 08aab46c5a56..349f892631bf 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -51,7 +51,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> -int kvmppc_get_htab_fd(bool write);
> +int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp);
>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
>                             uint16_t n_valid, uint16_t n_invalid);
> @@ -245,7 +245,7 @@ static inline int kvmppc_define_rtas_kernel_token(uint32_t token,
>      return -1;
>  }
>  
> -static inline int kvmppc_get_htab_fd(bool write)
> +static inline int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>  {
>      return -1;
>  }
> 

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