[Qemu-devel] [PATCH 0/3] PPC KVM bringup patches round 2

[Qemu-devel] [PATCH 0/3] PPC KVM bringup patches round 2

[Alexander Graf]

Thanks a lot for commiting the previous patches. I still needed these to get
a guest booted as broken as with TCG ;-).

Alexander Graf (3):
  Move mp_state to CPU_COMMON
  Assume PPC64 host on PPC32 KVM
  PPC: Round VGA BIOS size to page boundary

 cpu-defs.h        |    3 ++-
 hw/ppc_newworld.c |    3 +++
 hw/ppc_oldworld.c |    4 ++++
 kvm-all.c         |    6 ++++++
 target-i386/cpu.h |    1 -
 5 files changed, 15 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH 1/3] Move mp_state to CPU_COMMON

[Alexander Graf]

MP State is implemented in the generic code, so let's move the variable
it accesses to generic code as well.

Unbreaks PPC w/ KVM.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 cpu-defs.h        |    3 ++-
 target-i386/cpu.h |    1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index d73ec0a..2482398 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -195,6 +195,7 @@ typedef struct CPUWatchpoint {
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
     struct kvm_run *kvm_run;                                            \
-    int kvm_fd;
+    int kvm_fd;                                                         \
+    int mp_state;
 
 #endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 08200ed..ce3fbc1 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -689,7 +689,6 @@ typedef struct CPUX86State {
 
     /* For KVM */
     uint64_t interrupt_bitmap[256 / 64];
-    uint32_t mp_state;
 
     /* in order to simplify APIC support, we leave this pointer to the
        user */
-- 
1.6.0.2

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

[Alexander Graf]

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);
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/3] PPC: Round VGA BIOS size to page boundary

[Alexander Graf]

When giving KVM a slot of a size not on page boundary, it chokes. So let's
just round up the VGA BIOS size so nobody complains anymore and we don't need
to implement sub-page slots.

Required for booting a PPC guest in KVM.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc_newworld.c |    3 +++
 hw/ppc_oldworld.c |    4 ++++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 4e5043c..b28a23d 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -179,6 +179,9 @@ static void ppc_core99_init (ram_addr_t ram_size,
         vga_bios_ptr[3] = 'V';
         cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
         vga_bios_size += 8;
+
+        /* Round to page boundary */
+        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) & TARGET_PAGE_MASK;
     }
 
     if (linux_boot) {
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index b26e407..0daa25b 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -212,6 +212,9 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
         vga_bios_ptr[3] = 'V';
         cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
         vga_bios_size += 8;
+
+        /* Round to page boundary */
+        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) & TARGET_PAGE_MASK;
     }
 
     if (linux_boot) {
@@ -310,6 +313,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
     pic = heathrow_pic_init(&pic_mem_index, 1, heathrow_irqs);
     pci_bus = pci_grackle_init(0xfec00000, pic);
     pci_vga_init(pci_bus, vga_bios_offset, vga_bios_size);
+printf("Mapping VGA to 0x%lx - 0x%lx\n", vga_bios_offset, vga_bios_offset + vga_bios_size);
 
     escc_mem_index = escc_init(0x80013000, pic[0x0f], pic[0x10], serial_hds[0],
                                serial_hds[1], ESCC_CLOCK, 4);
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 3/3] PPC: Round VGA BIOS size to page boundary

[Alexander Graf]

On 23.07.2009, at 23:31, Alexander Graf wrote:

> When giving KVM a slot of a size not on page boundary, it chokes. So  
> let's
> just round up the VGA BIOS size so nobody complains anymore and we  
> don't need
> to implement sub-page slots.
>
> Required for booting a PPC guest in KVM.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> hw/ppc_newworld.c |    3 +++
> hw/ppc_oldworld.c |    4 ++++
> 2 files changed, 7 insertions(+), 0 deletions(-)

[...]

>     if (linux_boot) {
> @@ -310,6 +313,7 @@ static void ppc_heathrow_init (ram_addr_t  
> ram_size,
>     pic = heathrow_pic_init(&pic_mem_index, 1, heathrow_irqs);
>     pci_bus = pci_grackle_init(0xfec00000, pic);
>     pci_vga_init(pci_bus, vga_bios_offset, vga_bios_size);
> +printf("Mapping VGA to 0x%lx - 0x%lx\n", vga_bios_offset,  
> vga_bios_offset + vga_bios_size);

Don't do this at home kids :-).

Please skip the last chunk.

Alex

[Qemu-devel] Re: [PATCH 3/3] PPC: Round VGA BIOS size to page boundary

[Alexander Graf]

On 24.07.2009, at 09:26, Juan Quintela wrote:

> Alexander Graf <agraf@suse.de> wrote:
>> When giving KVM a slot of a size not on page boundary, it chokes.  
>> So let's
>> just round up the VGA BIOS size so nobody complains anymore and we  
>> don't need
>> to implement sub-page slots.
>>
>> Required for booting a PPC guest in KVM.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc_newworld.c |    3 +++
>> hw/ppc_oldworld.c |    4 ++++
>> 2 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>> index 4e5043c..b28a23d 100644
>> --- a/hw/ppc_newworld.c
>> +++ b/hw/ppc_newworld.c
>> @@ -179,6 +179,9 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>         vga_bios_ptr[3] = 'V';
>>         cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
>>         vga_bios_size += 8;
>> +
>> +        /* Round to page boundary */
>> +        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) &  
>> TARGET_PAGE_MASK;
>>     }
>>
>>     if (linux_boot) {
>> diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
>> index b26e407..0daa25b 100644
>> --- a/hw/ppc_oldworld.c
>> +++ b/hw/ppc_oldworld.c
>> @@ -212,6 +212,9 @@ static void ppc_heathrow_init (ram_addr_t  
>> ram_size,
>>         vga_bios_ptr[3] = 'V';
>>         cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
>>         vga_bios_size += 8;
>> +
>> +        /* Round to page boundary */
>> +        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) &  
>> TARGET_PAGE_MASK;
>>     }
>>
>>     if (linux_boot) {
>> @@ -310,6 +313,7 @@ static void ppc_heathrow_init (ram_addr_t  
>> ram_size,
>>     pic = heathrow_pic_init(&pic_mem_index, 1, heathrow_irqs);
>>     pci_bus = pci_grackle_init(0xfec00000, pic);
>>     pci_vga_init(pci_bus, vga_bios_offset, vga_bios_size);
>> +printf("Mapping VGA to 0x%lx - 0x%lx\n", vga_bios_offset,  
>> vga_bios_offset + vga_bios_size);
>
> bad indentation.

Thanks :-). I usually do this bad indentation to indicate to myself  
that this is a temporary debug line that I need to remove before  
committing.

I think I wrote a reply to the post saying that this chunk should be  
removed, but it's good to know that someone glimpses over my  
patches :-).

Thanks again!

Alex

[Qemu-devel] Re: [PATCH 3/3] PPC: Round VGA BIOS size to page boundary

[Jan Kiszka]

Alexander Graf wrote:
> When giving KVM a slot of a size not on page boundary, it chokes. So let's
> just round up the VGA BIOS size so nobody complains anymore and we don't need
> to implement sub-page slots.
> 
> Required for booting a PPC guest in KVM.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ppc_newworld.c |    3 +++
>  hw/ppc_oldworld.c |    4 ++++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
> index 4e5043c..b28a23d 100644
> --- a/hw/ppc_newworld.c
> +++ b/hw/ppc_newworld.c
> @@ -179,6 +179,9 @@ static void ppc_core99_init (ram_addr_t ram_size,
>          vga_bios_ptr[3] = 'V';
>          cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
>          vga_bios_size += 8;
> +
> +        /* Round to page boundary */
> +        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) & TARGET_PAGE_MASK;

To be really nit-picky: ;)

(vga_bios_size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;

Jan

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

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

[Jan Kiszka]

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.

Jan

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

[Qemu-devel] Re: [PATCH 3/3] PPC: Round VGA BIOS size to page boundary

[Alexander Graf]

On 24.07.2009, at 12:52, Jan Kiszka wrote:

> Alexander Graf wrote:
>> When giving KVM a slot of a size not on page boundary, it chokes.  
>> So let's
>> just round up the VGA BIOS size so nobody complains anymore and we  
>> don't need
>> to implement sub-page slots.
>>
>> Required for booting a PPC guest in KVM.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ppc_newworld.c |    3 +++
>> hw/ppc_oldworld.c |    4 ++++
>> 2 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
>> index 4e5043c..b28a23d 100644
>> --- a/hw/ppc_newworld.c
>> +++ b/hw/ppc_newworld.c
>> @@ -179,6 +179,9 @@ static void ppc_core99_init (ram_addr_t ram_size,
>>         vga_bios_ptr[3] = 'V';
>>         cpu_to_be32w((uint32_t *)(vga_bios_ptr + 4), vga_bios_size);
>>         vga_bios_size += 8;
>> +
>> +        /* Round to page boundary */
>> +        vga_bios_size = (vga_bios_size + TARGET_PAGE_SIZE) &  
>> TARGET_PAGE_MASK;
>
> To be really nit-picky: ;)
>
> (vga_bios_size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;

Right :-).

Alex

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

[Alexander Graf]

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.

Alex

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

[Jan Kiszka]

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

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

[Alexander Graf]

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.

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.

Alex

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

[Jan Kiszka]

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

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

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

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

[Jan Kiszka]

Alexander Graf wrote:
> 
> 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 <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?
> 
> Well it's a design decision in the (generic KVM) ABI.

I wouldn't call it a "decision" :). I think it happened to be ulong as
KVM grew up on x86.

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

By design or by lack of implementation?

And what if some other arch with support for both pops up? #ifdef
HOST_PPC is no long-term solutions. And I see no need to install a
temporary workaround given the currently existing kernel support.

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

x86 and ia64 are little endian for which is doesn't matter, PowerPC and
s390 don't support dirty logging so far. Or what would break?

Jan

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

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

[Alexander Graf]

On 24.07.2009, at 14:57, Jan Kiszka wrote:

> Alexander Graf wrote:
>>
>> On 24.07.2009, at 13:51, Jan Kiszka wrote:
>>

[...]

>> There are no 32-bit PowerPC KVM kernels that can do dirty logging.
>
> By design or by lack of implementation?

Lack of implementation.

> And what if some other arch with support for both pops up? #ifdef
> HOST_PPC is no long-term solutions. And I see no need to install a
> temporary workaround given the currently existing kernel support.

I'd like to have the qemu bits sorted out while polishing the kvm  
parts, so when kvm support shows up mainline there's a working  
userspace part.

>>> In any case, I suggest to pin down the word size and use it for all
>>> hosts.
>>
>> That would break backwards compatibility.
>
> x86 and ia64 are little endian for which is doesn't matter, PowerPC  
> and
> s390 don't support dirty logging so far. Or what would break?

Oh you mean just always use u32* and be done with it?

Sounds appealing...

Alex

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

[Jan Kiszka]

Alexander Graf wrote:
> 
> On 24.07.2009, at 14:57, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>>
>>> On 24.07.2009, at 13:51, Jan Kiszka wrote:
>>>
> 
> [...]
> 
>>> There are no 32-bit PowerPC KVM kernels that can do dirty logging.
>>
>> By design or by lack of implementation?
> 
> Lack of implementation.
> 
>> And what if some other arch with support for both pops up? #ifdef
>> HOST_PPC is no long-term solutions. And I see no need to install a
>> temporary workaround given the currently existing kernel support.
> 
> I'd like to have the qemu bits sorted out while polishing the kvm parts,
> so when kvm support shows up mainline there's a working userspace part.

I understand. But the risk is that what you submit to qemu and kvm may
receive different reviews and finally look different. So for stuff which
concerns not yet existing (or fully implemented) interfaces, the other
way around is better. Specifically if there is a "risk" that it makes
into a qemu release before the corresponding kernel bits are official.

> 
>>>> In any case, I suggest to pin down the word size and use it for all
>>>> hosts.
>>>
>>> That would break backwards compatibility.
>>
>> x86 and ia64 are little endian for which is doesn't matter, PowerPC and
>> s390 don't support dirty logging so far. Or what would break?
> 
> Oh you mean just always use u32* and be done with it?
> 

Or u64* as you suggested. It doesn't matter for existing support, and
future support should simply use that type even on 32-bit hosts.

> Sounds appealing...
> 
> Alex
> 

Jan

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

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

[Alexander Graf]

On 24.07.2009, at 15:15, Jan Kiszka wrote:

> Alexander Graf wrote:
>>
>> On 24.07.2009, at 14:57, Jan Kiszka wrote:
>>
>>> Alexander Graf wrote:
>>>>
>>>> On 24.07.2009, at 13:51, Jan Kiszka wrote:
>>>>
>>
>> [...]
>>
>>>> There are no 32-bit PowerPC KVM kernels that can do dirty logging.
>>>
>>> By design or by lack of implementation?
>>
>> Lack of implementation.
>>
>>> And what if some other arch with support for both pops up? #ifdef
>>> HOST_PPC is no long-term solutions. And I see no need to install a
>>> temporary workaround given the currently existing kernel support.
>>
>> I'd like to have the qemu bits sorted out while polishing the kvm  
>> parts,
>> so when kvm support shows up mainline there's a working userspace  
>> part.
>
> I understand. But the risk is that what you submit to qemu and kvm may
> receive different reviews and finally look different. So for stuff  
> which
> concerns not yet existing (or fully implemented) interfaces, the other
> way around is better. Specifically if there is a "risk" that it makes
> into a qemu release before the corresponding kernel bits are official.

Well, it looks like doing the qemu parts first was a good idea,  
because we can still change the interfaces :-).

>
>>
>>>>> In any case, I suggest to pin down the word size and use it for  
>>>>> all
>>>>> hosts.
>>>>
>>>> That would break backwards compatibility.
>>>
>>> x86 and ia64 are little endian for which is doesn't matter,  
>>> PowerPC and
>>> s390 don't support dirty logging so far. Or what would break?
>>
>> Oh you mean just always use u32* and be done with it?
>>
>
> Or u64* as you suggested. It doesn't matter for existing support, and
> future support should simply use that type even on 32-bit hosts.

Right. I guess it's time for v2!

Alex