Patchwork [Xen-devel,0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move

login
register
mail settings
Submitter Ian Jackson
Date Dec. 7, 2012, 4:33 p.m.
Message ID <20674.6749.646806.53950@mariner.uk.xensource.com>
Download mbox | patch
Permalink /patch/204574/
State New
Headers show

Comments

Ian Jackson - Dec. 7, 2012, 4:33 p.m.
Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio	and cpu_ioreq_move"):
> On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > +    if (req->df) addr -= offset;
> > +    else addr -= offset;
> 
> One of these -= should be a += I presume?

Uh, yes.

> [...]
> > +                write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);
> 
> This seems to be the only one with this cast, why?

This is a mistake.

> write_phys_req_item takes a target_phys_addr_t so this will happen
> regardless I think.

Indeed.

Ian.

commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Fri Dec 7 16:02:04 2012 +0000

    cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item
    
    The current code compare i (int) with req->count (uint32_t) in a for
    loop, risking an infinite loop if req->count is >INT_MAX.  It also
    does the multiplication of req->size in a too-small type, leading to
    integer overflows.
    
    Turn read_physical and write_physical into two different helper
    functions, read_phys_req_item and write_phys_req_item, that take care
    of adding or subtracting offset depending on sign.
    
    This removes the formulaic multiplication to a single place where the
    integer overflows can be dealt with by casting to wide-enough unsigned
    types.
    
    Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
    Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
    
    --
    v2: Fix sign when !!req->df.  Remove a useless cast.
Dongxiao Xu - Dec. 11, 2012, 5:50 a.m.
> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> Sent: Saturday, December 08, 2012 12:34 AM
> To: Ian Campbell
> Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> qemu-devel@nongnu.org
> Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> cpu_ioreq_move
> 
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> cpu_ioreq_pio	and cpu_ioreq_move"):
> > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > +    if (req->df) addr -= offset;
> > > +    else addr -= offset;
> >
> > One of these -= should be a += I presume?
> 
> Uh, yes.
> 
> > [...]
> > > +                write_phys_req_item((target_phys_addr_t) req->data,
> req, i, &tmp);
> >
> > This seems to be the only one with this cast, why?
> 
> This is a mistake.
> 
> > write_phys_req_item takes a target_phys_addr_t so this will happen
> > regardless I think.
> 
> Indeed.
> 
> Ian.

Tested this v2 patch on my system, and it works.

Thanks,
Dongxiao


> 
> commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Fri Dec 7 16:02:04 2012 +0000
> 
>     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> write_phys_req_item
> 
>     The current code compare i (int) with req->count (uint32_t) in a for
>     loop, risking an infinite loop if req->count is >INT_MAX.  It also
>     does the multiplication of req->size in a too-small type, leading to
>     integer overflows.
> 
>     Turn read_physical and write_physical into two different helper
>     functions, read_phys_req_item and write_phys_req_item, that take care
>     of adding or subtracting offset depending on sign.
> 
>     This removes the formulaic multiplication to a single place where the
>     integer overflows can be dealt with by casting to wide-enough unsigned
>     types.
> 
>     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
>     --
>     v2: Fix sign when !!req->df.  Remove a useless cast.
> 
> diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> index c6d049c..63a938b 100644
> --- a/i386-dm/helper2.c
> +++ b/i386-dm/helper2.c
> @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long
> addr,
>      }
>  }
> 
> -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> +/*
> + * Helper functions which read/write an object from/to physical guest
> + * memory, as part of the implementation of an ioreq.
> + *
> + * Equivalent to
> + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> + *                          val, req->size, 0/1)
> + * except without the integer overflow problems.
> + */
> +static void rw_phys_req_item(target_phys_addr_t addr,
> +                             ioreq_t *req, uint32_t i, void *val, int rw)
>  {
> -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
> +    /* Do everything unsigned so overflow just results in a truncated result
> +     * and accesses to undesired parts of guest memory, which is up
> +     * to the guest */
> +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> +    if (req->df) addr -= offset;
> +    else addr += offset;
> +    cpu_physical_memory_rw(addr, val, req->size, rw);
>  }
> -
> -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> +static inline void read_phys_req_item(target_phys_addr_t addr,
> +                                      ioreq_t *req, uint32_t i, void
> *val)
>  {
> -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
> +    rw_phys_req_item(addr, req, i, val, 0);
> +}
> +static inline void write_phys_req_item(target_phys_addr_t addr,
> +                                       ioreq_t *req, uint32_t i, void
> *val)
> +{
> +    rw_phys_req_item(addr, req, i, val, 1);
>  }
> 
>  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>  {
> -    int i, sign;
> -
> -    sign = req->df ? -1 : 1;
> +    uint32_t i;
> 
>      if (req->dir == IOREQ_READ) {
>          if (!req->data_is_ptr) {
> @@ -363,9 +382,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),
> -                  req->size, &tmp);
> +                write_phys_req_item(req->data, req, i, &tmp);
>              }
>          }
>      } else if (req->dir == IOREQ_WRITE) {
> @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>              for (i = 0; i < req->count; i++) {
>                  unsigned long tmp = 0;
> 
> -                read_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->data, req, i, &tmp);
>                  do_outp(env, req->addr, req->size, tmp);
>              }
>          }
> @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> *req)
> 
>  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
>  {
> -    int i, sign;
> -
> -    sign = req->df ? -1 : 1;
> +    uint32_t i;
> 
>      if (!req->data_is_ptr) {
>          if (req->dir == IOREQ_READ) {
>              for (i = 0; i < req->count; i++) {
> -                read_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &req->data);
> +                read_phys_req_item(req->addr, req, i, &req->data);
>              }
>          } else if (req->dir == IOREQ_WRITE) {
>              for (i = 0; i < req->count; i++) {
> -                write_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &req->data);
> +                write_phys_req_item(req->addr, req, i, &req->data);
>              }
>          }
>      } else {
> @@ -409,21 +418,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),
> -                  req->size, &tmp);
> -                write_physical((target_phys_addr_t )req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->addr, req, i, &tmp);
> +                write_phys_req_item(req->data, req, i, &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),
> -                  req->size, &tmp);
> -                write_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->data, req, i, &tmp);
> +                write_phys_req_item(req->addr, req, i, &tmp);
>              }
>          }
>      }
Dongxiao Xu - Jan. 8, 2013, 1:49 a.m.
> -----Original Message-----
> From: Xu, Dongxiao
> Sent: Tuesday, December 11, 2012 1:50 PM
> To: Ian Jackson; Ian Campbell
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com;
> qemu-devel@nongnu.org
> Subject: RE: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> cpu_ioreq_move
> 
> > -----Original Message-----
> > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > Sent: Saturday, December 08, 2012 12:34 AM
> > To: Ian Campbell
> > Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> > qemu-devel@nongnu.org
> > Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > cpu_ioreq_move
> >
> > Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> > cpu_ioreq_pio	and cpu_ioreq_move"):
> > > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > +    if (req->df) addr -= offset;
> > > > +    else addr -= offset;
> > >
> > > One of these -= should be a += I presume?
> >
> > Uh, yes.
> >
> > > [...]
> > > > +                write_phys_req_item((target_phys_addr_t)
> req->data,
> > req, i, &tmp);
> > >
> > > This seems to be the only one with this cast, why?
> >
> > This is a mistake.
> >
> > > write_phys_req_item takes a target_phys_addr_t so this will happen
> > > regardless I think.
> >
> > Indeed.
> >
> > Ian.
> 
> Tested this v2 patch on my system, and it works.

Are we planning to check this patch into qemu-traditional? Since it is a critical patch to boot Xen on Xen.

Thanks,
Dongxiao

> 
> Thanks,
> Dongxiao
> 
> 
> >
> > commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > Date:   Fri Dec 7 16:02:04 2012 +0000
> >
> >     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> > write_phys_req_item
> >
> >     The current code compare i (int) with req->count (uint32_t) in a for
> >     loop, risking an infinite loop if req->count is >INT_MAX.  It also
> >     does the multiplication of req->size in a too-small type, leading to
> >     integer overflows.
> >
> >     Turn read_physical and write_physical into two different helper
> >     functions, read_phys_req_item and write_phys_req_item, that take
> care
> >     of adding or subtracting offset depending on sign.
> >
> >     This removes the formulaic multiplication to a single place where the
> >     integer overflows can be dealt with by casting to wide-enough unsigned
> >     types.
> >
> >     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
> >     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> >     --
> >     v2: Fix sign when !!req->df.  Remove a useless cast.
> >
> > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> > index c6d049c..63a938b 100644
> > --- a/i386-dm/helper2.c
> > +++ b/i386-dm/helper2.c
> > @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long
> > addr,
> >      }
> >  }
> >
> > -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> > +/*
> > + * Helper functions which read/write an object from/to physical guest
> > + * memory, as part of the implementation of an ioreq.
> > + *
> > + * Equivalent to
> > + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> > + *                          val, req->size, 0/1)
> > + * except without the integer overflow problems.
> > + */
> > +static void rw_phys_req_item(target_phys_addr_t addr,
> > +                             ioreq_t *req, uint32_t i, void *val, int
> rw)
> >  {
> > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> 0);
> > +    /* Do everything unsigned so overflow just results in a truncated result
> > +     * and accesses to undesired parts of guest memory, which is up
> > +     * to the guest */
> > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > +    if (req->df) addr -= offset;
> > +    else addr += offset;
> > +    cpu_physical_memory_rw(addr, val, req->size, rw);
> >  }
> > -
> > -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> > +static inline void read_phys_req_item(target_phys_addr_t addr,
> > +                                      ioreq_t *req, uint32_t i, void
> > *val)
> >  {
> > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> 1);
> > +    rw_phys_req_item(addr, req, i, val, 0);
> > +}
> > +static inline void write_phys_req_item(target_phys_addr_t addr,
> > +                                       ioreq_t *req, uint32_t i,
> void
> > *val)
> > +{
> > +    rw_phys_req_item(addr, req, i, val, 1);
> >  }
> >
> >  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
> >  {
> > -    int i, sign;
> > -
> > -    sign = req->df ? -1 : 1;
> > +    uint32_t i;
> >
> >      if (req->dir == IOREQ_READ) {
> >          if (!req->data_is_ptr) {
> > @@ -363,9 +382,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),
> > -                  req->size, &tmp);
> > +                write_phys_req_item(req->data, req, i, &tmp);
> >              }
> >          }
> >      } else if (req->dir == IOREQ_WRITE) {
> > @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> *req)
> >              for (i = 0; i < req->count; i++) {
> >                  unsigned long tmp = 0;
> >
> > -                read_physical((target_phys_addr_t) req->data
> > -                  + (sign * i * req->size),
> > -                  req->size, &tmp);
> > +                read_phys_req_item(req->data, req, i, &tmp);
> >                  do_outp(env, req->addr, req->size, tmp);
> >              }
> >          }
> > @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > *req)
> >
> >  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
> >  {
> > -    int i, sign;
> > -
> > -    sign = req->df ? -1 : 1;
> > +    uint32_t i;
> >
> >      if (!req->data_is_ptr) {
> >          if (req->dir == IOREQ_READ) {
> >              for (i = 0; i < req->count; i++) {
> > -                read_physical(req->addr
> > -                  + (sign * i * req->size),
> > -                  req->size, &req->data);
> > +                read_phys_req_item(req->addr, req, i, &req->data);
> >              }
> >          } else if (req->dir == IOREQ_WRITE) {
> >              for (i = 0; i < req->count; i++) {
> > -                write_physical(req->addr
> > -                  + (sign * i * req->size),
> > -                  req->size, &req->data);
> > +                write_phys_req_item(req->addr, req, i, &req->data);
> >              }
> >          }
> >      } else {
> > @@ -409,21 +418,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),
> > -                  req->size, &tmp);
> > -                write_physical((target_phys_addr_t )req->data
> > -                  + (sign * i * req->size),
> > -                  req->size, &tmp);
> > +                read_phys_req_item(req->addr, req, i, &tmp);
> > +                write_phys_req_item(req->data, req, i, &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),
> > -                  req->size, &tmp);
> > -                write_physical(req->addr
> > -                  + (sign * i * req->size),
> > -                  req->size, &tmp);
> > +                read_phys_req_item(req->data, req, i, &tmp);
> > +                write_phys_req_item(req->addr, req, i, &tmp);
> >              }
> >          }
> >      }
Pasi Kärkkäinen - Jan. 9, 2013, 7:09 a.m.
On Tue, Jan 08, 2013 at 01:49:06AM +0000, Xu, Dongxiao wrote:
> > 
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: Saturday, December 08, 2012 12:34 AM
> > > To: Ian Campbell
> > > Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> > > qemu-devel@nongnu.org
> > > Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > > cpu_ioreq_move
> > >
> > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> > > cpu_ioreq_pio	and cpu_ioreq_move"):
> > > > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > > +    if (req->df) addr -= offset;
> > > > > +    else addr -= offset;
> > > >
> > > > One of these -= should be a += I presume?
> > >
> > > Uh, yes.
> > >
> > > > [...]
> > > > > +                write_phys_req_item((target_phys_addr_t)
> > req->data,
> > > req, i, &tmp);
> > > >
> > > > This seems to be the only one with this cast, why?
> > >
> > > This is a mistake.
> > >
> > > > write_phys_req_item takes a target_phys_addr_t so this will happen
> > > > regardless I think.
> > >
> > > Indeed.
> > >
> > > Ian.
> > 
> > Tested this v2 patch on my system, and it works.
> 
> Are we planning to check this patch into qemu-traditional? Since it is a critical patch to boot Xen on Xen.
> 

We should definitely get Xen-on-Xen working.. 


-- Pasi

> Thanks,
> Dongxiao
> 
> > 
> > Thanks,
> > Dongxiao
> > 
> > 
> > >
> > > commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> > > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Date:   Fri Dec 7 16:02:04 2012 +0000
> > >
> > >     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> > > write_phys_req_item
> > >
> > >     The current code compare i (int) with req->count (uint32_t) in a for
> > >     loop, risking an infinite loop if req->count is >INT_MAX.  It also
> > >     does the multiplication of req->size in a too-small type, leading to
> > >     integer overflows.
> > >
> > >     Turn read_physical and write_physical into two different helper
> > >     functions, read_phys_req_item and write_phys_req_item, that take
> > care
> > >     of adding or subtracting offset depending on sign.
> > >
> > >     This removes the formulaic multiplication to a single place where the
> > >     integer overflows can be dealt with by casting to wide-enough unsigned
> > >     types.
> > >
> > >     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
> > >     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > >
> > >     --
> > >     v2: Fix sign when !!req->df.  Remove a useless cast.
> > >
> > > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> > > index c6d049c..63a938b 100644
> > > --- a/i386-dm/helper2.c
> > > +++ b/i386-dm/helper2.c
> > > @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long
> > > addr,
> > >      }
> > >  }
> > >
> > > -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> > > +/*
> > > + * Helper functions which read/write an object from/to physical guest
> > > + * memory, as part of the implementation of an ioreq.
> > > + *
> > > + * Equivalent to
> > > + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> > > + *                          val, req->size, 0/1)
> > > + * except without the integer overflow problems.
> > > + */
> > > +static void rw_phys_req_item(target_phys_addr_t addr,
> > > +                             ioreq_t *req, uint32_t i, void *val, int
> > rw)
> > >  {
> > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > 0);
> > > +    /* Do everything unsigned so overflow just results in a truncated result
> > > +     * and accesses to undesired parts of guest memory, which is up
> > > +     * to the guest */
> > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > +    if (req->df) addr -= offset;
> > > +    else addr += offset;
> > > +    cpu_physical_memory_rw(addr, val, req->size, rw);
> > >  }
> > > -
> > > -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> > > +static inline void read_phys_req_item(target_phys_addr_t addr,
> > > +                                      ioreq_t *req, uint32_t i, void
> > > *val)
> > >  {
> > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > 1);
> > > +    rw_phys_req_item(addr, req, i, val, 0);
> > > +}
> > > +static inline void write_phys_req_item(target_phys_addr_t addr,
> > > +                                       ioreq_t *req, uint32_t i,
> > void
> > > *val)
> > > +{
> > > +    rw_phys_req_item(addr, req, i, val, 1);
> > >  }
> > >
> > >  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
> > >  {
> > > -    int i, sign;
> > > -
> > > -    sign = req->df ? -1 : 1;
> > > +    uint32_t i;
> > >
> > >      if (req->dir == IOREQ_READ) {
> > >          if (!req->data_is_ptr) {
> > > @@ -363,9 +382,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),
> > > -                  req->size, &tmp);
> > > +                write_phys_req_item(req->data, req, i, &tmp);
> > >              }
> > >          }
> > >      } else if (req->dir == IOREQ_WRITE) {
> > > @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > *req)
> > >              for (i = 0; i < req->count; i++) {
> > >                  unsigned long tmp = 0;
> > >
> > > -                read_physical((target_phys_addr_t) req->data
> > > -                  + (sign * i * req->size),
> > > -                  req->size, &tmp);
> > > +                read_phys_req_item(req->data, req, i, &tmp);
> > >                  do_outp(env, req->addr, req->size, tmp);
> > >              }
> > >          }
> > > @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > > *req)
> > >
> > >  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
> > >  {
> > > -    int i, sign;
> > > -
> > > -    sign = req->df ? -1 : 1;
> > > +    uint32_t i;
> > >
> > >      if (!req->data_is_ptr) {
> > >          if (req->dir == IOREQ_READ) {
> > >              for (i = 0; i < req->count; i++) {
> > > -                read_physical(req->addr
> > > -                  + (sign * i * req->size),
> > > -                  req->size, &req->data);
> > > +                read_phys_req_item(req->addr, req, i, &req->data);
> > >              }
> > >          } else if (req->dir == IOREQ_WRITE) {
> > >              for (i = 0; i < req->count; i++) {
> > > -                write_physical(req->addr
> > > -                  + (sign * i * req->size),
> > > -                  req->size, &req->data);
> > > +                write_phys_req_item(req->addr, req, i, &req->data);
> > >              }
> > >          }
> > >      } else {
> > > @@ -409,21 +418,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),
> > > -                  req->size, &tmp);
> > > -                write_physical((target_phys_addr_t )req->data
> > > -                  + (sign * i * req->size),
> > > -                  req->size, &tmp);
> > > +                read_phys_req_item(req->addr, req, i, &tmp);
> > > +                write_phys_req_item(req->data, req, i, &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),
> > > -                  req->size, &tmp);
> > > -                write_physical(req->addr
> > > -                  + (sign * i * req->size),
> > > -                  req->size, &tmp);
> > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > +                write_phys_req_item(req->addr, req, i, &tmp);
> > >              }
> > >          }
> > >      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Pasi Kärkkäinen - Jan. 24, 2013, 1:44 p.m.
Hello,

IanJ: I can't see this patch in qemu-xen-unstable .. 
Does the the patch still need something to be fixed before it can be applied? 

Thanks,

-- Pasi

On Wed, Jan 09, 2013 at 09:09:26AM +0200, Pasi Kärkkäinen wrote:
> On Tue, Jan 08, 2013 at 01:49:06AM +0000, Xu, Dongxiao wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > > Sent: Saturday, December 08, 2012 12:34 AM
> > > > To: Ian Campbell
> > > > Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> > > > qemu-devel@nongnu.org
> > > > Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > > > cpu_ioreq_move
> > > >
> > > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> > > > cpu_ioreq_pio	and cpu_ioreq_move"):
> > > > > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > > > +    if (req->df) addr -= offset;
> > > > > > +    else addr -= offset;
> > > > >
> > > > > One of these -= should be a += I presume?
> > > >
> > > > Uh, yes.
> > > >
> > > > > [...]
> > > > > > +                write_phys_req_item((target_phys_addr_t)
> > > req->data,
> > > > req, i, &tmp);
> > > > >
> > > > > This seems to be the only one with this cast, why?
> > > >
> > > > This is a mistake.
> > > >
> > > > > write_phys_req_item takes a target_phys_addr_t so this will happen
> > > > > regardless I think.
> > > >
> > > > Indeed.
> > > >
> > > > Ian.
> > > 
> > > Tested this v2 patch on my system, and it works.
> > 
> > Are we planning to check this patch into qemu-traditional? Since it is a critical patch to boot Xen on Xen.
> > 
> 
> We should definitely get Xen-on-Xen working.. 
> 
> 
> -- Pasi
> 
> > Thanks,
> > Dongxiao
> > 
> > > 
> > > Thanks,
> > > Dongxiao
> > > 
> > > 
> > > >
> > > > commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> > > > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > Date:   Fri Dec 7 16:02:04 2012 +0000
> > > >
> > > >     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> > > > write_phys_req_item
> > > >
> > > >     The current code compare i (int) with req->count (uint32_t) in a for
> > > >     loop, risking an infinite loop if req->count is >INT_MAX.  It also
> > > >     does the multiplication of req->size in a too-small type, leading to
> > > >     integer overflows.
> > > >
> > > >     Turn read_physical and write_physical into two different helper
> > > >     functions, read_phys_req_item and write_phys_req_item, that take
> > > care
> > > >     of adding or subtracting offset depending on sign.
> > > >
> > > >     This removes the formulaic multiplication to a single place where the
> > > >     integer overflows can be dealt with by casting to wide-enough unsigned
> > > >     types.
> > > >
> > > >     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
> > > >     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > >
> > > >     --
> > > >     v2: Fix sign when !!req->df.  Remove a useless cast.
> > > >
> > > > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> > > > index c6d049c..63a938b 100644
> > > > --- a/i386-dm/helper2.c
> > > > +++ b/i386-dm/helper2.c
> > > > @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long
> > > > addr,
> > > >      }
> > > >  }
> > > >
> > > > -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> > > > +/*
> > > > + * Helper functions which read/write an object from/to physical guest
> > > > + * memory, as part of the implementation of an ioreq.
> > > > + *
> > > > + * Equivalent to
> > > > + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> > > > + *                          val, req->size, 0/1)
> > > > + * except without the integer overflow problems.
> > > > + */
> > > > +static void rw_phys_req_item(target_phys_addr_t addr,
> > > > +                             ioreq_t *req, uint32_t i, void *val, int
> > > rw)
> > > >  {
> > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > > 0);
> > > > +    /* Do everything unsigned so overflow just results in a truncated result
> > > > +     * and accesses to undesired parts of guest memory, which is up
> > > > +     * to the guest */
> > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > +    if (req->df) addr -= offset;
> > > > +    else addr += offset;
> > > > +    cpu_physical_memory_rw(addr, val, req->size, rw);
> > > >  }
> > > > -
> > > > -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> > > > +static inline void read_phys_req_item(target_phys_addr_t addr,
> > > > +                                      ioreq_t *req, uint32_t i, void
> > > > *val)
> > > >  {
> > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > > 1);
> > > > +    rw_phys_req_item(addr, req, i, val, 0);
> > > > +}
> > > > +static inline void write_phys_req_item(target_phys_addr_t addr,
> > > > +                                       ioreq_t *req, uint32_t i,
> > > void
> > > > *val)
> > > > +{
> > > > +    rw_phys_req_item(addr, req, i, val, 1);
> > > >  }
> > > >
> > > >  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
> > > >  {
> > > > -    int i, sign;
> > > > -
> > > > -    sign = req->df ? -1 : 1;
> > > > +    uint32_t i;
> > > >
> > > >      if (req->dir == IOREQ_READ) {
> > > >          if (!req->data_is_ptr) {
> > > > @@ -363,9 +382,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),
> > > > -                  req->size, &tmp);
> > > > +                write_phys_req_item(req->data, req, i, &tmp);
> > > >              }
> > > >          }
> > > >      } else if (req->dir == IOREQ_WRITE) {
> > > > @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > > *req)
> > > >              for (i = 0; i < req->count; i++) {
> > > >                  unsigned long tmp = 0;
> > > >
> > > > -                read_physical((target_phys_addr_t) req->data
> > > > -                  + (sign * i * req->size),
> > > > -                  req->size, &tmp);
> > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > >                  do_outp(env, req->addr, req->size, tmp);
> > > >              }
> > > >          }
> > > > @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > > > *req)
> > > >
> > > >  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
> > > >  {
> > > > -    int i, sign;
> > > > -
> > > > -    sign = req->df ? -1 : 1;
> > > > +    uint32_t i;
> > > >
> > > >      if (!req->data_is_ptr) {
> > > >          if (req->dir == IOREQ_READ) {
> > > >              for (i = 0; i < req->count; i++) {
> > > > -                read_physical(req->addr
> > > > -                  + (sign * i * req->size),
> > > > -                  req->size, &req->data);
> > > > +                read_phys_req_item(req->addr, req, i, &req->data);
> > > >              }
> > > >          } else if (req->dir == IOREQ_WRITE) {
> > > >              for (i = 0; i < req->count; i++) {
> > > > -                write_physical(req->addr
> > > > -                  + (sign * i * req->size),
> > > > -                  req->size, &req->data);
> > > > +                write_phys_req_item(req->addr, req, i, &req->data);
> > > >              }
> > > >          }
> > > >      } else {
> > > > @@ -409,21 +418,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),
> > > > -                  req->size, &tmp);
> > > > -                write_physical((target_phys_addr_t )req->data
> > > > -                  + (sign * i * req->size),
> > > > -                  req->size, &tmp);
> > > > +                read_phys_req_item(req->addr, req, i, &tmp);
> > > > +                write_phys_req_item(req->data, req, i, &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),
> > > > -                  req->size, &tmp);
> > > > -                write_physical(req->addr
> > > > -                  + (sign * i * req->size),
> > > > -                  req->size, &tmp);
> > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > > +                write_phys_req_item(req->addr, req, i, &tmp);
> > > >              }
> > > >          }
> > > >      }
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Dongxiao Xu - Jan. 31, 2013, 2:23 a.m.
> -----Original Message-----
> From: Pasi Kärkkäinen [mailto:pasik@iki.fi]
> Sent: Thursday, January 24, 2013 9:45 PM
> To: Ian Jackson
> Cc: xen-devel@lists.xensource.com; Ian Campbell; Stefano Stabellini;
> qemu-devel@nongnu.org; Xu, Dongxiao
> Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> cpu_ioreq_move / Xen on Xen nested virt
> 
> Hello,
> 
> IanJ: I can't see this patch in qemu-xen-unstable ..
> Does the the patch still need something to be fixed before it can be applied?

Yes, I am also confused why we still keep this fix out of the qemu-xen tree. :(

Our QA needs to apply additional patch to test nested virtualization cases.

Thanks,
Dongxiao

> 
> Thanks,
> 
> -- Pasi
> 
> On Wed, Jan 09, 2013 at 09:09:26AM +0200, Pasi Kärkkäinen wrote:
> > On Tue, Jan 08, 2013 at 01:49:06AM +0000, Xu, Dongxiao wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > > > Sent: Saturday, December 08, 2012 12:34 AM
> > > > > To: Ian Campbell
> > > > > Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> > > > > qemu-devel@nongnu.org
> > > > > Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio
> and
> > > > > cpu_ioreq_move
> > > > >
> > > > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> > > > > cpu_ioreq_pio	and cpu_ioreq_move"):
> > > > > > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size *
> i;
> > > > > > > +    if (req->df) addr -= offset;
> > > > > > > +    else addr -= offset;
> > > > > >
> > > > > > One of these -= should be a += I presume?
> > > > >
> > > > > Uh, yes.
> > > > >
> > > > > > [...]
> > > > > > > +                write_phys_req_item((target_phys_addr_t)
> > > > req->data,
> > > > > req, i, &tmp);
> > > > > >
> > > > > > This seems to be the only one with this cast, why?
> > > > >
> > > > > This is a mistake.
> > > > >
> > > > > > write_phys_req_item takes a target_phys_addr_t so this will happen
> > > > > > regardless I think.
> > > > >
> > > > > Indeed.
> > > > >
> > > > > Ian.
> > > >
> > > > Tested this v2 patch on my system, and it works.
> > >
> > > Are we planning to check this patch into qemu-traditional? Since it is a
> critical patch to boot Xen on Xen.
> > >
> >
> > We should definitely get Xen-on-Xen working..
> >
> >
> > -- Pasi
> >
> > > Thanks,
> > > Dongxiao
> > >
> > > >
> > > > Thanks,
> > > > Dongxiao
> > > >
> > > >
> > > > >
> > > > > commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> > > > > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > Date:   Fri Dec 7 16:02:04 2012 +0000
> > > > >
> > > > >     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> > > > > write_phys_req_item
> > > > >
> > > > >     The current code compare i (int) with req->count (uint32_t) in a for
> > > > >     loop, risking an infinite loop if req->count is >INT_MAX.  It also
> > > > >     does the multiplication of req->size in a too-small type, leading to
> > > > >     integer overflows.
> > > > >
> > > > >     Turn read_physical and write_physical into two different helper
> > > > >     functions, read_phys_req_item and write_phys_req_item, that
> take
> > > > care
> > > > >     of adding or subtracting offset depending on sign.
> > > > >
> > > > >     This removes the formulaic multiplication to a single place where
> the
> > > > >     integer overflows can be dealt with by casting to wide-enough
> unsigned
> > > > >     types.
> > > > >
> > > > >     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
> > > > >     Signed-off-by: Stefano Stabellini
> <stefano.stabellini@eu.citrix.com>
> > > > >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > >
> > > > >     --
> > > > >     v2: Fix sign when !!req->df.  Remove a useless cast.
> > > > >
> > > > > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> > > > > index c6d049c..63a938b 100644
> > > > > --- a/i386-dm/helper2.c
> > > > > +++ b/i386-dm/helper2.c
> > > > > @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned
> long
> > > > > addr,
> > > > >      }
> > > > >  }
> > > > >
> > > > > -static inline void read_physical(uint64_t addr, unsigned long size, void
> *val)
> > > > > +/*
> > > > > + * Helper functions which read/write an object from/to physical guest
> > > > > + * memory, as part of the implementation of an ioreq.
> > > > > + *
> > > > > + * Equivalent to
> > > > > + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size *
> i,
> > > > > + *                          val, req->size, 0/1)
> > > > > + * except without the integer overflow problems.
> > > > > + */
> > > > > +static void rw_phys_req_item(target_phys_addr_t addr,
> > > > > +                             ioreq_t *req, uint32_t i, void *val,
> int
> > > > rw)
> > > > >  {
> > > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val,
> size,
> > > > 0);
> > > > > +    /* Do everything unsigned so overflow just results in a truncated
> result
> > > > > +     * and accesses to undesired parts of guest memory, which is up
> > > > > +     * to the guest */
> > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > > +    if (req->df) addr -= offset;
> > > > > +    else addr += offset;
> > > > > +    cpu_physical_memory_rw(addr, val, req->size, rw);
> > > > >  }
> > > > > -
> > > > > -static inline void write_physical(uint64_t addr, unsigned long size, void
> *val)
> > > > > +static inline void read_phys_req_item(target_phys_addr_t addr,
> > > > > +                                      ioreq_t *req, uint32_t i,
> void
> > > > > *val)
> > > > >  {
> > > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val,
> size,
> > > > 1);
> > > > > +    rw_phys_req_item(addr, req, i, val, 0);
> > > > > +}
> > > > > +static inline void write_phys_req_item(target_phys_addr_t addr,
> > > > > +                                       ioreq_t *req, uint32_t
> i,
> > > > void
> > > > > *val)
> > > > > +{
> > > > > +    rw_phys_req_item(addr, req, i, val, 1);
> > > > >  }
> > > > >
> > > > >  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
> > > > >  {
> > > > > -    int i, sign;
> > > > > -
> > > > > -    sign = req->df ? -1 : 1;
> > > > > +    uint32_t i;
> > > > >
> > > > >      if (req->dir == IOREQ_READ) {
> > > > >          if (!req->data_is_ptr) {
> > > > > @@ -363,9 +382,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),
> > > > > -                  req->size, &tmp);
> > > > > +                write_phys_req_item(req->data, req, i, &tmp);
> > > > >              }
> > > > >          }
> > > > >      } else if (req->dir == IOREQ_WRITE) {
> > > > > @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env,
> ioreq_t
> > > > *req)
> > > > >              for (i = 0; i < req->count; i++) {
> > > > >                  unsigned long tmp = 0;
> > > > >
> > > > > -                read_physical((target_phys_addr_t) req->data
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > > >                  do_outp(env, req->addr, req->size, tmp);
> > > > >              }
> > > > >          }
> > > > > @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env,
> ioreq_t
> > > > > *req)
> > > > >
> > > > >  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
> > > > >  {
> > > > > -    int i, sign;
> > > > > -
> > > > > -    sign = req->df ? -1 : 1;
> > > > > +    uint32_t i;
> > > > >
> > > > >      if (!req->data_is_ptr) {
> > > > >          if (req->dir == IOREQ_READ) {
> > > > >              for (i = 0; i < req->count; i++) {
> > > > > -                read_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &req->data);
> > > > > +                read_phys_req_item(req->addr, req, i,
> &req->data);
> > > > >              }
> > > > >          } else if (req->dir == IOREQ_WRITE) {
> > > > >              for (i = 0; i < req->count; i++) {
> > > > > -                write_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &req->data);
> > > > > +                write_phys_req_item(req->addr, req, i,
> &req->data);
> > > > >              }
> > > > >          }
> > > > >      } else {
> > > > > @@ -409,21 +418,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),
> > > > > -                  req->size, &tmp);
> > > > > -                write_physical((target_phys_addr_t )req->data
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->addr, req, i, &tmp);
> > > > > +                write_phys_req_item(req->data, req, i, &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),
> > > > > -                  req->size, &tmp);
> > > > > -                write_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > > > +                write_phys_req_item(req->addr, req, i, &tmp);
> > > > >              }
> > > > >          }
> > > > >      }
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
Pasi Kärkkäinen - Feb. 19, 2013, 7:38 p.m.
On Thu, Jan 24, 2013 at 03:44:41PM +0200, Pasi Kärkkäinen wrote:
> Hello,
> 
> IanJ: I can't see this patch in qemu-xen-unstable .. 
> Does the the patch still need something to be fixed before it can be applied? 
>

Ping again? I wonder if I've gotten into some blacklist already for nagging about this.. 

This is a required patch for making Xen-on-Xen Nested virt working on Intel..

thanks,

-- Pasi
 
> 
> On Wed, Jan 09, 2013 at 09:09:26AM +0200, Pasi Kärkkäinen wrote:
> > On Tue, Jan 08, 2013 at 01:49:06AM +0000, Xu, Dongxiao wrote:
> > > > 
> > > > > -----Original Message-----
> > > > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > > > Sent: Saturday, December 08, 2012 12:34 AM
> > > > > To: Ian Campbell
> > > > > Cc: Stefano Stabellini; Xu, Dongxiao; xen-devel@lists.xensource.com;
> > > > > qemu-devel@nongnu.org
> > > > > Subject: Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > > > > cpu_ioreq_move
> > > > >
> > > > > Ian Campbell writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify
> > > > > cpu_ioreq_pio	and cpu_ioreq_move"):
> > > > > > On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> > > > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > > > > +    if (req->df) addr -= offset;
> > > > > > > +    else addr -= offset;
> > > > > >
> > > > > > One of these -= should be a += I presume?
> > > > >
> > > > > Uh, yes.
> > > > >
> > > > > > [...]
> > > > > > > +                write_phys_req_item((target_phys_addr_t)
> > > > req->data,
> > > > > req, i, &tmp);
> > > > > >
> > > > > > This seems to be the only one with this cast, why?
> > > > >
> > > > > This is a mistake.
> > > > >
> > > > > > write_phys_req_item takes a target_phys_addr_t so this will happen
> > > > > > regardless I think.
> > > > >
> > > > > Indeed.
> > > > >
> > > > > Ian.
> > > > 
> > > > Tested this v2 patch on my system, and it works.
> > > 
> > > Are we planning to check this patch into qemu-traditional? Since it is a critical patch to boot Xen on Xen.
> > > 
> > 
> > We should definitely get Xen-on-Xen working.. 
> > 
> > 
> > -- Pasi
> > 
> > > Thanks,
> > > Dongxiao
> > > 
> > > > 
> > > > Thanks,
> > > > Dongxiao
> > > > 
> > > > 
> > > > >
> > > > > commit fd3865f8e0d867a203b4ddcb22eefa827cfaca0a
> > > > > Author: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > Date:   Fri Dec 7 16:02:04 2012 +0000
> > > > >
> > > > >     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item,
> > > > > write_phys_req_item
> > > > >
> > > > >     The current code compare i (int) with req->count (uint32_t) in a for
> > > > >     loop, risking an infinite loop if req->count is >INT_MAX.  It also
> > > > >     does the multiplication of req->size in a too-small type, leading to
> > > > >     integer overflows.
> > > > >
> > > > >     Turn read_physical and write_physical into two different helper
> > > > >     functions, read_phys_req_item and write_phys_req_item, that take
> > > > care
> > > > >     of adding or subtracting offset depending on sign.
> > > > >
> > > > >     This removes the formulaic multiplication to a single place where the
> > > > >     integer overflows can be dealt with by casting to wide-enough unsigned
> > > > >     types.
> > > > >
> > > > >     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
> > > > >     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > >     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > >
> > > > >     --
> > > > >     v2: Fix sign when !!req->df.  Remove a useless cast.
> > > > >
> > > > > diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> > > > > index c6d049c..63a938b 100644
> > > > > --- a/i386-dm/helper2.c
> > > > > +++ b/i386-dm/helper2.c
> > > > > @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long
> > > > > addr,
> > > > >      }
> > > > >  }
> > > > >
> > > > > -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> > > > > +/*
> > > > > + * Helper functions which read/write an object from/to physical guest
> > > > > + * memory, as part of the implementation of an ioreq.
> > > > > + *
> > > > > + * Equivalent to
> > > > > + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> > > > > + *                          val, req->size, 0/1)
> > > > > + * except without the integer overflow problems.
> > > > > + */
> > > > > +static void rw_phys_req_item(target_phys_addr_t addr,
> > > > > +                             ioreq_t *req, uint32_t i, void *val, int
> > > > rw)
> > > > >  {
> > > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > > > 0);
> > > > > +    /* Do everything unsigned so overflow just results in a truncated result
> > > > > +     * and accesses to undesired parts of guest memory, which is up
> > > > > +     * to the guest */
> > > > > +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> > > > > +    if (req->df) addr -= offset;
> > > > > +    else addr += offset;
> > > > > +    cpu_physical_memory_rw(addr, val, req->size, rw);
> > > > >  }
> > > > > -
> > > > > -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> > > > > +static inline void read_phys_req_item(target_phys_addr_t addr,
> > > > > +                                      ioreq_t *req, uint32_t i, void
> > > > > *val)
> > > > >  {
> > > > > -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size,
> > > > 1);
> > > > > +    rw_phys_req_item(addr, req, i, val, 0);
> > > > > +}
> > > > > +static inline void write_phys_req_item(target_phys_addr_t addr,
> > > > > +                                       ioreq_t *req, uint32_t i,
> > > > void
> > > > > *val)
> > > > > +{
> > > > > +    rw_phys_req_item(addr, req, i, val, 1);
> > > > >  }
> > > > >
> > > > >  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
> > > > >  {
> > > > > -    int i, sign;
> > > > > -
> > > > > -    sign = req->df ? -1 : 1;
> > > > > +    uint32_t i;
> > > > >
> > > > >      if (req->dir == IOREQ_READ) {
> > > > >          if (!req->data_is_ptr) {
> > > > > @@ -363,9 +382,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),
> > > > > -                  req->size, &tmp);
> > > > > +                write_phys_req_item(req->data, req, i, &tmp);
> > > > >              }
> > > > >          }
> > > > >      } else if (req->dir == IOREQ_WRITE) {
> > > > > @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > > > *req)
> > > > >              for (i = 0; i < req->count; i++) {
> > > > >                  unsigned long tmp = 0;
> > > > >
> > > > > -                read_physical((target_phys_addr_t) req->data
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > > >                  do_outp(env, req->addr, req->size, tmp);
> > > > >              }
> > > > >          }
> > > > > @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t
> > > > > *req)
> > > > >
> > > > >  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
> > > > >  {
> > > > > -    int i, sign;
> > > > > -
> > > > > -    sign = req->df ? -1 : 1;
> > > > > +    uint32_t i;
> > > > >
> > > > >      if (!req->data_is_ptr) {
> > > > >          if (req->dir == IOREQ_READ) {
> > > > >              for (i = 0; i < req->count; i++) {
> > > > > -                read_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &req->data);
> > > > > +                read_phys_req_item(req->addr, req, i, &req->data);
> > > > >              }
> > > > >          } else if (req->dir == IOREQ_WRITE) {
> > > > >              for (i = 0; i < req->count; i++) {
> > > > > -                write_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &req->data);
> > > > > +                write_phys_req_item(req->addr, req, i, &req->data);
> > > > >              }
> > > > >          }
> > > > >      } else {
> > > > > @@ -409,21 +418,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),
> > > > > -                  req->size, &tmp);
> > > > > -                write_physical((target_phys_addr_t )req->data
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->addr, req, i, &tmp);
> > > > > +                write_phys_req_item(req->data, req, i, &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),
> > > > > -                  req->size, &tmp);
> > > > > -                write_physical(req->addr
> > > > > -                  + (sign * i * req->size),
> > > > > -                  req->size, &tmp);
> > > > > +                read_phys_req_item(req->data, req, i, &tmp);
> > > > > +                write_phys_req_item(req->addr, req, i, &tmp);
> > > > >              }
> > > > >          }
> > > > >      }
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Ian Jackson - Feb. 20, 2013, 3:42 p.m.
Pasi Kärkkäinen writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio	and cpu_ioreq_move / Xen on Xen nested virt"):
> Ping again? I wonder if I've gotten into some blacklist already for nagging about this.. 
> 
> This is a required patch for making Xen-on-Xen Nested virt working on Intel..

Sorry about that, I have applied it.

Ian.
Pasi Kärkkäinen - Feb. 20, 2013, 3:53 p.m.
On Wed, Feb 20, 2013 at 03:42:15PM +0000, Ian Jackson wrote:
> Pasi Kärkkäinen writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio	and cpu_ioreq_move / Xen on Xen nested virt"):
> > Ping again? I wonder if I've gotten into some blacklist already for nagging about this.. 
> > 
> > This is a required patch for making Xen-on-Xen Nested virt working on Intel..
> 
> Sorry about that, I have applied it.
>

Thanks!

-- Pasi

Patch

diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..63a938b 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -339,21 +339,40 @@  static void do_outp(CPUState *env, unsigned long addr,
     }
 }
 
-static inline void read_physical(uint64_t addr, unsigned long size, void *val)
+/*
+ * Helper functions which read/write an object from/to physical guest
+ * memory, as part of the implementation of an ioreq.
+ *
+ * Equivalent to
+ *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
+ *                          val, req->size, 0/1)
+ * except without the integer overflow problems.
+ */
+static void rw_phys_req_item(target_phys_addr_t addr,
+                             ioreq_t *req, uint32_t i, void *val, int rw)
 {
-    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
+    /* Do everything unsigned so overflow just results in a truncated result
+     * and accesses to undesired parts of guest memory, which is up
+     * to the guest */
+    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
+    if (req->df) addr -= offset;
+    else addr += offset;
+    cpu_physical_memory_rw(addr, val, req->size, rw);
 }
-
-static inline void write_physical(uint64_t addr, unsigned long size, void *val)
+static inline void read_phys_req_item(target_phys_addr_t addr,
+                                      ioreq_t *req, uint32_t i, void *val)
 {
-    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
+    rw_phys_req_item(addr, req, i, val, 0);
+}
+static inline void write_phys_req_item(target_phys_addr_t addr,
+                                       ioreq_t *req, uint32_t i, void *val)
+{
+    rw_phys_req_item(addr, req, i, val, 1);
 }
 
 static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
 {
-    int i, sign;
-
-    sign = req->df ? -1 : 1;
+    uint32_t i;
 
     if (req->dir == IOREQ_READ) {
         if (!req->data_is_ptr) {
@@ -363,9 +382,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),
-                  req->size, &tmp);
+                write_phys_req_item(req->data, req, i, &tmp);
             }
         }
     } else if (req->dir == IOREQ_WRITE) {
@@ -375,9 +392,7 @@  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
             for (i = 0; i < req->count; i++) {
                 unsigned long tmp = 0;
 
-                read_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->data, req, i, &tmp);
                 do_outp(env, req->addr, req->size, tmp);
             }
         }
@@ -386,22 +401,16 @@  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
 
 static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
 {
-    int i, sign;
-
-    sign = req->df ? -1 : 1;
+    uint32_t i;
 
     if (!req->data_is_ptr) {
         if (req->dir == IOREQ_READ) {
             for (i = 0; i < req->count; i++) {
-                read_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &req->data);
+                read_phys_req_item(req->addr, req, i, &req->data);
             }
         } else if (req->dir == IOREQ_WRITE) {
             for (i = 0; i < req->count; i++) {
-                write_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &req->data);
+                write_phys_req_item(req->addr, req, i, &req->data);
             }
         }
     } else {
@@ -409,21 +418,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),
-                  req->size, &tmp);
-                write_physical((target_phys_addr_t )req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->addr, req, i, &tmp);
+                write_phys_req_item(req->data, req, i, &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),
-                  req->size, &tmp);
-                write_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->data, req, i, &tmp);
+                write_phys_req_item(req->addr, req, i, &tmp);
             }
         }
     }