[Qemu-devel] [PATCH 0/4] Add preliminary KVM support for non-embedded PPC v3

[Qemu-devel] [PATCH 0/4] Add preliminary KVM support for non-embedded PPC v3

[Alexander Graf]

With KVM support emerging for Book3S (PPC64), let's add support for it in Qemu
too, so we have a user of this awesome new infrastructure.

This patchset enabled building of Qemu on KVM, though it does not work
completely due to

 1) Brokenness of Qemu (PPC32 and PPC64 targets are broken for me atm)
 2) Brokenness in Slot management (guest breaks in Linux's PCI init code)

Nevertheless, this is a good starting point for anyone who wants to get
involved with KVM on PowerPC!

V1 -> V2

 - remove unnecessary code for MP patch
 - include SoB

V2 -> V3

 - update to new configure
 - don't break 440
 - relax slot setting
 - fake dirty log when it's not available

Alexander Graf (7):
  Enable PPC KVM for non-embedded
  Set PVR in sregs
  Add mp_state to PPC CPU struct
  Fix warning in kvm-all.c
  Use correct input constant
  Set slots more carefully
  Fake dirty loggin when it's not there

 configure        |    3 ++-
 kvm-all.c        |   17 ++++++++++++++---
 target-ppc/cpu.h |    4 ++++
 target-ppc/kvm.c |   18 ++++++++++++++++--
 4 files changed, 36 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/7] Enable PPC KVM for non-embedded

[Alexander Graf]

We now have KVM on PPC64 too and might get it on PPC32 as well, as soon
as someone writes it.

So let's enable KVM for PPC32 and PPC64 targets.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 configure |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index 2a6ae40..753061b 100755
--- a/configure
+++ b/configure
@@ -2016,11 +2016,12 @@ case "$target_arch2" in
     fi
 esac
 case "$target_arch2" in
-  i386|x86_64|ppcemb)
+  i386|x86_64|ppcemb|ppc|ppc64)
     # Make sure the target and host cpus are compatible
     if test "$kvm" = "yes" -a "$target_softmmu" = "yes" -a \
       \( "$target_arch2" = "$cpu" -o \
       \( "$target_arch2" = "ppcemb" -a "$cpu" = "ppc" \) -o \
+      \( "$target_arch2" = "ppc64"  -a "$cpu" = "ppc" \) -o \
       \( "$target_arch2" = "x86_64" -a "$cpu" = "i386"   \) -o \
       \( "$target_arch2" = "i386"   -a "$cpu" = "x86_64" \) \) ; then
       echo "CONFIG_KVM=y" >> $config_mak
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/7] Set PVR in sregs

[Alexander Graf]

We need to tell the kernel about some initial CPU state we don't have yet,
so let's use the "sregs" IOCTL for that and simply put the Processor Version
Register in there.

Now the kernel knows which guest CPU to virtualize.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/kvm.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index acbb1ab..04bb305 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -44,7 +44,13 @@ int kvm_arch_init(KVMState *s, int smp_cpus)
 
 int kvm_arch_init_vcpu(CPUState *cenv)
 {
-    return 0;
+    int ret = 0;
+    struct kvm_sregs sregs;
+
+    sregs.pvr = cenv->spr[SPR_PVR];
+    ret = kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs);
+
+    return ret;
 }
 
 int kvm_arch_put_registers(CPUState *env)
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/7] Add mp_state to PPC CPU struct

[Alexander Graf]

Some generic code tries to access env->mp_state. Let's just make
it happy and provide it.

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

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 69c1d58..39b0d82 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -659,6 +659,10 @@ struct CPUPPCState {
     target_ulong hreset_vector;
 #endif
 
+#ifdef CONFIG_KVM
+    uint32_t mp_state;
+#endif
+
     /* Those resources are used only during code translation */
     /* Next instruction pointer */
     target_ulong nip;
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/7] Fix warning in kvm-all.c

[Alexander Graf]

This fixes a warning I stumbled across while compiling qemu on PPC64.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 kvm-all.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 8567ac9..961fa32 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -226,7 +226,7 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
     if (mem == NULL)  {
             fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
                     TARGET_FMT_plx "\n", __func__, phys_addr,
-                    phys_addr + size - 1);
+                    (target_phys_addr_t)(phys_addr + size - 1));
             return -EINVAL;
     }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/7] Use correct input constant

[Alexander Graf]

440 and desktop codes use different input constants for interrupt indication.

Let's use the respective ones for KVM.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/kvm.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 04bb305..b53d6e9 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -124,6 +124,14 @@ int kvm_arch_get_registers(CPUState *env)
     return 0;
 }
 
+#if defined(TARGET_PPCEMB)
+#define PPC_INPUT_INT PPC40x_INPUT_INT
+#elif defined(TARGET_PPC64)
+#define PPC_INPUT_INT PPC970_INPUT_INT
+#else
+#define PPC_INPUT_INT PPC6xx_INPUT_INT
+#endif
+
 int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
 {
     int r;
@@ -133,7 +141,7 @@ int kvm_arch_pre_run(CPUState *env, struct kvm_run *run)
      * interrupt, reset, etc) in PPC-specific env->irq_input_state. */
     if (run->ready_for_interrupt_injection &&
         (env->interrupt_request & CPU_INTERRUPT_HARD) &&
-        (env->irq_input_state & (1<<PPC40x_INPUT_INT)))
+        (env->irq_input_state & (1<<PPC_INPUT_INT)))
     {
         /* For now KVM disregards the 'irq' argument. However, in the
          * future KVM could cache it in-kernel to avoid a heavyweight exit
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/7] Set slots more carefully

[Alexander Graf]

KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
sets the framebuffer to some odd size though, causing the current code
to simply abort().

So let's bet graceful here. We can just allocate memory sizes that are of
PAGE_SIZE granularity and everything that exceeds that comes in as MMIO and
gets handled too - just slower.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 kvm-all.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 961fa32..60b76cf 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
 
     mem.slot = slot->slot;
     mem.guest_phys_addr = slot->start_addr;
-    mem.memory_size = slot->memory_size;
+    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);
     mem.userspace_addr = (unsigned long)qemu_get_ram_ptr(slot->phys_offset);
     mem.flags = slot->flags;
     if (s->migration_log) {
-- 
1.6.0.2

[Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

Some KVM platforms don't support dirty logging yet, like IA64 and PPC,
so in order to still have screen updates on those, we need to fake it.

This patch just tells the getter function for dirty bitmaps, that all
pages within a slot are dirty when the slot has dirty logging enabled.

That way we can implement dirty logging on those platforms sometime when
it drags down performance, but share the rest of the code with dirty
logging capable platforms.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 kvm-all.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 60b76cf..72b7935 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -300,6 +300,7 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
     KVMDirtyLog d;
     KVMSlot *mem;
     int ret = 0;
+    int r;
 
     d.dirty_bitmap = NULL;
     while (start_addr < end_addr) {
@@ -308,6 +309,11 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
             break;
         }
 
+        /* We didn't activate dirty logging? Don't care then. */
+        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
+            continue;
+        }
+
         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
         if (!d.dirty_bitmap) {
             d.dirty_bitmap = qemu_malloc(size);
@@ -319,7 +325,8 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
 
         d.slot = mem->slot;
 
-        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
+        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
+        if (r == -EINVAL) {
             dprintf("ioctl failed %d\n", errno);
             ret = -1;
             break;
@@ -335,6 +342,10 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
 
             if ((bitmap[word] >> bit) & 1) {
                 cpu_physical_memory_set_dirty(addr);
+            } else if (r < 0) {
+                /* When our KVM implementation doesn't know about dirty logging
+                 * we can just assume it's always dirty and be fine. */
+                cpu_physical_memory_set_dirty(addr);
             }
         }
         start_addr = phys_addr;
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Stefano Stabellini]

On Fri, 17 Jul 2009, Alexander Graf wrote:
> Some KVM platforms don't support dirty logging yet, like IA64 and PPC,
> so in order to still have screen updates on those, we need to fake it.
> 
> This patch just tells the getter function for dirty bitmaps, that all
> pages within a slot are dirty when the slot has dirty logging enabled.
> 
> That way we can implement dirty logging on those platforms sometime when
> it drags down performance, but share the rest of the code with dirty
> logging capable platforms.

Isn't it going to be very very slow?
I think a memcmp based approach as a fallback would be a better idea,
and if we could make this generic it would be even better :)

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 14:10, Stefano Stabellini wrote:

> On Fri, 17 Jul 2009, Alexander Graf wrote:
>> Some KVM platforms don't support dirty logging yet, like IA64 and  
>> PPC,
>> so in order to still have screen updates on those, we need to fake  
>> it.
>>
>> This patch just tells the getter function for dirty bitmaps, that all
>> pages within a slot are dirty when the slot has dirty logging  
>> enabled.
>>
>> That way we can implement dirty logging on those platforms sometime  
>> when
>> it drags down performance, but share the rest of the code with dirty
>> logging capable platforms.
>
> Isn't it going to be very very slow?
> I think a memcmp based approach as a fallback would be a better idea,
> and if we could make this generic it would be even better :)

I just ran a guest with it and it's not really bad.
It's really only about setting the dirty flag. The memory is written  
to directly because it's a slot.

Alex

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 14:10, Stefano Stabellini wrote:

> On Fri, 17 Jul 2009, Alexander Graf wrote:
>> Some KVM platforms don't support dirty logging yet, like IA64 and  
>> PPC,
>> so in order to still have screen updates on those, we need to fake  
>> it.
>>
>> This patch just tells the getter function for dirty bitmaps, that all
>> pages within a slot are dirty when the slot has dirty logging  
>> enabled.
>>
>> That way we can implement dirty logging on those platforms sometime  
>> when
>> it drags down performance, but share the rest of the code with dirty
>> logging capable platforms.
>
> Isn't it going to be very very slow?
> I think a memcmp based approach as a fallback would be a better idea,
> and if we could make this generic it would be even better :)

Oh memcmp not memcpy - sorry I misread that.

Yeah, one could do memcmp there. I don't know if it's worth it though.  
At least the VNC target does a dirty verification itself again anyways.

Alex

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Stefano Stabellini]

On Fri, 17 Jul 2009, Alexander Graf wrote:
> Oh memcmp not memcpy - sorry I misread that.
> 
> Yeah, one could do memcmp there. I don't know if it's worth it though.  
> At least the VNC target does a dirty verification itself again anyways.

Yes, but it does that once per client connected, I wouldn't want to rely
on that...

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 14:18, Alexander Graf wrote:

>
> On 17.07.2009, at 14:10, Stefano Stabellini wrote:
>
>> On Fri, 17 Jul 2009, Alexander Graf wrote:
>>> Some KVM platforms don't support dirty logging yet, like IA64 and  
>>> PPC,
>>> so in order to still have screen updates on those, we need to fake  
>>> it.
>>>
>>> This patch just tells the getter function for dirty bitmaps, that  
>>> all
>>> pages within a slot are dirty when the slot has dirty logging  
>>> enabled.
>>>
>>> That way we can implement dirty logging on those platforms  
>>> sometime when
>>> it drags down performance, but share the rest of the code with dirty
>>> logging capable platforms.
>>
>> Isn't it going to be very very slow?
>> I think a memcmp based approach as a fallback would be a better idea,
>> and if we could make this generic it would be even better :)
>
> Oh memcmp not memcpy - sorry I misread that.
>
> Yeah, one could do memcmp there. I don't know if it's worth it  
> though. At least the VNC target does a dirty verification itself  
> again anyways.

Thinking about this again, you'd have to copy it somewhere before. So  
on every dirty check you'd have to copy all memory off to "somewhere"  
to be able to check for it again later.

Sounds feasible, but is it really worth the effort? Can Xen do dirty  
logging?

Alex

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Stefano Stabellini]

On Fri, 17 Jul 2009, Alexander Graf wrote:
> Thinking about this again, you'd have to copy it somewhere before. So  
> on every dirty check you'd have to copy all memory off to "somewhere"  
> to be able to check for it again later.
> 
> Sounds feasible, but is it really worth the effort? Can Xen do dirty  
> logging?

Sure it can now, but it used to have a memcmp based mechanism before,
and it had decent performances, while setting everything as dirty every
time it is going to be a killer (at least if you do this under xen it is).

Besides I don't know how the dirty tracking is implemented in kvm, but
it is not so good from a performance point of view in the HAP case,
because you have to set the videoram as read only and catch every write
(this how we do it at least) so it would be interesting to compare the
two approaches.

Re: [Qemu-devel] [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 15:18, Stefano Stabellini wrote:

> On Fri, 17 Jul 2009, Alexander Graf wrote:
>> Thinking about this again, you'd have to copy it somewhere before. So
>> on every dirty check you'd have to copy all memory off to "somewhere"
>> to be able to check for it again later.
>>
>> Sounds feasible, but is it really worth the effort? Can Xen do dirty
>> logging?
>
> Sure it can now, but it used to have a memcmp based mechanism before,
> and it had decent performances, while setting everything as dirty  
> every
> time it is going to be a killer (at least if you do this under xen  
> it is).

Hum, strage. For now the PPC target has bigger (performance) issues  
than dirty logging though :-).

> Besides I don't know how the dirty tracking is implemented in kvm, but
> it is not so good from a performance point of view in the HAP case,
> because you have to set the videoram as read only and catch every  
> write
> (this how we do it at least) so it would be interesting to compare the
> two approaches.

The current implementation does the same. Videoram is set r/o and on  
first write the log gets filled.

I was thinking of implementing a lazy dirty log that would check the  
pagetable's dirty bits when the dirty log is requested. That way we  
might save some time. We'd need to invalidate the entries in the  
current scheme anyways, so why not just read dirty and set it to 0?

On PPC this gets even more complicated, because nobody guarantees that  
the page you mapped is still in the "pagetable". So it might as well  
only live in the TLB or not at all anymore.

But then again I need to get dirty logging implemented for PPC at all  
first :-)

Alex

[Qemu-devel] Re: [PATCH 6/7] Set slots more carefully

[Jan Kiszka]

Alexander Graf wrote:
> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
> sets the framebuffer to some odd size though, causing the current code
> to simply abort().
> 
> So let's bet graceful here. We can just allocate memory sizes that are of
> PAGE_SIZE granularity and everything that exceeds that comes in as MMIO and
> gets handled too - just slower.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  kvm-all.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 961fa32..60b76cf 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
>  
>      mem.slot = slot->slot;
>      mem.guest_phys_addr = slot->start_addr;
> -    mem.memory_size = slot->memory_size;
> +    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);

TARGET_PAGE_MASK? And I bet you want to round up here...

Does the caller use the odd size consistently (i.e. also for dirty log
enable/disable)? Otherwise we may run into troubles while looking up
that slot as it will be registered in user space via its odd end
address. Not sure yet, but maybe it's better to round up at the call
sites of kvm_set_user_memory_region.

Jan

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

[Qemu-devel] Re: [PATCH 6/7] Set slots more carefully

[Alexander Graf]

On 17.07.2009, at 15:48, Jan Kiszka wrote:

> Alexander Graf wrote:
>> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
>> sets the framebuffer to some odd size though, causing the current  
>> code
>> to simply abort().
>>
>> So let's bet graceful here. We can just allocate memory sizes that  
>> are of
>> PAGE_SIZE granularity and everything that exceeds that comes in as  
>> MMIO and
>> gets handled too - just slower.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> kvm-all.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 961fa32..60b76cf 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState  
>> *s, KVMSlot *slot)
>>
>>     mem.slot = slot->slot;
>>     mem.guest_phys_addr = slot->start_addr;
>> -    mem.memory_size = slot->memory_size;
>> +    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);
>
> TARGET_PAGE_MASK? And I bet you want to round up here...


Eh - yeah. Same thing, no?

And no, I don't want to round up. After the memory backed slot could  
easily be real MMIO space. I had such strange configurations with the  
ESCC overlapping in the same page as graphic memory once.

Better be safe than sorry.

> Does the caller use the odd size consistently (i.e. also for dirty log
> enable/disable)? Otherwise we may run into troubles while looking up
> that slot as it will be registered in user space via its odd end
> address. Not sure yet, but maybe it's better to round up at the call
> sites of kvm_set_user_memory_region.

Well if the destroyer does not call with the same region it's totally  
broken, no?

Alex

[Qemu-devel] Re: [PATCH 6/7] Set slots more carefully

[Jan Kiszka]

Alexander Graf wrote:
> 
> On 17.07.2009, at 15:48, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
>>> sets the framebuffer to some odd size though, causing the current code
>>> to simply abort().
>>>
>>> So let's bet graceful here. We can just allocate memory sizes that
>>> are of
>>> PAGE_SIZE granularity and everything that exceeds that comes in as
>>> MMIO and
>>> gets handled too - just slower.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> kvm-all.c |    2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 961fa32..60b76cf 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState
>>> *s, KVMSlot *slot)
>>>
>>>     mem.slot = slot->slot;
>>>     mem.guest_phys_addr = slot->start_addr;
>>> -    mem.memory_size = slot->memory_size;
>>> +    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);
>>
>> TARGET_PAGE_MASK? And I bet you want to round up here...
> 
> 
> Eh - yeah. Same thing, no?

Just look at its definition...

> 
> And no, I don't want to round up. After the memory backed slot could
> easily be real MMIO space. I had such strange configurations with the
> ESCC overlapping in the same page as graphic memory once.

Overlapping is handled by the kvm layer in user space. For sure, if
there is a mapping conflict, we are in trouble (kvm-wise) as the second
request overwrites the mapping type of the overlapping page(s).

> 
> Better be safe than sorry.

No, something wrong. If you cut off the odd "overhead", you effectively
exclude that page. So either the caller should not include it in the
first place as it is unused or kvm should try to map the whole page just
like the rest of the slot. If there remains an unresolvable conflict, we
need to enhance the slot management with some sub-page dispatching
mechanism.

> 
>> Does the caller use the odd size consistently (i.e. also for dirty log
>> enable/disable)? Otherwise we may run into troubles while looking up
>> that slot as it will be registered in user space via its odd end
>> address. Not sure yet, but maybe it's better to round up at the call
>> sites of kvm_set_user_memory_region.
> 
> Well if the destroyer does not call with the same region it's totally
> broken, no?

Yes, but I was more concerned about consistency between registration and
(potentially partial) logging updates.

Jan

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

[Qemu-devel] Re: [PATCH 6/7] Set slots more carefully

[Alexander Graf]

On 17.07.2009, at 16:18, Jan Kiszka wrote:

> Alexander Graf wrote:
>>
>> On 17.07.2009, at 15:48, Jan Kiszka wrote:
>>
>>> Alexander Graf wrote:
>>>> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the  
>>>> OS
>>>> sets the framebuffer to some odd size though, causing the current  
>>>> code
>>>> to simply abort().
>>>>
>>>> So let's bet graceful here. We can just allocate memory sizes that
>>>> are of
>>>> PAGE_SIZE granularity and everything that exceeds that comes in as
>>>> MMIO and
>>>> gets handled too - just slower.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> kvm-all.c |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>> index 961fa32..60b76cf 100644
>>>> --- a/kvm-all.c
>>>> +++ b/kvm-all.c
>>>> @@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState
>>>> *s, KVMSlot *slot)
>>>>
>>>>    mem.slot = slot->slot;
>>>>    mem.guest_phys_addr = slot->start_addr;
>>>> -    mem.memory_size = slot->memory_size;
>>>> +    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);
>>>
>>> TARGET_PAGE_MASK? And I bet you want to round up here...
>>
>>
>> Eh - yeah. Same thing, no?
>
> Just look at its definition...
>
>>
>> And no, I don't want to round up. After the memory backed slot could
>> easily be real MMIO space. I had such strange configurations with the
>> ESCC overlapping in the same page as graphic memory once.
>
> Overlapping is handled by the kvm layer in user space. For sure, if
> there is a mapping conflict, we are in trouble (kvm-wise) as the  
> second
> request overwrites the mapping type of the overlapping page(s).
>
>>
>> Better be safe than sorry.
>
> No, something wrong. If you cut off the odd "overhead", you  
> effectively
> exclude that page. So either the caller should not include it in the
> first place as it is unused or kvm should try to map the whole page  
> just
> like the rest of the slot. If there remains an unresolvable  
> conflict, we
> need to enhance the slot management with some sub-page dispatching
> mechanism.

Not mapped = MMIO

MMIO then gets passed to userspace which puts it into qemu's read/ 
write functions which then decide if it's real MMIO or just a plain  
RAM access.

So all we get is a couple more MMIO accesses, right?

Alex

[Qemu-devel] Re: [PATCH 6/7] Set slots more carefully

[Jan Kiszka]

Alexander Graf wrote:
> 
> On 17.07.2009, at 16:18, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>>
>>> On 17.07.2009, at 15:48, Jan Kiszka wrote:
>>>
>>>> Alexander Graf wrote:
>>>>> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
>>>>> sets the framebuffer to some odd size though, causing the current code
>>>>> to simply abort().
>>>>>
>>>>> So let's bet graceful here. We can just allocate memory sizes that
>>>>> are of
>>>>> PAGE_SIZE granularity and everything that exceeds that comes in as
>>>>> MMIO and
>>>>> gets handled too - just slower.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>> kvm-all.c |    2 +-
>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>> index 961fa32..60b76cf 100644
>>>>> --- a/kvm-all.c
>>>>> +++ b/kvm-all.c
>>>>> @@ -134,7 +134,7 @@ static int kvm_set_user_memory_region(KVMState
>>>>> *s, KVMSlot *slot)
>>>>>
>>>>>    mem.slot = slot->slot;
>>>>>    mem.guest_phys_addr = slot->start_addr;
>>>>> -    mem.memory_size = slot->memory_size;
>>>>> +    mem.memory_size = slot->memory_size & ~(TARGET_PAGE_SIZE - 1);
>>>>
>>>> TARGET_PAGE_MASK? And I bet you want to round up here...
>>>
>>>
>>> Eh - yeah. Same thing, no?
>>
>> Just look at its definition...
>>
>>>
>>> And no, I don't want to round up. After the memory backed slot could
>>> easily be real MMIO space. I had such strange configurations with the
>>> ESCC overlapping in the same page as graphic memory once.
>>
>> Overlapping is handled by the kvm layer in user space. For sure, if
>> there is a mapping conflict, we are in trouble (kvm-wise) as the second
>> request overwrites the mapping type of the overlapping page(s).
>>
>>>
>>> Better be safe than sorry.
>>
>> No, something wrong. If you cut off the odd "overhead", you effectively
>> exclude that page. So either the caller should not include it in the
>> first place as it is unused or kvm should try to map the whole page just
>> like the rest of the slot. If there remains an unresolvable conflict, we
>> need to enhance the slot management with some sub-page dispatching
>> mechanism.
> 
> Not mapped = MMIO

Right.

> 
> MMIO then gets passed to userspace which puts it into qemu's read/write
> functions which then decide if it's real MMIO or just a plain RAM access.
> 
> So all we get is a couple more MMIO accesses, right?

In this particular case. But it only happens to work due to the given
setup. You can't derive a generic rounding rule for KVM slots from this.

You may say if slot A is MMIO and slot B RAM, that a overlapping page
should be handled as MMIO. But that's not what your code does.

Jan

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

[Qemu-devel] Re: [PATCH 7/7] Fake dirty loggin when it's not there

[Jan Kiszka]

Alexander Graf wrote:
> Some KVM platforms don't support dirty logging yet, like IA64 and PPC,
> so in order to still have screen updates on those, we need to fake it.
> 
> This patch just tells the getter function for dirty bitmaps, that all
> pages within a slot are dirty when the slot has dirty logging enabled.
> 
> That way we can implement dirty logging on those platforms sometime when
> it drags down performance, but share the rest of the code with dirty
> logging capable platforms.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  kvm-all.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 60b76cf..72b7935 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -300,6 +300,7 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>      KVMDirtyLog d;
>      KVMSlot *mem;
>      int ret = 0;
> +    int r;
>  
>      d.dirty_bitmap = NULL;
>      while (start_addr < end_addr) {
> @@ -308,6 +309,11 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>              break;
>          }
>  
> +        /* We didn't activate dirty logging? Don't care then. */
> +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
> +            continue;
> +        }
> +

Isn't this worth a separate patch? Do we already run into this patch on
dirty-logging archs (ie. x86)? I'm still wondering why it's needed.

>          size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>          if (!d.dirty_bitmap) {
>              d.dirty_bitmap = qemu_malloc(size);
> @@ -319,7 +325,8 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>  
>          d.slot = mem->slot;
>  
> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
> +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> +        if (r == -EINVAL) {

I would say, better exclude 0 and the error you for missing support here
than to assume only EINVAL is a "real" error.

>              dprintf("ioctl failed %d\n", errno);
>              ret = -1;
>              break;
> @@ -335,6 +342,10 @@ int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>  
>              if ((bitmap[word] >> bit) & 1) {
>                  cpu_physical_memory_set_dirty(addr);
> +            } else if (r < 0) {
> +                /* When our KVM implementation doesn't know about dirty logging
> +                 * we can just assume it's always dirty and be fine. */
> +                cpu_physical_memory_set_dirty(addr);
>              }
>          }
>          start_addr = phys_addr;

Jan

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

[Qemu-devel] Re: [PATCH 7/7] Fake dirty loggin when it's not there

[Alexander Graf]

On 17.07.2009, at 16:37, Jan Kiszka wrote:

> Alexander Graf wrote:
>> Some KVM platforms don't support dirty logging yet, like IA64 and  
>> PPC,
>> so in order to still have screen updates on those, we need to fake  
>> it.
>>
>> This patch just tells the getter function for dirty bitmaps, that all
>> pages within a slot are dirty when the slot has dirty logging  
>> enabled.
>>
>> That way we can implement dirty logging on those platforms sometime  
>> when
>> it drags down performance, but share the rest of the code with dirty
>> logging capable platforms.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> kvm-all.c |   13 ++++++++++++-
>> 1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 60b76cf..72b7935 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -300,6 +300,7 @@ int  
>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>     KVMDirtyLog d;
>>     KVMSlot *mem;
>>     int ret = 0;
>> +    int r;
>>
>>     d.dirty_bitmap = NULL;
>>     while (start_addr < end_addr) {
>> @@ -308,6 +309,11 @@ int  
>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>             break;
>>         }
>>
>> +        /* We didn't activate dirty logging? Don't care then. */
>> +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>> +            continue;
>> +        }
>> +
>
> Isn't this worth a separate patch? Do we already run into this patch  
> on
> dirty-logging archs (ie. x86)? I'm still wondering why it's needed.

We don't. I'm just being paranoid.

>>         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>         if (!d.dirty_bitmap) {
>>             d.dirty_bitmap = qemu_malloc(size);
>> @@ -319,7 +325,8 @@ int  
>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>
>>         d.slot = mem->slot;
>>
>> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
>> +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
>> +        if (r == -EINVAL) {
>
> I would say, better exclude 0 and the error you for missing support  
> here
> than to assume only EINVAL is a "real" error.

Hum, this was the logic that was there before and it makes sense. - 
EINVAL is used for errors whole -ENOTSUPP is used for N/A.  
Unfortunately -ENOTSUPP is only exported when __KERNEL__ is defined.

Alex

[Qemu-devel] Re: [PATCH 7/7] Fake dirty loggin when it's not there

[Jan Kiszka]

Alexander Graf wrote:
> 
> On 17.07.2009, at 16:37, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> Some KVM platforms don't support dirty logging yet, like IA64 and PPC,
>>> so in order to still have screen updates on those, we need to fake it.
>>>
>>> This patch just tells the getter function for dirty bitmaps, that all
>>> pages within a slot are dirty when the slot has dirty logging enabled.
>>>
>>> That way we can implement dirty logging on those platforms sometime when
>>> it drags down performance, but share the rest of the code with dirty
>>> logging capable platforms.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> kvm-all.c |   13 ++++++++++++-
>>> 1 files changed, 12 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 60b76cf..72b7935 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -300,6 +300,7 @@ int
>>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>>     KVMDirtyLog d;
>>>     KVMSlot *mem;
>>>     int ret = 0;
>>> +    int r;
>>>
>>>     d.dirty_bitmap = NULL;
>>>     while (start_addr < end_addr) {
>>> @@ -308,6 +309,11 @@ int
>>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>>             break;
>>>         }
>>>
>>> +        /* We didn't activate dirty logging? Don't care then. */
>>> +        if(!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>>> +            continue;
>>> +        }
>>> +
>>
>> Isn't this worth a separate patch? Do we already run into this patch on
>> dirty-logging archs (ie. x86)? I'm still wondering why it's needed.
> 
> We don't. I'm just being paranoid.
> 
>>>         size = ((mem->memory_size >> TARGET_PAGE_BITS) + 7) / 8;
>>>         if (!d.dirty_bitmap) {
>>>             d.dirty_bitmap = qemu_malloc(size);
>>> @@ -319,7 +325,8 @@ int
>>> kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>>>
>>>         d.slot = mem->slot;
>>>
>>> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
>>> +        r = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
>>> +        if (r == -EINVAL) {
>>
>> I would say, better exclude 0 and the error you for missing support here
>> than to assume only EINVAL is a "real" error.
> 
> Hum, this was the logic that was there before and it makes sense.
> -EINVAL is used for errors whole -ENOTSUPP is used for N/A.
> Unfortunately -ENOTSUPP is only exported when __KERNEL__ is defined.

Uuh, kvm returns a private error code to user space? That would be a
bug. So we need a check for the to-be-fixed error code and >= 512.

Jan

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

[Qemu-devel] Re: [PATCH 0/4] Add preliminary KVM support for non-embedded PPC v3

[Hollis Blanchard]

On Fri, 2009-07-17 at 13:51 +0200, Alexander Graf wrote:
> Alexander Graf (7):
>   Enable PPC KVM for non-embedded

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

>   Set PVR in sregs

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

>   Add mp_state to PPC CPU struct

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

>   Fix warning in kvm-all.c

No comment.

>   Use correct input constant

Acked-by: Hollis Blanchard <hollisb@us.ibm.com>

>   Set slots more carefully

No comment.

>   Fake dirty loggin when it's not there

No comment.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 6/7] Set slots more carefully

[Anthony Liguori]

Alexander Graf wrote:
> KVM only supports slot sizes of PAGE_SIZE granilarity. On PPC the OS
> sets the framebuffer to some odd size though, causing the current code
> to simply abort().
>   

Why is the caller registering a memory region that isn't PAGE_SIZE?  Is 
it relying on subpage memory registration?

If so, this isn't something we handle in KVM today.

If the PPC framebuffer is just being registered with the wrong size, I'd 
rather see that fixed.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/7] Add mp_state to PPC CPU struct

[Anthony Liguori]

Alexander Graf wrote:
> Some generic code tries to access env->mp_state. Let's just make
> it happy and provide it.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>   

Or we can move that code from kvm-all.c to target-i386/kvm.c where it 
belongs :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/7] Add mp_state to PPC CPU struct

[Hollis Blanchard]

On Wed, 2009-07-22 at 10:34 -0500, Anthony Liguori wrote:
> Alexander Graf wrote:
> > Some generic code tries to access env->mp_state. Let's just make
> > it happy and provide it.
> >
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> >   
> 
> Or we can move that code from kvm-all.c to target-i386/kvm.c where it 
> belongs :-)

Common code uses mp_state, and i386 is the only thing calling that code.
However, the ioctl used by that code is valid for all architectures; it
will just return -EINVAL. Someday we might need it in common code, but
then again it's not even clear to me it will be useful outside of x86.

I'm fine with moving it too, as long as it's also renamed at the same
time. :)

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 3/7] Add mp_state to PPC CPU struct

[Anthony Liguori]

Hollis Blanchard wrote:
> On Wed, 2009-07-22 at 10:34 -0500, Anthony Liguori wrote:
>   
>> Alexander Graf wrote:
>>     
>>> Some generic code tries to access env->mp_state. Let's just make
>>> it happy and provide it.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>   
>>>       
>> Or we can move that code from kvm-all.c to target-i386/kvm.c where it 
>> belongs :-)
>>     
>
> Common code uses mp_state, and i386 is the only thing calling that code.
> However, the ioctl used by that code is valid for all architectures; it
> will just return -EINVAL. Someday we might need it in common code, but
> then again it's not even clear to me it will be useful outside of x86.
>
> I'm fine with moving it too, as long as it's also renamed at the same
> time. :)
>   

Why is it a common ioctl instead of an arch specific ioctl?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/7] Add mp_state to PPC CPU struct

[Hollis Blanchard]

On Wed, 2009-07-22 at 11:22 -0500, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > On Wed, 2009-07-22 at 10:34 -0500, Anthony Liguori wrote:
> >   
> >> Alexander Graf wrote:
> >>     
> >>> Some generic code tries to access env->mp_state. Let's just make
> >>> it happy and provide it.
> >>>
> >>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>   
> >>>       
> >> Or we can move that code from kvm-all.c to target-i386/kvm.c where it 
> >> belongs :-)
> >>     
> >
> > Common code uses mp_state, and i386 is the only thing calling that code.
> > However, the ioctl used by that code is valid for all architectures; it
> > will just return -EINVAL. Someday we might need it in common code, but
> > then again it's not even clear to me it will be useful outside of x86.
> >
> > I'm fine with moving it too, as long as it's also renamed at the same
> > time. :)
> >   
> 
> Why is it a common ioctl instead of an arch specific ioctl?

I can only speculate: Marcelo probably thought other architectures may
have something useful to put there.

-- 
Hollis Blanchard
IBM Linux Technology Center

[Qemu-devel] Re: [PATCH 3/7] Add mp_state to PPC CPU struct

[Jan Kiszka]

Hollis Blanchard wrote:
> On Wed, 2009-07-22 at 11:22 -0500, Anthony Liguori wrote:
>> Hollis Blanchard wrote:
>>> On Wed, 2009-07-22 at 10:34 -0500, Anthony Liguori wrote:
>>>   
>>>> Alexander Graf wrote:
>>>>     
>>>>> Some generic code tries to access env->mp_state. Let's just make
>>>>> it happy and provide it.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>   
>>>>>       
>>>> Or we can move that code from kvm-all.c to target-i386/kvm.c where it 
>>>> belongs :-)
>>>>     
>>> Common code uses mp_state, and i386 is the only thing calling that code.
>>> However, the ioctl used by that code is valid for all architectures; it
>>> will just return -EINVAL. Someday we might need it in common code, but
>>> then again it's not even clear to me it will be useful outside of x86.
>>>
>>> I'm fine with moving it too, as long as it's also renamed at the same
>>> time. :)
>>>   
>> Why is it a common ioctl instead of an arch specific ioctl?
> 
> I can only speculate: Marcelo probably thought other architectures may
> have something useful to put there.
> 

mp_state is also used by (yet unsupported) ia64. So I think it's better
to keep the related bits in the arch-independent part.

Jan

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

[Qemu-devel] Re: [PATCH 3/7] Add mp_state to PPC CPU struct

[Anthony Liguori]

Jan Kiszka wrote:
> mp_state is also used by (yet unsupported) ia64. So I think it's better
> to keep the related bits in the arch-independent part.
>   

Then it should go in CPU_COMMON instead of CPUPPCState

Regards,

Anthony Liguori

> Jan
>
>   

[Qemu-devel] Re: [PATCH 3/7] Add mp_state to PPC CPU struct

[Jan Kiszka]

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

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> mp_state is also used by (yet unsupported) ia64. So I think it's better
>> to keep the related bits in the arch-independent part.
>>   
> 
> Then it should go in CPU_COMMON instead of CPUPPCState

That's true. Probably I looked too long at qemu-kvm while porting this. :)

Jan


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