Re: [Qemu-devel] qemu-kvm-1.0 crashes with threaded vnc server?

[Corentin Chary]

On Thu, Feb 9, 2012 at 7:08 PM, Peter Lieven  wrote:
> Hi,
>
> is anyone aware if there are still problems when enabling the threaded vnc
> server?
> I saw some VMs crashing when using a qemu-kvm build with
> --enable-vnc-thread.
>
> qemu-kvm-1.0[22646]: segfault at 0 ip 7fec1ca7ea0b sp 7fec19d056d0
> error 6 in libz.so.1.2.3.3[7fec1ca75000+16000]
> qemu-kvm-1.0[26056]: segfault at 7f06d8d6e010 ip 7f06e0a30d71 sp
> 7f06df035748 error 6 in libc-2.11.1.so[7f06e09aa000+17a000]
>
> I had no time to debug further. It seems to happen shortly after migrating,
> but thats uncertain. At least the segfault in libz seems to
> give a hint to VNC since I cannot image of any other part of qemu-kvm using
> libz except for VNC server.
>
> Thanks,
> Peter
>
>

Hi Peter,
I found two patches on my git tree that I sent long ago but somehow
get lost on the mailing list. I rebased the tree but did not have the
time (yet) to test them.
http://git.iksaif.net/?p=qemu.git;a=shortlog;h=refs/heads/wip
Feel free to try them. If QEMU segfault again, please send a full gdb
backtrace / valgrind trace / way to reproduce :).
Thanks,


-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] Restore consistent formatting

[Blue Swirl]

On Wed, Feb 8, 2012 at 15:23, Anthony Liguori  wrote:
> On 02/08/2012 09:04 AM, malc wrote:
>>
>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>
>>> malc,
>>>
>>> Arbitrarily reformatting your files is not okay. If you want a different
>>> formatting, you need to fix checkpatch.pl first to not error on that
>>> formatting in your files.
>>
>>
>> It was always formatter like this (internally consistent), then others
>> added code which made it not so.
>
>
> We do have a mixed style in the audio layer.  I'm not happy about that but I
> also feel strongly that going through and doing a reformat is not a
> worthwhile exercise.
>
> I can also understand the desire to keep things consistent.  But patches
> should always go to the mailing list.  I certainly would have acked such a
> patch FWIW.

Really? First, this would imply that local consistency is more
important to you than global consistency and second, pure reformatting
patches are now acceptable to you (despite previous git blame breakage
reasoning).

What would your reaction be if I declared that CODING_STYLE for
target-sparc/* is Linux kernel one (some pieces could probably be
found which would comply even now to make the precedent case for
argument's sake) and post a reformatting patch to the list?

> I think people get a bit too excited about coding style.  There are much
> more important things to worry about in life than the number of spaces
> before a parenthesis :-)

In general, a coding style is one aspect of development guidance. The
level of guidance can vary, it could be missing, there could be a
loose convention, a guideline or even a iron hard rule. It could apply
differently case by case or globally.

In case of coding style, I'd strongly prefer that the level of
guidance is clear to everyone and it doesn't change very often. As to
the actual content of the guidance, that can never be precise.

In this specific case, CODING_STYLE says nothing about spaces after
function identifiers. However, vast majority of code omits them.
Spaces are used in GNU style, not in K&R or Linux style and I think
QEMU style is closer to them than to GNU style. I'd say this case is
not unlike the previous cases where braces were added to code where
there were none before, so reformatting is going backwards (unless
local consistency is given priority).

Personally I prefer strong, enforced rules to loose conventions but
it's more important to me that everybody in a project plays by the
same rules (or agrees to lack of them), whatever they are. Personally
I also hate GNU style and would prefer that malc would swallow his
pride and reformat audio etc. to match rest of the world, but I
respect his status as taking responsibility for maintaining the audio
layer.

> Regards,
>
> Anthony Liguori
>
>>
>> [..snip..]
>>
>

[Qemu-devel] [PATCH] tcg: Remove unneeded include statements

[Stefan Weil]

The standard include files are already included in qemu-common.h.

malloc.h and alloca.h were needed for alloca() which was removed
from TCG code some years ago when switching from dyngen to TCG
(see commit 49516bc0d622112caac9df628caf19010fda8b67).

Signed-off-by: Stefan Weil 
---
 tcg/tcg.c |   12 
 1 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 6f9328e..023cb51 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -33,18 +33,6 @@
 #define NDEBUG
 #endif
 
-#include 
-#include 
-#include 
-#include 
-#include 
-#ifdef _WIN32
-#include 
-#endif
-#ifdef _AIX
-#include 
-#endif
-
 #include "qemu-common.h"
 #include "cache-utils.h"
 #include "host-utils.h"
-- 
1.7.8.3

Re: [Qemu-devel] Restore consistent formatting

[Blue Swirl]

On Wed, Feb 8, 2012 at 15:36, Andreas Färber  wrote:
> Am 08.02.2012 16:23, schrieb Anthony Liguori:
>> On 02/08/2012 09:04 AM, malc wrote:
>>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>>
 Arbitrarily reformatting your files is not okay. If you want a different
 formatting, you need to fix checkpatch.pl first to not error on that
 formatting in your files.
>>>
>>> It was always formatter like this (internally consistent), then others
>>> added code which made it not so.
>>
>> We do have a mixed style in the audio layer.  I'm not happy about that
>> but I also feel strongly that going through and doing a reformat is not
>> a worthwhile exercise.
>>
>> I can also understand the desire to keep things consistent.  But patches
>> should always go to the mailing list.  I certainly would have acked such
>> a patch FWIW.
>>
>> I think people get a bit too excited about coding style.  There are much
>> more important things to worry about in life than the number of spaces
>> before a parenthesis :-)
>
> This is not about whether or not we put a space somewhere.
>
> It's about reviewers and SubmitAPatch telling people to run
> checkpatch.pl on patches and checkpatch.pl reporting this as an ERROR,
> not a WARNING. So if you follow Stefan's instructions on running the
> script as a commit hook (which is the only sane way to run it when
> handling lots of patches) you can't commit a patch or your local changes
> when there are ERRORs.

The warning levels are from Linux, they could well be incorrect.

> I just spent half the night trying to find out why checkpatch.pl reports
> CPUX86State *env, CPUYState *env, CPyState *env as ERRORs but not
> CPUState *env. I did not succeed in really understanding it.

Maybe the CPUState #defines in target-*/cpu.h confuse checkpatch.

> So either we need to all stop using and telling to use checkpatch.pl or
> someone needs to fix it.

It's not that black or white. Obviously checkpatch.pl can make
mistakes and some of its rules are actually not based on CODING_STYLE
but they are in line with the current code (except for example in this
audio case), so the output is usually helpful to make code more
uniform. I wouldn't "fix" checkpatch to consider local style
variations. If we don't want a rule that complains spaces after
function identifiers, it could be removed entirely. Though then
someone could submit patches to introduce them elsewhere, but if there
is no rule, that should be OK.

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [BUG] checkpatch: ERROR due to * recognized as operator

[Blue Swirl]

On Thu, Feb 9, 2012 at 15:30, Andreas Färber  wrote:
> Hello Blue,
>
> I recently stumbled over the following checkpatch.pl false positive:
>
> --8<--
>
> --- a/hw/his.c
> +++ b/hw/his.c
> @@ -1,1 +1,1 @@
> -    cpu_reset(CPUState *env);
> +    cpu_state_reset(CPUState *env);
>
> --- a/hw/hers.c
> +++ b/hw/hers.c
> @@ -1,1 +1,1 @@
> -    cpu_reset(CPUX86State *env);
> +    cpu_state_reset(CPUX86State *env);
>
> --- a/hw/its.c
> +++ b/hw/its.c
> @@ -1,1 +1,1 @@
> -cpu_reset(CPUState *env);
> +cpu_state_reset(CPUState *env);
>
> --- a/hw/theirs.c
> +++ b/hw/theirs.c
> @@ -1,2 +1,2 @@
>  typedef struct CPUState CPUState;
> -cpu_reset(CPUState *env);
> +cpu_state_reset(CPUState *env);
>
> --8<--
>
> results in:
>
>
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #5: FILE: hw/his.c:1:
> +    cpu_state_reset(CPUState *env);
>                              ^
>
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #11: FILE: hw/hers.c:1:
> +    cpu_state_reset(CPUX86State *env);
>                                 ^
>
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #17: FILE: hw/its.c:1:
> +cpu_state_reset(CPUState *env);
>                          ^
>
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #24: FILE: hw/theirs.c:2:
> +cpu_state_reset(CPUState *env);
>                          ^
>
> total: 4 errors, 0 warnings, 9 lines checked
>
>
> So, it seems to interpret the * symbol as multiplication rather than
> pointer.
>
> Surprisingly, in my real code, using CPUState in place of CPUX86State
> was actually able to remedy the ERROR but not in this simplified test
> case. I added some prints around that place and it seems, in the working
> CPUState case it didn't even enter the op checking code path.
>
> Any ideas?

IIRC Linux does not use typedefs much, so maybe typedefs combined with
#defines confuse checkpatch.pl.

But I don't know why this case would be different to:
typedef long long int64_t;
#define off_t int64_t
void func(off_t *e);

Perhaps using a typedef in place of #define could help but I'd not
make such a change just to silence checkpatch.

>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] checkpatch: Don't WARN about missing spaces in audio files

[Blue Swirl]

On Thu, Feb 9, 2012 at 14:59, Andreas Färber  wrote:
> Disable warnings for spaces before opening parenthesis in
> hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c.
>
> Signed-off-by: Andreas Färber 
> Cc: Blue Swirl 
> Cc: malc 
> ---
>  scripts/checkpatch.pl |    5 +
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 8850a5f..5433736 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -246,6 +246,8 @@ our $allowed_asm_includes = qr{(?x:
>  )};
>  # memory.h: ARM has a custom one
>
> +our $audio_files = 
> qr{hw/ac97.c|hw/adlib.c|hw/cs4231a.c|hw/es1370.c|hw/gus.c|hw/sb16.c};
> +
>  sub build_types {
>        my $mods = "(?x:  \n" . join("|\n  ", @modifierList) . "\n)";
>        my $all = "(?x:  \n" . join("|\n  ", @typeList) . "\n)";
> @@ -1966,6 +1968,9 @@ sub process {
>                                asm|__asm__)$/x)
>                        {
>
> +                       # malc wants to keep spacing consistent in the audio 
> files.
> +                       } elsif ($realfile =~ /($audio_files)/) {
> +
>                        # cpp #define statements have non-optional spaces, ie
>                        # if there is a space between the name and the open
>                        # parenthesis it is simply not a parameter group.
> --
> 1.7.7
>

NACK to such special casing.

Re: [Qemu-devel] [PATCH] checkpatch: Don't WARN about missing spaces in audio files

[Blue Swirl]

On Fri, Feb 10, 2012 at 17:47, Anthony Liguori  wrote:
> On 02/09/2012 10:02 PM, malc wrote:
>>
>> On Fri, 10 Feb 2012, Evgeny Voevodin wrote:
>>
>>> On 02/09/2012 06:59 PM, Andreas F?rber wrote:

 Disable warnings for spaces before opening parenthesis in
 hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c.
>>>
>>>
>>> Why audio files are such a special thing?
>>
>>
>> Because they are consistently formatted the way they are.
>
>
> I personally hate the QEMU Coding Style I dislike inconsistency more than
> any particular style.

I dislike unclear rules more than inconsistency or coding styles.

> So I'm with malc here.  I'd be opposed to introducing a new file that
> deviated from Coding Style but for the ones that already do, I see no reason
> to convert them all at once or make the code deviate from the style it's
> already using.

I'd make a rule, specify the level of importance and try to stick to
it. I would not oppose global reformatting to GNU style even (which I
hate) if that would be the rule. I don't like laissez faire, but if
that is the rule then fine.

>
>>
>>> Isn't it be better to revert a patch that introduced checkpatch.pl
>>> errors?
>>
>>
>> No.
>
>
> Regards,
>
> Anthony Liguori
>
>
>>
>

[Qemu-devel] [Bug 741887] Re: virsh snapshot-create too slow (kvm, qcow2, savevm)

[Cinquero]

Cool. Writes about 9 times the data of the actual snapshot size.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/741887

Title:
  virsh snapshot-create too slow (kvm, qcow2, savevm)

Status in QEMU:
  New
Status in “qemu-kvm” package in Ubuntu:
  Won't Fix

Bug description:
  Action
  ==
  # time virsh snapshot-create 1

  * Taking snapshot of a running KVM virtual machine

  Result
  ==
  Domain snapshot 1300983161 created
  real4m46.994s
  user0m0.000s
  sys 0m0.010s

  Expected result
  ===
  * Snapshot taken after few seconds instead of minutes.

  Environment
  ===
  * Ubuntu Natty Narwhal upgraded from Lucid and Meerkat, fully updated.

  * Stock natty packages of libvirt and qemu installed (libvirt-bin
  0.8.8-1ubuntu5; libvirt0 0.8.8-1ubuntu5; qemu-common 0.14.0+noroms-
  0ubuntu3; qemu-kvm 0.14.0+noroms-0ubuntu3).

  * Virtual machine disk format is qcow2 (debian 5 installed)
  image: /storage/debian.qcow2
  file format: qcow2
  virtual size: 10G (10737418240 bytes)
  disk size: 1.2G
  cluster_size: 65536
  Snapshot list:
  IDTAG VM SIZEDATE   VM CLOCK
  1 snap01  48M 2011-03-24 09:46:33   00:00:58.899
  2 1300979368  58M 2011-03-24 11:09:28   00:01:03.589
  3 1300983161  57M 2011-03-24 12:12:41   00:00:51.905

  * qcow2 disk is stored on ext4 filesystem, without RAID or LVM or any
  special setup.

  * running guest VM takes about 40M RAM from inside, from outside 576M
  are given to that machine

  * host has fast dual-core pentium cpu with virtualization support,
  around 8G of RAM and 7200rpm harddrive (dd from urandom to file gives
  about 20M/s)

  * running processes: sshd, atd (empty), crond (empty), libvirtd, tmux,
  bash, rsyslogd, upstart-socket-bridge, udevd, dnsmasq, iotop (python)

  * networking is done by bridging and bonding

  
  Detail description
  ==

  * Under root, command 'virsh create-snapshot 1' is issued on booted
  and running KVM machine with debian inside.

  * After about four minutes, the process is done.

  * 'iotop' shows two 'kvm' processes reading/writing to disk. First one
  has IO around 1500 K/s, second one has around 400 K/s. That takes
  about three minutes. Then first process grabs about 3 M/s of IO and
  suddenly dissapears (1-2 sec). Then second process does about 7.5 M/s
  of IO for around a 1-2 minutes.

  * Snapshot is successfuly created and is usable for reverting or
  extracting.

  * Pretty much the same behaviour occurs when command 'savevm' is
  issued directly from qemu monitor, without using libvirt at all
  (actually, virsh snapshot-create just calls 'savevm' to the monitor
  socket).

  * This behaviour was observed on lucid, meerkat, natty and even with
  git version of libvirt (f44bfb7fb978c9313ce050a1c4149bf04aa0a670).
  Also slowsave packages from
  https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/524447 gave
  this issue.

  
  Thank you for helping to solve this issue!

  ProblemType: Bug
  DistroRelease: Ubuntu 11.04
  Package: libvirt-bin 0.8.8-1ubuntu5
  ProcVersionSignature: Ubuntu 2.6.38-7.38-server 2.6.38
  Uname: Linux 2.6.38-7-server x86_64
  Architecture: amd64
  Date: Thu Mar 24 12:19:41 2011
  InstallationMedia: Ubuntu-Server 10.04.2 LTS "Lucid Lynx" - Release amd64 
(20110211.1)
  ProcEnviron:
   LANG=en_US.UTF-8
   SHELL=/bin/bash
  SourcePackage: libvirt
  UpgradeStatus: No upgrade log present (probably fresh install)

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/741887/+subscriptions

[Qemu-devel] virtio-blk throughput

[Prateek Sharma]

Hello everyone,
   I am testing virtio-blk throughput (single thread, using hdparm -tT
). I want to know what is the guest/baremetal sequential read ratio
with the current qemu / qemu-kvm builds.
I have tried with qemu 1.0, 0.15, 0.14.1 , but my guest throughput is
limited to 65 MB/s. Baremetal is at 112 MB/s .

Here is my VM configuration

$QEMU  -cpu core2duo,+vmx  -drive file=$VM_PATH,if=virtio,aio=native
-drive file=viotest.img,if=virtio,index=2 -net tap -net
nic,macaddr=00:01:02:03:04:$VNC_OFFSET -kernel $KERNEL_IMAGE -append
'root=/dev/vda1 rw memmap=10M$100M kmemcheck=on' -m 1000 -vnc
0.0.0.0:$VNC_OFFSET

Is there something wrong in the configuration ? I was hoping
virtio-blk would give ~75% baremetal throughput atleast for large
streaming reads.

Thanks,
Prateek

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Blue Swirl]

On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
> As we have thread-local cpu_single_env now and KVM uses exactly one
> thread per VCPU, we can drop the cpu_single_env updates from the loop
> and initialize this variable only once during setup.

I don't think this is correct. Maybe you missed the part that sets
cpu_single_env to NULL, which I think is to annoy broken code that
assumes that some CPU state is always globally available. This is not
true for monitor context.

> Signed-off-by: Jan Kiszka 
> ---
>  cpus.c    |    1 +
>  kvm-all.c |    5 -
>  2 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index f45a438..d0c8340 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -714,6 +714,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>     qemu_mutex_lock(&qemu_global_mutex);
>     qemu_thread_get_self(env->thread);
>     env->thread_id = qemu_get_thread_id();
> +    cpu_single_env = env;
>
>     r = kvm_init_vcpu(env);
>     if (r < 0) {
> diff --git a/kvm-all.c b/kvm-all.c
> index c4babda..e2cbc03 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1118,8 +1118,6 @@ int kvm_cpu_exec(CPUState *env)
>         return EXCP_HLT;
>     }
>
> -    cpu_single_env = env;
> -
>     do {
>         if (env->kvm_vcpu_dirty) {
>             kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
> @@ -1136,13 +1134,11 @@ int kvm_cpu_exec(CPUState *env)
>              */
>             qemu_cpu_kick_self();
>         }
> -        cpu_single_env = NULL;
>         qemu_mutex_unlock_iothread();
>
>         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>
>         qemu_mutex_lock_iothread();
> -        cpu_single_env = env;
>         kvm_arch_post_run(env, run);
>
>         kvm_flush_coalesced_mmio_buffer();
> @@ -1206,7 +1202,6 @@ int kvm_cpu_exec(CPUState *env)
>     }
>
>     env->exit_request = 0;
> -    cpu_single_env = NULL;
>     return ret;
>  }
>
> --
> 1.7.3.4
>
>

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 11:02, Blue Swirl wrote:
> On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
>> As we have thread-local cpu_single_env now and KVM uses exactly one
>> thread per VCPU, we can drop the cpu_single_env updates from the loop
>> and initialize this variable only once during setup.
> 
> I don't think this is correct. Maybe you missed the part that sets
> cpu_single_env to NULL, which I think is to annoy broken code that
> assumes that some CPU state is always globally available. This is not
> true for monitor context.

I did check this before changing, and I see no such need. Particularly
as this old debugging help prevents valid use case.

Jan

> 
>> Signed-off-by: Jan Kiszka 
>> ---
>>  cpus.c|1 +
>>  kvm-all.c |5 -
>>  2 files changed, 1 insertions(+), 5 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index f45a438..d0c8340 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -714,6 +714,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>> qemu_mutex_lock(&qemu_global_mutex);
>> qemu_thread_get_self(env->thread);
>> env->thread_id = qemu_get_thread_id();
>> +cpu_single_env = env;
>>
>> r = kvm_init_vcpu(env);
>> if (r < 0) {
>> diff --git a/kvm-all.c b/kvm-all.c
>> index c4babda..e2cbc03 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -1118,8 +1118,6 @@ int kvm_cpu_exec(CPUState *env)
>> return EXCP_HLT;
>> }
>>
>> -cpu_single_env = env;
>> -
>> do {
>> if (env->kvm_vcpu_dirty) {
>> kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
>> @@ -1136,13 +1134,11 @@ int kvm_cpu_exec(CPUState *env)
>>  */
>> qemu_cpu_kick_self();
>> }
>> -cpu_single_env = NULL;
>> qemu_mutex_unlock_iothread();
>>
>> run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>>
>> qemu_mutex_lock_iothread();
>> -cpu_single_env = env;
>> kvm_arch_post_run(env, run);
>>
>> kvm_flush_coalesced_mmio_buffer();
>> @@ -1206,7 +1202,6 @@ int kvm_cpu_exec(CPUState *env)
>> }
>>
>> env->exit_request = 0;
>> -cpu_single_env = NULL;
>> return ret;
>>  }
>>
>> --
>> 1.7.3.4
>>
>>
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] ppc: remove unused variables

[Blue Swirl]

On Mon, Feb 6, 2012 at 15:57, Alexander Graf  wrote:
>
> On 06.02.2012, at 16:21, Andreas Färber wrote:
>
>> Am 04.02.2012 12:49, schrieb Blue Swirl:
>>> Fix this error:
>>> /src/qemu/target-ppc/helper.c: In function 'booke206_tlb_to_page_size':
>>> /src/qemu/target-ppc/helper.c:1296:14: error: variable 'tlbncfg' set
>>> but not used [-Werror=unused-but-set-variable]
>>>
>>> Signed-off-by: Blue Swirl 
>>
>> Tested-by: Andreas Färber 
>
> Yes, Blue, can you push it please?

Pushed.

>
>
> Alex
>

Re: [Qemu-devel] [PATCH] cfi02: Fix lazy ROMD switching - once again

[Blue Swirl]

Thanks, applied.

On Sat, Feb 4, 2012 at 14:58, Jan Kiszka  wrote:
> The conversion to memory regions broke lazy ROMD switching by forgetting
> to update the rom_mode state variable.
>
> Signed-off-by: Jan Kiszka 
> ---
>  hw/pflash_cfi02.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pflash_cfi02.c b/hw/pflash_cfi02.c
> index a9e88b9..2ca0fd4 100644
> --- a/hw/pflash_cfi02.c
> +++ b/hw/pflash_cfi02.c
> @@ -102,6 +102,7 @@ static void pflash_setup_mappings(pflash_t *pfl)
>  static void pflash_register_memory(pflash_t *pfl, int rom_mode)
>  {
>     memory_region_rom_device_set_readable(&pfl->orig_mem, rom_mode);
> +    pfl->rom_mode = rom_mode;
>  }
>
>  static void pflash_timer (void *opaque)
> --
> 1.7.3.4

Re: [Qemu-devel] [PATCH v3] memory-region: Report if region is read-only or write-only on info mtree

[Blue Swirl]

Thanks, applied.

On Sat, Feb 4, 2012 at 15:25, Jan Kiszka  wrote:
> From: Jan Kiszka 
>
> Helpful to understand guest configurations of things like the i440FX's
> PAM or the state of ROM devices.
>
> Signed-off-by: Jan Kiszka 
> ---
>
> Changes in v3:
>  - refactored writable condition (which DO makes sense as a ROM device
>   is either in ROM or in write mode. It's readable and writable in
>   both modes, but only in special ways. So let's tag it abstractly.)
>
>  memory.c |   14 +++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 5e77d8a..22816e2 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1609,23 +1609,31 @@ static void mtree_print_mr(fprintf_function 
> mon_printf, void *f,
>             ml->printed = false;
>             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
>         }
> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): alias 
> %s @%s "
> -                   TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
> +        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx
> +                   " (prio %d, %c%c): alias %s @%s " TARGET_FMT_plx
> +                   "-" TARGET_FMT_plx "\n",
>                    base + mr->addr,
>                    base + mr->addr
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>                    mr->priority,
> +                   mr->readable ? 'R' : '-',
> +                   !mr->readonly && !(mr->rom_device && mr->readable) ? 'W'
> +                                                                      : '-',
>                    mr->name,
>                    mr->alias->name,
>                    mr->alias_offset,
>                    mr->alias_offset
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1);
>     } else {
> -        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d): %s\n",
> +        mon_printf(f,
> +                   TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %c%c): 
> %s\n",
>                    base + mr->addr,
>                    base + mr->addr
>                    + (target_phys_addr_t)int128_get64(mr->size) - 1,
>                    mr->priority,
> +                   mr->readable ? 'R' : '-',
> +                   !mr->readonly && !(mr->rom_device && mr->readable) ? 'W'
> +                                                                      : '-',
>                    mr->name);
>     }
>
> --
> 1.7.3.4

Re: [Qemu-devel] [PATCH] vga: Fix full updates in graphic mode

[Blue Swirl]

Thanks, applied.

On Tue, Feb 7, 2012 at 15:03, Jan Kiszka  wrote:
> This fixes the regression introduced by cd7a45c95e: We lost the or'ing
> with the full_update flag.
>
> Signed-off-by: Jan Kiszka 
> ---
>
> Applies on top of the other related fix:
> http://thread.gmane.org/gmane.comp.emulators.qemu/134958
>
>  hw/vga.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/vga.c b/hw/vga.c
> index d27700d..c1029db 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1777,10 +1777,11 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>         if (!(s->cr[VGA_CRTC_MODE] & 2)) {
>             addr = (addr & ~0x8000) | ((y1 & 2) << 14);
>         }
> +        update = full_update;
>         page0 = addr;
>         page1 = addr + bwidth - 1;
> -        update = memory_region_get_dirty(&s->vram, page0, page1 - page0,
> -                                         DIRTY_MEMORY_VGA);
> +        update |= memory_region_get_dirty(&s->vram, page0, page1 - page0,
> +                                          DIRTY_MEMORY_VGA);
>         /* explicit invalidation for the hardware cursor */
>         update |= (s->invalidated_y_table[y >> 5] >> (y & 0x1f)) & 1;
>         if (update) {
> --
> 1.7.3.4

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Blue Swirl]

On Sat, Feb 11, 2012 at 10:06, Jan Kiszka  wrote:
> On 2012-02-11 11:02, Blue Swirl wrote:
>> On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
>>> As we have thread-local cpu_single_env now and KVM uses exactly one
>>> thread per VCPU, we can drop the cpu_single_env updates from the loop
>>> and initialize this variable only once during setup.
>>
>> I don't think this is correct. Maybe you missed the part that sets
>> cpu_single_env to NULL, which I think is to annoy broken code that
>> assumes that some CPU state is always globally available. This is not
>> true for monitor context.
>
> I did check this before changing, and I see no such need. Particularly
> as this old debugging help prevents valid use case.

It looks like monitor code is safe now. But in several places there
are checks like this in pc.c:
DeviceState *cpu_get_current_apic(void)
{
if (cpu_single_env) {
return cpu_single_env->apic_state;
} else {
return NULL;
}
}

In cpu-exec.c, there are these lines:
/* fail safe : never use cpu_single_env outside cpu_exec() */
cpu_single_env = NULL;

I think using cpu_single_env is an indication of a problem, like poor
code, layering violation or poor API (vmport). What is your use case?

>
> Jan
>
>>
>>> Signed-off-by: Jan Kiszka 
>>> ---
>>>  cpus.c    |    1 +
>>>  kvm-all.c |    5 -
>>>  2 files changed, 1 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index f45a438..d0c8340 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -714,6 +714,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>>     qemu_mutex_lock(&qemu_global_mutex);
>>>     qemu_thread_get_self(env->thread);
>>>     env->thread_id = qemu_get_thread_id();
>>> +    cpu_single_env = env;
>>>
>>>     r = kvm_init_vcpu(env);
>>>     if (r < 0) {
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index c4babda..e2cbc03 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -1118,8 +1118,6 @@ int kvm_cpu_exec(CPUState *env)
>>>         return EXCP_HLT;
>>>     }
>>>
>>> -    cpu_single_env = env;
>>> -
>>>     do {
>>>         if (env->kvm_vcpu_dirty) {
>>>             kvm_arch_put_registers(env, KVM_PUT_RUNTIME_STATE);
>>> @@ -1136,13 +1134,11 @@ int kvm_cpu_exec(CPUState *env)
>>>              */
>>>             qemu_cpu_kick_self();
>>>         }
>>> -        cpu_single_env = NULL;
>>>         qemu_mutex_unlock_iothread();
>>>
>>>         run_ret = kvm_vcpu_ioctl(env, KVM_RUN, 0);
>>>
>>>         qemu_mutex_lock_iothread();
>>> -        cpu_single_env = env;
>>>         kvm_arch_post_run(env, run);
>>>
>>>         kvm_flush_coalesced_mmio_buffer();
>>> @@ -1206,7 +1202,6 @@ int kvm_cpu_exec(CPUState *env)
>>>     }
>>>
>>>     env->exit_request = 0;
>>> -    cpu_single_env = NULL;
>>>     return ret;
>>>  }
>>>
>>> --
>>> 1.7.3.4
>>>
>>>
>>
>>
>
>

Re: [Qemu-devel] [BUG] checkpatch: ERROR due to * recognized as operator

[Andreas Färber]

Am 11.02.2012 10:26, schrieb Blue Swirl:
> On Thu, Feb 9, 2012 at 15:30, Andreas Färber  wrote:
>> Hello Blue,
>>
>> I recently stumbled over the following checkpatch.pl false positive:
>>
>> --8<--
>>
>> --- a/hw/his.c
>> +++ b/hw/his.c
>> @@ -1,1 +1,1 @@
>> -cpu_reset(CPUState *env);
>> +cpu_state_reset(CPUState *env);
>>
>> --- a/hw/hers.c
>> +++ b/hw/hers.c
>> @@ -1,1 +1,1 @@
>> -cpu_reset(CPUX86State *env);
>> +cpu_state_reset(CPUX86State *env);
>>
>> --- a/hw/its.c
>> +++ b/hw/its.c
>> @@ -1,1 +1,1 @@
>> -cpu_reset(CPUState *env);
>> +cpu_state_reset(CPUState *env);
>>
>> --- a/hw/theirs.c
>> +++ b/hw/theirs.c
>> @@ -1,2 +1,2 @@
>>  typedef struct CPUState CPUState;
>> -cpu_reset(CPUState *env);
>> +cpu_state_reset(CPUState *env);
>>
>> --8<--
>>
>> results in:
>>
>>
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #5: FILE: hw/his.c:1:
>> +cpu_state_reset(CPUState *env);
>>  ^
>>
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #11: FILE: hw/hers.c:1:
>> +cpu_state_reset(CPUX86State *env);
>> ^
>>
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #17: FILE: hw/its.c:1:
>> +cpu_state_reset(CPUState *env);
>>  ^
>>
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #24: FILE: hw/theirs.c:2:
>> +cpu_state_reset(CPUState *env);
>>  ^
>>
>> total: 4 errors, 0 warnings, 9 lines checked
>>
>>
>> So, it seems to interpret the * symbol as multiplication rather than
>> pointer.
>>
>> Surprisingly, in my real code, using CPUState in place of CPUX86State
>> was actually able to remedy the ERROR but not in this simplified test
>> case. I added some prints around that place and it seems, in the working
>> CPUState case it didn't even enter the op checking code path.
>>
>> Any ideas?
> 
> IIRC Linux does not use typedefs much, so maybe typedefs combined with
> #defines confuse checkpatch.pl.
> 
> But I don't know why this case would be different to:
> typedef long long int64_t;
> #define off_t int64_t
> void func(off_t *e);

I don't think typedefs matter but there was a specific regex for *_t.

Andreas

> 
> Perhaps using a typedef in place of #define could help but I'd not
> make such a change just to silence checkpatch.
> 
>>
>> Regards,
>> Andreas
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 7/8] optionsrom: Reserve space for checksum

[Andreas Färber]

Am 10.02.2012 19:31, schrieb Jan Kiszka:
> Always add a byte before the final 512-bytes alignment to reserve the
> space for the ROM checksum.
> 
> Signed-off-by: Jan Kiszka 
> ---
>  pc-bios/optionrom/optionrom.h |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h
> index aa783de..3daf7da 100644
> --- a/pc-bios/optionrom/optionrom.h
> +++ b/pc-bios/optionrom/optionrom.h
> @@ -124,7 +124,8 @@
>   movw%ax, %ds;
>  
>  #define OPTION_ROM_END   \
> -.align 512, 0;   \
> + .byte   0;  \
> + .align  512, 0; \

Tabs.

Andreas

>  _end:
>  
>  #define BOOT_ROM_END \

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

Am 11.02.2012 12:25, schrieb Blue Swirl:
> I think using cpu_single_env is an indication of a problem, like poor
> code, layering violation or poor API (vmport). What is your use case?

I couldn't spot any in this series. Jan, note that any new use of env or
cpu_single_env will need to be redone when we convert to QOM CPU.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] Restore consistent formatting

[Andreas Färber]

Am 11.02.2012 10:19, schrieb Blue Swirl:
> On Wed, Feb 8, 2012 at 15:36, Andreas Färber  wrote:
>> This is not about whether or not we put a space somewhere.
>>
>> It's about reviewers and SubmitAPatch telling people to run
>> checkpatch.pl on patches and checkpatch.pl reporting this as an ERROR,
>> not a WARNING. So if you follow Stefan's instructions on running the
>> script as a commit hook (which is the only sane way to run it when
>> handling lots of patches) you can't commit a patch or your local changes
>> when there are ERRORs.
> 
> The warning levels are from Linux, they could well be incorrect.

My memory tricked me, I must've mixed it up with the other issue I was
investigating (too little sleep and very annoyed about the monolithic
fragments of Perl source code and checkpatch.pl not conforming to its
own rules, e.g. tab indention and > 80 chars).

Still, my point remains: what shows up in checkpatch.pl output will get
fixed by new contributors, whether WARNING or ERROR.

And checkpatch.pl cannot distinguish between changes that correctly
update old-style code (e.g., adding braces for if) and code that aims
for consistency in a file.

Leaving checkpatch.pl unchanged and demanding this special case doesn't
go along well and will cause frustration on all sides and reformatting
back and forth, something that was declared forbidden elsewhere and
patches doing reformattings were turned down.

Andreas

P.S. For the record, Stefan pointed me to git-commit --no-verify for
ignoring checkpatch ERRORs.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 12:25, Blue Swirl wrote:
> On Sat, Feb 11, 2012 at 10:06, Jan Kiszka  wrote:
>> On 2012-02-11 11:02, Blue Swirl wrote:
>>> On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
 As we have thread-local cpu_single_env now and KVM uses exactly one
 thread per VCPU, we can drop the cpu_single_env updates from the loop
 and initialize this variable only once during setup.
>>>
>>> I don't think this is correct. Maybe you missed the part that sets
>>> cpu_single_env to NULL, which I think is to annoy broken code that
>>> assumes that some CPU state is always globally available. This is not
>>> true for monitor context.
>>
>> I did check this before changing, and I see no such need. Particularly
>> as this old debugging help prevents valid use case.
> 
> It looks like monitor code is safe now. But in several places there
> are checks like this in pc.c:
> DeviceState *cpu_get_current_apic(void)
> {
> if (cpu_single_env) {
> return cpu_single_env->apic_state;
> } else {
> return NULL;
> }
> }
> 
> In cpu-exec.c, there are these lines:
> /* fail safe : never use cpu_single_env outside cpu_exec() */
> cpu_single_env = NULL;

That's legacy stuff from the pre-io-thread times. Nowadays,
cpu_single_env is logically either always valid (KVM) or switching
seamlessly between the VCPUs (TCG).

> 
> I think using cpu_single_env is an indication of a problem, like poor
> code, layering violation or poor API (vmport). What is your use case?

We have a few poor ABIs like the VMware stuff or the KVM VAPIC, so we
have to live with it already for that use cases. Moreover, cpus.c use it
internally, like for vm_stop, to find out the caller context.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 12:49, Andreas Färber wrote:
> Am 11.02.2012 12:25, schrieb Blue Swirl:
>> I think using cpu_single_env is an indication of a problem, like poor
>> code, layering violation or poor API (vmport). What is your use case?
> 
> I couldn't spot any in this series. Jan, note that any new use of env or
> cpu_single_env will need to be redone when we convert to QOM CPU.

cpu_single_env should have nothing to do with QOM.

The ABIs of vmport and the KVM VAPI require a reference to the calling
VCPU, and that's why you find tons of them in patch 5.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 7/8] optionsrom: Reserve space for checksum

[Jan Kiszka]

On 2012-02-11 12:46, Andreas Färber wrote:
> Am 10.02.2012 19:31, schrieb Jan Kiszka:
>> Always add a byte before the final 512-bytes alignment to reserve the
>> space for the ROM checksum.
>>
>> Signed-off-by: Jan Kiszka 
>> ---
>>  pc-bios/optionrom/optionrom.h |3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h
>> index aa783de..3daf7da 100644
>> --- a/pc-bios/optionrom/optionrom.h
>> +++ b/pc-bios/optionrom/optionrom.h
>> @@ -124,7 +124,8 @@
>>  movw%ax, %ds;
>>  
>>  #define OPTION_ROM_END  \
>> -.align 512, 0;  \
>> +.byte   0;  \
>> +.align  512, 0; \
> 
> Tabs.

For sure, like in the whole file.

If a codestyle fix is desired, I'll post one for all assembly files. But
I guess there are different views on such changes.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 7/8] optionsrom: Reserve space for checksum

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 13:45, schrieb Jan Kiszka:
> On 2012-02-11 12:46, Andreas Färber wrote:
>> Am 10.02.2012 19:31, schrieb Jan Kiszka:
>>> Always add a byte before the final 512-bytes alignment to
>>> reserve the space for the ROM checksum.
>>> 
>>> Signed-off-by: Jan Kiszka  --- 
>>> pc-bios/optionrom/optionrom.h |3 ++- 1 files changed, 2
>>> insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/pc-bios/optionrom/optionrom.h
>>> b/pc-bios/optionrom/optionrom.h index aa783de..3daf7da 100644 
>>> --- a/pc-bios/optionrom/optionrom.h +++
>>> b/pc-bios/optionrom/optionrom.h @@ -124,7 +124,8 @@ movw%ax,
>>> %ds;
>>> 
>>> #define OPTION_ROM_END  \ -.align 
>>> 512, 0;   \ + .byte
>>> 0;  \ + .align  512, 0; 
>>> \
>> 
>> Tabs.
> 
> For sure, like in the whole file.

No, as we can see in this patch, .align above and _align below use 4
spaces, so this looks inconsistent in Thunderbird.

I don't really mind, just noticed and thought you might want to fix.

Andreas

> 
> If a codestyle fix is desired, I'll post one for all assembly
> files. But I guess there are different views on such changes.
> 
> Jan

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNmQ2AAoJEPou0S0+fgE/nnEQALknNqkUrCgdXTiRwaKCZehU
U+lnRwAY2DoFKaq8IqmvmiwNW2g4U0qPy0Stc7qvsOeTHSk/MX3R1Ym1+a03Hk22
tIzyJjvQPgVkbQtAljC7BObGzxGq9mUlV4+/TuTQgTsDXig121CmVVmrKXDRdP6o
vXmtLgkXQB75ArOQSr0sByCFuBbA+AlIcmtIpvlR6znMZcfp0QFYbAzfAINzad+A
Qg40GXUsFyyHlstM3Njt9H0ArtvE+NIPH9ZABk+58aV92CAk31rOigiiBAE1sPja
qR0AIEMmwnASEWsn/yX45b+lhxXfVadZNzobhnJ/KqG/K7hrfzcLIeD5pKMt8USu
TPFfdswt4utm/JDGhP9moRpe3cLGITAN93vybyOv/7W0CK18etFgevQvX0C98V9O
+FUjwqsOwfXkswXcglwYmUVqJarUooh1MlXF/QnS+/d7aefIl59Y/YaGzRryjZNm
Nd1hN7L5J34LfBCgltWNXA9KQB+0LyCYCX0lzE1+428WwqbZ8nGa9ZiUY0w4N2O7
maWrWnmI1VsiUL8RdsQpx7+nRfUEZW6JDfm1bA5nbd+uLuE+TwpAWWkC2kVOhnfi
UQggicoM6J88zcbiGT74whjfZgx7CXcE/F/Tncd5ebi5qi++jXB6C0f3LSFcx2KP
+Zv19XpWHzZXNNYiGFW0
=rQEQ
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 7/8] optionsrom: Reserve space for checksum

[Jan Kiszka]

On 2012-02-11 13:51, Andreas Färber wrote:
> Am 11.02.2012 13:45, schrieb Jan Kiszka:
>> On 2012-02-11 12:46, Andreas Färber wrote:
>>> Am 10.02.2012 19:31, schrieb Jan Kiszka:
 Always add a byte before the final 512-bytes alignment to
 reserve the space for the ROM checksum.

 Signed-off-by: Jan Kiszka  --- 
 pc-bios/optionrom/optionrom.h |3 ++- 1 files changed, 2
 insertions(+), 1 deletions(-)

 diff --git a/pc-bios/optionrom/optionrom.h
 b/pc-bios/optionrom/optionrom.h index aa783de..3daf7da 100644 
 --- a/pc-bios/optionrom/optionrom.h +++
 b/pc-bios/optionrom/optionrom.h @@ -124,7 +124,8 @@ movw   %ax,
 %ds;

 #define OPTION_ROM_END \ -.align 
 512, 0;   \ + .byte
 0; \ + .align  512, 0; 
 \
>>>
>>> Tabs.
> 
>> For sure, like in the whole file.
> 
> No, as we can see in this patch, .align above and _align below use 4
> spaces, so this looks inconsistent in Thunderbird.

Right, but that is consistent with other labels in this file. OTOH,
label indention is different again in the option rom source files. Well,
no optimal solution here... ;)

Thanks,
Jan

> 
> I don't really mind, just noticed and thought you might want to fix.
> 
> Andreas
> 
> 
>> If a codestyle fix is desired, I'll post one for all assembly
>> files. But I guess there are different views on such changes.
> 
>> Jan
> 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 13:43, schrieb Jan Kiszka:
> On 2012-02-11 12:49, Andreas Färber wrote:
>> Am 11.02.2012 12:25, schrieb Blue Swirl:
>>> I think using cpu_single_env is an indication of a problem,
>>> like poor code, layering violation or poor API (vmport). What
>>> is your use case?
>> 
>> I couldn't spot any in this series. Jan, note that any new use of
>> env or cpu_single_env will need to be redone when we convert to
>> QOM CPU.
> 
> cpu_single_env should have nothing to do with QOM.

It does, cf. my patch series: Current CPU*State is being embedded in
the QOM object and most future code outside TCG will use a CPU rather
than CPUState pointer. The reason is that CPUState is totally
target-specific and does not belong in common code.

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNme8AAoJEPou0S0+fgE/bPcP/RRc85K6aJZEqRyw/lvN8+FB
2FtwOqCm6zTBiEfBOfs816YzBDl75F5BVRbNapMLi1Yp4y/BFwQF1lbpu7INF90R
ZvY5BjjW8+xjBbGN0BhmkbjKdXZS1spjYNXDjIcUTvfj/GXW8Aamfj4IQVTpd+0D
l1s6A/X4BgGoxEqLtnHi8mZojafFFW6Dy0tX7BOmAPwBJle+IK91huO/cmL3Ou3v
0X1Rl4UJlq7j5AxFZlbBkkMrB9vozMPZi983SpAyhieQTVqTB+XuRobwZZVWww0m
ff2cBPBckFSF+i5L7eWvL+HfCD2aeYgwTCmfxtxOxjThwvM7gkyz59gQznUmb3yZ
0SLi9aj0dYQkuidoLxORZaAG20pqfvGCMezJQ6p45jhGmq7W3RzMqJX5Hh7GN0bY
J+Yp1W/Svop9XS1MumERufO6E1+2TNpbtwGDizKV52DpT2dTtwQZJ9UjHUvLz52c
avM5DvuuYLGDIyMteURoAh1eo27kfHFZs9vI6HFK3uXrmihgGihtzlVvFxf887kR
LWt/QO8K/VzuktRKj9NutiMqJOUxIzddikxpkEU/80FOtMedy1Ne1cVpMWOTqXRh
U0iayaZ3FKK+NfSYgHjSGCHubJG3JwV/Hawu01nWuUR1aGOsbQuxm1sNcQ+VV1zJ
iGvD5Fdn+9+o+UTJSkiQ
=zss6
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 14:06, Andreas Färber wrote:
> Am 11.02.2012 13:43, schrieb Jan Kiszka:
>> On 2012-02-11 12:49, Andreas Färber wrote:
>>> Am 11.02.2012 12:25, schrieb Blue Swirl:
 I think using cpu_single_env is an indication of a problem,
 like poor code, layering violation or poor API (vmport). What
 is your use case?
>>>
>>> I couldn't spot any in this series. Jan, note that any new use of
>>> env or cpu_single_env will need to be redone when we convert to
>>> QOM CPU.
> 
>> cpu_single_env should have nothing to do with QOM.
> 
> It does, cf. my patch series: Current CPU*State is being embedded in
> the QOM object and most future code outside TCG will use a CPU rather
> than CPUState pointer. The reason is that CPUState is totally
> target-specific and does not belong in common code.

So are the devices that depend on a current CPU pointer. You will have
to provide something equivalent.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCHv3 0/3] unicore32: add unicore32-linux-user support for qemu 0.14

[Andreas Färber]

Hello,

Am 12.04.2011 10:25, schrieb Guan Xuetao:
> 
> The patch set adds new unicore32-linux-user support for qemu-stable-0.14
> Patch 1 adds target-unicore32 directory
> Patch 2 adds linux-user/unicore32 directory
> Patch 3 adds necessary modifications for other files
> 
> V2->V3: rebase on master branch of qemu
> 
> V1 -> V2: changed by advice from Blue Swirl
> 
> Guan Xuetao (3):
>   unicore32: add target-unicore32 directory for unicore32-linux-user
> support
>   unicore32: add necessry headers in linux-user/unicore32 for unicore32
> support
>   unicore32: necessary modifications for other files to support
> unicore32

>  create mode 100644 target-unicore32/cpu.h
>  create mode 100644 target-unicore32/exec.h
>  create mode 100644 target-unicore32/helper.c
>  create mode 100644 target-unicore32/helper.h
>  create mode 100644 target-unicore32/op_helper.c
>  create mode 100644 target-unicore32/translate.c

Last year you added a unicore32 target to QEMU. The new files you added
in target-unicore32/ carry the following license notice:

 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.

Are you able to change this to the original GNU GPLv2 version notice
that has "or (at your option) any later version"?

 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see
 * 

I would like to add QOM support to unicore32 and would very much like to
have that under a future-proof license.

Cf. http://wiki.qemu.org/Relicensing

Thanks in advance,

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 14:07, schrieb Jan Kiszka:
> On 2012-02-11 14:06, Andreas Färber wrote:
>> Am 11.02.2012 13:43, schrieb Jan Kiszka:
>>> On 2012-02-11 12:49, Andreas Färber wrote:
 Am 11.02.2012 12:25, schrieb Blue Swirl:
> I think using cpu_single_env is an indication of a
> problem, like poor code, layering violation or poor API
> (vmport). What is your use case?
 
 I couldn't spot any in this series. Jan, note that any new
 use of env or cpu_single_env will need to be redone when we
 convert to QOM CPU.
>> 
>>> cpu_single_env should have nothing to do with QOM.
>> 
>> It does, cf. my patch series: Current CPU*State is being embedded
>> in the QOM object and most future code outside TCG will use a

Let me stress this:

>> CPU rather than CPUState pointer.

>> The reason is that CPUState is totally target-specific and does
>> not belong in common code.
> 
> So are the devices that depend on a current CPU pointer. You will
> have to provide something equivalent.

CPU base class v3:
http://patchwork.ozlabs.org/patch/139284/ (v4 coming up)

That doesn't prevent target-specific devices. Although Paolo does want
that to change wrt properties.

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNmtzAAoJEPou0S0+fgE/SRQP+gLK/FvwIOXZqvSn+i+ooxin
jXOvH3oBtfiIQp5+59KGlOd7dSjILFwoPtH3U5tGDpI5HHLFpQQOsuppsiBwVOC9
9QUgqFt9d/xodvPJ0gv5ShghoEmCZNdFwNnBYeqB69mEDm5sZwYlvWgXaOgRti2+
0lhGFVISetImmQbiy5l7ubMONwcGUCVuT7pjiZ+S/Cew7wvGW5O7fpo3P8b4Xw4E
P7qX6y785Sm4Wn8iEangFOUqer5ALAS0fL2xHo5NYUUZ8jgn2xwDIT8TP9t8Pkei
5U0kWm+mNyvJ4VLxsN449LNGDV+c3AMyzPodRmV2KJBYISDRIFYlar/SkJGiBkvo
cNKdJLrkm4KIEt6eomyhYgSHJi5nUeoT60lAaZkHIDNonKoFw8swhf85wSi7sQmq
38nIY+F5YAHZ3TQCfTfxTDHy2Wbc6G7bn792FWKOxCVLWtD2Bp3iQv8J3MlYEhMJ
fnJv+/nKUQuPlti4LNwrhJyRLPUNrc6PKgzC8He4dupLMASFPuSMh4mKRlWj41+/
SYKvXz42elSqv2Z798eA8VNCbs7e+0EH67BJQLIL3QEuD4vY/Yfeulr3CGsfkLEL
m+UIAAntzloSkZvxuKmI5MP5XrjHTAbWuab+Gh9kYVyEsWZj4TAvn1hobKtU7sv9
lMd32AbfCskRN8jAw8So
=0Rvo
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 14:21, Andreas Färber wrote:
> Am 11.02.2012 14:07, schrieb Jan Kiszka:
>> On 2012-02-11 14:06, Andreas Färber wrote:
>>> Am 11.02.2012 13:43, schrieb Jan Kiszka:
 On 2012-02-11 12:49, Andreas Färber wrote:
> Am 11.02.2012 12:25, schrieb Blue Swirl:
>> I think using cpu_single_env is an indication of a
>> problem, like poor code, layering violation or poor API
>> (vmport). What is your use case?
>
> I couldn't spot any in this series. Jan, note that any new
> use of env or cpu_single_env will need to be redone when we
> convert to QOM CPU.
>>>
 cpu_single_env should have nothing to do with QOM.
>>>
>>> It does, cf. my patch series: Current CPU*State is being embedded
>>> in the QOM object and most future code outside TCG will use a
> 
> Let me stress this:
> 
>>> CPU rather than CPUState pointer.
> 
>>> The reason is that CPUState is totally target-specific and does
>>> not belong in common code.
> 
>> So are the devices that depend on a current CPU pointer. You will
>> have to provide something equivalent.
> 
> CPU base class v3:
> http://patchwork.ozlabs.org/patch/139284/ (v4 coming up)
> 
> That doesn't prevent target-specific devices. Although Paolo does want
> that to change wrt properties.

This patch doesn't explain yet what shall happen to cpu_single_env and
CPUState accesses from target-specific devices. Do you plan accessors
for registers? And a service to return the CPU object associated with
the execution context?

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Blue Swirl]

On Sat, Feb 11, 2012 at 12:43, Jan Kiszka  wrote:
> On 2012-02-11 12:49, Andreas Färber wrote:
>> Am 11.02.2012 12:25, schrieb Blue Swirl:
>>> I think using cpu_single_env is an indication of a problem, like poor
>>> code, layering violation or poor API (vmport). What is your use case?
>>
>> I couldn't spot any in this series. Jan, note that any new use of env or
>> cpu_single_env will need to be redone when we convert to QOM CPU.
>
> cpu_single_env should have nothing to do with QOM.
>
> The ABIs of vmport and the KVM VAPI require a reference to the calling
> VCPU, and that's why you find tons of them in patch 5.

Yes, this seems to be another case of a badly designed ABI. I guess
there is no way to change that anymore, just like vmport?

Some of the cpu_single_env accesses in patch 5 could be avoided when
APIC is moved closer to CPU. VAPIC should be also close to APIC so it
should be able to access the CPU directly. In some other cases the
current state could be passed around instead once it is known.

>
> Jan
>

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 14:35, schrieb Jan Kiszka:
> On 2012-02-11 14:21, Andreas Färber wrote:
>> CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4
>> coming up)
>> 
>> That doesn't prevent target-specific devices. Although Paolo does
>> want that to change wrt properties.
> 
> This patch doesn't explain yet what shall happen to cpu_single_env
> and CPUState accesses from target-specific devices.

True. We can't change them before all targets are converted. So far I
have 3/14 and still some review comments to work in.

Another patch in that series uses a macro
s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while we
convert targets.

Depending on our taste, cpu_single_env might become cpu_single_cpu,
single_cpu or cpu_single.

> Do you plan accessors for registers?

No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, etc.
and their CPU*State. It would be possible to have static inline
accessors but so far I've seen no need.

> And a service to return the CPU object associated with the
> execution context?

What do you mean by execution context? TLS? The modelling of the state
is pretty orthogonal to how/where we store it.

HTE,

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNnQvAAoJEPou0S0+fgE/yYAP/j4IUTrrP1invYoVLOiea7wn
9yJ3TgssSUPnQyQRUmkXFvx1hj0Z6umlyxTK+dc1DQF8jJtoouo3CS0D+tyIZEhd
GHN9J0qtgiDzvl1c+3b092VTw47gtv+rXjGUcyKSiTqyGl3OCdbIgt21HK8cT6CN
U7n2pFGBeZiX8GEYiZmhAglyJ45jGpWmVulGYiqzOlBYPLaYi0CQ25NVUalBpq4I
MIEqdW/W8lx0+h5Onl+qUo2btHNQCnG1oPH/BmdVf7Pe1G99VynOXwFVNXeJ2gR4
Lb7wnzKFqdybktkNLtkAIuC0gFj+Ph9+wfVw/QGsCBc0r6gotE8O9uDTyxs1ro+2
in6Am/A+o4M02sOf9uhaYx0l3uryOyXifiIAVzj4Y8s0QhyeDfPa6f8p4iQKh+gE
m/bvbDTb5hU9nW68IuiFXK9dfQMmU2ub5Gx7UAHuyOgEzV0gOPAf4nugYux3owIw
kYJy6sWFUMH/+l94nKAI0FanmL6JSOmA8hfaLXCXOfvfX9CfJEN+KEotj7Ma0Tcz
+YFAlGkwZYmnJmvFakxFlecRUYY/lpwlIusqRJsw1KP40pHuT8GZUDzv6Wn1UD2R
/6xeL7007iUmD+mafc+3xFbKMXS+kyF6+syM3xh/7r1SRyAIZQeJnZvLm8pZXS0T
tsKlb6nYaV+NLwD/rKy0
=09lZ
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 14:54, Blue Swirl wrote:
> On Sat, Feb 11, 2012 at 12:43, Jan Kiszka  wrote:
>> On 2012-02-11 12:49, Andreas Färber wrote:
>>> Am 11.02.2012 12:25, schrieb Blue Swirl:
 I think using cpu_single_env is an indication of a problem, like poor
 code, layering violation or poor API (vmport). What is your use case?
>>>
>>> I couldn't spot any in this series. Jan, note that any new use of env or
>>> cpu_single_env will need to be redone when we convert to QOM CPU.
>>
>> cpu_single_env should have nothing to do with QOM.
>>
>> The ABIs of vmport and the KVM VAPI require a reference to the calling
>> VCPU, and that's why you find tons of them in patch 5.
> 
> Yes, this seems to be another case of a badly designed ABI. I guess
> there is no way to change that anymore, just like vmport?

Believe me, I grumbled over it more than once while porting it from
qemu-kvm. The point is that some (Windows) VMs out there are running
already with this option ROM loaded and working this unfortunate ABI.

> 
> Some of the cpu_single_env accesses in patch 5 could be avoided when
> APIC is moved closer to CPU. VAPIC should be also close to APIC so it
> should be able to access the CPU directly. In some other cases the
> current state could be passed around instead once it is known.

Some callbacks are I/O-port originated, ie. not associated with the
per-CPU MMIO area or some MSR. So we would have to pass down the causing
CPU to every I/O handler - not sure if that is desired...

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 14:59, Andreas Färber wrote:
> Am 11.02.2012 14:35, schrieb Jan Kiszka:
>> On 2012-02-11 14:21, Andreas Färber wrote:
>>> CPU base class v3: http://patchwork.ozlabs.org/patch/139284/ (v4
>>> coming up)
>>>
>>> That doesn't prevent target-specific devices. Although Paolo does
>>> want that to change wrt properties.
> 
>> This patch doesn't explain yet what shall happen to cpu_single_env
>> and CPUState accesses from target-specific devices.
> 
> True. We can't change them before all targets are converted. So far I
> have 3/14 and still some review comments to work in.
> 
> Another patch in that series uses a macro
> s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while we
> convert targets.
> 
> Depending on our taste, cpu_single_env might become cpu_single_cpu,
> single_cpu or cpu_single.
> 
>> Do you plan accessors for registers?
> 
> No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU, etc.
> and their CPU*State. It would be possible to have static inline
> accessors but so far I've seen no need.

Then the devices need to have access to a CPUState pointer, just as so far.

> 
>> And a service to return the CPU object associated with the
>> execution context?
> 
> What do you mean by execution context? TLS? The modelling of the state
> is pretty orthogonal to how/where we store it.

I mean "Where come this I/O access from?" and "am I running over some
VCPU thread?". This questions need to be answered in target-specific
device models and some parts of cpus.c (the latter is one motivation for
this patch).

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Blue Swirl]

On Sat, Feb 11, 2012 at 14:00, Jan Kiszka  wrote:
> On 2012-02-11 14:54, Blue Swirl wrote:
>> On Sat, Feb 11, 2012 at 12:43, Jan Kiszka  wrote:
>>> On 2012-02-11 12:49, Andreas Färber wrote:
 Am 11.02.2012 12:25, schrieb Blue Swirl:
> I think using cpu_single_env is an indication of a problem, like poor
> code, layering violation or poor API (vmport). What is your use case?

 I couldn't spot any in this series. Jan, note that any new use of env or
 cpu_single_env will need to be redone when we convert to QOM CPU.
>>>
>>> cpu_single_env should have nothing to do with QOM.
>>>
>>> The ABIs of vmport and the KVM VAPI require a reference to the calling
>>> VCPU, and that's why you find tons of them in patch 5.
>>
>> Yes, this seems to be another case of a badly designed ABI. I guess
>> there is no way to change that anymore, just like vmport?
>
> Believe me, I grumbled over it more than once while porting it from
> qemu-kvm. The point is that some (Windows) VMs out there are running
> already with this option ROM loaded and working this unfortunate ABI.

Maybe in time those could be deprecated and a ROM using a sane ABI
introduced instead. After some grace time the old ABI could be finally
removed.

>>
>> Some of the cpu_single_env accesses in patch 5 could be avoided when
>> APIC is moved closer to CPU. VAPIC should be also close to APIC so it
>> should be able to access the CPU directly. In some other cases the
>> current state could be passed around instead once it is known.
>
> Some callbacks are I/O-port originated, ie. not associated with the
> per-CPU MMIO area or some MSR. So we would have to pass down the causing
> CPU to every I/O handler - not sure if that is desired...

I meant things like vapic_enable_tpr_reporting(), current CPUState
could be passed via vapic_prepare() easily.

> Jan
>

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 15:02, schrieb Jan Kiszka:
> On 2012-02-11 14:59, Andreas Färber wrote:
>> Am 11.02.2012 14:35, schrieb Jan Kiszka:
>>> On 2012-02-11 14:21, Andreas Färber wrote:
 CPU base class v3: http://patchwork.ozlabs.org/patch/139284/
 (v4 coming up)
 
 That doesn't prevent target-specific devices. Although Paolo
 does want that to change wrt properties.
>> 
>>> This patch doesn't explain yet what shall happen to
>>> cpu_single_env and CPUState accesses from target-specific
>>> devices.
>> 
>> True. We can't change them before all targets are converted. So
>> far I have 3/14 and still some review comments to work in.
>> 
>> Another patch in that series uses a macro 
>> s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while
>> we convert targets.
>> 
>> Depending on our taste, cpu_single_env might become
>> cpu_single_cpu, single_cpu or cpu_single.
>> 
>>> Do you plan accessors for registers?
>> 
>> No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU,
>> etc. and their CPU*State. It would be possible to have static
>> inline accessors but so far I've seen no need.
> 
> Then the devices need to have access to a CPUState pointer, just as
> so far.

Yes and no. They can have any target-specific pointer they want, just
as before. But no global first_cpu / cpu_single_env pointer - that's
replaced by CPU pointers, through which members of derived classes can
be accessed (which did not work for CPUState due to CPU_COMMON members
being at target-specific offset in the middle).

There's nothing wrong with your patch per se, just that it may need to
get refactored some time soonish. We need to be aware of it so that we
don't create merge conflicts for Anthony.

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNndNAAoJEPou0S0+fgE/+EoQAJFau/xsn2CYcuKPEsJmAMRk
yhOPBT6EJ2Q+34h31uLr3iftxQO9JpnLfEhB7ekTs36i0GklUsCQKgn4rg6vmPKj
tLvUk/hF4zuqJzUJOwxnYxYjuzdEGHuEbkCYgclUtNnywHCo3GLXhqP0izSds9mF
MhmqD45GblecjUpH7zdM/WTvulQm824hbDFPTCQaH8IQsw0QxT1Y4B71gpQtFJvJ
pVk2+qfc488ClhOhPISC5IiQFPnR7DVju82FuDgn6JFq/db9o3KXqIRlQg7pqkPc
h4K+Nz/rhzWpR6jtbTKqJV3yWBV9vxs6YDMSICZnGBabTlHh+tKoabg25Aj5zbcM
6Dmw10uFybi+jKlygKiSSxExParaRC9B3EFCk4dUhMC28B+qFSEkRA62Qpjndxwg
HCmzg2kSQpufyrWNdWj8W+mNygU/0rm8xcB7fX1vhSOmdu3DNTPIH7P4C9hOfC1g
hdIo0DpSd4AFfEIjZ0Loq0XOWKO9V05pOlcVsGmnCmGmfPXFPHWCFq3LGPz9Bj/7
rK1YtReDMXFOhq+QsOuRDuz1pCpPEfT4YhiXRuPsLlIaSszjFx3i6WAxBmh/tTtA
oxoGZQPUI3SRZYZPN5W+J5HqRyNkB16ffsrbcHVTmCrUm33yT+7a6S/vPE9NlZpm
zy92ShUp7JDvFjtnyOLK
=uTCH
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 2/8] Allow to use pause_all_vcpus from VCPU context

[Blue Swirl]

On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
> In order to perform critical manipulations on the VM state in the
> context of a VCPU, specifically code patching, stopping and resuming of
> all VCPUs may be necessary. resume_all_vcpus is already compatible, now
> enable pause_all_vcpus for this use case by stopping the calling context
> before starting to wait for the whole gang.
>
> CC: Paolo Bonzini 
> Signed-off-by: Jan Kiszka 
> ---
>  cpus.c |   12 
>  1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index d0c8340..5adfc6b 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -870,6 +870,18 @@ void pause_all_vcpus(void)
>         penv = (CPUState *)penv->next_cpu;
>     }
>
> +    if (!qemu_thread_is_self(&io_thread)) {
> +        cpu_stop_current();
> +        if (!kvm_enabled()) {
> +            while (penv) {
> +                penv->stop = 0;
> +                penv->stopped = 1;
> +                penv = (CPUState *)penv->next_cpu;

The cast is useless, next_cpu is already CPUState *. I wonder why it
is used in other cases too.

> +            }
> +            return;
> +        }
> +    }
> +
>     while (!all_vcpus_paused()) {
>         qemu_cond_wait(&qemu_pause_cond, &qemu_global_mutex);
>         penv = first_cpu;
> --
> 1.7.3.4
>
>

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 15:11, Blue Swirl wrote:
> On Sat, Feb 11, 2012 at 14:00, Jan Kiszka  wrote:
>> On 2012-02-11 14:54, Blue Swirl wrote:
>>> On Sat, Feb 11, 2012 at 12:43, Jan Kiszka  wrote:
 On 2012-02-11 12:49, Andreas Färber wrote:
> Am 11.02.2012 12:25, schrieb Blue Swirl:
>> I think using cpu_single_env is an indication of a problem, like poor
>> code, layering violation or poor API (vmport). What is your use case?
>
> I couldn't spot any in this series. Jan, note that any new use of env or
> cpu_single_env will need to be redone when we convert to QOM CPU.

 cpu_single_env should have nothing to do with QOM.

 The ABIs of vmport and the KVM VAPI require a reference to the calling
 VCPU, and that's why you find tons of them in patch 5.
>>>
>>> Yes, this seems to be another case of a badly designed ABI. I guess
>>> there is no way to change that anymore, just like vmport?
>>
>> Believe me, I grumbled over it more than once while porting it from
>> qemu-kvm. The point is that some (Windows) VMs out there are running
>> already with this option ROM loaded and working this unfortunate ABI.
> 
> Maybe in time those could be deprecated and a ROM using a sane ABI
> introduced instead. After some grace time the old ABI could be finally
> removed.

At some point. But now we have this interface and no other even thought
out. Given that we want to provide a migration path from qemu-kvm to
upstream rather sooner than later, I think there is no way around this
model for a certain, not too short period.

> 
>>>
>>> Some of the cpu_single_env accesses in patch 5 could be avoided when
>>> APIC is moved closer to CPU. VAPIC should be also close to APIC so it
>>> should be able to access the CPU directly. In some other cases the
>>> current state could be passed around instead once it is known.
>>
>> Some callbacks are I/O-port originated, ie. not associated with the
>> per-CPU MMIO area or some MSR. So we would have to pass down the causing
>> CPU to every I/O handler - not sure if that is desired...
> 
> I meant things like vapic_enable_tpr_reporting(), current CPUState
> could be passed via vapic_prepare() easily.

Oh, there is in fact dead code in vapic_enable_tpr_reporting. It just
iterates over all VCPUs, no need to know the current one.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 2/4] cadence_ttc: initial version of device model

[Paul Brook]

> +static void cadence_timer_sync(CadenceTimerState *s)
> +{
>...
> +r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
> +x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r :
> r); +
> +for (i = 0; i < 3; ++i) {
> +if (is_between((int64_t)s->reg_match[i] << 16, s->reg_value, x)) {
> +s->reg_intr |= (2 << i);
> +}
> +}

By my reading this will miss events if they happen after the timer wraps.
e.g. for a count-up timer with reg_match==1 and the tick callback happens to 
get delayed by 4 cycles, the timer may wrap straight to reg_value = 3.

Paul

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Blue Swirl]

On Sat, Feb 11, 2012 at 14:18, Jan Kiszka  wrote:
> On 2012-02-11 15:11, Blue Swirl wrote:
>> On Sat, Feb 11, 2012 at 14:00, Jan Kiszka  wrote:
>>> On 2012-02-11 14:54, Blue Swirl wrote:
 On Sat, Feb 11, 2012 at 12:43, Jan Kiszka  wrote:
> On 2012-02-11 12:49, Andreas Färber wrote:
>> Am 11.02.2012 12:25, schrieb Blue Swirl:
>>> I think using cpu_single_env is an indication of a problem, like poor
>>> code, layering violation or poor API (vmport). What is your use case?
>>
>> I couldn't spot any in this series. Jan, note that any new use of env or
>> cpu_single_env will need to be redone when we convert to QOM CPU.
>
> cpu_single_env should have nothing to do with QOM.
>
> The ABIs of vmport and the KVM VAPI require a reference to the calling
> VCPU, and that's why you find tons of them in patch 5.

 Yes, this seems to be another case of a badly designed ABI. I guess
 there is no way to change that anymore, just like vmport?
>>>
>>> Believe me, I grumbled over it more than once while porting it from
>>> qemu-kvm. The point is that some (Windows) VMs out there are running
>>> already with this option ROM loaded and working this unfortunate ABI.
>>
>> Maybe in time those could be deprecated and a ROM using a sane ABI
>> introduced instead. After some grace time the old ABI could be finally
>> removed.
>
> At some point. But now we have this interface and no other even thought
> out. Given that we want to provide a migration path from qemu-kvm to
> upstream rather sooner than later, I think there is no way around this
> model for a certain, not too short period.

Yes, that is inevitable.

>>

 Some of the cpu_single_env accesses in patch 5 could be avoided when
 APIC is moved closer to CPU. VAPIC should be also close to APIC so it
 should be able to access the CPU directly. In some other cases the
 current state could be passed around instead once it is known.
>>>
>>> Some callbacks are I/O-port originated, ie. not associated with the
>>> per-CPU MMIO area or some MSR. So we would have to pass down the causing
>>> CPU to every I/O handler - not sure if that is desired...
>>
>> I meant things like vapic_enable_tpr_reporting(), current CPUState
>> could be passed via vapic_prepare() easily.
>
> Oh, there is in fact dead code in vapic_enable_tpr_reporting. It just
> iterates over all VCPUs, no need to know the current one.

Ok.

> Jan
>

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Jan Kiszka]

On 2012-02-11 15:12, Andreas Färber wrote:
> Am 11.02.2012 15:02, schrieb Jan Kiszka:
>> On 2012-02-11 14:59, Andreas Färber wrote:
>>> Am 11.02.2012 14:35, schrieb Jan Kiszka:
 On 2012-02-11 14:21, Andreas Färber wrote:
> CPU base class v3: http://patchwork.ozlabs.org/patch/139284/
> (v4 coming up)
>
> That doesn't prevent target-specific devices. Although Paolo
> does want that to change wrt properties.
>>>
 This patch doesn't explain yet what shall happen to
 cpu_single_env and CPUState accesses from target-specific
 devices.
>>>
>>> True. We can't change them before all targets are converted. So
>>> far I have 3/14 and still some review comments to work in.
>>>
>>> Another patch in that series uses a macro 
>>> s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU while
>>> we convert targets.
>>>
>>> Depending on our taste, cpu_single_env might become
>>> cpu_single_cpu, single_cpu or cpu_single.
>>>
 Do you plan accessors for registers?
>>>
>>> No, registers are in target-specific ARMCPU, S390CPU, MIPSCPU,
>>> etc. and their CPU*State. It would be possible to have static
>>> inline accessors but so far I've seen no need.
> 
>> Then the devices need to have access to a CPUState pointer, just as
>> so far.
> 
> Yes and no. They can have any target-specific pointer they want, just
> as before. But no global first_cpu / cpu_single_env pointer - that's

If you want to drop first_cpu, you need to provide a for_each_cpu
iterating service instead. And cpu_single_env can only be obsoleted if
I/O access handlers can otherwise query the triggering CPU.

> replaced by CPU pointers, through which members of derived classes can
> be accessed (which did not work for CPUState due to CPU_COMMON members
> being at target-specific offset in the middle).
> 
> There's nothing wrong with your patch per se, just that it may need to
> get refactored some time soonish. We need to be aware of it so that we
> don't create merge conflicts for Anthony.

There can't be logical merge conflicts as the no fundamentally new
requirements are introduced with this series. And we have no code
proposal seen yet to address them already for the existing use cases.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 2/8] Allow to use pause_all_vcpus from VCPU context

[Jan Kiszka]

On 2012-02-11 15:16, Blue Swirl wrote:
> On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
>> In order to perform critical manipulations on the VM state in the
>> context of a VCPU, specifically code patching, stopping and resuming of
>> all VCPUs may be necessary. resume_all_vcpus is already compatible, now
>> enable pause_all_vcpus for this use case by stopping the calling context
>> before starting to wait for the whole gang.
>>
>> CC: Paolo Bonzini 
>> Signed-off-by: Jan Kiszka 
>> ---
>>  cpus.c |   12 
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index d0c8340..5adfc6b 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -870,6 +870,18 @@ void pause_all_vcpus(void)
>> penv = (CPUState *)penv->next_cpu;
>> }
>>
>> +if (!qemu_thread_is_self(&io_thread)) {
>> +cpu_stop_current();
>> +if (!kvm_enabled()) {
>> +while (penv) {
>> +penv->stop = 0;
>> +penv->stopped = 1;
>> +penv = (CPUState *)penv->next_cpu;
> 
> The cast is useless, next_cpu is already CPUState *. I wonder why it
> is used in other cases too.

Indeed, weird. We can clean the others up separately.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 3/8] target-i386: Add infrastructure for reporting TPR MMIO accesses

[Blue Swirl]

On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
> This will allow the APIC core to file a TPR access report. Depending on
> the accelerator and kernel irqchip mode, it will either be delivered
> right away or queued for later reporting.
>
> In TCG mode, we can restart the triggering instruction and can therefore
> forward the event directly. KVM does not allows us to restart, so we
> postpone the delivery of events recording in the user space APIC until
> the current instruction is completed.
>
> Note that KVM without in-kernel irqchip will report the address after
> the instruction that triggered a write access. In contrast, read
> accesses will return the precise information.
>
> Signed-off-by: Jan Kiszka 
> ---
>  cpu-all.h            |    3 ++-
>  hw/apic.h            |    2 ++
>  hw/apic_common.c     |    4 
>  target-i386/cpu.h    |    9 +
>  target-i386/helper.c |   19 +++
>  target-i386/kvm.c    |   24 ++--
>  6 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index e2c3c49..80e6d42 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -375,8 +375,9 @@ DECLARE_TLS(CPUState *,cpu_single_env);
>  #define CPU_INTERRUPT_TGT_INT_0   0x0100
>  #define CPU_INTERRUPT_TGT_INT_1   0x0400
>  #define CPU_INTERRUPT_TGT_INT_2   0x0800
> +#define CPU_INTERRUPT_TGT_INT_3   0x2000
>
> -/* First unused bit: 0x2000.  */
> +/* First unused bit: 0x4000.  */
>
>  /* The set of all bits that should be masked when single-stepping.  */
>  #define CPU_INTERRUPT_SSTEP_MASK \
> diff --git a/hw/apic.h b/hw/apic.h
> index a62d83b..45598bd 100644
> --- a/hw/apic.h
> +++ b/hw/apic.h
> @@ -18,6 +18,8 @@ void cpu_set_apic_tpr(DeviceState *s, uint8_t val);
>  uint8_t cpu_get_apic_tpr(DeviceState *s);
>  void apic_init_reset(DeviceState *s);
>  void apic_sipi(DeviceState *s);
> +void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
> +                                   int access);
>
>  /* pc.c */
>  int cpu_is_bsp(CPUState *env);
> diff --git a/hw/apic_common.c b/hw/apic_common.c
> index 8373d79..588531b 100644
> --- a/hw/apic_common.c
> +++ b/hw/apic_common.c
> @@ -68,6 +68,10 @@ uint8_t cpu_get_apic_tpr(DeviceState *d)
>     return s ? s->tpr >> 4 : 0;
>  }
>
> +void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip, int 
> access)
> +{
> +}
> +
>  void apic_report_irq_delivered(int delivered)
>  {
>     apic_irq_delivered += delivered;
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 37dde79..92e9c87 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -482,6 +482,7 @@
>  #define CPU_INTERRUPT_VIRQ      CPU_INTERRUPT_TGT_INT_0
>  #define CPU_INTERRUPT_INIT      CPU_INTERRUPT_TGT_INT_1
>  #define CPU_INTERRUPT_SIPI      CPU_INTERRUPT_TGT_INT_2
> +#define CPU_INTERRUPT_TPR       CPU_INTERRUPT_TGT_INT_3
>
>
>  enum {
> @@ -772,6 +773,9 @@ typedef struct CPUX86State {
>     XMMReg ymmh_regs[CPU_NB_REGS];
>
>     uint64_t xcr0;
> +
> +    target_ulong tpr_access_ip;
> +    int tpr_access_type;
>  } CPUX86State;
>
>  CPUX86State *cpu_x86_init(const char *cpu_model);
> @@ -1064,4 +1068,9 @@ void svm_check_intercept(CPUState *env1, uint32_t type);
>
>  uint32_t cpu_cc_compute_all(CPUState *env1, int op);
>
> +#define TPR_ACCESS_READ     0
> +#define TPR_ACCESS_WRITE    1

enum would be nicer.

> +
> +void cpu_report_tpr_access(CPUState *env, int access);
> +
>  #endif /* CPU_I386_H */
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 2586aff..eca20cd 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1189,6 +1189,25 @@ void cpu_x86_inject_mce(Monitor *mon, CPUState *cenv, 
> int bank,
>         }
>     }
>  }
> +
> +void cpu_report_tpr_access(CPUState *env, int access)
> +{
> +    TranslationBlock *tb;
> +
> +    if (kvm_enabled()) {
> +        cpu_synchronize_state(env);
> +
> +        env->tpr_access_ip = env->eip;
> +        env->tpr_access_type = access;
> +
> +        cpu_interrupt(env, CPU_INTERRUPT_TPR);
> +    } else {
> +        tb = tb_find_pc(env->mem_io_pc);
> +        cpu_restore_state(tb, env, env->mem_io_pc);
> +
> +        apic_handle_tpr_access_report(env->apic_state, env->eip, access);
> +    }
> +}
>  #endif /* !CONFIG_USER_ONLY */
>
>  static void mce_init(CPUX86State *cenv)
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 981192d..fa77f9d 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1635,8 +1635,10 @@ void kvm_arch_pre_run(CPUState *env, struct kvm_run 
> *run)
>     }
>
>     if (!kvm_irqchip_in_kernel()) {
> -        /* Force the VCPU out of its inner loop to process the INIT request 
> */
> -        if (env->interrupt_request & CPU_INTERRUPT_INIT) {
> +        /* Force the VCPU out of its inner loop to process any INIT requests
> +         * or pending TPR access reports. */
> +        if (env->interrupt_request &
> +            (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
>             env->exit_request = 1;
>      

Re: [Qemu-devel] [PATCH v2 1/8] kvm: Set cpu_single_env only once

[Andreas Färber]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Am 11.02.2012 15:24, schrieb Jan Kiszka:
> On 2012-02-11 15:12, Andreas Färber wrote:
>> Am 11.02.2012 15:02, schrieb Jan Kiszka:
>>> On 2012-02-11 14:59, Andreas Färber wrote:
 Am 11.02.2012 14:35, schrieb Jan Kiszka:
> On 2012-02-11 14:21, Andreas Färber wrote:
>> CPU base class v3:
>> http://patchwork.ozlabs.org/patch/139284/ (v4 coming up)
>> 
>> That doesn't prevent target-specific devices. Although
>> Paolo does want that to change wrt properties.
 
> This patch doesn't explain yet what shall happen to 
> cpu_single_env and CPUState accesses from target-specific 
> devices.
 
 True. We can't change them before all targets are converted.
 So far I have 3/14 and still some review comments to work
 in.
 
 Another patch in that series uses a macro 
 s/ENV_GET_OBJECT/ENV_GET_CPU/ to go from CPUState -> CPU
 while we convert targets.
 
 Depending on our taste, cpu_single_env might become 
 cpu_single_cpu, single_cpu or cpu_single.
 
> Do you plan accessors for registers?
 
 No, registers are in target-specific ARMCPU, S390CPU,
 MIPSCPU, etc. and their CPU*State. It would be possible to
 have static inline accessors but so far I've seen no need.
>> 
>>> Then the devices need to have access to a CPUState pointer,
>>> just as so far.
>> 
>> Yes and no. They can have any target-specific pointer they want,
>> just as before. But no global first_cpu / cpu_single_env pointer
>> - that's
> 
> If you want to drop first_cpu, you need to provide a for_each_cpu 
> iterating service instead. And cpu_single_env can only be obsoleted
> if I/O access handlers can otherwise query the triggering CPU.

I already answered that above. Please join #qemu if you further want
to discuss that, for this thread seems to lead nowhere.

Andreas

> 
>> replaced by CPU pointers, through which members of derived
>> classes can be accessed (which did not work for CPUState due to
>> CPU_COMMON members being at target-specific offset in the
>> middle).
>> 
>> There's nothing wrong with your patch per se, just that it may
>> need to get refactored some time soonish. We need to be aware of
>> it so that we don't create merge conflicts for Anthony.
> 
> There can't be logical merge conflicts as the no fundamentally new 
> requirements are introduced with this series. And we have no code 
> proposal seen yet to address them already for the existing use
> cases.
> 
> Jan
> 


- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.18 (GNU/Linux)

iQIcBAEBAgAGBQJPNoALAAoJEPou0S0+fgE/OHAQALK7X6v9nA0A4tZ8umD4Ak8p
DkyHX9N0pkv8Jc9y06WWLCzsgCQJFxPKp0n0mpWHhG96mWryez+Cd8x00W9wJJWx
A15beRV80jwpDWlkNMtnQ+T9kvVamUsL3a090Bgcb662EkCpfR5UtjDlrYBM7X7f
C/ejV31NYnFIXM5F2TcsURrXZ7GRXNeSRsoXrt2WoCBhFkf+DBek8ejEsYcFS6q0
lrqoggHTVKRZuGbBIJ9yS3/L/pf6gWDOv1ZyUAHfAUeWt2rD3NxNFHHFLbrl3d47
k5Yev4acZOTe6ozgvK3qWcrvA2t42BmKTCA7FqLKg2057szll277wKHf0K2xqlvs
oYTbSk4t9IWI4StBFevgVDM0kaXg6OAGKiDDP8PRrBI3YzJajLL6zkDVitA5Hp0N
LPryOYwhI+KtO3Too7R919UDZIoZ+pg2Mm+L1/1IuneB8Ar1MeiPwU0zXLYGiDVx
ZrMzjhhbYJn+PPC8FxI9gnaPkLVkZzSfcXkpA1RXLZdpkjdmt4rwA0KfFNB000DU
fag3rAGTdcvT8O58K2FXYAWe8VFqA979VHWxsTOLrVX3rL9Xbi63SUfvz/joMryO
mZMYsJ/NGHd5IVYdWP0kBdxuXRtFUaqHnp7PFnwj0IXtnV13csgB2nN+HN5wJULs
A855i5ibqUahcKGej48W
=jxwX
-END PGP SIGNATURE-

Re: [Qemu-devel] [PATCH v2 5/8] kvmvapic: Introduce TPR access optimization for Windows guests

[Blue Swirl]

On Fri, Feb 10, 2012 at 18:31, Jan Kiszka  wrote:
> This enables acceleration for MMIO-based TPR registers accesses of
> 32-bit Windows guest systems. It is mostly useful with KVM enabled,
> either on older Intel CPUs (without flexpriority feature, can also be
> manually disabled for testing) or any current AMD processor.
>
> The approach introduced here is derived from the original version of
> qemu-kvm. It was refactored, documented, and extended by support for
> user space APIC emulation, both with and without KVM acceleration. The
> VMState format was kept compatible, so was the ABI to the option ROM
> that implements the guest-side para-virtualized driver service. This
> enables seamless migration from qemu-kvm to upstream or, one day,
> between KVM and TCG mode.
>
> The basic concept goes like this:
>  - VAPIC PV interface consisting of I/O port 0x7e and (for KVM in-kernel
>   irqchip) a vmcall hypercall is registered
>  - VAPIC option ROM is loaded into guest
>  - option ROM activates TPR MMIO access reporting via port 0x7e
>  - TPR accesses are trapped and patched in the guest to call into option
>   ROM instead, VAPIC support is enabled
>  - option ROM TPR helpers track state in memory and invoke hypercall to
>   poll for pending IRQs if required
>
> Signed-off-by: Jan Kiszka 

I must say that I find the approach horrible, patching guests and ROMs
and looking up Windows internals. Taking the same approach to extreme,
we could for example patch Xen guest to become a KVM guest. Not that I
object merging.

> ---
>  Makefile.target    |    3 +-
>  hw/apic.c          |  126 -
>  hw/apic_common.c   |   64 +-
>  hw/apic_internal.h |   27 ++
>  hw/kvm/apic.c      |   32 +++
>  hw/kvmvapic.c      |  774 
> 
>  6 files changed, 1012 insertions(+), 14 deletions(-)
>  create mode 100644 hw/kvmvapic.c
>
> diff --git a/Makefile.target b/Makefile.target
> index 68481a3..ec7eff8 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -230,7 +230,8 @@ obj-y += device-hotplug.o
>
>  # Hardware support
>  obj-i386-y += mc146818rtc.o pc.o
> -obj-i386-y += sga.o apic_common.o apic.o ioapic_common.o ioapic.o piix_pci.o
> +obj-i386-y += apic_common.o apic.o kvmvapic.o
> +obj-i386-y += sga.o ioapic_common.o ioapic.o piix_pci.o
>  obj-i386-y += vmport.o
>  obj-i386-y += pci-hotplug.o smbios.o wdt_ib700.o
>  obj-i386-y += debugcon.o multiboot.o
> diff --git a/hw/apic.c b/hw/apic.c
> index 086c544..2ebf3ca 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -35,6 +35,10 @@
>  #define MSI_ADDR_DEST_ID_SHIFT         12
>  #define        MSI_ADDR_DEST_ID_MASK           0x000
>
> +#define SYNC_FROM_VAPIC                 0x1
> +#define SYNC_TO_VAPIC                   0x2
> +#define SYNC_ISR_IRR_TO_VAPIC           0x4

Enum, please.

> +
>  static APICCommonState *local_apics[MAX_APICS + 1];
>
>  static void apic_set_irq(APICCommonState *s, int vector_num, int 
> trigger_mode);
> @@ -78,6 +82,70 @@ static inline int get_bit(uint32_t *tab, int index)
>     return !!(tab[i] & mask);
>  }
>
> +/* return -1 if no bit is set */
> +static int get_highest_priority_int(uint32_t *tab)
> +{
> +    int i;
> +    for (i = 7; i >= 0; i--) {
> +        if (tab[i] != 0) {
> +            return i * 32 + fls_bit(tab[i]);
> +        }
> +    }
> +    return -1;
> +}
> +
> +static void apic_sync_vapic(APICCommonState *s, int sync_type)
> +{
> +    VAPICState vapic_state;
> +    size_t length;
> +    off_t start;
> +    int vector;
> +
> +    if (!s->vapic_paddr) {
> +        return;
> +    }
> +    if (sync_type & SYNC_FROM_VAPIC) {
> +        cpu_physical_memory_rw(s->vapic_paddr, (void *)&vapic_state,
> +                               sizeof(vapic_state), 0);
> +        s->tpr = vapic_state.tpr;
> +    }
> +    if (sync_type & (SYNC_TO_VAPIC | SYNC_ISR_IRR_TO_VAPIC)) {
> +        start = offsetof(VAPICState, isr);
> +        length = offsetof(VAPICState, enabled) - offsetof(VAPICState, isr);
> +
> +        if (sync_type & SYNC_TO_VAPIC) {
> +            assert(qemu_cpu_is_self(s->cpu_env));
> +
> +            vapic_state.tpr = s->tpr;
> +            vapic_state.enabled = 1;
> +            start = 0;
> +            length = sizeof(VAPICState);
> +        }
> +
> +        vector = get_highest_priority_int(s->isr);
> +        if (vector < 0) {
> +            vector = 0;
> +        }
> +        vapic_state.isr = vector & 0xf0;
> +
> +        vapic_state.zero = 0;
> +
> +        vector = get_highest_priority_int(s->irr);
> +        if (vector < 0) {
> +            vector = 0;
> +        }
> +        vapic_state.irr = vector & 0xff;
> +
> +        cpu_physical_memory_write_rom(s->vapic_paddr + start,
> +                                      ((void *)&vapic_state) + start, 
> length);

This assumes that the vapic_state structure matches guest what guest
expect without conversion. Is this true for i386 on x86_64? I didn't
check the structure in question.

> +    }
> +}
> +
> +static void

Re: [Qemu-devel] [PATCH v2 22/27] qdev: access properties via QOM

[Blue Swirl]

On Sat, Feb 4, 2012 at 08:02, Paolo Bonzini  wrote:
> Do not poke anymore in the struct when accessing qdev properties.
> Instead, ask the object to set the right value.
>
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/qdev-addr.c       |    5 ++-
>  hw/qdev-properties.c |   78 ++---
>  hw/qdev.h            |    4 +--
>  3 files changed, 59 insertions(+), 28 deletions(-)
>
> diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
> index 5976dcd..8daa733 100644
> --- a/hw/qdev-addr.c
> +++ b/hw/qdev-addr.c
> @@ -71,5 +71,8 @@ PropertyInfo qdev_prop_taddr = {
>
>  void qdev_prop_set_taddr(DeviceState *dev, const char *name, 
> target_phys_addr_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_TADDR);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
> +
>  }
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index debb37f..5a11676 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1115,7 +1115,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, 
> const char *value)
>     return 0;
>  }
>
> -void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum 
> PropertyType type)
> +static void qdev_prop_set(DeviceState *dev, const char *name, void *src, 
> enum PropertyType type)
>  {
>     Property *prop;
>
> @@ -1135,52 +1135,63 @@ void qdev_prop_set(DeviceState *dev, const char 
> *name, void *src, enum PropertyT
>
>  void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_BIT);
> +    Error *errp = NULL;
> +    object_property_set_bool(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT8);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT16);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT32);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_INT32);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT64);
> +    Error *errp = NULL;
> +    object_property_set_int(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  void qdev_prop_set_string(DeviceState *dev, const char *name, char *value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_STRING);
> +    Error *errp = NULL;
> +    object_property_set_str(OBJECT(dev), value, name, &errp);
> +    assert(!errp);
>  }
>
>  int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState 
> *value)
>  {
> -    int res;
> -
> -    res = bdrv_attach_dev(value, dev);
> -    if (res < 0) {
> -        error_report("Can't attach drive %s to %s.%s: %s",
> -                     bdrv_get_device_name(value),
> -                     dev->id ? dev->id : object_get_typename(OBJECT(dev)),
> -                     name, strerror(-res));
> +    Error *errp = NULL;
> +    object_property_set_str(OBJECT(dev), bdrv_get_device_name(value),
> +                            name, &errp);
> +    if (errp) {
> +        qerror_report_err(errp);
> +        error_free(errp);
>         return -1;
>     }
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE);
>     return 0;
>  }
>
> @@ -1192,28 +1203,47 @@ void qdev_prop_set_drive_nofail(DeviceState *dev, 
> const char *name, BlockDriverS
>  }
>  void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState 
> *value)
>  {
> -    qdev_prop_set(dev, name, &value, PROP_TYPE_CHR);
> +    Error *errp = NULL;
> +    assert(value->label);

This breaks Sparc32 and PPC:
Program received signal SIGSEGV, Segmentation fault.
qdev_prop_set_chr (dev=0x157e170, name=0x5bda5f "chrB", value=0x0)
at /src/qemu/hw/qdev-properties.c:1142
1142assert(value->label);
(gdb) bt
#0  qdev_prop_set_chr (dev=0x157e170, name=0x5bda5f "chrB", value=0x0)
at /src/qemu/hw/qdev-properties.c:1142
#1  0x0046e1af in slavio_serial_ms_kbd_init (base=1895825408,
irq=0x11cdab0, disabled=0, clock=4915200, it_shift=1)
at /src/qemu/hw/escc.c:859
#2  0x0054117d in sun4m_hw_init (hwdef=0x5ef660, RAM_size=134217728,

Re: [Qemu-devel] [PATCH v3 2/4] cadence_ttc: initial version of device model

[Peter Crosthwaite]

2012/2/12 Paul Brook 

> > +static void cadence_timer_sync(CadenceTimerState *s)
> > +{
> >...
> > +r = (int64_t)cadence_timer_get_steps(s, s->cpu_time - old_time);
> > +x = (int64_t)s->reg_value + ((s->reg_count & COUNTER_CTRL_DEC) ? -r
> :
> > r); +
> > +for (i = 0; i < 3; ++i) {
> > +if (is_between((int64_t)s->reg_match[i] << 16, s->reg_value,
> x)) {
> > +s->reg_intr |= (2 << i);
> > +}
> > +}
>
> By my reading this will miss events if they happen after the timer wraps.
> e.g. for a count-up timer with reg_match==1 and the tick callback happens
> to
> get delayed by 4 cycles, the timer may wrap straight to reg_value = 3.
>
> Paul
>

Yeh, good catch. Will change to something like this on the next revision:

for (i = 0; i < 3; ++i) {
 int64_t m =  (int64_t)s->reg_match[i] << 16;
 if (is_between(m, s->reg_value, x) ||
is_between(m+interval, s->reg_value, x) ||
is_between(m-interval, s->reg_value, x)
 ) {
  s->reg_intr |= (2 << i);
 }
}

Any other thoughts or comments on this series?

[Qemu-devel] How to enable DEBUG_IOPORT?

[Wizard]

All

When I reading the code, i find in the  cpu_log_items the
CPU_LOG_IOPORT is not enabled since the
DEBUD_IOPORT is not defined.

I searched the configure file but find no option to enable this.
So currently the only way to do this is comment out the macro in ioport.c?

-- 
Wizard

Re: [Qemu-devel] [RFC] Next gen kvm api

[Takuya Yoshikawa]

Avi Kivity  wrote:

> > >  Slot searching is quite fast since there's a small number of slots, and 
> > > we sort the larger ones to be in the front, so positive lookups are fast. 
> > >  We cache negative lookups in the shadow page tables (an spte can be 
> > > either "not mapped", "mapped to RAM", or "not mapped and known to be 
> > > mmio") so we rarely need to walk the entire list.
> >
> > Well, we don't always have shadow page tables. Having hints for unmapped 
> > guest memory like this is pretty tricky.
> > We're currently running into issues with device assignment though, where we 
> > get a lot of small slots mapped to real hardware. I'm sure that will hit us 
> > on x86 sooner or later too.
> 
> For x86 that's not a problem, since once you map a page, it stays mapped 
> (on modern hardware).
> 

I was once thinking about how to search a slot reasonably fast for every case,
even when we do not have mmio-spte cache.

One possible way I thought up was to sort slots according to their base_gfn.
Then the problem would become:  "find the first slot whose base_gfn + npages
is greater than this gfn."

Since we can do binary search, the search cost is O(log(# of slots)).

But I guess that most of the time was wasted on reading many memslots just to
know their base_gfn and npages.

So the most practically effective thing is to make a separate array which holds
just their base_gfn.  This will make the task a simple, and cache friendly,
search on an integer array:  probably faster than using *-tree data structure.

If needed, we should make cmp_memslot() architecture specific in the end?

Takuya