Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
> >> Acked-by: Christian Borntraeger
> >>
> >>
> >> 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.
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
> >> Acked-by: Christian Borntraeger
> >>
> >>
> >> 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.
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
>> Acked-by: Christian Borntraeger
>>
>>
>> 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.
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
>> Acked-by: Christian Borntraeger
>>
>>
>> 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.
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
> Acked-by: Christian Borntraeger
>
>
> 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
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
> Acked-by: Christian Borntraeger
>
>
> 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
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
Acked-by: Christian Borntraeger
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)
>
Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
Acked-by: Christian Borntraeger
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)
>
[PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
---
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
[PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition
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
---
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
