* [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
@ 2017-07-10 14:44 Gleb Fotengauer-Malinovskiy
2017-07-10 18:43 ` Christian Borntraeger
0 siblings, 1 reply; 6+ messages in thread
From: Gleb Fotengauer-Malinovskiy @ 2017-07-10 14:44 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář, Claudio Imbrenda
Cc: Dmitry V. Levin, kvm, linux-kernel
This ioctl actually writes to parameter too.
Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
---
include/uapi/linux/kvm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c0b6dfe..ebd604c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
/* Available with KVM_CAP_X86_SMM */
#define KVM_SMI _IO(KVMIO, 0xb7)
/* Available with KVM_CAP_S390_CMMA_MIGRATION */
-#define KVM_S390_GET_CMMA_BITS _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
+#define KVM_S390_GET_CMMA_BITS _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
#define KVM_S390_SET_CMMA_BITS _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
--
glebfm
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
2017-07-10 14:44 [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition Gleb Fotengauer-Malinovskiy
@ 2017-07-10 18:43 ` Christian Borntraeger
2017-07-10 21:22 ` [PATCH v2] " Gleb Fotengauer-Malinovskiy
2017-07-10 21:23 ` [PATCH] " Gleb Fotengauer-Malinovskiy
0 siblings, 2 replies; 6+ messages in thread
From: Christian Borntraeger @ 2017-07-10 18:43 UTC (permalink / raw)
To: Gleb Fotengauer-Malinovskiy, Paolo Bonzini,
Radim Krčmář, Claudio Imbrenda
Cc: Dmitry V. Levin, kvm, linux-kernel
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.
> 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?
> ---
> include/uapi/linux/kvm.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c0b6dfe..ebd604c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
> /* Available with KVM_CAP_X86_SMM */
> #define KVM_SMI _IO(KVMIO, 0xb7)
> /* Available with KVM_CAP_S390_CMMA_MIGRATION */
> -#define KVM_S390_GET_CMMA_BITS _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
> +#define KVM_S390_GET_CMMA_BITS _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
> #define KVM_S390_SET_CMMA_BITS _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
>
> #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
2017-07-10 18:43 ` Christian Borntraeger
@ 2017-07-10 21:22 ` Gleb Fotengauer-Malinovskiy
2017-07-10 21:23 ` [PATCH] " Gleb Fotengauer-Malinovskiy
1 sibling, 0 replies; 6+ messages in thread
From: Gleb Fotengauer-Malinovskiy @ 2017-07-10 21:22 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Paolo Bonzini, Radim Krčmář, Claudio Imbrenda,
Dmitry V. Levin, kvm, linux-kernel
In case of KVM_S390_GET_CMMA_BITS, the kernel does not only read struct
kvm_s390_cmma_log passed from userspace (which constitutes _IOC_WRITE),
it also writes back a return value (which constitutes _IOC_READ) making
this an _IOWR ioctl instead of _IOW.
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>
---
include/uapi/linux/kvm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c0b6dfe..ebd604c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
/* Available with KVM_CAP_X86_SMM */
#define KVM_SMI _IO(KVMIO, 0xb7)
/* Available with KVM_CAP_S390_CMMA_MIGRATION */
-#define KVM_S390_GET_CMMA_BITS _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
+#define KVM_S390_GET_CMMA_BITS _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
#define KVM_S390_SET_CMMA_BITS _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
--
glebfm
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
2017-07-10 18:43 ` Christian Borntraeger
2017-07-10 21:22 ` [PATCH v2] " Gleb Fotengauer-Malinovskiy
@ 2017-07-10 21:23 ` Gleb Fotengauer-Malinovskiy
2017-07-11 8:21 ` Christian Borntraeger
1 sibling, 1 reply; 6+ messages in thread
From: Gleb Fotengauer-Malinovskiy @ 2017-07-10 21:23 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Paolo Bonzini, Radim Krčmář, Claudio Imbrenda,
Dmitry V. Levin, kvm, linux-kernel
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.
--
glebfm
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
2017-07-10 21:23 ` [PATCH] " Gleb Fotengauer-Malinovskiy
@ 2017-07-11 8:21 ` Christian Borntraeger
2017-07-11 14:21 ` Radim Krčmář
0 siblings, 1 reply; 6+ messages in thread
From: Christian Borntraeger @ 2017-07-11 8:21 UTC (permalink / raw)
To: Gleb Fotengauer-Malinovskiy
Cc: Paolo Bonzini, Radim Krčmář, Claudio Imbrenda,
Dmitry V. Levin, kvm, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
2017-07-11 8:21 ` Christian Borntraeger
@ 2017-07-11 14:21 ` Radim Krčmář
0 siblings, 0 replies; 6+ messages in thread
From: Radim Krčmář @ 2017-07-11 14:21 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Gleb Fotengauer-Malinovskiy, Paolo Bonzini, Claudio Imbrenda,
Dmitry V. Levin, kvm, linux-kernel
2017-07-11 10:21+0200, 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@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.
Having correct hints would allow us to have one common
copy_from_user/copy_to_user and I think it's not too late to rename it
with the real behavior. Applied for the second merge-window pull
request,
thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-11 14:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-10 14:44 [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition Gleb Fotengauer-Malinovskiy
2017-07-10 18:43 ` Christian Borntraeger
2017-07-10 21:22 ` [PATCH v2] " Gleb Fotengauer-Malinovskiy
2017-07-10 21:23 ` [PATCH] " Gleb Fotengauer-Malinovskiy
2017-07-11 8:21 ` Christian Borntraeger
2017-07-11 14:21 ` Radim Krčmář
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox