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

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 06.02.20 11:32, Thomas Huth wrote:
> 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;
[...]


ack

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

ack

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

ack
[...]

>>  };
>>  
>> -/* for KVM_S390_MEM_OP */
>> +/* for KVM_S390_MEM_OP and KVM_S390_SIDA_OP */
> 
> Remove this change now, please.

ack

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

thanks