[Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[Alvise Rigo]

Both KVM and TCG populate the cpreg_list with 64 bit registers IDs, but in the TCG side the cpreg_list is sorted using the 32 bit id version while in the kvm side the 64 bit id version is used.
This patch makes the sorting of the cpreg_list consistent between KVM and TCG.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/helper.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2a98be7..834041e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -225,10 +225,14 @@ static void count_cpreg(gpointer key, gpointer opaque)
 
 static gint cpreg_key_compare(gconstpointer a, gconstpointer b)
 {
-    uint32_t aidx = *(uint32_t *)a;
-    uint32_t bidx = *(uint32_t *)b;
-
-    return aidx - bidx;
+    uint64_t aidx = cpreg_to_kvm_id(*(uint32_t *)a);
+    uint64_t bidx = cpreg_to_kvm_id(*(uint32_t *)b);
+    
+    if (aidx > bidx)
+	return 1;
+    if (aidx < bidx)
+	return -1;
+    return 0;
 }
 
 static void cpreg_make_keylist(gpointer key, gpointer value, gpointer udata)
-- 
1.8.1.2

[Qemu-devel] [PATCH 2/2] target-arm: fix sorting iussue of KVM cpreg list

[Alvise Rigo]

The compare_u64 function was not sorting the KVM cpreg_list in the right way due to the wrong returned value. Since we are comparing two 64bit values we can't simply return their difference if the returned type is int.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/kvm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index b92e00d..9120f72 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -67,7 +67,11 @@ static bool reg_syncs_via_tuple_list(uint64_t regidx)
 
 static int compare_u64(const void *a, const void *b)
 {
-    return *(uint64_t *)a - *(uint64_t *)b;
+    if (*(uint64_t *)a > *(uint64_t *)b)
+        return 1;
+    if (*(uint64_t *)a < *(uint64_t *)b)
+	return -1;
+    return 0;
 }
 
 int kvm_arch_init_vcpu(CPUState *cs)
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[Peter Maydell]

On 12 October 2013 02:38, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> Both KVM and TCG populate the cpreg_list with 64 bit registers IDs, but in the TCG side the cpreg_list is sorted using the 32 bit id version while in the kvm side the 64 bit id version is used.
> This patch makes the sorting of the cpreg_list consistent between KVM and TCG.

...this commit message doesn't say why this is a bad thing.

> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  target-arm/helper.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 2a98be7..834041e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -225,10 +225,14 @@ static void count_cpreg(gpointer key, gpointer opaque)
>
>  static gint cpreg_key_compare(gconstpointer a, gconstpointer b)
>  {
> -    uint32_t aidx = *(uint32_t *)a;
> -    uint32_t bidx = *(uint32_t *)b;
> -
> -    return aidx - bidx;
> +    uint64_t aidx = cpreg_to_kvm_id(*(uint32_t *)a);
> +    uint64_t bidx = cpreg_to_kvm_id(*(uint32_t *)b);
> +
> +    if (aidx > bidx)
> +       return 1;
> +    if (aidx < bidx)
> +       return -1;

Coding standard requires braces.

> +    return 0;
>  }
>
>  static void cpreg_make_keylist(gpointer key, gpointer value, gpointer udata)
> --
> 1.8.1.2
>

Re: [Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[alvise rigo]

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

Sorry, I should have mentioned that an improper sorting of the cpreg_list
could lead to a migration failure when cpu_post_load considers an incoming
register as missing in the cpreg_indexes array.
However, as long as the two lists are exactly the same, the problem does
not occur.

On Fri, Oct 11, 2013 at 8:41 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 12 October 2013 02:38, Alvise Rigo <a.rigo@virtualopensystems.com>
> wrote:
> > Both KVM and TCG populate the cpreg_list with 64 bit registers IDs, but
> in the TCG side the cpreg_list is sorted using the 32 bit id version while
> in the kvm side the 64 bit id version is used.
> > This patch makes the sorting of the cpreg_list consistent between KVM
> and TCG.
>
> ...this commit message doesn't say why this is a bad thing.
>
> > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> > ---
> >  target-arm/helper.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 2a98be7..834041e 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -225,10 +225,14 @@ static void count_cpreg(gpointer key, gpointer
> opaque)
> >
> >  static gint cpreg_key_compare(gconstpointer a, gconstpointer b)
> >  {
> > -    uint32_t aidx = *(uint32_t *)a;
> > -    uint32_t bidx = *(uint32_t *)b;
> > -
> > -    return aidx - bidx;
> > +    uint64_t aidx = cpreg_to_kvm_id(*(uint32_t *)a);
> > +    uint64_t bidx = cpreg_to_kvm_id(*(uint32_t *)b);
> > +
> > +    if (aidx > bidx)
> > +       return 1;
> > +    if (aidx < bidx)
> > +       return -1;
>
> Coding standard requires braces.
>
> > +    return 0;
> >  }
> >
> >  static void cpreg_make_keylist(gpointer key, gpointer value, gpointer
> udata)
> > --
> > 1.8.1.2
> >
>

[-- Attachment #2: Type: text/html, Size: 2549 bytes --]

Re: [Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[Peter Maydell]

On 14 October 2013 11:45, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> Sorry, I should have mentioned that an improper sorting of the cpreg_list
> could lead to a migration failure when cpu_post_load considers an incoming
> register as missing in the cpreg_indexes array.
> However, as long as the two lists are exactly the same, the problem does not
> occur.

Yeah, makes sense. Note that this is an entirely theoretical
problem at the moment: TCG<->KVM migration isn't supported
and won't work because the set of coprocessor registers
each supports is different. (At some point we should probably
fix this by getting TCG to support the right set of regs, but
it's not a very high priority for me.)

-- PMM

Re: [Qemu-devel] [PATCH 1/2] target-arm: sort TCG cpreg list by 64bit id version

[Peter Maydell]

On 11 October 2013 18:38, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> Both KVM and TCG populate the cpreg_list with 64 bit registers IDs, but in the TCG side the cpreg_list is sorted using the 32 bit id version while in the kvm side the 64 bit id version is used.
> This patch makes the sorting of the cpreg_list consistent between KVM and TCG.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>

Thanks, applied this and 2/2 to target-arm.next.
A couple of formatting notes for next time:
 * please use checkpatch.pl to check you haven't got
   coding style violations (both these patches had bad indent
   and missing braces)
 * please wrap your commit messages rather than having them
   be one very long line
 * if you're submitting a patchset with more than one patch
   in it please include a cover letter email (this set doesn't
   seem to have one)

I've fixed these issues up in my queue this time round since
I wanted to get the patches out in a pullreq this week, but
usually I'd just bounce a patch back for that sort of error.

(If you haven't read http://qemu-project.org/Contribute/SubmitAPatch
I'd recommend it; it tries to list various minor formatting
and process issues that can trip up first-time submitters.)

thanks
-- PMM