* [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-09-10 18:04 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:04 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian Jackson, qemu-devel, Stefano Stabellini
Hi all,
after reviewing the patch "fix multiply issue for int and uint types"
with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
in much need for a simplification as well as removal of a possible
integer overflow.
This patch series tries to accomplish both switching to two new helper
functions and using a more obvious arithmetic. Doing so it should also
fix the original problem that Dongxiao was experiencing. The C language
can be a nasty backstabber when signed and unsigned integers are
involved.
The current patch series if for qemu-xen-traditional but if the patches
are deemed correct I'll submit an equivalent set for QEMU upstream (with
the appropriate code style changes).
Stefano Stabellini (2):
i should be uint32_t rather than int
introduce read_physical_offset and write_physical_offset
i386-dm/helper2.c | 66 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 42 insertions(+), 24 deletions(-)
Cheers,
Stefano
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-09-10 18:04 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:04 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian Jackson, qemu-devel, Stefano Stabellini
Hi all,
after reviewing the patch "fix multiply issue for int and uint types"
with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
in much need for a simplification as well as removal of a possible
integer overflow.
This patch series tries to accomplish both switching to two new helper
functions and using a more obvious arithmetic. Doing so it should also
fix the original problem that Dongxiao was experiencing. The C language
can be a nasty backstabber when signed and unsigned integers are
involved.
The current patch series if for qemu-xen-traditional but if the patches
are deemed correct I'll submit an equivalent set for QEMU upstream (with
the appropriate code style changes).
Stefano Stabellini (2):
i should be uint32_t rather than int
introduce read_physical_offset and write_physical_offset
i386-dm/helper2.c | 66 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 42 insertions(+), 24 deletions(-)
Cheers,
Stefano
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 1/2] i should be uint32_t rather than int
2012-09-10 18:04 ` Stefano Stabellini
@ 2012-09-10 18:06 ` Stefano Stabellini
-1 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian.Jackson, qemu-devel, Stefano Stabellini
The current code compare i (int) with req->count (uint32_t) in a for
loop, risking an infinite loop if req->count is equal to UINT_MAX.
Also i is only used in comparisons or multiplications with unsigned
integers.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
i386-dm/helper2.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..8f2a893 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -351,7 +351,8 @@ static inline void write_physical(uint64_t addr, unsigned long size, void *val)
static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
{
- int i, sign;
+ uint32_t i;
+ int sign;
sign = req->df ? -1 : 1;
@@ -386,7 +387,8 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
{
- int i, sign;
+ uint32_t i;
+ int sign;
sign = req->df ? -1 : 1;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 1/2] i should be uint32_t rather than int
@ 2012-09-10 18:06 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian.Jackson, qemu-devel, Stefano Stabellini
The current code compare i (int) with req->count (uint32_t) in a for
loop, risking an infinite loop if req->count is equal to UINT_MAX.
Also i is only used in comparisons or multiplications with unsigned
integers.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
i386-dm/helper2.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..8f2a893 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -351,7 +351,8 @@ static inline void write_physical(uint64_t addr, unsigned long size, void *val)
static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
{
- int i, sign;
+ uint32_t i;
+ int sign;
sign = req->df ? -1 : 1;
@@ -386,7 +387,8 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
{
- int i, sign;
+ uint32_t i;
+ int sign;
sign = req->df ? -1 : 1;
--
1.7.2.5
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH 2/2] introduce read_physical_offset and write_physical_offset
2012-09-10 18:04 ` Stefano Stabellini
@ 2012-09-10 18:06 ` Stefano Stabellini
-1 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian.Jackson, qemu-devel, Stefano Stabellini
Remove read_physical and write_physical.
Introduce two new helper functions, read_physical_offset and
write_physical_offset, that take care of adding or subtracting offset
depending on sign. This way we avoid the automatic casting of sign to
uint32_t that is clearly not a very good idea and can easily cause
overflows. It also makes the code easier to understand.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
i386-dm/helper2.c | 60 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index 8f2a893..5eb1901 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -339,14 +339,30 @@ static void do_outp(CPUState *env, unsigned long addr,
}
}
-static inline void read_physical(uint64_t addr, unsigned long size, void *val)
+static inline void read_physical_offset(target_phys_addr_t addr,
+ unsigned long offset,
+ int sign,
+ unsigned long size,
+ void *val)
{
- return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
+ if (sign >= 0)
+ addr += offset;
+ else
+ addr -= offset;
+ return cpu_physical_memory_rw(addr, val, size, 0);
}
-static inline void write_physical(uint64_t addr, unsigned long size, void *val)
+static inline void write_physical_offset(target_phys_addr_t addr,
+ unsigned long offset,
+ int sign,
+ unsigned long size,
+ void *val)
{
- return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
+ if (sign >= 0)
+ addr += offset;
+ else
+ addr -= offset;
+ return cpu_physical_memory_rw(addr, val, size, 1);
}
static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
@@ -364,9 +380,9 @@ 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_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
+ req->size, &tmp);
}
}
} else if (req->dir == IOREQ_WRITE) {
@@ -376,9 +392,9 @@ 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_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
+ req->size, &tmp);
do_outp(env, req->addr, req->size, tmp);
}
}
@@ -395,14 +411,14 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
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),
+ read_physical_offset(req->addr,
+ i * req->size, sign,
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),
+ write_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &req->data);
}
}
@@ -411,20 +427,20 @@ 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),
+ read_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &tmp);
- write_physical((target_phys_addr_t )req->data
- + (sign * i * req->size),
+ write_physical_offset((target_phys_addr_t )req->data,
+ i * req->size, sign,
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),
+ read_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
req->size, &tmp);
- write_physical(req->addr
- + (sign * i * req->size),
+ write_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &tmp);
}
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/2] introduce read_physical_offset and write_physical_offset
@ 2012-09-10 18:06 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-10 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: dongxiao.xu, Ian.Jackson, qemu-devel, Stefano Stabellini
Remove read_physical and write_physical.
Introduce two new helper functions, read_physical_offset and
write_physical_offset, that take care of adding or subtracting offset
depending on sign. This way we avoid the automatic casting of sign to
uint32_t that is clearly not a very good idea and can easily cause
overflows. It also makes the code easier to understand.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
i386-dm/helper2.c | 60 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index 8f2a893..5eb1901 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -339,14 +339,30 @@ static void do_outp(CPUState *env, unsigned long addr,
}
}
-static inline void read_physical(uint64_t addr, unsigned long size, void *val)
+static inline void read_physical_offset(target_phys_addr_t addr,
+ unsigned long offset,
+ int sign,
+ unsigned long size,
+ void *val)
{
- return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
+ if (sign >= 0)
+ addr += offset;
+ else
+ addr -= offset;
+ return cpu_physical_memory_rw(addr, val, size, 0);
}
-static inline void write_physical(uint64_t addr, unsigned long size, void *val)
+static inline void write_physical_offset(target_phys_addr_t addr,
+ unsigned long offset,
+ int sign,
+ unsigned long size,
+ void *val)
{
- return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
+ if (sign >= 0)
+ addr += offset;
+ else
+ addr -= offset;
+ return cpu_physical_memory_rw(addr, val, size, 1);
}
static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
@@ -364,9 +380,9 @@ 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_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
+ req->size, &tmp);
}
}
} else if (req->dir == IOREQ_WRITE) {
@@ -376,9 +392,9 @@ 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_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
+ req->size, &tmp);
do_outp(env, req->addr, req->size, tmp);
}
}
@@ -395,14 +411,14 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
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),
+ read_physical_offset(req->addr,
+ i * req->size, sign,
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),
+ write_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &req->data);
}
}
@@ -411,20 +427,20 @@ 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),
+ read_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &tmp);
- write_physical((target_phys_addr_t )req->data
- + (sign * i * req->size),
+ write_physical_offset((target_phys_addr_t )req->data,
+ i * req->size, sign,
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),
+ read_physical_offset((target_phys_addr_t) req->data,
+ i * req->size, sign,
req->size, &tmp);
- write_physical(req->addr
- + (sign * i * req->size),
+ write_physical_offset(req->addr,
+ i * req->size, sign,
req->size, &tmp);
}
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-09-10 18:04 ` Stefano Stabellini
@ 2012-09-17 23:18 ` Xu, Dongxiao
-1 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-09-17 23:18 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Keir Fraser <keir@xen.org> (keir@xen.org), Ian Jackson, qemu-devel
Hi Stefano,
Is these patches merged with Xen 4.2? I didn't see them in the upstream.
The uint/int fix is critical to fix the nested guest boot issue.
Thanks,
Dongxiao
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, September 11, 2012 2:05 AM
> To: xen-devel@lists.xensource.com
> Cc: Xu, Dongxiao; Ian Jackson; qemu-devel@nongnu.org; Stefano Stabellini
> Subject: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
>
> Hi all,
> after reviewing the patch "fix multiply issue for int and uint types"
> with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are in
> much need for a simplification as well as removal of a possible integer overflow.
>
> This patch series tries to accomplish both switching to two new helper
> functions and using a more obvious arithmetic. Doing so it should also fix the
> original problem that Dongxiao was experiencing. The C language can be a
> nasty backstabber when signed and unsigned integers are involved.
>
> The current patch series if for qemu-xen-traditional but if the patches are
> deemed correct I'll submit an equivalent set for QEMU upstream (with the
> appropriate code style changes).
>
>
>
> Stefano Stabellini (2):
> i should be uint32_t rather than int
> introduce read_physical_offset and write_physical_offset
>
> i386-dm/helper2.c | 66
> +++++++++++++++++++++++++++++++++-------------------
> 1 files changed, 42 insertions(+), 24 deletions(-)
>
>
>
> Cheers,
>
> Stefano
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-09-17 23:18 ` Xu, Dongxiao
0 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-09-17 23:18 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: Keir Fraser <keir@xen.org> (keir@xen.org), Ian Jackson, qemu-devel
Hi Stefano,
Is these patches merged with Xen 4.2? I didn't see them in the upstream.
The uint/int fix is critical to fix the nested guest boot issue.
Thanks,
Dongxiao
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, September 11, 2012 2:05 AM
> To: xen-devel@lists.xensource.com
> Cc: Xu, Dongxiao; Ian Jackson; qemu-devel@nongnu.org; Stefano Stabellini
> Subject: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
>
> Hi all,
> after reviewing the patch "fix multiply issue for int and uint types"
> with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are in
> much need for a simplification as well as removal of a possible integer overflow.
>
> This patch series tries to accomplish both switching to two new helper
> functions and using a more obvious arithmetic. Doing so it should also fix the
> original problem that Dongxiao was experiencing. The C language can be a
> nasty backstabber when signed and unsigned integers are involved.
>
> The current patch series if for qemu-xen-traditional but if the patches are
> deemed correct I'll submit an equivalent set for QEMU upstream (with the
> appropriate code style changes).
>
>
>
> Stefano Stabellini (2):
> i should be uint32_t rather than int
> introduce read_physical_offset and write_physical_offset
>
> i386-dm/helper2.c | 66
> +++++++++++++++++++++++++++++++++-------------------
> 1 files changed, 42 insertions(+), 24 deletions(-)
>
>
>
> Cheers,
>
> Stefano
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-09-17 23:18 ` Xu, Dongxiao
@ 2012-09-18 10:24 ` Stefano Stabellini
-1 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-18 10:24 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel, Stefano Stabellini
On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> Hi Stefano,
>
> Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> The uint/int fix is critical to fix the nested guest boot issue.
They are not. Ian decided that he wanted to merge a different version of
them.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-09-18 10:24 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-09-18 10:24 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel, Stefano Stabellini
On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> Hi Stefano,
>
> Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> The uint/int fix is critical to fix the nested guest boot issue.
They are not. Ian decided that he wanted to merge a different version of
them.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-09-18 10:24 ` Stefano Stabellini
@ 2012-11-29 3:22 ` Xu, Dongxiao
-1 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-11-29 3:22 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, September 18, 2012 6:24 PM
> To: Xu, Dongxiao
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Ian Jackson;
> qemu-devel@nongnu.org; Keir (Xen.org)
> Subject: RE: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> cpu_ioreq_move
>
> On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> > Hi Stefano,
> >
> > Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> > The uint/int fix is critical to fix the nested guest boot issue.
>
> They are not. Ian decided that he wanted to merge a different version of them.
Hi Stefano and Ian,
What's the status of the two patches? I still didn't see them merged...
Also I think 4.2.1 need these patches to enable the basic Xen on Xen nested virtualization usage scenario.
Thanks,
Dongxiao
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-11-29 3:22 ` Xu, Dongxiao
0 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-11-29 3:22 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel
> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, September 18, 2012 6:24 PM
> To: Xu, Dongxiao
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Ian Jackson;
> qemu-devel@nongnu.org; Keir (Xen.org)
> Subject: RE: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> cpu_ioreq_move
>
> On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> > Hi Stefano,
> >
> > Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> > The uint/int fix is critical to fix the nested guest boot issue.
>
> They are not. Ian decided that he wanted to merge a different version of them.
Hi Stefano and Ian,
What's the status of the two patches? I still didn't see them merged...
Also I think 4.2.1 need these patches to enable the basic Xen on Xen nested virtualization usage scenario.
Thanks,
Dongxiao
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-11-29 3:22 ` Xu, Dongxiao
@ 2012-11-29 13:21 ` Stefano Stabellini
-1 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-11-29 13:21 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel, Stefano Stabellini
On Thu, 29 Nov 2012, Xu, Dongxiao wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Tuesday, September 18, 2012 6:24 PM
> > To: Xu, Dongxiao
> > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Ian Jackson;
> > qemu-devel@nongnu.org; Keir (Xen.org)
> > Subject: RE: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > cpu_ioreq_move
> >
> > On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> > > Hi Stefano,
> > >
> > > Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> > > The uint/int fix is critical to fix the nested guest boot issue.
> >
> > They are not. Ian decided that he wanted to merge a different version of them.
>
> Hi Stefano and Ian,
>
> What's the status of the two patches? I still didn't see them merged...
Ian, do you have any updates?
> Also I think 4.2.1 need these patches to enable the basic Xen on Xen nested virtualization usage scenario.
I agree.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-11-29 13:21 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-11-29 13:21 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: Keir (Xen.org), xen-devel, Ian Jackson, qemu-devel, Stefano Stabellini
On Thu, 29 Nov 2012, Xu, Dongxiao wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: Tuesday, September 18, 2012 6:24 PM
> > To: Xu, Dongxiao
> > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Ian Jackson;
> > qemu-devel@nongnu.org; Keir (Xen.org)
> > Subject: RE: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and
> > cpu_ioreq_move
> >
> > On Tue, 18 Sep 2012, Xu, Dongxiao wrote:
> > > Hi Stefano,
> > >
> > > Is these patches merged with Xen 4.2? I didn't see them in the upstream.
> > > The uint/int fix is critical to fix the nested guest boot issue.
> >
> > They are not. Ian decided that he wanted to merge a different version of them.
>
> Hi Stefano and Ian,
>
> What's the status of the two patches? I still didn't see them merged...
Ian, do you have any updates?
> Also I think 4.2.1 need these patches to enable the basic Xen on Xen nested virtualization usage scenario.
I agree.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-11-29 13:21 ` Stefano Stabellini
@ 2012-11-29 13:57 ` Ian Campbell
-1 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2012-11-29 13:57 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Ian Jackson, Xu, Dongxiao, xen-devel, Keir (Xen.org), qemu-devel
On Thu, 2012-11-29 at 13:21 +0000, Stefano Stabellini wrote:
> > Also I think 4.2.1 need these patches to enable the basic Xen on Xen
> > nested virtualization usage scenario.
>
> I agree.
Nested virt was a tech preview in 4.2.0, is it really worth
backporting ?
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-11-29 13:57 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2012-11-29 13:57 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Ian Jackson, Xu, Dongxiao, xen-devel, Keir (Xen.org), qemu-devel
On Thu, 2012-11-29 at 13:21 +0000, Stefano Stabellini wrote:
> > Also I think 4.2.1 need these patches to enable the basic Xen on Xen
> > nested virtualization usage scenario.
>
> I agree.
Nested virt was a tech preview in 4.2.0, is it really worth
backporting ?
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-11-29 13:57 ` Ian Campbell
@ 2012-11-29 15:10 ` Pasi Kärkkäinen
-1 siblings, 0 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2012-11-29 15:10 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel, Keir (Xen.org),
Stefano Stabellini, Ian Jackson, qemu-devel, Xu, Dongxiao
On Thu, Nov 29, 2012 at 01:57:11PM +0000, Ian Campbell wrote:
> On Thu, 2012-11-29 at 13:21 +0000, Stefano Stabellini wrote:
> > > Also I think 4.2.1 need these patches to enable the basic Xen on Xen
> > > nested virtualization usage scenario.
> >
> > I agree.
>
> Nested virt was a tech preview in 4.2.0, is it really worth
> backporting ?
>
IIRC AMD Nested Virt works on 4.2.0, so if this backport makes Intel Nested Virt
working aswell it'd be nice..
-- Pasi
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-11-29 15:10 ` Pasi Kärkkäinen
0 siblings, 0 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2012-11-29 15:10 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel, Keir (Xen.org),
Stefano Stabellini, Ian Jackson, qemu-devel, Xu, Dongxiao
On Thu, Nov 29, 2012 at 01:57:11PM +0000, Ian Campbell wrote:
> On Thu, 2012-11-29 at 13:21 +0000, Stefano Stabellini wrote:
> > > Also I think 4.2.1 need these patches to enable the basic Xen on Xen
> > > nested virtualization usage scenario.
> >
> > I agree.
>
> Nested virt was a tech preview in 4.2.0, is it really worth
> backporting ?
>
IIRC AMD Nested Virt works on 4.2.0, so if this backport makes Intel Nested Virt
working aswell it'd be nice..
-- Pasi
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-09-10 18:04 ` Stefano Stabellini
@ 2012-12-07 16:14 ` Ian Jackson
-1 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-07 16:14 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: dongxiao.xu, xen-devel, qemu-devel
Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> after reviewing the patch "fix multiply issue for int and uint types"
> with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> in much need for a simplification as well as removal of a possible
> integer overflow.
>
> This patch series tries to accomplish both switching to two new helper
> functions and using a more obvious arithmetic. Doing so it should also
> fix the original problem that Dongxiao was experiencing. The C language
> can be a nasty backstabber when signed and unsigned integers are
> involved.
I think the attached patch is better as it removes some formulaic
code. I don't think I have a guest which can repro the bug so I have
only compile tested it.
Dongxiao, would you care to take a look ?
PS: I'm pretty sure the original overflows aren't security problems.
Thanks,
Ian.
commit d19731e4e452e3415a5c03771d0406efc803baa9
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>
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..9b8552c 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((target_phys_addr_t) 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);
}
}
}
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-07 16:14 ` Ian Jackson
0 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-07 16:14 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: dongxiao.xu, xen-devel, qemu-devel
Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> after reviewing the patch "fix multiply issue for int and uint types"
> with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> in much need for a simplification as well as removal of a possible
> integer overflow.
>
> This patch series tries to accomplish both switching to two new helper
> functions and using a more obvious arithmetic. Doing so it should also
> fix the original problem that Dongxiao was experiencing. The C language
> can be a nasty backstabber when signed and unsigned integers are
> involved.
I think the attached patch is better as it removes some formulaic
code. I don't think I have a guest which can repro the bug so I have
only compile tested it.
Dongxiao, would you care to take a look ?
PS: I'm pretty sure the original overflows aren't security problems.
Thanks,
Ian.
commit d19731e4e452e3415a5c03771d0406efc803baa9
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>
diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..9b8552c 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((target_phys_addr_t) 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);
}
}
}
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-07 16:14 ` Ian Jackson
@ 2012-12-07 16:30 ` Ian Campbell
-1 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2012-12-07 16:30 UTC (permalink / raw)
To: Ian Jackson; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
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?
[...]
> + write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);
This seems to be the only one with this cast, why?
write_phys_req_item takes a target_phys_addr_t so this will happen
regardless I think.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-07 16:30 ` Ian Campbell
0 siblings, 0 replies; 39+ messages in thread
From: Ian Campbell @ 2012-12-07 16:30 UTC (permalink / raw)
To: Ian Jackson; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
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?
[...]
> + write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);
This seems to be the only one with this cast, why?
write_phys_req_item takes a target_phys_addr_t so this will happen
regardless I think.
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-07 16:30 ` Ian Campbell
@ 2012-12-07 16:33 ` Ian Jackson
-1 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-07 16:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
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.
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);
}
}
}
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-07 16:33 ` Ian Jackson
0 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-07 16:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
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.
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);
}
}
}
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-07 16:14 ` Ian Jackson
@ 2012-12-10 16:08 ` Stefano Stabellini
-1 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-12-10 16:08 UTC (permalink / raw)
To: Ian Jackson; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
On Fri, 7 Dec 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> > after reviewing the patch "fix multiply issue for int and uint types"
> > with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> > in much need for a simplification as well as removal of a possible
> > integer overflow.
> >
> > This patch series tries to accomplish both switching to two new helper
> > functions and using a more obvious arithmetic. Doing so it should also
> > fix the original problem that Dongxiao was experiencing. The C language
> > can be a nasty backstabber when signed and unsigned integers are
> > involved.
>
> I think the attached patch is better as it removes some formulaic
> code. I don't think I have a guest which can repro the bug so I have
> only compile tested it.
>
> Dongxiao, would you care to take a look ?
>
> PS: I'm pretty sure the original overflows aren't security problems.
>
> Thanks,
> Ian.
>
> commit d19731e4e452e3415a5c03771d0406efc803baa9
> 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>
>
> diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> index c6d049c..9b8552c 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;
This can't be right, can it?
The search/replace changes below look correct.
For the sake of consistency, could you please send a patch against
upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
(cpu_ioreq_pio and cpu_ioreq_move).
> + 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((target_phys_addr_t) 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);
> }
> }
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-10 16:08 ` Stefano Stabellini
0 siblings, 0 replies; 39+ messages in thread
From: Stefano Stabellini @ 2012-12-10 16:08 UTC (permalink / raw)
To: Ian Jackson; +Cc: dongxiao.xu, xen-devel, qemu-devel, Stefano Stabellini
On Fri, 7 Dec 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> > after reviewing the patch "fix multiply issue for int and uint types"
> > with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> > in much need for a simplification as well as removal of a possible
> > integer overflow.
> >
> > This patch series tries to accomplish both switching to two new helper
> > functions and using a more obvious arithmetic. Doing so it should also
> > fix the original problem that Dongxiao was experiencing. The C language
> > can be a nasty backstabber when signed and unsigned integers are
> > involved.
>
> I think the attached patch is better as it removes some formulaic
> code. I don't think I have a guest which can repro the bug so I have
> only compile tested it.
>
> Dongxiao, would you care to take a look ?
>
> PS: I'm pretty sure the original overflows aren't security problems.
>
> Thanks,
> Ian.
>
> commit d19731e4e452e3415a5c03771d0406efc803baa9
> 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>
>
> diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> index c6d049c..9b8552c 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;
This can't be right, can it?
The search/replace changes below look correct.
For the sake of consistency, could you please send a patch against
upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
(cpu_ioreq_pio and cpu_ioreq_move).
> + 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((target_phys_addr_t) 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);
> }
> }
> }
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-10 16:08 ` Stefano Stabellini
@ 2012-12-10 17:01 ` Ian Jackson
-1 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-10 17:01 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: dongxiao.xu, xen-devel, qemu-devel
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> On Fri, 7 Dec 2012, Ian Jackson wrote:
...
> > + if (req->df) addr -= offset;
> > + else addr -= offset;
>
> This can't be right, can it?
Indeed not. v2 has this fixed.
> The search/replace changes below look correct.
Thanks.
> For the sake of consistency, could you please send a patch against
> upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
> (cpu_ioreq_pio and cpu_ioreq_move).
I will do that.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-10 17:01 ` Ian Jackson
0 siblings, 0 replies; 39+ messages in thread
From: Ian Jackson @ 2012-12-10 17:01 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: dongxiao.xu, xen-devel, qemu-devel
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> On Fri, 7 Dec 2012, Ian Jackson wrote:
...
> > + if (req->df) addr -= offset;
> > + else addr -= offset;
>
> This can't be right, can it?
Indeed not. v2 has this fixed.
> The search/replace changes below look correct.
Thanks.
> For the sake of consistency, could you please send a patch against
> upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
> (cpu_ioreq_pio and cpu_ioreq_move).
I will do that.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-07 16:33 ` Ian Jackson
@ 2012-12-11 5:50 ` Xu, Dongxiao
-1 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-12-11 5:50 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel, qemu-devel, Stefano Stabellini
> -----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);
> }
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2012-12-11 5:50 ` Xu, Dongxiao
0 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2012-12-11 5:50 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel, qemu-devel, Stefano Stabellini
> -----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);
> }
> }
> }
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2012-12-07 16:33 ` Ian Jackson
@ 2013-01-08 1:49 ` Xu, Dongxiao
-1 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2013-01-08 1:49 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel, qemu-devel, Stefano Stabellini
> -----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);
> > }
> > }
> > }
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2013-01-08 1:49 ` Xu, Dongxiao
0 siblings, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2013-01-08 1:49 UTC (permalink / raw)
To: Ian Jackson, Ian Campbell; +Cc: xen-devel, qemu-devel, Stefano Stabellini
> -----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);
> > }
> > }
> > }
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
2013-01-08 1:49 ` Xu, Dongxiao
@ 2013-01-09 7:09 ` Pasi Kärkkäinen
-1 siblings, 0 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2013-01-09 7:09 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini, qemu-devel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
@ 2013-01-09 7:09 ` Pasi Kärkkäinen
0 siblings, 0 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2013-01-09 7:09 UTC (permalink / raw)
To: Xu, Dongxiao
Cc: xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini, qemu-devel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt
2013-01-09 7:09 ` Pasi Kärkkäinen
(?)
@ 2013-01-24 13:44 ` Pasi Kärkkäinen
2013-01-31 2:23 ` Xu, Dongxiao
2013-02-19 19:38 ` Pasi Kärkkäinen
-1 siblings, 2 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2013-01-24 13:44 UTC (permalink / raw)
To: Ian Jackson
Cc: qemu-devel, Xu, Dongxiao, xen-devel, Ian Campbell, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt
2013-01-24 13:44 ` [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt Pasi Kärkkäinen
@ 2013-01-31 2:23 ` Xu, Dongxiao
2013-02-19 19:38 ` Pasi Kärkkäinen
1 sibling, 0 replies; 39+ messages in thread
From: Xu, Dongxiao @ 2013-01-31 2:23 UTC (permalink / raw)
To: Pasi K?rkk?inen, Ian Jackson
Cc: qemu-devel, xen-devel, Ian Campbell, Stefano Stabellini
> -----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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt
2013-01-24 13:44 ` [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt Pasi Kärkkäinen
2013-01-31 2:23 ` Xu, Dongxiao
@ 2013-02-19 19:38 ` Pasi Kärkkäinen
2013-02-20 15:42 ` Ian Jackson
1 sibling, 1 reply; 39+ messages in thread
From: Pasi Kärkkäinen @ 2013-02-19 19:38 UTC (permalink / raw)
To: Ian Jackson
Cc: Xu, Dongxiao, xen-devel, qemu-devel, Stefano Stabellini, Ian Campbell
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt
2013-02-19 19:38 ` Pasi Kärkkäinen
@ 2013-02-20 15:42 ` Ian Jackson
2013-02-20 15:53 ` Pasi Kärkkäinen
0 siblings, 1 reply; 39+ messages in thread
From: Ian Jackson @ 2013-02-20 15:42 UTC (permalink / raw)
To: Pasi Kärkkäinen
Cc: Xu, Dongxiao, xen-devel, qemu-devel, Stefano Stabellini, Ian Campbell
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.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt
2013-02-20 15:42 ` Ian Jackson
@ 2013-02-20 15:53 ` Pasi Kärkkäinen
0 siblings, 0 replies; 39+ messages in thread
From: Pasi Kärkkäinen @ 2013-02-20 15:53 UTC (permalink / raw)
To: Ian Jackson
Cc: Xu, Dongxiao, xen-devel, qemu-devel, Stefano Stabellini, Ian Campbell
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
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2013-02-20 15:53 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-10 18:04 [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move Stefano Stabellini
2012-09-10 18:04 ` Stefano Stabellini
2012-09-10 18:06 ` [Qemu-devel] [PATCH 1/2] i should be uint32_t rather than int Stefano Stabellini
2012-09-10 18:06 ` Stefano Stabellini
2012-09-10 18:06 ` [Qemu-devel] [PATCH 2/2] introduce read_physical_offset and write_physical_offset Stefano Stabellini
2012-09-10 18:06 ` Stefano Stabellini
2012-09-17 23:18 ` [Qemu-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move Xu, Dongxiao
2012-09-17 23:18 ` Xu, Dongxiao
2012-09-18 10:24 ` [Qemu-devel] " Stefano Stabellini
2012-09-18 10:24 ` Stefano Stabellini
2012-11-29 3:22 ` [Qemu-devel] " Xu, Dongxiao
2012-11-29 3:22 ` Xu, Dongxiao
2012-11-29 13:21 ` [Qemu-devel] " Stefano Stabellini
2012-11-29 13:21 ` Stefano Stabellini
2012-11-29 13:57 ` [Qemu-devel] [Xen-devel] " Ian Campbell
2012-11-29 13:57 ` Ian Campbell
2012-11-29 15:10 ` [Qemu-devel] " Pasi Kärkkäinen
2012-11-29 15:10 ` Pasi Kärkkäinen
2012-12-07 16:14 ` [Qemu-devel] [Xen-devel] " Ian Jackson
2012-12-07 16:14 ` Ian Jackson
2012-12-07 16:30 ` [Qemu-devel] " Ian Campbell
2012-12-07 16:30 ` Ian Campbell
2012-12-07 16:33 ` [Qemu-devel] [Xen-devel] " Ian Jackson
2012-12-07 16:33 ` Ian Jackson
2012-12-11 5:50 ` [Qemu-devel] " Xu, Dongxiao
2012-12-11 5:50 ` Xu, Dongxiao
2013-01-08 1:49 ` [Qemu-devel] " Xu, Dongxiao
2013-01-08 1:49 ` Xu, Dongxiao
2013-01-09 7:09 ` [Qemu-devel] " Pasi Kärkkäinen
2013-01-09 7:09 ` Pasi Kärkkäinen
2013-01-24 13:44 ` [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move / Xen on Xen nested virt Pasi Kärkkäinen
2013-01-31 2:23 ` Xu, Dongxiao
2013-02-19 19:38 ` Pasi Kärkkäinen
2013-02-20 15:42 ` Ian Jackson
2013-02-20 15:53 ` Pasi Kärkkäinen
2012-12-10 16:08 ` [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move Stefano Stabellini
2012-12-10 16:08 ` Stefano Stabellini
2012-12-10 17:01 ` [Qemu-devel] " Ian Jackson
2012-12-10 17:01 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.