Re: [PATCH kernel 4/4] KVM: PPC: Add support for 64bit TCE windows

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

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

On Wed, Jan 27, 2016 at 02:29:25PM +1100, Alexey Kardashevskiy wrote:
> On 01/25/2016 04:37 PM, David Gibson wrote:
> >On Thu, Jan 21, 2016 at 07:15:26PM +1100, Alexey Kardashevskiy wrote:
> >>The existing KVM_CREATE_SPAPR_TCE only supports 32bit windows which is not
> >>enough for directly mapped windows as the guest can get more than 4GB.
> >>
> >>This adds KVM_CREATE_SPAPR_TCE_64 ioctl and advertises it
> >>via KVM_CAP_SPAPR_TCE_64 capability.
> >>
> >>Since 64bit windows are to support Dynamic DMA windows (DDW), let's add
> >>@bus_offset and @page_shift which are also required by DDW.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>---
> >>  Documentation/virtual/kvm/api.txt   | 32 ++++++++++++++++++++++++++++++++
> >>  arch/powerpc/include/asm/kvm_ppc.h  |  2 +-
> >>  arch/powerpc/include/uapi/asm/kvm.h |  9 +++++++++
> >>  arch/powerpc/kvm/book3s_64_vio.c    | 10 +++++++---
> >>  arch/powerpc/kvm/powerpc.c          | 25 ++++++++++++++++++++++++-
> >>  include/uapi/linux/kvm.h            |  2 ++
> >>  6 files changed, 75 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >>index da39435..d1c5655 100644
> >>--- a/Documentation/virtual/kvm/api.txt
> >>+++ b/Documentation/virtual/kvm/api.txt
> >>@@ -3060,6 +3060,38 @@ an implementation for these despite the in kernel acceleration.
> >>
> >>  This capability is always enabled.
> >>
> >>+4.96 KVM_CREATE_SPAPR_TCE_64
> >>+
> >>+Capability: KVM_CAP_SPAPR_TCE_64
> >>+Architectures: powerpc
> >>+Type: vm ioctl
> >>+Parameters: struct kvm_create_spapr_tce_64 (in)
> >>+Returns: file descriptor for manipulating the created TCE table
> >>+
> >>+This is an extension for KVM_CAP_SPAPR_TCE which only supports 32bit
> >>+windows, described in 4.62 KVM_CREATE_SPAPR_TCE
> >>+
> >>+This capability uses extended struct in ioctl interface:
> >>+
> >>+/* for KVM_CAP_SPAPR_TCE_64 */
> >>+struct kvm_create_spapr_tce_64 {
> >>+	__u64 liobn;
> >>+	__u32 page_shift;
> >>+	__u64 offset;	/* in pages */
> >>+	__u64 size; 	/* in pages */
> >>+	__u32 flags;
> >
> >Best to move page_shift after offset and size, so the structure
> >doesn't get an alignment gap.
> 
> Agrh. I did this again :-/
> 
> 
> >>+};
> >>+
> >>+The aim of extension is to support an additional bigger DMA window with
> >>+a variable page size.
> >>+KVM_CREATE_SPAPR_TCE_64 receives a 64bit window size, an IOMMU page shift and
> >>+a bus offset of the corresponding DMA window, @size and @offset are numbers
> >>+of IOMMU pages.
> >>+
> >>+@flags are not used at the moment.
> >>+
> >>+The rest of functionality is identical to KVM_CREATE_SPAPR_TCE.
> >>+
> >>  5. The kvm_run structure
> >>  ------------------------
> >>
> >>diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> >>index 4cadee5..6e4d1dc 100644
> >>--- a/arch/powerpc/include/asm/kvm_ppc.h
> >>+++ b/arch/powerpc/include/asm/kvm_ppc.h
> >>@@ -165,7 +165,7 @@ extern void kvmppc_map_vrma(struct kvm_vcpu *vcpu,
> >>  extern int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu);
> >>
> >>  extern long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> >>-				struct kvm_create_spapr_tce *args);
> >>+				struct kvm_create_spapr_tce_64 *args);
> >>  extern struct kvmppc_spapr_tce_table *kvmppc_find_table(
> >>  		struct kvm_vcpu *vcpu, unsigned long liobn);
> >>  extern long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
> >>diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >>index ab4d473..9c8b4cbc 100644
> >>--- a/arch/powerpc/include/uapi/asm/kvm.h
> >>+++ b/arch/powerpc/include/uapi/asm/kvm.h
> >>@@ -333,6 +333,15 @@ struct kvm_create_spapr_tce {
> >>  	__u32 window_size;
> >>  };
> >>
> >>+/* for KVM_CAP_SPAPR_TCE_64 */
> >>+struct kvm_create_spapr_tce_64 {
> >>+	__u64 liobn;
> >>+	__u32 page_shift;
> >>+	__u64 offset;	/* in pages */
> >>+	__u64 size;	/* in pages */
> >>+	__u32 flags;
> >>+};
> >>+
> >>  /* for KVM_ALLOCATE_RMA */
> >>  struct kvm_allocate_rma {
> >>  	__u64 rma_size;
> >>diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> >>index 85ee572..5479446 100644
> >>--- a/arch/powerpc/kvm/book3s_64_vio.c
> >>+++ b/arch/powerpc/kvm/book3s_64_vio.c
> >>@@ -144,20 +144,23 @@ static const struct file_operations kvm_spapr_tce_fops = {
> >>  };
> >>
> >>  long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> >>-				   struct kvm_create_spapr_tce *args)
> >>+				   struct kvm_create_spapr_tce_64 *args)
> >>  {
> >>  	struct kvmppc_spapr_tce_table *stt = NULL;
> >>  	unsigned long npages, size;
> >>  	int ret = -ENOMEM;
> >>  	int i;
> >>
> >>+	if (!args->size)
> >>+		return -EINVAL;
> >>+
> >>  	/* Check this LIOBN hasn't been previously allocated */
> >>  	list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
> >>  		if (stt->liobn == args->liobn)
> >>  			return -EBUSY;
> >>  	}
> >>
> >>-	size = args->window_size >> IOMMU_PAGE_SHIFT_4K;
> >>+	size = args->size;
> >
> >Doesn't this need some kind of bounds on the allowed size?
> 
> 
> kvmppc_account_memlimit() below is some kind of bound, not enough?

Ah, yes, that should be enough.

Sorry, missed that.

-- 
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: 819 bytes --]