From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47671) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S6Llf-0000q8-BU for qemu-devel@nongnu.org; Sat, 10 Mar 2012 07:51:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S6Lld-000623-9K for qemu-devel@nongnu.org; Sat, 10 Mar 2012 07:51:26 -0500 Message-ID: <4F5B4E41.6050803@suse.de> Date: Sat, 10 Mar 2012 13:51:13 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1328680405-31731-1-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1328680405-31731-1-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] pseries: Don't try to munmap() a malloc()ed TCE table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org 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()e= d > from a KVM ioctl. The latter option is used when available, because it > allows the (frequent bottlenext) H_PUT_TCE hypercall to be KVM accelera= ted. > However, even when KVM is persent, TCE acceleration is not always possi= ble. > 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 t= o > fall back on the malloc()ed table. >=20 > 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. >=20 > This patch fixes the bug, and cleans up error messages relating to this > path while we're at it. >=20 > Signed-off-by: Benjamin Herrenschmidt > Signed-off-by: David Gibson 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(-) >=20 > 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, uin= t32_t window_size, int *pfd) > int fd; > void *table; > =20 > + /* 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 =3D -1; > if (!cap_spapr_tce) { > return NULL; > } > =20 > fd =3D 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; > } > =20 > @@ -857,6 +863,8 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint3= 2_t window_size, int *pfd) > =20 > table =3D mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0)= ; > if (table =3D=3D 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, ui= nt32_t window_size) > len =3D (window_size / SPAPR_VIO_TCE_PAGE_SIZE)*sizeof(VIOsPAPR_RT= CE); > 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 */ > } > =20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg