[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 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 <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?
> 
> 32-bit userland on 64-bit kernel.

OK. So this is an issue due to an underspecified KVM ABI, right?

> 
> 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?

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

Jan

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