[Qemu-devel] [6684] Fix "info registers" under kvm.

[Qemu-devel] [6684] Fix "info registers" under kvm.

[Andrzej Zaborowski]

Revision: 6684
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6684
Author:   balrog
Date:     2009-03-04 21:00:07 +0000 (Wed, 04 Mar 2009)
Log Message:
-----------
Fix "info registers" under kvm.

Modified Paths:
--------------
    trunk/target-i386/helper.c

Modified: trunk/target-i386/helper.c
===================================================================
--- trunk/target-i386/helper.c	2009-03-04 19:25:22 UTC (rev 6683)
+++ trunk/target-i386/helper.c	2009-03-04 21:00:07 UTC (rev 6684)
@@ -578,6 +578,9 @@
     char cc_op_name[32];
     static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
 
+    if (kvm_enabled())
+        kvm_arch_get_registers(env);
+
     eflags = env->eflags;
 #ifdef TARGET_X86_64
     if (env->hflags & HF_CS64_MASK) {

[Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Jan Kiszka]

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

Andrzej Zaborowski wrote:
> Revision: 6684
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6684
> Author:   balrog
> Date:     2009-03-04 21:00:07 +0000 (Wed, 04 Mar 2009)
> Log Message:
> -----------
> Fix "info registers" under kvm.
> 
> Modified Paths:
> --------------
>     trunk/target-i386/helper.c
> 
> Modified: trunk/target-i386/helper.c
> ===================================================================
> --- trunk/target-i386/helper.c	2009-03-04 19:25:22 UTC (rev 6683)
> +++ trunk/target-i386/helper.c	2009-03-04 21:00:07 UTC (rev 6684)
> @@ -578,6 +578,9 @@
>      char cc_op_name[32];
>      static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
>  
> +    if (kvm_enabled())
> +        kvm_arch_get_registers(env);
> +
>      eflags = env->eflags;
>  #ifdef TARGET_X86_64
>      if (env->hflags & HF_CS64_MASK) {
> 

On the one hand, this patch also takes care of sync'ing with KVM in case
of cpu_dump_state on fatal exists. On the other hand, it only solves one
part of monitor issue. See [1] for a more complete sync.

I'm just still waiting for a reply from Anthony on how to embed best all
the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
to merge gdbstub support for upstream kvm.

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/36994
[2] http://permalink.gmane.org/gmane.comp.emulators.qemu/37067


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

[Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Anthony Liguori]

Jan Kiszka wrote:
> Andrzej Zaborowski wrote:
>   
>> Revision: 6684
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6684
>> Author:   balrog
>> Date:     2009-03-04 21:00:07 +0000 (Wed, 04 Mar 2009)
>> Log Message:
>> -----------
>> Fix "info registers" under kvm.
>>
>> Modified Paths:
>> --------------
>>     trunk/target-i386/helper.c
>>
>> Modified: trunk/target-i386/helper.c
>> ===================================================================
>> --- trunk/target-i386/helper.c	2009-03-04 19:25:22 UTC (rev 6683)
>> +++ trunk/target-i386/helper.c	2009-03-04 21:00:07 UTC (rev 6684)
>> @@ -578,6 +578,9 @@
>>      char cc_op_name[32];
>>      static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS", "GS" };
>>  
>> +    if (kvm_enabled())
>> +        kvm_arch_get_registers(env);
>> +
>>      eflags = env->eflags;
>>  #ifdef TARGET_X86_64
>>      if (env->hflags & HF_CS64_MASK) {
>>
>>     
>
> On the one hand, this patch also takes care of sync'ing with KVM in case
> of cpu_dump_state on fatal exists. On the other hand, it only solves one
> part of monitor issue. See [1] for a more complete sync.
>
> I'm just still waiting for a reply from Anthony on how to embed best all
> the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
> to merge gdbstub support for upstream kvm.
>   

I really don't have a great suggestion.  I've been hoping we could come 
up with something better than if (kvm_enabled()) foo().  If we can't, we 
can't.

Regards,

Anthony Liguori

> Jan
>
> [1] http://permalink.gmane.org/gmane.comp.emulators.qemu/36994
> [2] http://permalink.gmane.org/gmane.comp.emulators.qemu/37067
>
>   

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Avi Kivity]

Jan Kiszka wrote:
> I'm just still waiting for a reply from Anthony on how to embed best all
> the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
> to merge gdbstub support for upstream kvm.
>
>   

Wouldn't 'if (!kvm_enabled()) return;' inside the callees suffice?


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Avi Kivity]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> I'm just still waiting for a reply from Anthony on how to embed best all
>> the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
>> to merge gdbstub support for upstream kvm.
>>
>>   
>
> Wouldn't 'if (!kvm_enabled()) return;' inside the callees suffice?

Rather,

static inline void kvm_foo(...)
{
    if (!kvm_enabled()) {
        return;
    }

#ifdef CONFIG_KVM
   kvm_do_foo();
#endif
}



-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Jan Kiszka]

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

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Andrzej Zaborowski wrote:
>>  
>>> Revision: 6684
>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6684
>>> Author:   balrog
>>> Date:     2009-03-04 21:00:07 +0000 (Wed, 04 Mar 2009)
>>> Log Message:
>>> -----------
>>> Fix "info registers" under kvm.
>>>
>>> Modified Paths:
>>> --------------
>>>     trunk/target-i386/helper.c
>>>
>>> Modified: trunk/target-i386/helper.c
>>> ===================================================================
>>> --- trunk/target-i386/helper.c    2009-03-04 19:25:22 UTC (rev 6683)
>>> +++ trunk/target-i386/helper.c    2009-03-04 21:00:07 UTC (rev 6684)
>>> @@ -578,6 +578,9 @@
>>>      char cc_op_name[32];
>>>      static const char *seg_name[6] = { "ES", "CS", "SS", "DS", "FS",
>>> "GS" };
>>>  
>>> +    if (kvm_enabled())
>>> +        kvm_arch_get_registers(env);
>>> +
>>>      eflags = env->eflags;
>>>  #ifdef TARGET_X86_64
>>>      if (env->hflags & HF_CS64_MASK) {
>>>
>>>     
>>
>> On the one hand, this patch also takes care of sync'ing with KVM in case
>> of cpu_dump_state on fatal exists. On the other hand, it only solves one
>> part of monitor issue. See [1] for a more complete sync.
>>
>> I'm just still waiting for a reply from Anthony on how to embed best all
>> the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
>> to merge gdbstub support for upstream kvm.
>>   
> 
> I really don't have a great suggestion.  I've been hoping we could come
> up with something better than if (kvm_enabled()) foo().  If we can't, we
> can't.

We could do

static inline void kvm_get_registers(CPUState *env)
{
	if (kvm_enabled())
		kvm_arch_get_registers(env);
}

in kvm.h (and kvm_put_registers analogously). That would still be
kvm-specific, though, but it would reduce the impact on the code that
has to be extended this way.

Generic hooks is something I would really like to postpone here until we
start working on some accelerator abstraction.

Jan


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

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Andreas Färber]

Am 08.03.2009 um 17:10 schrieb Anthony Liguori:

> Jan Kiszka wrote:
>> I'm just still waiting for a reply from Anthony on how to embed  
>> best all
>> the "if (kvm_enabled()) foo();" patterns [2]. [...]
>
> I really don't have a great suggestion.  I've been hoping we could  
> come up with something better than if (kvm_enabled()) foo().  If we  
> can't, we can't.

What about this pattern:

#define KVM(x) if (kvm_enabled()) { x; }
KVM(foo());

That would make it shorter to type and allows to #define KVM(x) for  
the no-KVM case.

Andreas

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Blue Swirl]

On 3/8/09, Avi Kivity <avi@redhat.com> wrote:
> Avi Kivity wrote:
>
> > Jan Kiszka wrote:
> >
> > > I'm just still waiting for a reply from Anthony on how to embed best all
> > > the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
> > > to merge gdbstub support for upstream kvm.
> > >
> > >
> > >
> >
> > Wouldn't 'if (!kvm_enabled()) return;' inside the callees suffice?
> >
>
>  Rather,
>
>  static inline void kvm_foo(...)
>  {
>    if (!kvm_enabled()) {
>        return;
>    }
>
>  #ifdef CONFIG_KVM
>   kvm_do_foo();
>  #endif
>
>  }

kvm_foo may then be unused and produce warnings unless also hidden
with #ifdef CONFIG_KVM.

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Anthony Liguori]

Avi Kivity wrote:
> Avi Kivity wrote:
>> Jan Kiszka wrote:
>>> I'm just still waiting for a reply from Anthony on how to embed best 
>>> all
>>> the "if (kvm_enabled()) foo();" patterns [2]. That would also allow us
>>> to merge gdbstub support for upstream kvm.
>>>
>>>   
>>
>> Wouldn't 'if (!kvm_enabled()) return;' inside the callees suffice?
>
> Rather,
>
> static inline void kvm_foo(...)
> {
>    if (!kvm_enabled()) {
>        return;
>    }
>
> #ifdef CONFIG_KVM
>   kvm_do_foo();
> #endif
> }

In the case of save/load registers, I'd prefer wrapper functions like:

cpu_state_update(CPUState *env, int is_dirty)

That could be hooked by something like Xen.  The implementation would be:

static void cpu_state_update(CPUState *env, int is_dirty)
{
    if (kvm_enabled()) {
        if (is_dirty)
           kvm_arch_save_registers(env);
        else
           kvm_arch_load_registers(env);
    }
}

Regards,

Anthony Liguori

>
>

[Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Anthony Liguori]

Jan Kiszka wrote:
> We could do
>
> static inline void kvm_get_registers(CPUState *env)
> {
> 	if (kvm_enabled())
> 		kvm_arch_get_registers(env);
> }
>
> in kvm.h (and kvm_put_registers analogously). That would still be
> kvm-specific, though, but it would reduce the impact on the code that
> has to be extended this way.
>
> Generic hooks is something I would really like to postpone here until we
> start working on some accelerator abstraction.
>   

I'd rather add hooks where we need them and then later take all of the 
hooks and build an abstraction from that.  Otherwise, you start 
inventing hooks that may not be needed.

BTW, Gerd, how close are we to seeing another round of Xen patches?  I 
think what's needed has shown up in the xen tree by now, correct?  Or 
are you still waiting for the DisplayState changes to get merged into 
Ian's tree?

Regards,

Anthony Liguori

> Jan
>
>   

[Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Gerd Hoffmann]

Anthony Liguori wrote:
> BTW, Gerd, how close are we to seeing another round of Xen patches?  I
> think what's needed has shown up in the xen tree by now, correct?  Or
> are you still waiting for the DisplayState changes to get merged into
> Ian's tree?

Merging DisplayState into qemu-xen didn't happen yet.  Patches are ready
though, Stefano Stabellini posted them on xen-devel last week, so I hope
that it happens RSN.

I've setup a git tree meanwhile, so you can look there:
http://git.et.redhat.com/?p=qemu-kraxel.git

It has xen pv support only, which I plan to get merged first.  xenner
isn't there yet, I have to un-bitrot these bits first once the first
batch is merged ...

cheers,
  Gerd

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Avi Kivity]

Anthony Liguori wrote:
>
> In the case of save/load registers, I'd prefer wrapper functions like:
>
> cpu_state_update(CPUState *env, int is_dirty)
>
> That could be hooked by something like Xen.  The implementation would be:
>
> static void cpu_state_update(CPUState *env, int is_dirty)
> {
>    if (kvm_enabled()) {
>        if (is_dirty)
>           kvm_arch_save_registers(env);
>        else
>           kvm_arch_load_registers(env);
>    }
> } 

That reduces readability IMO (boolean parameters do that, as well as a 
function where the direction of data movement isn't clear).

But we could keep dirty in the environment, and do everything automatically.

random qemu code:
  cpu_state_sync(env);
  // read registers
  // write registers
  cpu_state_dirty(env);

kvm arch code:
  if (env->registers_dirty) {
      copy registers to kernel
      env->registers_dirty = 0;
  }

The kernel code does similar things for registers which can be either in 
memory or in the vmcs; it uses accessors so cpu_state_sync() and 
cpu_state_dirty() aren't needed;

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Avi Kivity]

Blue Swirl wrote:
>>  static inline void kvm_foo(...)
>>  {
>>    if (!kvm_enabled()) {
>>        return;
>>    }
>>
>>  #ifdef CONFIG_KVM
>>   kvm_do_foo();
>>  #endif
>>
>>  }
>>     
>
> kvm_foo may then be unused and produce warnings unless also hidden
> with #ifdef CONFIG_KVM.
>   

static inlines don't produce unused warnings.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [6684] Fix "info registers" under kvm.

[Avi Kivity]

Andreas Färber wrote:
>
> Am 08.03.2009 um 17:10 schrieb Anthony Liguori:
>
>> Jan Kiszka wrote:
>>> I'm just still waiting for a reply from Anthony on how to embed best 
>>> all
>>> the "if (kvm_enabled()) foo();" patterns [2]. [...]
>>
>> I really don't have a great suggestion.  I've been hoping we could 
>> come up with something better than if (kvm_enabled()) foo().  If we 
>> can't, we can't.
>
> What about this pattern:
>
> #define KVM(x) if (kvm_enabled()) { x; }
> KVM(foo());
>
> That would make it shorter to type and allows to #define KVM(x) for 
> the no-KVM case.
>

Need to work out what to do if the function returns a value.

-- 
error compiling committee.c: too many arguments to function