Re: [Qemu-devel] [PATCH qemu v10 04/14] spapr_iommu: Move table allocation to helpers

[Thomas Huth <thuth@redhat.com>]

On Mon,  6 Jul 2015 12:11:00 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> 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 <aik@ozlabs.ru>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  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.

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