Re: [Qemu-devel] [RFC PATCH 1/3] Factor out {linux, bsd}-user/qemu.h

[Peter Maydell]

On 25 January 2017 at 07:38, Alessandro Di Federico
 wrote:
> On Tue, 24 Jan 2017 11:18:07 +
> Peter Maydell  wrote:
>> If we had more active testing of bsd-user I'd be a bit keener
>> about removing some of the code duplication.
>
> Currently, the code looks exactly the same to me. If we don't factor it
> out I'll have to make a third copy of it for libtcg, which is kinda
> annoying. I'm factoring out only a portion of the header, which
> probably doesn't change that much. If in the future there's some change
> we're not sure about we can keep it in the non-factored out part.

OK, fair enough.

Regarding whether the linux-* and bsd-* put_user are equivalent,
they should be; it's just that bsd-* doesn't get fixes that
we make to linux-* because it's unmaintained code.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/3] Factor out {linux, bsd}-user/qemu.h

[Alessandro Di Federico]

On Tue, 24 Jan 2017 11:18:07 +
Peter Maydell  wrote:

> So my issue with this is that currently linux-user is pretty
> well maintained and tested, but bsd-user is basically unmaintained
> and not even compile tested. 

I can say it compiles. I've tried to build it on FreeBSD 11. Didn't
tested it though.

> So it's convenient to have bsd-user code be basically its own
> distinct standalone thing, because then when we fix linux-user we
> don't have to care too much about whether bsd-user might be
> accidentally broken by our changes.
> 
> If we had more active testing of bsd-user I'd be a bit keener
> about removing some of the code duplication.

Currently, the code looks exactly the same to me. If we don't factor it
out I'll have to make a third copy of it for libtcg, which is kinda
annoying. I'm factoring out only a portion of the header, which
probably doesn't change that much. If in the future there's some change
we're not sure about we can keep it in the non-factored out part.

I'll check if qemu-user for BSD has something in the testsuite.

--
Alessandro Di Federico
PhD student at Politecnico di Milano

Re: [Qemu-devel] [RFC PATCH 1/3] Factor out {linux, bsd}-user/qemu.h

[Alessandro Di Federico]

On Tue, 24 Jan 2017 10:10:08 +
Marc-André Lureau  wrote:

> 
> Looks good. Probably worth to mention that the main difference is in
> commit 658f2dc970996d547a641b5685e384ebe6f2648e not being applied to
> bsd-user.
> 

Yeah, I'll rebase my next patches on master.

My only doubt about this patch was whether the linux-* and bsd-*
implementations of put_user are equivalent. The linux one seems more
optimized, but they look equivalent to me.

--
Alessandro Di Federico
PhD student at Politecnico di Milano

Re: [Qemu-devel] [RFC PATCH 1/3] Factor out {linux, bsd}-user/qemu.h

[Peter Maydell]

On 21 January 2017 at 08:45, Alessandro Di Federico
 wrote:
> A quite large part of {linux,bsd}-user/qemu.h is shared, this patch
> introduces qemu-user-common.h which factors it out. This shared part is
> also the bare minimum required to build a linux-user-like target, and,
> in particular, it will be useful for the libtcg targets.
> ---
>  bsd-user/qemu.h| 193 
> +
>  linux-user/qemu.h  | 176 +---
>  qemu-user-common.h | 180 +
>  3 files changed, 182 insertions(+), 367 deletions(-)
>  create mode 100644 qemu-user-common.h

So my issue with this is that currently linux-user is pretty
well maintained and tested, but bsd-user is basically unmaintained
and not even compile tested. So it's convenient to have bsd-user
code be basically its own distinct standalone thing, because
then when we fix linux-user we don't have to care too much
about whether bsd-user might be accidentally broken by our
changes.

If we had more active testing of bsd-user I'd be a bit keener
about removing some of the code duplication.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/3] Factor out {linux, bsd}-user/qemu.h

[Marc-André Lureau]

Hi

On Sat, Jan 21, 2017 at 12:46 PM Alessandro Di Federico <
ale+q...@clearmind.me> wrote:

A quite large part of {linux,bsd}-user/qemu.h is shared, this patch
introduces qemu-user-common.h which factors it out. This shared part is
also the bare minimum required to build a linux-user-like target, and,
in particular, it will be useful for the libtcg targets.


Looks good. Probably worth to mention that the main difference is in commit
658f2dc970996d547a641b5685e384ebe6f2648e not being applied to bsd-user.

---
 bsd-user/qemu.h| 193
+
 linux-user/qemu.h  | 176 +---
 qemu-user-common.h | 180 +
 3 files changed, 182 insertions(+), 367 deletions(-)
 create mode 100644 qemu-user-common.h

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 2b2b9184e0..b51319caf0 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -17,10 +17,8 @@
 #ifndef QEMU_H
 #define QEMU_H

-
-#include "cpu.h"
+#include "qemu-user-common.h"
 #include "exec/exec-all.h"
-#include "exec/cpu_ldst.h"

 #undef DEBUG_REMAP
 #ifdef DEBUG_REMAP
@@ -217,195 +215,6 @@ void mmap_fork_end(int child);
 /* main.c */
 extern unsigned long x86_stack_size;

-/* user access */
-
-#define VERIFY_READ 0
-#define VERIFY_WRITE 1 /* implies read access */
-
-static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
-{
-return page_check_range((target_ulong)addr, size,
-(type == VERIFY_READ) ? PAGE_READ : (PAGE_READ
| PAGE_WRITE)) == 0;
-}
-
-/* NOTE __get_user and __put_user use host pointers and don't check
access. */
-/* These are usually used to access struct data members once the
- * struct has been locked - usually with lock_user_struct().
- */
-#define __put_user(x, hptr)\
-({\
-int size = sizeof(*hptr);\
-switch(size) {\
-case 1:\
-*(uint8_t *)(hptr) = (uint8_t)(typeof(*hptr))(x);\
-break;\
-case 2:\
-*(uint16_t *)(hptr) = tswap16((typeof(*hptr))(x));\
-break;\
-case 4:\
-*(uint32_t *)(hptr) = tswap32((typeof(*hptr))(x));\
-break;\
-case 8:\
-*(uint64_t *)(hptr) = tswap64((typeof(*hptr))(x));\
-break;\
-default:\
-abort();\
-}\
-0;\
-})
-
-#define __get_user(x, hptr) \
-({\
-int size = sizeof(*hptr);\
-switch(size) {\
-case 1:\
-x = (typeof(*hptr))*(uint8_t *)(hptr);\
-break;\
-case 2:\
-x = (typeof(*hptr))tswap16(*(uint16_t *)(hptr));\
-break;\
-case 4:\
-x = (typeof(*hptr))tswap32(*(uint32_t *)(hptr));\
-break;\
-case 8:\
-x = (typeof(*hptr))tswap64(*(uint64_t *)(hptr));\
-break;\
-default:\
-/* avoid warning */\
-x = 0;\
-abort();\
-}\
-0;\
-})
-
-/* put_user()/get_user() take a guest address and check access */
-/* These are usually used to access an atomic data type, such as an int,
- * that has been passed by address.  These internally perform locking
- * and unlocking on the data type.
- */
-#define put_user(x, gaddr, target_type) \
-({  \
-abi_ulong __gaddr = (gaddr);\
-target_type *__hptr;\
-abi_long __ret; \
-if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type),
0))) { \
-__ret = __put_user((x), __hptr);\
-unlock_user(__hptr, __gaddr, sizeof(target_type));  \
-} else  \
-__ret = -TARGET_EFAULT; \
-__ret;  \
-})
-
-#define get_user(x, gaddr, target_type) \
-({  \
-abi_ulong __gaddr = (gaddr);\
-target_type *__hptr;\
-abi_long __ret; \
-if ((__hptr = lock_user(VERIFY_READ, __gaddr, sizeof(target_type),
1))) { \
-__ret = __get_user((x), __hptr);\
-unlock_user(__hptr, __gaddr, 0);\
-} else {\
-/* avoid warning */ \
-(x) = 0;\
-__ret = -TARGET_EFAULT; \
-}   \
-__ret;