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