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

[Jan Kiszka <jan.kiszka@siemens.com>]

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 <agraf@suse.de>
>>> ---
>>> 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?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux