* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
@ 2010-04-20 11:15 ` Alexander Graf
2010-04-20 11:33 ` Alexander Graf
` (21 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-20 11:15 UTC (permalink / raw)
To: kvm-ia64
On 20.04.2010, at 13:03, Takuya Yoshikawa wrote:
> We can now export the addree of the bitmap created by do_mmap()
> to user space. For the sake of this, we introduce a new API:
>
> KVM_SWITCH_DIRTY_LOG: application can use this to trigger the
> switch of the bitmaps and get the address of the bitmap which
> has been used until now. This reduces the copy of the dirty
> bitmap from the kernel.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Cc: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> ---
> Documentation/kvm/api.txt | 23 +++++++++++++++++++++++
> arch/ia64/kvm/kvm-ia64.c | 19 ++++++++++++++-----
> arch/powerpc/kvm/book3s.c | 19 ++++++++++++++-----
> arch/x86/kvm/x86.c | 32 ++++++++++++++++++++++++++------
> include/linux/kvm.h | 6 ++++--
> include/linux/kvm_host.h | 7 ++++---
> virt/kvm/kvm_main.c | 41 ++++++++++++++++++++++++++++++++++++-----
> 7 files changed, 121 insertions(+), 26 deletions(-)
>
[...]
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 23ea022..9fa6f1e 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -12,7 +12,7 @@
> #include <linux/ioctl.h>
> #include <asm/kvm.h>
>
> -#define KVM_API_VERSION 12
> +#define KVM_API_VERSION 13
Is there a way to keep both interfaces around for some time at least? I'd prefer the API version not to change if not _really_ necessary.
To enable the new dirty mapping you could for example use KVM_CAP_ENABLE_CAP :-).
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
2010-04-20 11:15 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-20 11:33 ` Alexander Graf
2010-04-20 11:33 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (20 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-20 11:33 UTC (permalink / raw)
To: kvm-ia64
On 20.04.2010, at 13:33, Takuya Yoshikawa wrote:
> (2010/04/20 20:15), Alexander Graf wrote:
>>
>> On 20.04.2010, at 13:03, Takuya Yoshikawa wrote:
>>
>>> We can now export the addree of the bitmap created by do_mmap()
>>> to user space. For the sake of this, we introduce a new API:
>>>
>>> KVM_SWITCH_DIRTY_LOG: application can use this to trigger the
>>> switch of the bitmaps and get the address of the bitmap which
>>> has been used until now. This reduces the copy of the dirty
>>> bitmap from the kernel.
>>>
>>> Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>>> Cc: Fernando Luis Vazquez Cao<fernando@oss.ntt.co.jp>
>>> ---
>>> Documentation/kvm/api.txt | 23 +++++++++++++++++++++++
>>> arch/ia64/kvm/kvm-ia64.c | 19 ++++++++++++++-----
>>> arch/powerpc/kvm/book3s.c | 19 ++++++++++++++-----
>>> arch/x86/kvm/x86.c | 32 ++++++++++++++++++++++++++------
>>> include/linux/kvm.h | 6 ++++--
>>> include/linux/kvm_host.h | 7 ++++---
>>> virt/kvm/kvm_main.c | 41 ++++++++++++++++++++++++++++++++++++-----
>>> 7 files changed, 121 insertions(+), 26 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
>>> index 23ea022..9fa6f1e 100644
>>> --- a/include/linux/kvm.h
>>> +++ b/include/linux/kvm.h
>>> @@ -12,7 +12,7 @@
>>> #include<linux/ioctl.h>
>>> #include<asm/kvm.h>
>>>
>>> -#define KVM_API_VERSION 12
>>> +#define KVM_API_VERSION 13
>>
>> Is there a way to keep both interfaces around for some time at least? I'd prefer the API version not to change if not _really_ necessary.
>>
>> To enable the new dirty mapping you could for example use KVM_CAP_ENABLE_CAP :-).
>
> Thanks, I did not know what is the appropriate way for this kind of change.
>
> I just read the comments in the kvm.h and thought I had to update the number whenever
> I added some entries to kvm.h .
The rule of thumb is:
KVM_API_VERSION change -> complete breakage. No way to run older userspace on newer kernels.
new CAP -> optional feature
If I read correctly you're changing quite a few existing interfaces, so old userspace wouldn't work anymore. Hence I'm proposing using activating the new way of dirty logging optionally.
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
2010-04-20 11:15 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
2010-04-20 11:33 ` Alexander Graf
@ 2010-04-20 11:33 ` Takuya Yoshikawa
2010-04-20 11:44 ` Takuya Yoshikawa
` (19 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Takuya Yoshikawa @ 2010-04-20 11:33 UTC (permalink / raw)
To: kvm-ia64
(2010/04/20 20:15), Alexander Graf wrote:
>
> On 20.04.2010, at 13:03, Takuya Yoshikawa wrote:
>
>> We can now export the addree of the bitmap created by do_mmap()
>> to user space. For the sake of this, we introduce a new API:
>>
>> KVM_SWITCH_DIRTY_LOG: application can use this to trigger the
>> switch of the bitmaps and get the address of the bitmap which
>> has been used until now. This reduces the copy of the dirty
>> bitmap from the kernel.
>>
>> Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>> Cc: Fernando Luis Vazquez Cao<fernando@oss.ntt.co.jp>
>> ---
>> Documentation/kvm/api.txt | 23 +++++++++++++++++++++++
>> arch/ia64/kvm/kvm-ia64.c | 19 ++++++++++++++-----
>> arch/powerpc/kvm/book3s.c | 19 ++++++++++++++-----
>> arch/x86/kvm/x86.c | 32 ++++++++++++++++++++++++++------
>> include/linux/kvm.h | 6 ++++--
>> include/linux/kvm_host.h | 7 ++++---
>> virt/kvm/kvm_main.c | 41 ++++++++++++++++++++++++++++++++++++-----
>> 7 files changed, 121 insertions(+), 26 deletions(-)
>>
>
> [...]
>
>> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
>> index 23ea022..9fa6f1e 100644
>> --- a/include/linux/kvm.h
>> +++ b/include/linux/kvm.h
>> @@ -12,7 +12,7 @@
>> #include<linux/ioctl.h>
>> #include<asm/kvm.h>
>>
>> -#define KVM_API_VERSION 12
>> +#define KVM_API_VERSION 13
>
> Is there a way to keep both interfaces around for some time at least? I'd prefer the API version not to change if not _really_ necessary.
>
> To enable the new dirty mapping you could for example use KVM_CAP_ENABLE_CAP :-).
Thanks, I did not know what is the appropriate way for this kind of change.
I just read the comments in the kvm.h and thought I had to update the number whenever
I added some entries to kvm.h .
>
>
> Alex
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (2 preceding siblings ...)
2010-04-20 11:33 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
@ 2010-04-20 11:44 ` Takuya Yoshikawa
2010-04-21 8:29 `
` (18 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Takuya Yoshikawa @ 2010-04-20 11:44 UTC (permalink / raw)
To: kvm-ia64
(2010/04/20 20:33), Alexander Graf wrote:
>>>>
>>>> -#define KVM_API_VERSION 12
>>>> +#define KVM_API_VERSION 13
>>>
>>> Is there a way to keep both interfaces around for some time at least? I'd prefer the API version not to change if not _really_ necessary.
>>>
>>> To enable the new dirty mapping you could for example use KVM_CAP_ENABLE_CAP :-).
>>
>> Thanks, I did not know what is the appropriate way for this kind of change.
>>
>> I just read the comments in the kvm.h and thought I had to update the number whenever
>> I added some entries to kvm.h .
>
> The rule of thumb is:
>
> KVM_API_VERSION change -> complete breakage. No way to run older userspace on newer kernels.
> new CAP -> optional feature
>
> If I read correctly you're changing quite a few existing interfaces, so old userspace wouldn't work anymore. Hence I'm proposing using activating the new way of dirty logging optionally.
>
> Alex
>
The fact is, I am trying to keep the existing interfaces completely(KVM_API_VERSION part is my mistake).
For x86, I have checked that current qemu works without any change.
I mean we can use get_dirty_log() as is.
And as you propose, we can determine which api(swith_* or get_*) we use optionally.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (3 preceding siblings ...)
2010-04-20 11:44 ` Takuya Yoshikawa
@ 2010-04-21 8:29 `
2010-04-21 9:41 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
` (17 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: @ 2010-04-21 8:29 UTC (permalink / raw)
To: kvm-ia64
On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
> __u32 padding1;
> union {
> void __user *dirty_bitmap; /* one bit per page */
> - __u64 padding2;
> + __u64 addr;
This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
> + case KVM_SWITCH_DIRTY_LOG: {
> + struct kvm_dirty_log log;
> +
> + r = -EFAULT;
> + if (copy_from_user(&log, argp, sizeof log))
> + goto out;
> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
> + if (r)
> + goto out;
> + r = -EFAULT;
> + if (copy_to_user(argp, &log, sizeof log))
> + goto out;
> + r = 0;
> + break;
> + }
In x86_64-compat mode we are handling 32bit user-space addresses
so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (4 preceding siblings ...)
2010-04-21 8:29 `
@ 2010-04-21 9:41 ` Alexander Graf
2010-04-21 11:46 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
` (16 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-21 9:41 UTC (permalink / raw)
To: kvm-ia64
On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>> __u32 padding1;
>> union {
>> void __user *dirty_bitmap; /* one bit per page */
>> - __u64 padding2;
>> + __u64 addr;
>
> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
So the high 32 bits are zero. Where's the problem?
>
>
>> + case KVM_SWITCH_DIRTY_LOG: {
>> + struct kvm_dirty_log log;
>> +
>> + r = -EFAULT;
>> + if (copy_from_user(&log, argp, sizeof log))
>> + goto out;
>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>> + if (r)
>> + goto out;
>> + r = -EFAULT;
>> + if (copy_to_user(argp, &log, sizeof log))
>> + goto out;
>> + r = 0;
>> + break;
>> + }
>
> In x86_64-compat mode we are handling 32bit user-space addresses
> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
The compat code just forwards everything to the generic ioctls.
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (5 preceding siblings ...)
2010-04-21 9:41 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-21 11:46 ` Avi Kivity
2010-04-22 2:45 `
` (15 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-21 11:46 UTC (permalink / raw)
To: kvm-ia64
On 04/20/2010 02:03 PM, Takuya Yoshikawa wrote:
> We can now export the addree of the bitmap created by do_mmap()
> to user space. For the sake of this, we introduce a new API:
>
> KVM_SWITCH_DIRTY_LOG: application can use this to trigger the
> switch of the bitmaps and get the address of the bitmap which
> has been used until now. This reduces the copy of the dirty
> bitmap from the kernel.
>
>
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index baa8fde..f3d1c2a 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -848,6 +848,29 @@ function properly, this is the place to put them.
> __u8 pad[64];
> };
>
> +4.37 KVM_SWITCH_DIRTY_LOG (vm ioctl)
> +
> +Capability: basic
>
Capability: basic means that this is available on all kvm versions,
which isn't the case. This should say KVM_CAP_USER_DIRTY_BITMAP.
> +Architectures: x86
>
All architecures...
> +Type: vm ioctl
> +Parameters: struct kvm_dirty_log (in/out)
> +Returns: 0 on success, -1 on error
> +
> +/* for KVM_SWITCH_DIRTY_LOG */
> +struct kvm_dirty_log {
> + __u32 slot;
> + __u32 padding;
>
Please put a flags field here (and verify it is zero in the ioctl
implementation). This allows us to extend it later.
> + union {
> + void __user *dirty_bitmap; /* one bit per page */
> + __u64 addr;
> + };
>
Just make dirty_bitmap a __u64. With the union there is the risk that
someone forgets to zero the structure so we get some random bits in the
pointer, and also issues with big endian and s390 compat.
Reserve some extra space here for future expansion.
Hm. I see that this is the existing kvm_dirty_log structure. So we
can't play with it, ignore my comments about it.
> +};
> +
> +Given a memory slot, return the address of a bitmap containing any
> +pages dirtied since the last call to this ioctl. Bit 0 is the first
> +page in the memory slot. Ensure the entire structure is cleared to
> +avoid padding issues.
>
Document that bitmaps but be aligned and padded to 64-bits to avoid
32-on-64 issues. Explain that dirty_bitmap will be the next returned
(do we actually need the return value? userspace can simply remember it
from the last call).
How is the dirty log set when we start logging? With kernel allocated
bitmaps this isn't a problem, but with user allocated bitmaps?
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ad55353..45b728c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2718,11 +2718,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
> return 0;
> }
>
> -/*
> - * Get (and clear) the dirty memory log for a memory slot.
> - */
> -int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> - struct kvm_dirty_log *log)
> +static int kvm_x86_update_dirty_log(struct kvm *kvm,
> + struct kvm_dirty_log *log,
> + bool need_copy)
> {
> int r;
> struct kvm_memory_slot *memslot;
> @@ -2773,12 +2771,34 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> dirty_bitmap_old = dirty_bitmap;
> }
>
> - r = kvm_copy_dirty_bitmap(log->dirty_bitmap, dirty_bitmap_old, n);
> + if (need_copy) {
> + r = kvm_copy_dirty_bitmap(log->dirty_bitmap,
> + dirty_bitmap_old, n);
> + } else {
> + log->addr = (unsigned long)dirty_bitmap_old;
> + r = 0;
> + }
>
Instead of passing need_copy everywhere, you might check a flag in
log->. You'll need a flag to know whether to munmap() or not, no?
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 23ea022..9fa6f1e 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -12,7 +12,7 @@
> #include<linux/ioctl.h>
> #include<asm/kvm.h>
>
> -#define KVM_API_VERSION 12
> +#define KVM_API_VERSION 13
>
As Alex says, this breaks all known userspace.
>
> /* *** Deprecated interfaces *** */
>
> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
> __u32 padding1;
> union {
> void __user *dirty_bitmap; /* one bit per page */
> - __u64 padding2;
> + __u64 addr;
> };
> };
>
Update the comment above to note it applies to KVM_SWITCH_DIRTY_LOG.
> @@ -1699,6 +1714,21 @@ static long kvm_vm_ioctl(struct file *filp,
> goto out;
> break;
> }
> + case KVM_SWITCH_DIRTY_LOG: {
> + struct kvm_dirty_log log;
> +
> + r = -EFAULT;
> + if (copy_from_user(&log, argp, sizeof log))
> + goto out;
> + r = kvm_vm_ioctl_switch_dirty_log(kvm,&log);
> + if (r)
> + goto out;
> + r = -EFAULT;
> + if (copy_to_user(argp,&log, sizeof log))
> + goto out;
> + r = 0;
>
You return 0, contrary to the documentation...
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (6 preceding siblings ...)
2010-04-21 11:46 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
@ 2010-04-22 2:45 `
2010-04-22 6:09 `
` (14 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: @ 2010-04-22 2:45 UTC (permalink / raw)
To: kvm-ia64
On 04/21/2010 06:41 PM, Alexander Graf wrote:
> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>
>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>> __u32 padding1;
>>> union {
>>> void __user *dirty_bitmap; /* one bit per page */
>>> - __u64 padding2;
>>> + __u64 addr;
>>
>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>
> So the high 32 bits are zero. Where's the problem?
If we are careful enough to cast the addr appropriately we should be fine,
even if we keep the padding field in the union. I am not saying that it
breaks 32 architectures but that it can potentially be problematic.
>>> + case KVM_SWITCH_DIRTY_LOG: {
>>> + struct kvm_dirty_log log;
>>> +
>>> + r = -EFAULT;
>>> + if (copy_from_user(&log, argp, sizeof log))
>>> + goto out;
>>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>> + if (r)
>>> + goto out;
>>> + r = -EFAULT;
>>> + if (copy_to_user(argp, &log, sizeof log))
>>> + goto out;
>>> + r = 0;
>>> + break;
>>> + }
>>
>> In x86_64-compat mode we are handling 32bit user-space addresses
>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>
> The compat code just forwards everything to the generic ioctls.
The compat code uses struct compat_kvm_dirty_log instead of
struct kvm_dirty_log to communicate with user space so
the necessary conversions needs to be done before invoking
the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
By the way we probable should move the definition of struct
compat_kvm_dirty_log to a header file.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (7 preceding siblings ...)
2010-04-22 2:45 `
@ 2010-04-22 6:09 `
2010-04-22 9:34 ` Takuya Yoshikawa
` (13 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: @ 2010-04-22 6:09 UTC (permalink / raw)
To: kvm-ia64
On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>
>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>> __u32 padding1;
>>>> union {
>>>> void __user *dirty_bitmap; /* one bit per page */
>>>> - __u64 padding2;
>>>> + __u64 addr;
>>>
>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>
>> So the high 32 bits are zero. Where's the problem?
>
> If we are careful enough to cast the addr appropriately we should be fine,
> even if we keep the padding field in the union. I am not saying that it
> breaks 32 architectures but that it can potentially be problematic.
>
>>>> + case KVM_SWITCH_DIRTY_LOG: {
>>>> + struct kvm_dirty_log log;
>>>> +
>>>> + r = -EFAULT;
>>>> + if (copy_from_user(&log, argp, sizeof log))
>>>> + goto out;
>>>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>> + if (r)
>>>> + goto out;
>>>> + r = -EFAULT;
>>>> + if (copy_to_user(argp, &log, sizeof log))
>>>> + goto out;
>>>> + r = 0;
>>>> + break;
>>>> + }
>>>
>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>
>> The compat code just forwards everything to the generic ioctls.
>
> The compat code uses struct compat_kvm_dirty_log instead of
> struct kvm_dirty_log to communicate with user space so
> the necessary conversions needs to be done before invoking
> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
>
> By the way we probable should move the definition of struct
> compat_kvm_dirty_log to a header file.
It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
Are you considering a different approach to tackle the issues that we
have with a big-endian userspace?
Thanks,
Fernando
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (8 preceding siblings ...)
2010-04-22 6:09 `
@ 2010-04-22 9:34 ` Takuya Yoshikawa
2010-04-22 23:29 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
` (12 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Takuya Yoshikawa @ 2010-04-22 9:34 UTC (permalink / raw)
To: kvm-ia64
Thanks, I can know basic rules about kvm/api .
(2010/04/21 20:46), Avi Kivity wrote:
>
>> +Type: vm ioctl
>> +Parameters: struct kvm_dirty_log (in/out)
>> +Returns: 0 on success, -1 on error
>> +
>> +/* for KVM_SWITCH_DIRTY_LOG */
>> +struct kvm_dirty_log {
>> + __u32 slot;
>> + __u32 padding;
>
> Please put a flags field here (and verify it is zero in the ioctl
> implementation). This allows us to extend it later.
>
>> + union {
>> + void __user *dirty_bitmap; /* one bit per page */
>> + __u64 addr;
>> + };
>
> Just make dirty_bitmap a __u64. With the union there is the risk that
> someone forgets to zero the structure so we get some random bits in the
> pointer, and also issues with big endian and s390 compat.
>
> Reserve some extra space here for future expansion.
>
> Hm. I see that this is the existing kvm_dirty_log structure. So we can't
> play with it, ignore my comments about it.
So, introducing a new structure to export(and get) two bitmap addresses
with a flag is the thing?
1. Qemu calls ioctl to get the two addresses.
2. Qemu calls ioctl to make KVM switch the dirty bitmaps.
--> in this case, qemu have to change the address got in step 1.
...
3. Qemu calls ioctl(we can reuse 1. with different command flag) to free the bitmaps.
In my plan, we don't let qemu malloc() dirty bitmaps: at least, I want to make that
another patch set because this patch set already has some dependencies to other issues.
But, yes, I can make the struct considering the future expansion if it needed.
>>
>> - r = kvm_copy_dirty_bitmap(log->dirty_bitmap, dirty_bitmap_old, n);
>> + if (need_copy) {
>> + r = kvm_copy_dirty_bitmap(log->dirty_bitmap,
>> + dirty_bitmap_old, n);
>> + } else {
>> + log->addr = (unsigned long)dirty_bitmap_old;
>> + r = 0;
>> + }
>
> Instead of passing need_copy everywhere, you might check a flag in
> log->. You'll need a flag to know whether to munmap() or not, no?
To judge munmap() or not, putting in the memory slot's flag may be
more useful.
But as you suggest, we won't use kvm_dirty_log so parameters will change.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (9 preceding siblings ...)
2010-04-22 9:34 ` Takuya Yoshikawa
@ 2010-04-22 23:29 ` Alexander Graf
2010-04-23 10:17 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
` (11 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-22 23:29 UTC (permalink / raw)
To: kvm-ia64
On 22.04.2010, at 08:09, Fernando Luis Vázquez Cao wrote:
> On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
>> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>>
>>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>>> __u32 padding1;
>>>>> union {
>>>>> void __user *dirty_bitmap; /* one bit per page */
>>>>> - __u64 padding2;
>>>>> + __u64 addr;
>>>>
>>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>>
>>> So the high 32 bits are zero. Where's the problem?
>>
>> If we are careful enough to cast the addr appropriately we should be fine,
>> even if we keep the padding field in the union. I am not saying that it
>> breaks 32 architectures but that it can potentially be problematic.
>>
>>>>> + case KVM_SWITCH_DIRTY_LOG: {
>>>>> + struct kvm_dirty_log log;
>>>>> +
>>>>> + r = -EFAULT;
>>>>> + if (copy_from_user(&log, argp, sizeof log))
>>>>> + goto out;
>>>>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>>> + if (r)
>>>>> + goto out;
>>>>> + r = -EFAULT;
>>>>> + if (copy_to_user(argp, &log, sizeof log))
>>>>> + goto out;
>>>>> + r = 0;
>>>>> + break;
>>>>> + }
>>>>
>>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>>
>>> The compat code just forwards everything to the generic ioctls.
>>
>> The compat code uses struct compat_kvm_dirty_log instead of
>> struct kvm_dirty_log to communicate with user space so
>> the necessary conversions needs to be done before invoking
>> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
>>
>> By the way we probable should move the definition of struct
>> compat_kvm_dirty_log to a header file.
>
> It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
> Are you considering a different approach to tackle the issues that we
> have with a big-endian userspace?
IIRC the issue was a pointer inside of a nested structure, no?
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (10 preceding siblings ...)
2010-04-22 23:29 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-23 10:17 `
2010-04-23 10:20 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
` (10 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: @ 2010-04-23 10:17 UTC (permalink / raw)
To: kvm-ia64
On 04/23/2010 08:29 AM, Alexander Graf wrote:
>
> On 22.04.2010, at 08:09, Fernando Luis Vázquez Cao wrote:
>
>> On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
>>> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>>>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>>>
>>>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>>>> __u32 padding1;
>>>>>> union {
>>>>>> void __user *dirty_bitmap; /* one bit per page */
>>>>>> - __u64 padding2;
>>>>>> + __u64 addr;
>>>>>
>>>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>>>
>>>> So the high 32 bits are zero. Where's the problem?
>>>
>>> If we are careful enough to cast the addr appropriately we should be fine,
>>> even if we keep the padding field in the union. I am not saying that it
>>> breaks 32 architectures but that it can potentially be problematic.
>>>
>>>>>> + case KVM_SWITCH_DIRTY_LOG: {
>>>>>> + struct kvm_dirty_log log;
>>>>>> +
>>>>>> + r = -EFAULT;
>>>>>> + if (copy_from_user(&log, argp, sizeof log))
>>>>>> + goto out;
>>>>>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>>>> + if (r)
>>>>>> + goto out;
>>>>>> + r = -EFAULT;
>>>>>> + if (copy_to_user(argp, &log, sizeof log))
>>>>>> + goto out;
>>>>>> + r = 0;
>>>>>> + break;
>>>>>> + }
>>>>>
>>>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>>>
>>>> The compat code just forwards everything to the generic ioctls.
>>>
>>> The compat code uses struct compat_kvm_dirty_log instead of
>>> struct kvm_dirty_log to communicate with user space so
>>> the necessary conversions needs to be done before invoking
>>> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
>>>
>>> By the way we probable should move the definition of struct
>>> compat_kvm_dirty_log to a header file.
>>
>> It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
>> Are you considering a different approach to tackle the issues that we
>> have with a big-endian userspace?
>
> IIRC the issue was a pointer inside of a nested structure, no?
I would say the reason is that if we did not convert the user-space pointer to
a "void *" kvm_get_dirty_log() would end up copying the dirty log to
(log->dirty_bitmap << 32) | 0x00000000
Am I missing something?
Thanks,
Fernando
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (11 preceding siblings ...)
2010-04-23 10:17 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
@ 2010-04-23 10:20 ` Alexander Graf
2010-04-23 11:57 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
` (9 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-23 10:20 UTC (permalink / raw)
To: kvm-ia64
On 23.04.2010, at 12:17, Fernando Luis Vázquez Cao wrote:
> On 04/23/2010 08:29 AM, Alexander Graf wrote:
>>
>> On 22.04.2010, at 08:09, Fernando Luis Vázquez Cao wrote:
>>
>>> On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
>>>> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>>>>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>>>>
>>>>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>>>>> __u32 padding1;
>>>>>>> union {
>>>>>>> void __user *dirty_bitmap; /* one bit per page */
>>>>>>> - __u64 padding2;
>>>>>>> + __u64 addr;
>>>>>>
>>>>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>>>>
>>>>> So the high 32 bits are zero. Where's the problem?
>>>>
>>>> If we are careful enough to cast the addr appropriately we should be fine,
>>>> even if we keep the padding field in the union. I am not saying that it
>>>> breaks 32 architectures but that it can potentially be problematic.
>>>>
>>>>>>> + case KVM_SWITCH_DIRTY_LOG: {
>>>>>>> + struct kvm_dirty_log log;
>>>>>>> +
>>>>>>> + r = -EFAULT;
>>>>>>> + if (copy_from_user(&log, argp, sizeof log))
>>>>>>> + goto out;
>>>>>>> + r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>>>>> + if (r)
>>>>>>> + goto out;
>>>>>>> + r = -EFAULT;
>>>>>>> + if (copy_to_user(argp, &log, sizeof log))
>>>>>>> + goto out;
>>>>>>> + r = 0;
>>>>>>> + break;
>>>>>>> + }
>>>>>>
>>>>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>>>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>>>>
>>>>> The compat code just forwards everything to the generic ioctls.
>>>>
>>>> The compat code uses struct compat_kvm_dirty_log instead of
>>>> struct kvm_dirty_log to communicate with user space so
>>>> the necessary conversions needs to be done before invoking
>>>> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
>>>>
>>>> By the way we probable should move the definition of struct
>>>> compat_kvm_dirty_log to a header file.
>>>
>>> It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
>>> Are you considering a different approach to tackle the issues that we
>>> have with a big-endian userspace?
>>
>> IIRC the issue was a pointer inside of a nested structure, no?
>
> I would say the reason is that if we did not convert the user-space pointer to
> a "void *" kvm_get_dirty_log() would end up copying the dirty log to
>
> (log->dirty_bitmap << 32) | 0x00000000
Well yes, that was the problem. If we always set the __u64 value to the pointer we're safe though.
union {
void *p;
__u64 q;
}
void x(void *r)
{
// breaks:
p = r;
// works:
q = (ulong)r;
}
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (12 preceding siblings ...)
2010-04-23 10:20 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-23 11:57 ` Avi Kivity
2010-04-23 11:58 ` Avi Kivity
` (8 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-23 11:57 UTC (permalink / raw)
To: kvm-ia64
On 04/23/2010 01:20 PM, Alexander Graf wrote:
>
>> I would say the reason is that if we did not convert the user-space pointer to
>> a "void *" kvm_get_dirty_log() would end up copying the dirty log to
>>
>> (log->dirty_bitmap<< 32) | 0x00000000
>>
> Well yes, that was the problem. If we always set the __u64 value to the pointer we're safe though.
>
> union {
> void *p;
> __u64 q;
> }
>
> void x(void *r)
> {
> // breaks:
> p = r;
>
> // works:
> q = (ulong)r;
> }
>
In that case it's better to avoid p altogether, since users will
naturally assign to the pointer.
Using a 64-bit integer avoids the problem (though perhaps not sufficient
for s390, Arnd?)
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (13 preceding siblings ...)
2010-04-23 11:57 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
@ 2010-04-23 11:58 ` Avi Kivity
2010-04-23 12:26 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
` (7 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-23 11:58 UTC (permalink / raw)
To: kvm-ia64
On 04/22/2010 12:34 PM, Takuya Yoshikawa wrote:
> Thanks, I can know basic rules about kvm/api .
>
> (2010/04/21 20:46), Avi Kivity wrote:
>
>>
>>> +Type: vm ioctl
>>> +Parameters: struct kvm_dirty_log (in/out)
>>> +Returns: 0 on success, -1 on error
>>> +
>>> +/* for KVM_SWITCH_DIRTY_LOG */
>>> +struct kvm_dirty_log {
>>> + __u32 slot;
>>> + __u32 padding;
>>
>> Please put a flags field here (and verify it is zero in the ioctl
>> implementation). This allows us to extend it later.
>>
>>> + union {
>>> + void __user *dirty_bitmap; /* one bit per page */
>>> + __u64 addr;
>>> + };
>>
>> Just make dirty_bitmap a __u64. With the union there is the risk that
>> someone forgets to zero the structure so we get some random bits in the
>> pointer, and also issues with big endian and s390 compat.
>>
>> Reserve some extra space here for future expansion.
>>
>> Hm. I see that this is the existing kvm_dirty_log structure. So we can't
>> play with it, ignore my comments about it.
>
> So, introducing a new structure to export(and get) two bitmap addresses
> with a flag is the thing?
>
> 1. Qemu calls ioctl to get the two addresses.
> 2. Qemu calls ioctl to make KVM switch the dirty bitmaps.
> --> in this case, qemu have to change the address got in step 1.
> ...
> 3. Qemu calls ioctl(we can reuse 1. with different command flag) to
> free the bitmaps.
>
>
> In my plan, we don't let qemu malloc() dirty bitmaps: at least, I want
> to make that
> another patch set because this patch set already has some dependencies
> to other issues.
>
> But, yes, I can make the struct considering the future expansion if it
> needed.
I guess it's better, since this patch is a "future expansion" of
KVN_GET_DIRTY_LOG. If we had a flags field and some room there, we
could keep the old ioctl.
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (14 preceding siblings ...)
2010-04-23 11:58 ` Avi Kivity
@ 2010-04-23 12:26 ` Alexander Graf
2010-04-23 12:27 ` Arnd Bergmann
` (6 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-23 12:26 UTC (permalink / raw)
To: kvm-ia64
On 23.04.2010, at 13:57, Avi Kivity wrote:
> On 04/23/2010 01:20 PM, Alexander Graf wrote:
>>
>>> I would say the reason is that if we did not convert the user-space pointer to
>>> a "void *" kvm_get_dirty_log() would end up copying the dirty log to
>>>
>>> (log->dirty_bitmap<< 32) | 0x00000000
>>>
>> Well yes, that was the problem. If we always set the __u64 value to the pointer we're safe though.
>>
>> union {
>> void *p;
>> __u64 q;
>> }
>>
>> void x(void *r)
>> {
>> // breaks:
>> p = r;
>>
>> // works:
>> q = (ulong)r;
>> }
>>
>
> In that case it's better to avoid p altogether, since users will naturally assign to the pointer.
>
> Using a 64-bit integer avoids the problem (though perhaps not sufficient for s390, Arnd?)
We only support 64 bit on S390, so we should be safe here. Even if not, compat mode has 31 bits pointers, so padding them to 64 bit should work too.
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (15 preceding siblings ...)
2010-04-23 12:26 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-23 12:27 ` Arnd Bergmann
2010-04-23 12:42 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
` (5 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2010-04-23 12:27 UTC (permalink / raw)
To: kvm-ia64
On Friday 23 April 2010, Avi Kivity wrote:
> On 04/23/2010 01:20 PM, Alexander Graf wrote:
> >
> >> I would say the reason is that if we did not convert the user-space pointer to
> >> a "void *" kvm_get_dirty_log() would end up copying the dirty log to
> >>
> >> (log->dirty_bitmap<< 32) | 0x00000000
> >>
> > Well yes, that was the problem. If we always set the __u64 value to the pointer we're safe though.
> >
> > union {
> > void *p;
> > __u64 q;
> > }
> >
> > void x(void *r)
> > {
> > // breaks:
> > p = r;
> >
> > // works:
> > q = (ulong)r;
> > }
> >
>
> In that case it's better to avoid p altogether, since users will
> naturally assign to the pointer.
Right.
> Using a 64-bit integer avoids the problem (though perhaps not sufficient
> for s390, Arnd?)
When there is only a __u64 for the address, it will work on s390 as well,
gcc is smart enough to clear the upper bit on a cast from long to pointer.
The simple rule is to never put any 'long' or pointer into data structures
that you pass to an ioctl, and to add padding to multiples of 64 bit to
align the data structure for the x86 alignment problem.
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (16 preceding siblings ...)
2010-04-23 12:27 ` Arnd Bergmann
@ 2010-04-23 12:42 ` Avi Kivity
2010-04-23 12:46 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Arnd Bergmann
` (4 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-23 12:42 UTC (permalink / raw)
To: kvm-ia64
On 04/23/2010 03:27 PM, Arnd Bergmann wrote:
>
>> Using a 64-bit integer avoids the problem (though perhaps not sufficient
>> for s390, Arnd?)
>>
> When there is only a __u64 for the address, it will work on s390 as well,
> gcc is smart enough to clear the upper bit on a cast from long to pointer.
>
Ah, much more convenient than compat_ioctl. I assume it part of the
ABI, not a gccism?
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (17 preceding siblings ...)
2010-04-23 12:42 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
@ 2010-04-23 12:46 ` Arnd Bergmann
2010-04-23 12:53 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
` (3 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2010-04-23 12:46 UTC (permalink / raw)
To: kvm-ia64
On Friday 23 April 2010, Avi Kivity wrote:
> On 04/23/2010 03:27 PM, Arnd Bergmann wrote:
> >
> >> Using a 64-bit integer avoids the problem (though perhaps not sufficient
> >> for s390, Arnd?)
> >>
> > When there is only a __u64 for the address, it will work on s390 as well,
> > gcc is smart enough to clear the upper bit on a cast from long to pointer.
> >
>
> Ah, much more convenient than compat_ioctl. I assume it part of the
> ABI, not a gccism?
I don't think it's part of the ABI, but it's required to guarantee
that code like this works:
int compare_pointer(void *a, void *b)
{
unsigned long ai = (unsigned long)a, bi = (unsigned long)b;
return ai = bi; /* true if a and b point to the same object */
}
We certainly rely on this already.
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (18 preceding siblings ...)
2010-04-23 12:46 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Arnd Bergmann
@ 2010-04-23 12:53 ` Avi Kivity
2010-04-23 12:59 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
` (2 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-23 12:53 UTC (permalink / raw)
To: kvm-ia64
On 04/23/2010 03:46 PM, Arnd Bergmann wrote:
> On Friday 23 April 2010, Avi Kivity wrote:
>
>> On 04/23/2010 03:27 PM, Arnd Bergmann wrote:
>>
>>>
>>>> Using a 64-bit integer avoids the problem (though perhaps not sufficient
>>>> for s390, Arnd?)
>>>>
>>>>
>>> When there is only a __u64 for the address, it will work on s390 as well,
>>> gcc is smart enough to clear the upper bit on a cast from long to pointer.
>>>
>>>
>> Ah, much more convenient than compat_ioctl. I assume it part of the
>> ABI, not a gccism?
>>
> I don't think it's part of the ABI, but it's required to guarantee
> that code like this works:
>
> int compare_pointer(void *a, void *b)
> {
> unsigned long ai = (unsigned long)a, bi = (unsigned long)b;
>
> return ai = bi; /* true if a and b point to the same object */
> }
>
> We certainly rely on this already.
>
Ah so the 31st bit is optional as far as userspace is concerned? What
does it mean? (just curious)
What happens on the opposite conversion? is it restored?
What about
int compare_pointer(void *a, void *b)
{
unsigned long ai = (unsigned long)a;
void *aia = (void *)ai;
return a = b; /* true if a and b point to the same object */
}
Does gcc mask the big in pointer comparisons as well?
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (19 preceding siblings ...)
2010-04-23 12:53 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
@ 2010-04-23 12:59 ` Alexander Graf
2010-04-23 13:12 ` Arnd Bergmann
2010-04-23 13:20 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
22 siblings, 0 replies; 24+ messages in thread
From: Alexander Graf @ 2010-04-23 12:59 UTC (permalink / raw)
To: kvm-ia64
On 23.04.2010, at 14:53, Avi Kivity wrote:
> On 04/23/2010 03:46 PM, Arnd Bergmann wrote:
>> On Friday 23 April 2010, Avi Kivity wrote:
>>
>>> On 04/23/2010 03:27 PM, Arnd Bergmann wrote:
>>>
>>>>
>>>>> Using a 64-bit integer avoids the problem (though perhaps not sufficient
>>>>> for s390, Arnd?)
>>>>>
>>>>>
>>>> When there is only a __u64 for the address, it will work on s390 as well,
>>>> gcc is smart enough to clear the upper bit on a cast from long to pointer.
>>>>
>>>>
>>> Ah, much more convenient than compat_ioctl. I assume it part of the
>>> ABI, not a gccism?
>>>
>> I don't think it's part of the ABI, but it's required to guarantee
>> that code like this works:
>>
>> int compare_pointer(void *a, void *b)
>> {
>> unsigned long ai = (unsigned long)a, bi = (unsigned long)b;
>>
>> return ai = bi; /* true if a and b point to the same object */
>> }
>>
>> We certainly rely on this already.
>>
>
> Ah so the 31st bit is optional as far as userspace is concerned? What does it mean? (just curious)
The 0x80000000 bit declares that a pointer is in 24-bit mode, so that applications can use the spare upper bits for random data.
See http://en.wikipedia.org/wiki/31-bit for an explanation.
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (20 preceding siblings ...)
2010-04-23 12:59 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Alexander Graf
@ 2010-04-23 13:12 ` Arnd Bergmann
2010-04-23 13:20 ` [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Avi Kivity
22 siblings, 0 replies; 24+ messages in thread
From: Arnd Bergmann @ 2010-04-23 13:12 UTC (permalink / raw)
To: kvm-ia64
On Friday 23 April 2010, Avi Kivity wrote:
> Ah so the 31st bit is optional as far as userspace is concerned? What
> does it mean? (just curious)
On data pointers it's ignored. When you branch to a function, this bit
determines whether the target function is run in 24 or 31 bit mode.
This allows linking to legacy code on older operating systems that
also support 24 bit libraries.
> What happens on the opposite conversion? is it restored?
>
> What about
>
> int compare_pointer(void *a, void *b)
> {
> unsigned long ai = (unsigned long)a;
> void *aia = (void *)ai;
>
> return a = b; /* true if a and b point to the same object */
> }
Some instructions set the bit, others clear it, so aia and a may not
be bitwise identical.
> Does gcc mask the big in pointer comparisons as well?
Yes. To stay in the above example:
a = aia; /* true */
(unsigned long)a = (unsigned long)aia; /* true */
*(unsigned long *)&a = *(unsigned long *)&aia; /* undefined on s390 */
Arnd
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
2010-04-20 11:03 [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Takuya Yoshikawa
` (21 preceding siblings ...)
2010-04-23 13:12 ` Arnd Bergmann
@ 2010-04-23 13:20 ` Avi Kivity
22 siblings, 0 replies; 24+ messages in thread
From: Avi Kivity @ 2010-04-23 13:20 UTC (permalink / raw)
To: kvm-ia64
On 04/23/2010 03:59 PM, Alexander Graf wrote:
>
>> Ah so the 31st bit is optional as far as userspace is concerned? What does it mean? (just curious)
>>
> The 0x80000000 bit declares that a pointer is in 24-bit mode, so that applications can use the spare upper bits for random data.
>
> See http://en.wikipedia.org/wiki/31-bit for an explanation.
>
Interesting. Luckily AMD made the top 16 bits of pointers reserved in
x86-64.
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply [flat|nested] 24+ messages in thread