[Qemu-devel] Re: [PATCH 2/3] Assume PPC64 host on PPC32 KVM

[Alexander Graf]

On 24.07.2009, at 13:51, Jan Kiszka wrote:

> Alexander Graf wrote:
> > On 24.07.2009, at 13:17, Jan Kiszka wrote:
> >
> > > Alexander Graf wrote:
> > > > On 24.07.2009, at 12:59, Jan Kiszka wrote:
> > > >
> > > > > Alexander Graf wrote:
> > > > > > When talking to the kernel about dirty maps, we need to find  
> > > > > > out which
> > > > > > bits were actually set. This is done by set_bit and test_bit like
> > > > > > functiontality which uses the "long" variable type.
> > > > > >
> > > > > > Now, with PPC32 userspace and PPC64 kernel space (which is pretty
> > > > > > common),
> > > > > > we can't interpret the bits properly anymore, because we think  
> > > > > > long is
> > > > > > 32 bits wide.
> > > > > >
> > > > > > So for PPC dirty bitmap analysis, let's just assume we're always
> > > > > > running
> > > > > > on a PPC64 host. Currently there is no dirty bitmap  
> > > > > > implementation for
> > > > > > PPC32 / PPCEMB anyways.
> > > > > >
> > > > > > Unbreaks dirty logging on PPC.
> > > > > >
> > > > > > Signed-off-by: Alexander Graf <address@hidden>
> > > > > > ---
> > > > > > kvm-all.c |    6 ++++++
> > > > > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > > > > >
> > > > > > diff --git a/kvm-all.c b/kvm-all.c
> > > > > > index 824bb4c..bfaa623 100644
> > > > > > --- a/kvm-all.c
> > > > > > +++ b/kvm-all.c
> > > > > > @@ -357,7 +357,13 @@ int
> > > > > > kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
> > > > > >       for (phys_addr = mem->start_addr, addr = mem->phys_offset;
> > > > > >            phys_addr < mem->start_addr + mem->memory_size;
> > > > > >            phys_addr += TARGET_PAGE_SIZE, addr +=  
> > > > > > TARGET_PAGE_SIZE) {
> > > > > > +#ifdef HOST_PPC
> > > > > > +             /* Big endian keeps us from having different long  
> > > > > > sizes
> > > > > > in user and
> > > > > > +              * kernel space, so assume we're always on ppc64.  
> > > > > > */
> > > > > > +            uint64_t *bitmap = (uint64_t *)d.dirty_bitmap;
> > > > > > +#else
> > > > > >           unsigned long *bitmap = (unsigned long  
> > > > > > *)d.dirty_bitmap;
> > > > > > +#endif
> > > > > >           unsigned nr = (phys_addr - mem->start_addr) >>
> > > > > > TARGET_PAGE_BITS;
> > > > > >           unsigned word = nr / (sizeof(*bitmap) * 8);
> > > > > >           unsigned bit = nr % (sizeof(*bitmap) * 8);
> > > > >
> > > > > This rather screams for a generic fix. Current code assumes
> > > > > sizeof(unsigned long) == 8. That should already break on 32-bit  
> > > > > x86
> > > > > hosts. So either do (sizeof(*bitmap) * sizeof(unsigned long)) or  
> > > > > switch
> > > > > to uint64_t - but for ALL hosts.
> > > >
> > > > I don't see where that would break. The kernel treats the array as
> > > > ulong*, userspace treats it as ulong* and set_bit in kernel does
> > > > bitmap[word] |= (1 << bit). So as long as userspace long and  
> > > > kernel long
> > > > are the same, it works.
> > > >
> > > > In fact - it should even work out with little endian and  
> > > > different ulong
> > > > sizes. It just breaks on BE.
> > >
> > > Err, yes, forget it.
> > >
> > > But let's help me understanding the actual problem: Do you have
> > > different ulong sizes in your scenario? Why? Is it a compat issue of
> > > 32-bit userland on 64-bit kernel?
> >
> > 32-bit userland on 64-bit kernel.
>
> OK. So this is an issue due to an underspecified KVM ABI, right?

Well it's a design decision in the (generic KVM) ABI.

> > kernel: sizeof(ulong) = 8
> > userspace: sizeof(ulong) = 4
> >
> > now, with big endian, a "1" is on the rightmost byte - which means
> > looking at the bytes it's
> >
> > kernel: byte[7]
> > userspace: byte[3]
> >
> > So if you set bit nr "1" with the current logic, the kernel would set
> > bit "1" (in the first 8 bytes), userspace would read bit "1" in the
> > second byte, thus 32 + 1.
> >
> > On little endian, the lower word is on the first 4 bytes, so it would
> > still be bit "1" in the first byte.
>
> Big endian machines require us to agree on the word size of the bitmap
> so that 32-on-64-bit works - and 32-on-32 doesn't break. I think the
> latter would be the case with your patch, no? Or don't we have 32-bit
> KVM PowerPC kernels?

There are no 32-bit PowerPC KVM kernels that can do dirty logging.

> In any case, I suggest to pin down the word size and use it for all  
> hosts.

That would break backwards compatibility.

Alex