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

[David Gibson]

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 <address@hidden>

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

signature.asc
Description: PGP signature