Re: [Qemu-devel] [PATCH qemu v4 3/3] spapr/iommu: Enable in-kernel TCE acceleration via VFIO KVM device

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Thu, Jul 20, 2017 at 05:22:31PM +1000, Alexey Kardashevskiy wrote:
> This uses new kernel KVM_CAP_SPAPR_TCE_VFIO capability to enable
> in-kernel acceleration of TCE update requests which will go via
> the VFIO KVM device.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

I reviewed this before, but I realized a minor nit here.

> ---
>  target/ppc/kvm_ppc.h | 6 ++++++
>  hw/ppc/spapr_iommu.c | 4 ++++
>  target/ppc/kvm.c     | 7 ++++++-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 6bc6fb3e2d..96936ec50f 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -46,6 +46,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t page_shift,
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
>  int kvmppc_reset_htab(int shift_hint);
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> +bool kvmppc_has_cap_spapr_vfio(void);
>  #endif /* !CONFIG_USER_ONLY */
>  bool kvmppc_has_cap_epr(void);
>  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
> @@ -220,6 +221,11 @@ static inline bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
>      return true;
>  }
>  
> +static inline bool kvmppc_has_cap_spapr_vfio(void)
> +{
> +    return false;
> +}
> +
>  #endif /* !CONFIG_USER_ONLY */
>  
>  static inline bool kvmppc_has_cap_epr(void)
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 82fca61a75..3b4c1f50f4 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -289,6 +289,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio)
>  
>      tcet->need_vfio = need_vfio;
>  
> +    if (!need_vfio || (tcet->fd != -1 && kvmppc_has_cap_spapr_vfio())) {
> +        return;
> +    }
> +
>      oldtable = tcet->table;
>  
>      tcet->table = spapr_tce_alloc_table(tcet->liobn,
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 85713795de..413ce84509 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -134,7 +134,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
>      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
>      cap_spapr_multitce = kvm_check_extension(s, KVM_CAP_SPAPR_MULTITCE);
> -    cap_spapr_vfio = false;
> +    cap_spapr_vfio = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_VFIO);

This should probably be kvm_vm_check_extension().  It might not now,
but the availability of the VFIO TCE extension could logically depend
on whether we're KVM or PR, so we need the kernel to know which one
we're dealing with.  See "ppc/kvm: check some capabilities with
kvm_vm_check_extension()" in ppc-for-2.11 for some recent changes on
the same basis (not merged yet, so the SHA's not fixed).

>      cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
>      cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
>      cap_epr = kvm_check_extension(s, KVM_CAP_PPC_EPR);
> @@ -2428,6 +2428,11 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
>      return cap_mmu_hash_v3;
>  }
>  
> +bool kvmppc_has_cap_spapr_vfio(void)
> +{
> +    return cap_spapr_vfio;
> +}
> +
>  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void)
>  {
>      uint32_t host_pvr = mfpvr();

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]