From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC8Yo-0000El-9z for qemu-devel@nongnu.org; Mon, 06 Jul 2015 11:43:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZC8Yg-0004tO-6J for qemu-devel@nongnu.org; Mon, 06 Jul 2015 11:43:58 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:34286) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZC8Yf-0004sa-Up for qemu-devel@nongnu.org; Mon, 06 Jul 2015 11:43:50 -0400 Received: by pdbep18 with SMTP id ep18so108010653pdb.1 for ; Mon, 06 Jul 2015 08:43:49 -0700 (PDT) References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-5-git-send-email-aik@ozlabs.ru> <20150706171443.7421edd9@thh440s> From: Alexey Kardashevskiy Message-ID: <559AA22E.8030207@ozlabs.ru> Date: Tue, 7 Jul 2015 01:43:42 +1000 MIME-Version: 1.0 In-Reply-To: <20150706171443.7421edd9@thh440s> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v10 04/14] spapr_iommu: Move table allocation to helpers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org, David Gibson On 07/07/2015 01:14 AM, Thomas Huth wrote: > On Mon, 6 Jul 2015 12:11:00 +1000 > Alexey Kardashevskiy wrote: > >> At the moment presence of vfio-pci devices on a bus affect the way >> the guest view table is allocated. If there is no vfio-pci on a PHB >> and the host kernel supports KVM acceleration of H_PUT_TCE, a table >> is allocated in KVM. However, if there is vfio-pci and we do yet not >> KVM acceleration for these, the table has to be allocated by >> the userspace. At the moment the table is allocated once at boot time >> but next patches will reallocate it. >> >> This moves kvmppc_create_spapr_tce/g_malloc0 and their counterparts >> to helpers. >> >> Signed-off-by: Alexey Kardashevskiy >> Reviewed-by: David Gibson >> --- >> hw/ppc/spapr_iommu.c | 58 +++++++++++++++++++++++++++++++++++----------------- >> trace-events | 2 +- >> 2 files changed, 40 insertions(+), 20 deletions(-) >> >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index f61504e..0cf5010 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -74,6 +74,37 @@ static IOMMUAccessFlags spapr_tce_iommu_access_flags(uint64_t tce) >> } >> } >> >> +static uint64_t *spapr_tce_alloc_table(uint32_t liobn, >> + uint32_t nb_table, >> + uint32_t page_shift, >> + int *fd, >> + bool vfio_accel) >> +{ >> + uint64_t *table = NULL; >> + uint64_t window_size = (uint64_t)nb_table << page_shift; >> + >> + if (kvm_enabled() && !(window_size >> 32)) { >> + table = kvmppc_create_spapr_tce(liobn, window_size, fd, vfio_accel); >> + } >> + >> + if (!table) { >> + *fd = -1; >> + table = g_malloc0(nb_table * sizeof(uint64_t)); >> + } >> + >> + trace_spapr_iommu_alloc_table(liobn, table, *fd); >> + >> + return table; >> +} >> + >> +static void spapr_tce_free_table(uint64_t *table, int fd, uint32_t nb_table) >> +{ >> + if (!kvm_enabled() || >> + (kvmppc_remove_spapr_tce(table, fd, nb_table) != 0)) { >> + g_free(table); >> + } >> +} >> + >> /* Called from RCU critical section */ >> static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, >> bool is_write) >> @@ -140,21 +171,13 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { >> static int spapr_tce_table_realize(DeviceState *dev) >> { >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev); >> - uint64_t window_size = (uint64_t)tcet->nb_table << tcet->page_shift; >> >> - if (kvm_enabled() && !(window_size >> 32)) { >> - tcet->table = kvmppc_create_spapr_tce(tcet->liobn, >> - window_size, >> - &tcet->fd, >> - tcet->vfio_accel); >> - } >> - >> - if (!tcet->table) { >> - size_t table_size = tcet->nb_table * sizeof(uint64_t); >> - tcet->table = g_malloc0(table_size); >> - } >> - >> - trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd); >> + tcet->fd = -1; > > As far as I can see, spapr_tce_alloc_table() always initialized the fd > to -1 in case the allocation failed, so you can drop the above line, I > think. Later in the patchset I remove spapr_tce_alloc_table() so it is safer to initialize it here, I guess. >> + tcet->table = spapr_tce_alloc_table(tcet->liobn, >> + tcet->nb_table, >> + tcet->page_shift, >> + &tcet->fd, >> + tcet->vfio_accel); >> >> memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops, >> "iommu-spapr", > > Apart from the nit above, the patch looks fine to me. > > Thomas > -- Alexey