Subject: Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
To: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>,
        "Dmitry V. Levin" <ldv@altlinux.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20170710144410.GA10359@glebfm.cloud.tilaa.com>
 <09c0760e-729d-548a-c3c4-b3afd9e67225@de.ibm.com>
 <20170710212321.GB2703@glebfm.cloud.tilaa.com>
From: Christian Borntraeger <borntraeger@de.ibm.com>
Date: Tue, 11 Jul 2017 10:21:52 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.0
MIME-Version: 1.0
In-Reply-To: <20170710212321.GB2703@glebfm.cloud.tilaa.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-IE
Content-Transfer-Encoding: 8bit
Message-Id: <c5586f91-9a06-ee45-18ec-848cc0ed44ff@de.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1604
Lines: 39

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@altlinux.org>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>>
>> 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.