Re: [RFCv2 21/37] KVM: S390: protvirt: Introduce instruction data area bounce buffer

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3037 bytes --]

On 2/5/20 1:02 PM, Thomas Huth wrote:
> On 03/02/2020 14.19, 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.
>>
>> 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>
>> ---
> [...]
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index eab741bc12c3..20969ce12096 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 */
>>  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 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)
>> @@ -1510,6 +1516,7 @@ struct kvm_pv_cmd {
>>  /* Available with KVM_CAP_S390_PROTECTED */
>>  #define KVM_S390_PV_COMMAND		_IOW(KVMIO, 0xc5, struct kvm_pv_cmd)
>>  #define KVM_S390_PV_COMMAND_VCPU	_IOW(KVMIO, 0xc6, struct kvm_pv_cmd)
>> +#define KVM_S390_SIDA_OP		_IOW(KVMIO, 0xc7, struct kvm_s390_mem_op)
>>  
>>  /* Secure Encrypted Virtualization command */
>>  enum sev_cmd_id {
>>
> 
> Uh, why the mix of a new ioctl with the existing mem_op stuff? Could you
> please either properly integrate this into the MEM_OP ioctl (and e.g.
> use gaddr as offset for the new SIDA_READ and SIDA_WRITE subcodes), or
> completely separate it for a new ioctl, i.e. introduce a new struct for
> the new ioctl instead of recycling the struct kvm_s390_mem_op here?
> (and in case you ask me, I'd slightly prefer to integrate everything
> into MEM_OP instead of introducing a new ioctl here).

*cough* David and Christian didn't like the memop solution and it took
me a long time to get this to work properly in QEMU...

I wouldn't mind a dedicated struct.

> 
> In any case, please also update Documentation/virt/kvm/api.txt with the
> new SIDA functions.
> 
>  Thanks,
>   Thomas
> 



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