Xen devel - Aug 2012 - [PATCH] QEMU/helper2.c: Fix multiply issue for int and uint types

Hi, 

The following patch fixes an issue of (uint * int) multiply in QEMU.
The bug phenomenon is that, it causes the Level 1 (L1) Xen boots more than 30
minutes on Level 0 (L0) Xen (The nested virtualization case).
Please help to review and pull.

I saw the upstream QEMU also have a similar code piece, I will also send a patch
there.

Thanks,
Dongxiao


From 442297c3f5073b11033e6af2338f571418eff024 Mon Sep 17 00:00:00 2001
From: Dongxiao Xu <dongxiao.xu@intel.com>
Date: Mon, 20 Aug 2012 16:45:04 +0800
Subject: [PATCH] helper2: fix multiply issue for int and uint types

If the two multiply operands are int and uint types separately,
the int type will be transformed to uint firstly, which is not the
intent in our code piece. The fix is to add (int) transform for the
uint type before the multiply.

This helps to fix the Xen hypevisor slow booting issue (boots more
than 30 minutes) on another Xen hypervisor
(the nested virtualization case).

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
---
 i386-dm/helper2.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..2a66086 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -364,7 +364,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
             for (i = 0; i < req->count; i++) {
                 tmp = do_inp(env, req->addr, req->size);
                 write_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
             }
         }
@@ -376,7 +376,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
                 unsigned long tmp = 0;
 
                 read_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
                 do_outp(env, req->addr, req->size, tmp);
             }
@@ -394,13 +394,13 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
         if (req->dir == IOREQ_READ) {
             for (i = 0; i < req->count; i++) {
                 read_physical(req->addr
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &req->data);
             }
         } else if (req->dir == IOREQ_WRITE) {
             for (i = 0; i < req->count; i++) {
                 write_physical(req->addr
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &req->data);
             }
         }
@@ -410,19 +410,19 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
         if (req->dir == IOREQ_READ) {
             for (i = 0; i < req->count; i++) {
                 read_physical(req->addr
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
                 write_physical((target_phys_addr_t )req->data
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
             }
         } else if (req->dir == IOREQ_WRITE) {
             for (i = 0; i < req->count; i++) {
                 read_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
                 write_physical(req->addr
-                  + (sign * i * req->size),
+                  + (sign * i * (int)req->size),
                   req->size, &tmp);
             }
         }
-- 
1.7.1



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Mon, 20 Aug 2012, Xu, Dongxiao wrote:
> Hi, 
> 
> The following patch fixes an issue of (uint * int) multiply in QEMU.
> The bug phenomenon is that, it causes the Level 1 (L1) Xen boots more than
30 minutes on Level 0 (L0) Xen (The nested virtualization case).
> Please help to review and pull.
> 
> I saw the upstream QEMU also have a similar code piece, I will also send a
patch there.
> 
> Thanks,
> Dongxiao
> 
> 
> >From 442297c3f5073b11033e6af2338f571418eff024 Mon Sep 17 00:00:00 2001
> From: Dongxiao Xu <dongxiao.xu@intel.com>
> Date: Mon, 20 Aug 2012 16:45:04 +0800
> Subject: [PATCH] helper2: fix multiply issue for int and uint types
> 
> If the two multiply operands are int and uint types separately,
> the int type will be transformed to uint firstly, which is not the
> intent in our code piece. The fix is to add (int) transform for the
> uint type before the multiply.
well spotted!

> This helps to fix the Xen hypevisor slow booting issue (boots more
> than 30 minutes) on another Xen hypervisor
> (the nested virtualization case).
If we do that we suddenly restrict the maximum req->size from UINT_MAX to
INT_MAX.
I would rather cast req->size to int64_t instead.

> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> ---
>  i386-dm/helper2.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> index c6d049c..2a66086 100644
> --- a/i386-dm/helper2.c
> +++ b/i386-dm/helper2.c
> @@ -364,7 +364,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>              for (i = 0; i < req->count; i++) {
>                  tmp = do_inp(env, req->addr, req->size);
>                  write_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>              }
>          }
> @@ -376,7 +376,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>                  unsigned long tmp = 0;
>  
>                  read_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>                  do_outp(env, req->addr, req->size, tmp);
>              }
> @@ -394,13 +394,13 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t
*req)
>          if (req->dir == IOREQ_READ) {
>              for (i = 0; i < req->count; i++) {
>                  read_physical(req->addr
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &req->data);
>              }
>          } else if (req->dir == IOREQ_WRITE) {
>              for (i = 0; i < req->count; i++) {
>                  write_physical(req->addr
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &req->data);
>              }
>          }
> @@ -410,19 +410,19 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t
*req)
>          if (req->dir == IOREQ_READ) {
>              for (i = 0; i < req->count; i++) {
>                  read_physical(req->addr
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>                  write_physical((target_phys_addr_t )req->data
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>              }
>          } else if (req->dir == IOREQ_WRITE) {
>              for (i = 0; i < req->count; i++) {
>                  read_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>                  write_physical(req->addr
> -                  + (sign * i * req->size),
> +                  + (sign * i * (int)req->size),
>                    req->size, &tmp);
>              }
>          }
> -- 
> 1.7.1
> 
>

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Monday, August 20, 2012 8:18 PM
> To: Xu, Dongxiao
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH] QEMU/helper2.c: Fix multiply issue for int
and
> uint types
> 
> On Mon, 20 Aug 2012, Xu, Dongxiao wrote:
> > Hi,
> >
> > The following patch fixes an issue of (uint * int) multiply in QEMU.
> > The bug phenomenon is that, it causes the Level 1 (L1) Xen boots more
than
> 30 minutes on Level 0 (L0) Xen (The nested virtualization case).
> > Please help to review and pull.
> >
> > I saw the upstream QEMU also have a similar code piece, I will also
send a
> patch there.
> >
> > Thanks,
> > Dongxiao
> >
> >
> > >From 442297c3f5073b11033e6af2338f571418eff024 Mon Sep 17 00:00:00
> > >2001
> > From: Dongxiao Xu <dongxiao.xu@intel.com>
> > Date: Mon, 20 Aug 2012 16:45:04 +0800
> > Subject: [PATCH] helper2: fix multiply issue for int and uint types
> >
> > If the two multiply operands are int and uint types separately, the
> > int type will be transformed to uint firstly, which is not the intent
> > in our code piece. The fix is to add (int) transform for the uint type
> > before the multiply.
> 
> well spotted!
> 
> 
> > This helps to fix the Xen hypevisor slow booting issue (boots more
> > than 30 minutes) on another Xen hypervisor (the nested virtualization
> > case).
> 
> If we do that we suddenly restrict the maximum req->size from UINT_MAX
to
> INT_MAX.
> I would rather cast req->size to int64_t instead.
Yes, this is reasonable. I will update my patch.

Thanks,
Dongxiao
> 
> 
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> > ---
> >  i386-dm/helper2.c |   16 ++++++++--------
> >  1 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c index
> > c6d049c..2a66086 100644
> > --- a/i386-dm/helper2.c
> > +++ b/i386-dm/helper2.c
> > @@ -364,7 +364,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> *req)
> >              for (i = 0; i < req->count; i++) {
> >                  tmp = do_inp(env, req->addr, req->size);
> >                  write_physical((target_phys_addr_t) req->data
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >              }
> >          }
> > @@ -376,7 +376,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> *req)
> >                  unsigned long tmp = 0;
> >
> >                  read_physical((target_phys_addr_t) req->data
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >                  do_outp(env, req->addr, req->size, tmp);
> >              }
> > @@ -394,13 +394,13 @@ static void cpu_ioreq_move(CPUState *env,
> ioreq_t *req)
> >          if (req->dir == IOREQ_READ) {
> >              for (i = 0; i < req->count; i++) {
> >                  read_physical(req->addr
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &req->data);
> >              }
> >          } else if (req->dir == IOREQ_WRITE) {
> >              for (i = 0; i < req->count; i++) {
> >                  write_physical(req->addr
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &req->data);
> >              }
> >          }
> > @@ -410,19 +410,19 @@ static void cpu_ioreq_move(CPUState *env,
> ioreq_t *req)
> >          if (req->dir == IOREQ_READ) {
> >              for (i = 0; i < req->count; i++) {
> >                  read_physical(req->addr
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >                  write_physical((target_phys_addr_t )req->data
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >              }
> >          } else if (req->dir == IOREQ_WRITE) {
> >              for (i = 0; i < req->count; i++) {
> >                  read_physical((target_phys_addr_t) req->data
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >                  write_physical(req->addr
> > -                  + (sign * i * req->size),
> > +                  + (sign * i * (int)req->size),
> >                    req->size, &tmp);
> >              }
> >          }
> > --
> > 1.7.1
> >
> >