Re: [RFCv2.1] KVM: S390: protvirt: Introduce instruction data area bounce buffer

[Thomas Huth <thuth@redhat.com>]

On 06/02/2020 10.39, Christian Borntraeger wrote:
> From: Janosch Frank <frankja@linux.ibm.com>
> 
> Now that we can't access guest memory anymore, we have a dedicated
> sattelite block that's a bounce buffer for instruction data.

s/sattelite/satellite/

> We re-use the memop interface to copy the instruction data to / from
> userspace. This lets us re-use a lot of QEMU code which used that
> interface to make logical guest memory accesses which are not possible
> anymore in protected mode anyway.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 11 ++++++-
>  arch/s390/kvm/kvm-s390.c         | 49 ++++++++++++++++++++++++++++++++
>  arch/s390/kvm/pv.c               |  9 ++++++
>  include/uapi/linux/kvm.h         | 10 +++++--
>  4 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 9d7b248dcadc..2fe8d3c81951 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -127,6 +127,12 @@ struct mcck_volatile_info {
>  #define CR14_INITIAL_MASK (CR14_UNUSED_32 | CR14_UNUSED_33 | \
>  			   CR14_EXTERNAL_DAMAGE_SUBMASK)
>  
> +#define SIDAD_SIZE_MASK		0xff
> +#define sida_origin(sie_block) \
> +	(sie_block->sidad & PAGE_MASK)
> +#define sida_size(sie_block) \
> +	(((sie_block->sidad & SIDAD_SIZE_MASK) + 1) * PAGE_SIZE)
> +
>  #define CPUSTAT_STOPPED    0x80000000
>  #define CPUSTAT_WAIT       0x10000000
>  #define CPUSTAT_ECALL_PEND 0x08000000
> @@ -315,7 +321,10 @@ struct kvm_s390_sie_block {
>  #define CRYCB_FORMAT2 0x00000003
>  	__u32	crycbd;			/* 0x00fc */
>  	__u64	gcr[16];		/* 0x0100 */
> -	__u64	gbea;			/* 0x0180 */
> +	union {
> +		__u64	gbea;			/* 0x0180 */

Maybe adjust the spaces before the comment.

> +		__u64	sidad;
> +	};
>  	__u8    reserved188[8];		/* 0x0188 */
>  	__u64   sdnxo;			/* 0x0190 */
>  	__u8    reserved198[8];		/* 0x0198 */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 6f90d16cad92..56488f9ed190 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -4435,6 +4435,41 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	return r;
>  }
>  
> +static long kvm_s390_guest_sida_op(struct kvm_vcpu *vcpu,
> +				   struct kvm_s390_mem_op *mop)
> +{
> +	void __user *uaddr = (void __user *)mop->buf;
> +	int r = 0;
> +
> +	if (mop->flags || !mop->size)
> +		return -EINVAL;
> +
> +	if (mop->size > sida_size(vcpu->arch.sie_block))
> +		return -E2BIG;
> +
> +	if (mop->sida_offset > sida_size(vcpu->arch.sie_block))
> +		return -E2BIG;
> +
> +	if (mop->size + mop->sida_offset > sida_size(vcpu->arch.sie_block))
> +		return -E2BIG;
> +
> +	switch (mop->op) {
> +	case KVM_S390_MEMOP_SIDA_READ:
> +		r = 0;

r is alread pre-initialized with 0 where it is declared, so you could
remove the above line.

> +		if (copy_to_user(uaddr, (void *)(sida_origin(vcpu->arch.sie_block) +
> +				 mop->sida_offset), mop->size))
> +			r = -EFAULT;
> +
> +		break;
> +	case KVM_S390_MEMOP_SIDA_WRITE:
> +		r = 0;

dito.

> +		if (copy_from_user((void *)(sida_origin(vcpu->arch.sie_block) +
> +				   mop->sida_offset), uaddr, mop->size))
> +			r = -EFAULT;
> +		break;
> +	}
> +	return r;
> +}
>  static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  				  struct kvm_s390_mem_op *mop)
>  {
> @@ -4444,6 +4479,8 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  	const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
>  				    | KVM_S390_MEMOP_F_CHECK_ONLY;
>  
> +
> +	BUILD_BUG_ON(sizeof(*mop) != 64);
>  	if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
>  		return -EINVAL;
>  
> @@ -4460,6 +4497,10 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  
>  	switch (mop->op) {
>  	case KVM_S390_MEMOP_LOGICAL_READ:
> +		if (kvm_s390_pv_is_protected(vcpu->kvm)) {
> +			r = -EINVAL;
> +			break;
> +		}
>  		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
>  			r = check_gva_range(vcpu, mop->gaddr, mop->ar,
>  					    mop->size, GACC_FETCH);
> @@ -4472,6 +4513,10 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  		}
>  		break;
>  	case KVM_S390_MEMOP_LOGICAL_WRITE:
> +		if (kvm_s390_pv_is_protected(vcpu->kvm)) {
> +			r = -EINVAL;
> +			break;
> +		}
>  		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
>  			r = check_gva_range(vcpu, mop->gaddr, mop->ar,
>  					    mop->size, GACC_STORE);
> @@ -4483,6 +4528,10 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
>  		}
>  		r = write_guest(vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);
>  		break;
> +	case KVM_S390_MEMOP_SIDA_READ:
> +	case KVM_S390_MEMOP_SIDA_WRITE:
> +		r = kvm_s390_guest_sida_op(vcpu, mop);
> +		break;
>  	default:
>  		r = -EINVAL;
>  	}
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> index 05e5ca8eab4f..d6345a0e348c 100644
> --- a/arch/s390/kvm/pv.c
> +++ b/arch/s390/kvm/pv.c
> @@ -93,6 +93,7 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu)
>  
>  	free_pages(vcpu->arch.pv.stor_base,
>  		   get_order(uv_info.guest_cpu_stor_len));
> +	free_page(sida_origin(vcpu->arch.sie_block));
>  	vcpu->arch.sie_block->pv_handle_cpu = 0;
>  	vcpu->arch.sie_block->pv_handle_config = 0;
>  	memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
> @@ -122,6 +123,14 @@ int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu)
>  	uvcb.state_origin = (u64)vcpu->arch.sie_block;
>  	uvcb.stor_origin = (u64)vcpu->arch.pv.stor_base;
>  
> +	/* Alloc Secure Instruction Data Area Designation */
> +	vcpu->arch.sie_block->sidad = __get_free_page(GFP_KERNEL | __GFP_ZERO);
> +	if (!vcpu->arch.sie_block->sidad) {
> +		free_pages(vcpu->arch.pv.stor_base,
> +			   get_order(uv_info.guest_cpu_stor_len));
> +		return -ENOMEM;
> +	}
> +
>  	rc = uv_call(0, (u64)&uvcb);
>  	VCPU_EVENT(vcpu, 3, "PROTVIRT CREATE VCPU: cpu %d handle %llx rc %x rrc %x",
>  		   vcpu->vcpu_id, uvcb.cpu_handle, uvcb.header.rc,
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index eab741bc12c3..a772771baf9f 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -466,7 +466,7 @@ struct kvm_translation {
>  	__u8  pad[5];
>  };
>  
> -/* for KVM_S390_MEM_OP */
> +/* for KVM_S390_MEM_OP and KVM_S390_SIDA_OP */

Remove this change now, please.

>  struct kvm_s390_mem_op {
>  	/* in */
>  	__u64 gaddr;		/* the guest address */
> @@ -475,11 +475,17 @@ struct kvm_s390_mem_op {
>  	__u32 op;		/* type of operation */
>  	__u64 buf;		/* buffer in userspace */
>  	__u8 ar;		/* the access register number */
> -	__u8 reserved[31];	/* should be set to 0 */
> +	__u8 reserved21[3];	/* should be set to 0 */
> +	__u32 sida_offset;	/* offset into the sida */
> +	__u8 reserved28[24];	/* should be set to 0 */
>  };
> +
> +
>  /* types for kvm_s390_mem_op->op */
>  #define KVM_S390_MEMOP_LOGICAL_READ	0
>  #define KVM_S390_MEMOP_LOGICAL_WRITE	1
> +#define KVM_S390_MEMOP_SIDA_READ	2
> +#define KVM_S390_MEMOP_SIDA_WRITE	3
>  /* flags for kvm_s390_mem_op->flags */
>  #define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
>  #define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)

With the nits fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>