Re: [Qemu-ppc] [Qemu-devel] [PATCH] pseries: Don't try to munmap() a malloc()ed TCE table

[Andreas Färber]

Am 08.02.2012 06:53, schrieb David Gibson:
> For the pseries machine, TCE (IOMMU) tables can either be directly
> malloc()ed in qemu or, when running on a KVM which supports it, mmap()ed
> from a KVM ioctl.  The latter option is used when available, because it
> allows the (frequent bottlenext) H_PUT_TCE hypercall to be KVM accelerated.
> However, even when KVM is persent, TCE acceleration is not always possible.
> Only KVM HV supports this ioctl(), not KVM PR, or the kernel could run out
> of contiguous memory to allocate the new table.  In this case we need to
> fall back on the malloc()ed table.
> 
> When a device is removed, and we need to remove the TCE table, we need to
> either munmap() or free() the table as appropriate for how it was
> allocated.  The code is supposed to do that, but we buggily fail to
> initialize the tcet->fd variable in the malloc() case, which is used as a
> flag to determine which is the right choice.
> 
> This patch fixes the bug, and cleans up error messages relating to this
> path while we're at it.
> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> Signed-off-by: David Gibson <address@hidden>

Looks okay except that your fprintf()s should use PRIx32 for uint32_t
liobn for safety (no difference on Linux).

Andreas

> ---
>  target-ppc/kvm.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 50cfa02..90c6941 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -843,12 +843,18 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> window_size, int *pfd)
>      int fd;
>      void *table;
>  
> +    /* Must set fd to -1 so we don't try to munmap when called for
> +     * destroying the table, which the upper layers -will- do
> +     */
> +    *pfd = -1;
>      if (!cap_spapr_tce) {
>          return NULL;
>      }
>  
>      fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_SPAPR_TCE, &args);
>      if (fd < 0) {
> +        fprintf(stderr, "KVM: Failed to create TCE table for liobn 0x%x\n",
> +                liobn);
>          return NULL;
>      }
>  
> @@ -857,6 +863,8 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> window_size, int *pfd)
>  
>      table = mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>      if (table == MAP_FAILED) {
> +        fprintf(stderr, "KVM: Failed to map TCE table for liobn 0x%x\n",
> +                liobn);
>          close(fd);
>          return NULL;
>      }
> @@ -876,8 +884,8 @@ int kvmppc_remove_spapr_tce(void *table, int fd, uint32_t 
> window_size)
>      len = (window_size / SPAPR_VIO_TCE_PAGE_SIZE)*sizeof(VIOsPAPR_RTCE);
>      if ((munmap(table, len) < 0) ||
>          (close(fd) < 0)) {
> -        fprintf(stderr, "KVM: Unexpected error removing KVM SPAPR TCE "
> -                "table: %s", strerror(errno));
> +        fprintf(stderr, "KVM: Unexpected error removing TCE table: %s",
> +                strerror(errno));
>          /* Leak the table */
>      }
>  

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg