[Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[David Gibson]

From: Ben Herrenschmidt <benh@kernel.crashing.org>

If the kernel page size is larger than TARGET_PAGE_SIZE, which
happens for example on ppc64 with kernels compiled for 64K pages,
the dirty tracking doesn't work.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 kvm-all.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

I've sent this a number of times now, the last couple without comment.
It fixes a real bug, please apply.

diff --git a/kvm-all.c b/kvm-all.c
index ba2cee1..7e44429 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -354,6 +354,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
     unsigned long page_number, c;
     target_phys_addr_t addr, addr1;
     unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
+    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
 
     /*
      * bitmap-traveling is faster than memory-traveling (for addr...)
@@ -365,10 +366,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
             do {
                 j = ffsl(c) - 1;
                 c &= ~(1ul << j);
-                page_number = i * HOST_LONG_BITS + j;
+                page_number = (i * HOST_LONG_BITS + j) * hpratio;
                 addr1 = page_number * TARGET_PAGE_SIZE;
                 addr = section->offset_within_region + addr1;
-                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
+                memory_region_set_dirty(section->mr, addr,
+                                        TARGET_PAGE_SIZE * hpratio);
             } while (c != 0);
         }
     }
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Benjamin Herrenschmidt]

On Mon, 2012-04-02 at 14:04 +1000, David Gibson wrote:
> From: Ben Herrenschmidt <benh@kernel.crashing.org>
> 
> If the kernel page size is larger than TARGET_PAGE_SIZE, which
> happens for example on ppc64 with kernels compiled for 64K pages,
> the dirty tracking doesn't work.

BTW. That was discussed ages back on the list and we basically
agreed it was the right fix for now, but for some reason the patch
then went into a black hole :-)

Cheers,
Ben.

> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  kvm-all.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> I've sent this a number of times now, the last couple without comment.
> It fixes a real bug, please apply.
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index ba2cee1..7e44429 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -354,6 +354,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>      unsigned long page_number, c;
>      target_phys_addr_t addr, addr1;
>      unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
> +    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
>  
>      /*
>       * bitmap-traveling is faster than memory-traveling (for addr...)
> @@ -365,10 +366,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>              do {
>                  j = ffsl(c) - 1;
>                  c &= ~(1ul << j);
> -                page_number = i * HOST_LONG_BITS + j;
> +                page_number = (i * HOST_LONG_BITS + j) * hpratio;
>                  addr1 = page_number * TARGET_PAGE_SIZE;
>                  addr = section->offset_within_region + addr1;
> -                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
> +                memory_region_set_dirty(section->mr, addr,
> +                                        TARGET_PAGE_SIZE * hpratio);
>              } while (c != 0);
>          }
>      }

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

On 2012-04-02 06:04, David Gibson wrote:
> From: Ben Herrenschmidt <benh@kernel.crashing.org>
> 
> If the kernel page size is larger than TARGET_PAGE_SIZE, which
> happens for example on ppc64 with kernels compiled for 64K pages,
> the dirty tracking doesn't work.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  kvm-all.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> I've sent this a number of times now, the last couple without comment.
> It fixes a real bug, please apply.
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index ba2cee1..7e44429 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -354,6 +354,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>      unsigned long page_number, c;
>      target_phys_addr_t addr, addr1;
>      unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
> +    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
>  
>      /*
>       * bitmap-traveling is faster than memory-traveling (for addr...)
> @@ -365,10 +366,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>              do {
>                  j = ffsl(c) - 1;
>                  c &= ~(1ul << j);
> -                page_number = i * HOST_LONG_BITS + j;
> +                page_number = (i * HOST_LONG_BITS + j) * hpratio;
>                  addr1 = page_number * TARGET_PAGE_SIZE;
>                  addr = section->offset_within_region + addr1;
> -                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
> +                memory_region_set_dirty(section->mr, addr,
> +                                        TARGET_PAGE_SIZE * hpratio);
>              } while (c != 0);
>          }
>      }

Ack for this, but - as proposed earlier - please add an
assert(TARGET_PAGE_SIZE <= getpagesize()) + comment to kvm_init().

Also, what's about coalesced MMIO? I see that the ring definition
depends on [TARGET_]PAGE_SIZE. What page size does the power kernel use
for it, and does it make a relevant difference for space?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

On Tue, Apr 03, 2012 at 09:08:16AM +0200, Jan Kiszka wrote:
> On 2012-04-02 06:04, David Gibson wrote:
> > From: Ben Herrenschmidt <benh@kernel.crashing.org>
> > 
> > If the kernel page size is larger than TARGET_PAGE_SIZE, which
> > happens for example on ppc64 with kernels compiled for 64K pages,
> > the dirty tracking doesn't work.
> > 
> > Cc: Avi Kivity <avi@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  kvm-all.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > I've sent this a number of times now, the last couple without comment.
> > It fixes a real bug, please apply.
> > 
> > diff --git a/kvm-all.c b/kvm-all.c
> > index ba2cee1..7e44429 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -354,6 +354,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
> >      unsigned long page_number, c;
> >      target_phys_addr_t addr, addr1;
> >      unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
> > +    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
> >  
> >      /*
> >       * bitmap-traveling is faster than memory-traveling (for addr...)
> > @@ -365,10 +366,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
> >              do {
> >                  j = ffsl(c) - 1;
> >                  c &= ~(1ul << j);
> > -                page_number = i * HOST_LONG_BITS + j;
> > +                page_number = (i * HOST_LONG_BITS + j) * hpratio;
> >                  addr1 = page_number * TARGET_PAGE_SIZE;
> >                  addr = section->offset_within_region + addr1;
> > -                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
> > +                memory_region_set_dirty(section->mr, addr,
> > +                                        TARGET_PAGE_SIZE * hpratio);
> >              } while (c != 0);
> >          }
> >      }
> 
> Ack for this, but - as proposed earlier - please add an
> assert(TARGET_PAGE_SIZE <= getpagesize()) + comment to kvm_init().

Ok.

> Also, what's about coalesced MMIO? I see that the ring definition
> depends on [TARGET_]PAGE_SIZE. What page size does the power kernel use
> for it, and does it make a relevant difference for space?

Hr, so the HV variant of Power KVM doesn't do coalesced mmio.  The PR
variant does, but I don't know enough about it to easily answer that.
If there's a bug there, it hasn't bitten yet, so how about we fix that
another day.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Jan Kiszka]

On 2012-04-04 03:12, David Gibson wrote:
>> Also, what's about coalesced MMIO? I see that the ring definition
>> depends on [TARGET_]PAGE_SIZE. What page size does the power kernel use
>> for it, and does it make a relevant difference for space?
> 
> Hr, so the HV variant of Power KVM doesn't do coalesced mmio.  The PR
> variant does, but I don't know enough about it to easily answer that.
> If there's a bug there, it hasn't bitten yet, so how about we fix that
> another day.

Not really a smart approach, specifically now that we are aware of a
potential problem here.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[David Gibson]

On Wed, Apr 04, 2012 at 12:58:37PM +0200, Jan Kiszka wrote:
> On 2012-04-04 03:12, David Gibson wrote:
> >> Also, what's about coalesced MMIO? I see that the ring definition
> >> depends on [TARGET_]PAGE_SIZE. What page size does the power kernel use
> >> for it, and does it make a relevant difference for space?
> > 
> > Hr, so the HV variant of Power KVM doesn't do coalesced mmio.  The PR
> > variant does, but I don't know enough about it to easily answer that.
> > If there's a bug there, it hasn't bitten yet, so how about we fix that
> > another day.
> 
> Not really a smart approach, specifically now that we are aware of a
> potential problem here.

My point is that the fact that there may be another bug out there
should sure as hell not hold up putting this bug fix in.  The bugs may
have similar causes and solutions, but they're entirely different code
paths and the other fix will in no way replace or subsume this one.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Jan Kiszka]

On 2012-04-04 16:24, David Gibson wrote:
> On Wed, Apr 04, 2012 at 12:58:37PM +0200, Jan Kiszka wrote:
>> On 2012-04-04 03:12, David Gibson wrote:
>>>> Also, what's about coalesced MMIO? I see that the ring definition
>>>> depends on [TARGET_]PAGE_SIZE. What page size does the power kernel use
>>>> for it, and does it make a relevant difference for space?
>>>
>>> Hr, so the HV variant of Power KVM doesn't do coalesced mmio.  The PR
>>> variant does, but I don't know enough about it to easily answer that.
>>> If there's a bug there, it hasn't bitten yet, so how about we fix that
>>> another day.
>>
>> Not really a smart approach, specifically now that we are aware of a
>> potential problem here.
> 
> My point is that the fact that there may be another bug out there
> should sure as hell not hold up putting this bug fix in.  The bugs may
> have similar causes and solutions, but they're entirely different code
> paths and the other fix will in no way replace or subsume this one.

I don't disagree. Maybe I over-interpreted your reply as "let's wait if
this actually bites us".

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Avi Kivity]

On 05/10/2012 01:49 PM, Jan Kiszka wrote:
> On 2012-05-10 06:27, Avi Kivity wrote:
> > On 05/10/2012 01:23 AM, Anthony Liguori wrote:
> >> On 04/04/2012 03:41 AM, Avi Kivity wrote:
> >>> On 04/04/2012 04:15 AM, David Gibson wrote:
> >>>> If the kernel page size is larger than TARGET_PAGE_SIZE, which
> >>>> happens for example on ppc64 with kernels compiled for 64K pages,
> >>>> the dirty tracking doesn't work.
> >>>>
> >>>> Cc: Avi Kivity<avi@redhat.com>
> >>>> Cc: Marcelo Tosatti<mtosatti@redhat.com>
> >>>>
> >>>
> >>> Thanks, applied to uq/master.
> >>
> >> Can ya'll send a PULL request for 1.1 with any bug fixes from uq/master?
> > 
> > Sure, I'll trawl the list for missed patches (Jan, any known?) and let
> > autotest at it.
>
> Don't recall any ATM for which there wasn't a "thanks, applied".
>

Okay, so we have that one patch.  Since it's a nop for x86, which is the
only thing I test, I'll push it along.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

On 2012-05-10 06:27, Avi Kivity wrote:
> On 05/10/2012 01:23 AM, Anthony Liguori wrote:
>> On 04/04/2012 03:41 AM, Avi Kivity wrote:
>>> On 04/04/2012 04:15 AM, David Gibson wrote:
>>>> If the kernel page size is larger than TARGET_PAGE_SIZE, which
>>>> happens for example on ppc64 with kernels compiled for 64K pages,
>>>> the dirty tracking doesn't work.
>>>>
>>>> Cc: Avi Kivity<avi@redhat.com>
>>>> Cc: Marcelo Tosatti<mtosatti@redhat.com>
>>>>
>>>
>>> Thanks, applied to uq/master.
>>
>> Can ya'll send a PULL request for 1.1 with any bug fixes from uq/master?
> 
> Sure, I'll trawl the list for missed patches (Jan, any known?) and let
> autotest at it.

Don't recall any ATM for which there wasn't a "thanks, applied".

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Avi Kivity]

On 05/10/2012 01:23 AM, Anthony Liguori wrote:
> On 04/04/2012 03:41 AM, Avi Kivity wrote:
>> On 04/04/2012 04:15 AM, David Gibson wrote:
>>> If the kernel page size is larger than TARGET_PAGE_SIZE, which
>>> happens for example on ppc64 with kernels compiled for 64K pages,
>>> the dirty tracking doesn't work.
>>>
>>> Cc: Avi Kivity<avi@redhat.com>
>>> Cc: Marcelo Tosatti<mtosatti@redhat.com>
>>>
>>
>> Thanks, applied to uq/master.
>
> Can ya'll send a PULL request for 1.1 with any bug fixes from uq/master?

Sure, I'll trawl the list for missed patches (Jan, any known?) and let
autotest at it.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Anthony Liguori]

On 04/04/2012 03:41 AM, Avi Kivity wrote:
> On 04/04/2012 04:15 AM, David Gibson wrote:
>> If the kernel page size is larger than TARGET_PAGE_SIZE, which
>> happens for example on ppc64 with kernels compiled for 64K pages,
>> the dirty tracking doesn't work.
>>
>> Cc: Avi Kivity<avi@redhat.com>
>> Cc: Marcelo Tosatti<mtosatti@redhat.com>
>>
>
> Thanks, applied to uq/master.

Can ya'll send a PULL request for 1.1 with any bug fixes from uq/master?

Thanks,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[Avi Kivity]

On 04/04/2012 04:15 AM, David Gibson wrote:
> If the kernel page size is larger than TARGET_PAGE_SIZE, which
> happens for example on ppc64 with kernels compiled for 64K pages,
> the dirty tracking doesn't work.
>
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>

Thanks, applied to uq/master.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] [PATCH, RESEND] kvm: Fix dirty tracking with large kernel page size

[David Gibson]

If the kernel page size is larger than TARGET_PAGE_SIZE, which
happens for example on ppc64 with kernels compiled for 64K pages,
the dirty tracking doesn't work.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 kvm-all.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

Please apply.

Changes since last post:
 * Added assertion and comment in kvm_init() as requested by Jan Kiszka

diff --git a/kvm-all.c b/kvm-all.c
index ba2cee1..1f49d0a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -354,6 +354,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
     unsigned long page_number, c;
     target_phys_addr_t addr, addr1;
     unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
+    unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE;
 
     /*
      * bitmap-traveling is faster than memory-traveling (for addr...)
@@ -365,10 +366,11 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
             do {
                 j = ffsl(c) - 1;
                 c &= ~(1ul << j);
-                page_number = i * HOST_LONG_BITS + j;
+                page_number = (i * HOST_LONG_BITS + j) * hpratio;
                 addr1 = page_number * TARGET_PAGE_SIZE;
                 addr = section->offset_within_region + addr1;
-                memory_region_set_dirty(section->mr, addr, TARGET_PAGE_SIZE);
+                memory_region_set_dirty(section->mr, addr,
+                                        TARGET_PAGE_SIZE * hpratio);
             } while (c != 0);
         }
     }
@@ -986,6 +988,14 @@ int kvm_init(void)
 
     s = g_malloc0(sizeof(KVMState));
 
+    /*
+     * On systems where the kernel can support different base page
+     * sizes, host page size may be different from TARGET_PAGE_SIZE,
+     * even with KVM.  TARGET_PAGE_SIZE is assumed to be the minimum
+     * page size for the system though.
+     */
+    assert(TARGET_PAGE_SIZE <= getpagesize());
+
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     QTAILQ_INIT(&s->kvm_sw_breakpoints);
 #endif
-- 
1.7.9.1