Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Christian Borntraeger]

On 07/10/2017 11:23 PM, Gleb Fotengauer-Malinovskiy wrote:
> On Mon, Jul 10, 2017 at 08:43:12PM +0200, Christian Borntraeger wrote:
>> On 07/10/2017 04:44 PM, Gleb Fotengauer-Malinovskiy wrote:
>>> This ioctl actually writes to parameter too.
>>
>> Maybe rephrase that to:
>> The kernel does not only read struct kvm_s390_cmma_log for KVM_S390_GET_CMMA_BITS,
>> it also writes back a return value making this _IOWR instead of _IOW.
> 
> Ok, see v2.
> 
>>> Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
>>> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@xxxxxxxxxxxx>
>> Acked-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>>
>>
>> Out of curiosity, how did you notice that? 
> 
> I regenerated strace's ioctl lists.  It was obvious from the diff that
> *GET* and *SET* could not be both _IOC_WRITE.
> 

In fact we do have multiple GET/SET ioctls in KVM, where both provide a control
block that is _IOC_WRITE only. That control block then has an address that will 
be read/written to depending on get/set. 
E.g. look at 
#define KVM_SET_DEVICE_ATTR       _IOW(KVMIO,  0xe1, struct kvm_device_attr)
#define KVM_GET_DEVICE_ATTR       _IOW(KVMIO,  0xe2, struct kvm_device_attr)

but as far as I understand, the direction hints only qualify the referenced
struct and not the side effects. So for KVM_*_DEVICE_ATTR it is correct to have
IOW for both cases.

But for GET_CMMA we do indeed write back data. 

Paolo, Radim,

if we want to fix the direction hint, it would be good to merge this in as soon
as possible. The new interface was added during this merge window.