[Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
historically the kernel queues packets two times. once
at the device and second in qdisc. this is believed to cause
interface stalls if one of these queues overruns.
setting IFF_ONE_QUEUE is the default in kernels = 3.8. the
flag is ignored since then. see kernel commit
5d097109257c03a71845729f8db6b5770c4bbedc
Signed-off-by: Peter Lieven p...@kamp.de
---
net/tap-linux.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index a953189..2759b78 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -49,7 +49,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
return -1;
}
memset(ifr, 0, sizeof(ifr));
-ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
+ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE;
if (*vnet_hdr) {
unsigned int features;
--
1.7.9.5
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
Il 15/02/2013 03:49, Antoine Mathys ha scritto: First, the ds1338 code was in a poor state and never handled the 12 hour clock correctly. My first patch failed to fully fix the problem so I had to write a second one, but at no point did Peter or I introduce a regression, quite the opposite. Second, I don't know where you got the idea that I refuse to write test cases. I just didn't have one ready or in the works at the time. Third, bug 1090558 in mc146818rtc is a good example of a bug which was not due to insufficient testing, but to poorly structured code. There is no point worrying about unit testing if you accept code of such low quality. This goes for the tests too. For instance cmos_get_date_time() in tests/rtc-test.c doesn't work correctly in 12 hour mode. Fourth, I am not interested in the PC architecture, I only wrote a fix for bug 1090558 because Paolo asked me to. It is nice to see that fixing your crappy code makes me not a nice guy who is making things worse. But don't worry, I'll focus on ARM from now on. Hey hey, no reason to get excited. Yes, some code is of pretty low quality. We're getting better, the main problem is that without a testing infrastructure there's only so much you can do for code quality. Hence Andreas's request. Thanks for your PC patch. Nobody said you're making things worse. Paolo
Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output
On Thu, Feb 14, 2013 at 04:40:29PM +0100, Andreas Färber wrote:
CC'ing some more people from the debug output revamp RFC discussion.
Am 11.02.2013 20:01, schrieb Andreas Färber:
From: Andreas Färber andreas.faer...@web.de
Signed-off-by: Andreas Färber andreas.faer...@web.de
---
hw/tmp105.c | 27 +--
1 Datei geändert, 25 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-)
diff --git a/hw/tmp105.c b/hw/tmp105.c
index 3ad2d2f..5dafa37 100644
--- a/hw/tmp105.c
+++ b/hw/tmp105.c
@@ -23,6 +23,18 @@
#include tmp105.h
#include qapi/visitor.h
+#undef DEBUG_TMP105
+
+static inline void GCC_FMT_ATTR(1, 2) DPRINTF(const char *fmt, ...)
Being an inline function, I think it would be better to name this
tmp105_dprintf() and alias via: #define DPRINTF tmp105_dprintf
Assuming we want an uppercase conditional-output statement in the first
place?
dprintf() as used in some files (sheepdog, sPAPR, SPICE,
target-{i386,ppc,s390x}/kvm.c) is already used by system headers on some
platforms, so I'd rather avoid it.
Any other comments or suggestions? I'm preparing to respin the above
series in a similar, less-invasive style.
Here is a radical departure from the zoo of DPRINTF() approaches that
QEMU has today. I know not everyone is comfortable using tracing, even
though you can use --enable-trace-backend=stderr to get good old stderr
output.
In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204
Basically you do DBG(hello world\n) and it gets compiled out by
default using:
if (DBG_LOG) {
printf(hello world\n);
}
Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:
make DEBUG=e1000 # build QEMU with debug printfs only in e1000.o
The iPXE implementation is fancier than this. It supports different log
levels, hex dumping, MD5 hashing, etc.
But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
support that lets you say make DEBUG=e1000,rtl8139,net.
It would be a big step up from the mess we have today.
Personally, I think we should use tracing instead of debug printfs
though. Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like. You can mark debug trace events disabled in
./trace-events so they will not be compiled in by default - zero
performance overhead.
Stefan
Re: [Qemu-devel] [RFC PATCH 02/10] Fix errors and warnings while compiling with c++ compilier
Il 15/02/2013 04:56, Tomoki Sekiyama ha scritto: Now, is using C++ required? Why can't you use plain C? It is because Windows COM+ framework (which VSS uses) is designed based on C++ objective programming interface. Implementing this with plain C is theoretically possible, but that will require parsing C++ objects' vtables manually so the code would be much complex. (However, It might be possible to push Windows-specific C++ stuff into a DLL to and avoid involving qemu related headers.) I don't think this is necessary. Use C++ where you see fit, as long as it's a separate file it should not be a problem. The only problem could be that we need to use the C++ compiler to link, instead of the C compiler. Paolo
Re: [Qemu-devel] [PATCH for-1.4] e1000: unbreak the guest network migration to 1.3
On Thu, Feb 14, 2013 at 07:15:03PM +0200, Michael S. Tsirkin wrote: QEMU 1.3 does not emulate the link auto negotiation, so if migrate to a 1.3 machine during link auto negotiation, the guest link will be set to down. Fix this by just disabling auto negotiation for 1.3. Signed-off-by: Michael S. Tsirkin m...@redhat.com --- hw/e1000.c | 25 + hw/pc_piix.c | 4 2 files changed, 29 insertions(+) Besides the naming issues that have been pointed out: Reviewed-by: Stefan Hajnoczi stefa...@redhat.com
Re: [Qemu-devel] [RFC PATCH v2 03/23] qcow2: Change handle_dependency to byte granularity
On Thu, Feb 14, 2013 at 04:45:06PM +0100, Kevin Wolf wrote:
On Thu, Feb 14, 2013 at 03:48:51PM +0100, Stefan Hajnoczi wrote:
On Wed, Feb 13, 2013 at 02:21:53PM +0100, Kevin Wolf wrote:
+ *
+ * -EAGAIN if we had to wait for another request, previously gathered
+ * information on cluster allocation may be invalid now. The
caller
+ * must start over anyway, so consider *cur_bytes undefined.
*/
static int handle_dependencies(BlockDriverState *bs, uint64_t
guest_offset,
-unsigned int *nb_clusters)
+uint64_t *cur_bytes)
{
BDRVQcowState *s = bs-opaque;
QCowL2Meta *old_alloc;
+uint64_t bytes = *cur_bytes;
QLIST_FOREACH(old_alloc, s-cluster_allocs, next_in_flight) {
-uint64_t start = guest_offset s-cluster_bits;
-uint64_t end = start + *nb_clusters;
-uint64_t old_start = old_alloc-offset s-cluster_bits;
-uint64_t old_end = old_start + old_alloc-nb_clusters;
+uint64_t start = guest_offset;
I'm not sure this is safe. Previously the function caught write
requests which overlap at cluster granularity, now it will allow them?
At this point in the series, old_start and old_end are still aligned to
cluster boundaries. So the cases in which an overlap is detected stay exactly
the same with this patch. This is just a more precise description of what we
really wanted to compare.
Good point. In an overlap comparison between A and B only one of them
needs to be cluster-aligned. I've used this trick myself in the past
but missed it in this code review.
Stefan
Re: [Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
On Fri, Feb 15, 2013 at 09:27:18AM +0100, Peter Lieven wrote: historically the kernel queues packets two times. once at the device and second in qdisc. this is believed to cause interface stalls if one of these queues overruns. setting IFF_ONE_QUEUE is the default in kernels = 3.8. the flag is ignored since then. see kernel commit 5d097109257c03a71845729f8db6b5770c4bbedc Signed-off-by: Peter Lieven p...@kamp.de --- net/tap-linux.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi stefa...@redhat.com
Re: [Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
Am 15.02.2013 um 10:16 schrieb Stefan Hajnoczi stefa...@gmail.com: On Fri, Feb 15, 2013 at 09:27:18AM +0100, Peter Lieven wrote: historically the kernel queues packets two times. once at the device and second in qdisc. this is believed to cause interface stalls if one of these queues overruns. setting IFF_ONE_QUEUE is the default in kernels = 3.8. the flag is ignored since then. see kernel commit 5d097109257c03a71845729f8db6b5770c4bbedc Signed-off-by: Peter Lieven p...@kamp.de --- net/tap-linux.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi stefa...@redhat.com it seems that IFF_ONE_QUEUE is not defined in qemu headers. i will sent an update. sorry, peter
Re: [Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
On 15/02/13 09:27, Peter Lieven wrote:
historically the kernel queues packets two times. once
at the device and second in qdisc. this is believed to cause
interface stalls if one of these queues overruns.
setting IFF_ONE_QUEUE is the default in kernels = 3.8. the
flag is ignored since then. see kernel commit
5d097109257c03a71845729f8db6b5770c4bbedc
Signed-off-by: Peter Lieven p...@kamp.de
---
net/tap-linux.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index a953189..2759b78 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -49,7 +49,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
return -1;
}
memset(ifr, 0, sizeof(ifr));
-ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
+ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE;
if (*vnet_hdr) {
unsigned int features;
Wouldn't that break macvtap?
there is
case TUNSETIFF:
/* ignore the name, just look at flags */
if (get_user(u, ifr-ifr_flags))
return -EFAULT;
ret = 0;
if ((u ~IFF_VNET_HDR) != (IFF_NO_PI | IFF_TAP))
ret = -EINVAL;
in drivers/net/macvtap.c
Re: [Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
On 15/02/13 10:25, Peter Lieven wrote:
Am 15.02.2013 um 10:22 schrieb Christian Borntraeger borntrae...@de.ibm.com:
On 15/02/13 09:27, Peter Lieven wrote:
historically the kernel queues packets two times. once
at the device and second in qdisc. this is believed to cause
interface stalls if one of these queues overruns.
setting IFF_ONE_QUEUE is the default in kernels = 3.8. the
flag is ignored since then. see kernel commit
5d097109257c03a71845729f8db6b5770c4bbedc
Signed-off-by: Peter Lieven p...@kamp.de
---
net/tap-linux.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index a953189..2759b78 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -49,7 +49,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
return -1;
}
memset(ifr, 0, sizeof(ifr));
-ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
+ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE;
if (*vnet_hdr) {
unsigned int features;
Wouldn't that break macvtap?
there is
case TUNSETIFF:
/* ignore the name, just look at flags */
if (get_user(u, ifr-ifr_flags))
return -EFAULT;
ret = 0;
if ((u ~IFF_VNET_HDR) != (IFF_NO_PI | IFF_TAP))
ret = -EINVAL;
in drivers/net/macvtap.c
wasn`t aware I will modify this to only be set on linux
Peter
macvtap is a Linux driver. It is given to qemu usually via libvirt with
something
like -netdev tap,fd=21,id=hostnet0,vhost=on,vhostfd=22
and then having fd21 pointing to an open /dev/tap... device which is backed by
macvtap.
Christian
Re: [Qemu-devel] [ANNOUNCE] QEMU 1.4.0-rc2 is now available
On Thu, Feb 14, 2013 at 02:57:34PM -0600, Anthony Liguori wrote: Hi, On behalf of the QEMU Team, I'd like to announce the availability of the third release candidate for the QEMU 1.4 release. This release is meant for testing purposes and should not be used in a production environment. http://wiki.qemu.org/download/qemu-1.4.0-rc2.tar.bz2 You can help improve the quality of the QEMU 1.4 release by testing this release and reporting bugs on Launchpad: https://bugs.launchpad.net/qemu/ Andreas Faerber set up a Testing wiki page: http://wiki.qemu.org/Planning/1.4/Testing It can be used to track what has been tested successfully. This allows Anthony and the rest of the QEMU community to see which features, targets, and host platforms have received testing. Happy Testing! /me is off to run block tests on 1.4-rc2. Stefan
[Qemu-devel] [PATCHv2] tap: set IFF_ONE_QUEUE per default
historically the kernel queues packets two times. once
at the device and second in qdisc. this is believed to cause
interface stalls if one of these queues overruns.
setting IFF_ONE_QUEUE is the default in kernels = 3.8. the
flag is ignored since then. see kernel commit
5d097109257c03a71845729f8db6b5770c4bbedc
v2:
- do only set the flag on linux as it breaks macvtap
- define IFF_ONE_QUEUE in tap-linux.h
Signed-off-by: Peter Lieven p...@kamp.de
---
net/tap-linux.c |4
net/tap-linux.h |1 +
2 files changed, 5 insertions(+)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index a953189..d49f2fd 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -51,6 +51,10 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
memset(ifr, 0, sizeof(ifr));
ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
+#ifdef __linux__
+ifr.ifr_flags |= IFF_ONE_QUEUE;
+#endif
+
if (*vnet_hdr) {
unsigned int features;
diff --git a/net/tap-linux.h b/net/tap-linux.h
index 65087e1..13002fd 100644
--- a/net/tap-linux.h
+++ b/net/tap-linux.h
@@ -36,6 +36,7 @@
/* TUNSETIFF ifr flags */
#define IFF_TAP0x0002
#define IFF_NO_PI 0x1000
+#define IFF_ONE_QUEUE 0x2000
#define IFF_VNET_HDR 0x4000
#define IFF_MULTI_QUEUE 0x0100
#define IFF_ATTACH_QUEUE 0x0200
--
1.7.9.5
Re: [Qemu-devel] [PATCH] tap: set IFF_ONE_QUEUE per default
Am 15.02.2013 um 10:22 schrieb Christian Borntraeger borntrae...@de.ibm.com:
On 15/02/13 09:27, Peter Lieven wrote:
historically the kernel queues packets two times. once
at the device and second in qdisc. this is believed to cause
interface stalls if one of these queues overruns.
setting IFF_ONE_QUEUE is the default in kernels = 3.8. the
flag is ignored since then. see kernel commit
5d097109257c03a71845729f8db6b5770c4bbedc
Signed-off-by: Peter Lieven p...@kamp.de
---
net/tap-linux.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tap-linux.c b/net/tap-linux.c
index a953189..2759b78 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -49,7 +49,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
return -1;
}
memset(ifr, 0, sizeof(ifr));
-ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
+ifr.ifr_flags = IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE;
if (*vnet_hdr) {
unsigned int features;
Wouldn't that break macvtap?
there is
case TUNSETIFF:
/* ignore the name, just look at flags */
if (get_user(u, ifr-ifr_flags))
return -EFAULT;
ret = 0;
if ((u ~IFF_VNET_HDR) != (IFF_NO_PI | IFF_TAP))
ret = -EINVAL;
in drivers/net/macvtap.c
wasn`t aware I will modify this to only be set on linux
Peter
Re: [Qemu-devel] [RFC PATCH v2 13/23] qcow2: handle_copied(): Implement non-zero host_offset
On Thu, Feb 14, 2013 at 09:40:22PM +, Blue Swirl wrote:
On Thu, Feb 14, 2013 at 9:40 AM, Kevin Wolf kw...@redhat.com wrote:
Am 13.02.2013 22:17, schrieb Blue Swirl:
On Wed, Feb 13, 2013 at 1:22 PM, Kevin Wolf kw...@redhat.com wrote:
Look only for clusters that start at a given physical offset.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
block/qcow2-cluster.c | 26 ++
1 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5ce2c88..90fe36c 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -827,8 +827,6 @@ static int handle_dependencies(BlockDriverState *bs,
uint64_t guest_offset,
* the length of the area that can be written to.
*
* -errno: in error cases
- *
- * TODO Make non-zero host_offset behave like describe above
*/
static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
uint64_t *host_offset, uint64_t *bytes, QCowL2Meta **m)
@@ -843,7 +841,6 @@ static int handle_copied(BlockDriverState *bs,
uint64_t guest_offset,
trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset,
*host_offset,
*bytes);
-assert(*host_offset == 0);
/*
* Calculate the number of clusters to look for. We stop at L2 table
@@ -867,6 +864,15 @@ static int handle_copied(BlockDriverState *bs,
uint64_t guest_offset,
if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
(cluster_offset QCOW_OFLAG_COPIED))
{
+/* If a specific host_offset is required, check it */
+if (*host_offset != 0
+ (cluster_offset L2E_OFFSET_MASK) != *host_offset)
+{
Braces should cuddle with the previous line.
Can we get rid of this rule for multiline ifs? Having the
second/third/... line of the condition and the first line of the then
branch with no clear separation severely hurts readability in my opinion.
Perhaps the problem is that the condition is long, it could be
rewritten in this style:
bool has_host_offset = *host_offset != 0;
bool offset_matches = (cluster_offset L2E_OFFSET_MASK) != *host_offset;
if (has_host_offset offset_matches) {
I consider the usefulness of this about the same as adding code in order to
silence gcc warnings. Just that a complaining gcc breaks the build whereas a
complaining Blue Swirl doesn't. But I'll do it here.
Kevin
Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output
Am 15.02.2013 10:06, schrieb Stefan Hajnoczi:
In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204
Basically you do DBG(hello world\n) and it gets compiled out by
default using:
if (DBG_LOG) {
printf(hello world\n);
}
Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:
make DEBUG=e1000 # build QEMU with debug printfs only in e1000.o
The iPXE implementation is fancier than this. It supports different log
levels, hex dumping, MD5 hashing, etc.
But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
support that lets you say make DEBUG=e1000,rtl8139,net.
It would be a big step up from the mess we have today.
Thanks for that feedback. If you look at the previous discussion, that's
part of what I did in my RFC, and it was rejected in favor of keeping
#ifdef'able defines. Using inline functions to avoid some nasty
macro-is-not-function issues, there seemed to be no need to combine both
approaches due to the format string being checked one level above.
Personally, I think we should use tracing instead of debug printfs
though. Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like. You can mark debug trace events disabled in
./trace-events so they will not be compiled in by default - zero
performance overhead.
This series or patch is not against tracing. It might be an option to
use them for tmp105. But not being the author of the targets and all of
the devices my CPU refactorings potentially touch, it is infeasable for
me to determine an appropriate set of tracepoints everywhere. We'd also
need to decide whether we want per-target tracepoints or per-aspect
tracepoints, since it's a global list. Independent of that question,
some kind of include mechanism (like for the default-configs) would be
nice to decouple adding tracepoints in different areas.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] RFC: handling backend too fast in virtio-net
On Thu, Feb 14, 2013 at 07:21:57PM +0100, Luigi Rizzo wrote:
CCed Michael Tsirkin
virtio-style network devices (where the producer and consumer chase
each other through a shared memory block) can enter into a
bad operating regime when the consumer is too fast.
I am hitting this case right now when virtio is used on top of the
netmap/VALE backend that I posted a few weeks ago: what happens is that
the backend is so fast that the io thread keeps re-enabling notifications
every few packets. This results, on my test machine, in a throughput of
250-300Kpps (and extremely unstable, oscillating between 200 and 600Kpps).
I'd like to get some feedback on the following trivial patch to have
the thread keep spinning for a bounded amount of time when the producer
is slower than the consumer. This gives a relatively stable throughput
between 700 and 800 Kpps (we have something similar in our paravirtualized
e1000 driver, which is slightly faster at 900-1100 Kpps).
Did you experiment with tx timer instead of bh? It seems that
hw/virtio-net.c has two tx mitigation strategies - the bh approach that
you've tweaked and a true timer.
It seems you don't really want tx batching but you do want to avoid
guest-host notifications?
EXISTING LOGIC: reschedule the bh when at least a burst of packets has
been received. Otherwise enable notifications and do another
race-prevention check.
NEW LOGIC: when the bh is scheduled the first time, establish a
budget (currently 20) of reschedule attempts.
Every time virtio_net_flush_tx() returns 0 packets [this could
be changed to 'less than a full burst'] the counter is increased.
The bh is rescheduled until the counter reaches the budget,
at which point we re-enable notifications as above.
I am not 100% happy with this patch because there is a magic
constant (the maximum number of retries) which should be really
adaptive, or perhaps we should even bound the amount of time the
bh runs, rather than the number of retries.
In practice, having the thread spin (or sleep) for 10-20us
even without new packets is probably preferable to
re-enable notifications and request a kick.
opinions ?
cheers
luigi
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 573c669..5389088 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -49,6 +51,7 @@ typedef struct VirtIONet
NICState *nic;
uint32_t tx_timeout;
int32_t tx_burst;
+int32_t tx_retries; /* keep spinning a bit on tx */
uint32_t has_vnet_hdr;
size_t host_hdr_len;
size_t guest_hdr_len;
@@ -1062,7 +1065,9 @@ static void virtio_net_tx_bh(void *opaque)
/* If we flush a full burst of packets, assume there are
* more coming and immediately reschedule */
-if (ret = n-tx_burst) {
+if (ret == 0)
+ n-tx_retries++;
+if (n-tx_retries 20) {
qemu_bh_schedule(q-tx_bh);
q-tx_waiting = 1;
return;
@@ -1076,6 +1082,8 @@ static void virtio_net_tx_bh(void *opaque)
virtio_queue_set_notification(q-tx_vq, 0);
qemu_bh_schedule(q-tx_bh);
q-tx_waiting = 1;
+} else {
+ n-tx_retries = 0;
}
}
Re: [Qemu-devel] [SeaBIOS] (PAM stuff) reset doesn't work on OVMF + SeaBIOS CSM
On Fri, 2013-02-15 at 11:19 +0400, Michael Tokarev wrote: This patch is more than 2 years old and is applied to all more or less recent qemu versions. RHEL 6.3? I'm *not* seeing this bug with recent qemu versions. This does not tell us why disabling kvm (with this patch applied!) makes a difference. So there must be another (maybe similar) bug somewhere... Are you looking at the same patch I'm looking at? Before the patch, if KVM is enabled then the i440fx_update_memory_mappings() function just bails out without doing anything. As the commit message describes, it fails to remap the 0xf memory from ROM to RAM, so subsequent writes to the F-segment actually modify the *ROM* content instead of the RAM copy as they should. (KVM doesn't write-protect the ROM). So on reset, it ends up running the *modified* copy of the BIOS. That's an *exact* description of what Laszlo was seeing, surely? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature
[Qemu-devel] [PATCH] pseries: Implements h_read hcall
From: Erlon Cruz erlon.c...@br.flextronics.com
This h_call is useful for DLPAR in future amongst other things. Given an index
it fetches the corresponding PTE stored in the htab.
Signed-off-by: Erlon Cruz erlon.c...@br.flextronics.com
---
hw/spapr_hcall.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 2889742..1065277 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -323,6 +323,39 @@ static target_ulong h_protect(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
return H_SUCCESS;
}
+static target_ulong h_read(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+target_ulong opcode, target_ulong *args)
+{
+CPUPPCState *env = cpu-env;
+target_ulong flags = args[0];
+target_ulong pte_index = args[1];
+target_ulong v[4], r[4];
+uint8_t *hpte;
+int i, ridx, n_entries = 1;
+
+if ((pte_index * HASH_PTE_SIZE_64) ~env-htab_mask) {
+return H_PARAMETER;
+}
+
+if (flags H_READ_4) {
+/* Clear the two low order bits */
+pte_index = ~(3ULL);
+n_entries = 4;
+}
+
+hpte = env-external_htab + (pte_index * HASH_PTE_SIZE_64);
+
+for (i = 0, ridx = 0; i n_entries; i++) {
+v[i] = ldq_p(hpte);
+r[i] = ldq_p(hpte + (HASH_PTE_SIZE_64/2));
+args[ridx++] = v[i];
+args[ridx++] = r[i];
+hpte += HASH_PTE_SIZE_64;
+}
+
+return H_SUCCESS;
+}
+
static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPREnvironment *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -714,6 +747,7 @@ static void hypercall_register_types(void)
spapr_register_hypercall(H_ENTER, h_enter);
spapr_register_hypercall(H_REMOVE, h_remove);
spapr_register_hypercall(H_PROTECT, h_protect);
+spapr_register_hypercall(H_READ, h_read);
/* hcall-bulk */
spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
--
1.7.9.5
[Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
this patch adds iscsi_truncate which effectively allows for online
resizing of iscsi volumes. for this to work you have to resize
the volume on your storage and then call block_resize command
in qemu which will issue a readcapacity16 to update the capacity.
Signed-off-by: Peter Lieven p...@kamp.de
---
block/iscsi.c | 74 +
1 file changed, 74 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index deb3b68..37f12b3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -67,6 +67,12 @@ typedef struct IscsiAIOCB {
#endif
} IscsiAIOCB;
+struct IscsiTask {
+IscsiLun *iscsilun;
+int status;
+int complete;
+};
+
#define NOP_INTERVAL 5000
#define MAX_NOP_FAILURES 3
@@ -700,6 +706,73 @@ iscsi_getlength(BlockDriverState *bs)
return len;
}
+static void
+iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status,
+void *command_data, void *opaque)
+{
+struct IscsiTask *itask = opaque;
+struct scsi_readcapacity16 *rc16;
+struct scsi_task *task = command_data;
+
+if (status != 0) {
+error_report(iSCSI: Failed to read capacity of iSCSI lun. %s,
+ iscsi_get_error(iscsi));
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+rc16 = scsi_datain_unmarshall(task);
+if (rc16 == NULL) {
+error_report(iSCSI: Failed to unmarshall readcapacity16 data.);
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+itask-iscsilun-block_size = rc16-block_length;
+itask-iscsilun-num_blocks = rc16-returned_lba + 1;
+
+itask-status = 0;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+}
+
+static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
+{
+IscsiLun *iscsilun = bs-opaque;
+struct IscsiTask itask;
+struct scsi_task *task = NULL;
+
+if (iscsilun-type != TYPE_DISK) {
+return -ENOTSUP;
+}
+
+itask.iscsilun = iscsilun;
+itask.status = 0;
+itask.complete = 0;
+
+task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun,
+ iscsi_readcapacity16_cb, itask);
+if (task == NULL) {
+ error_report(iSCSI: failed to send readcapacity16 command.);
+ return -EINVAL;
+}
+
+while (!itask.complete) {
+iscsi_set_events(iscsilun);
+qemu_aio_wait();
+}
+
+if (offset iscsi_getlength(bs)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
static int parse_chap(struct iscsi_context *iscsi, const char *target)
{
QemuOptsList *list;
@@ -1093,6 +1166,7 @@ static BlockDriver bdrv_iscsi = {
.create_options = iscsi_create_options,
.bdrv_getlength = iscsi_getlength,
+.bdrv_truncate = iscsi_truncate,
.bdrv_aio_readv = iscsi_aio_readv,
.bdrv_aio_writev = iscsi_aio_writev,
--
1.7.9.5
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
Il 15/02/2013 11:58, Peter Lieven ha scritto:
this patch adds iscsi_truncate which effectively allows for online
resizing of iscsi volumes. for this to work you have to resize
the volume on your storage and then call block_resize command
in qemu which will issue a readcapacity16 to update the capacity.
Signed-off-by: Peter Lieven p...@kamp.de
---
block/iscsi.c | 74
+
1 file changed, 74 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index deb3b68..37f12b3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -67,6 +67,12 @@ typedef struct IscsiAIOCB {
#endif
} IscsiAIOCB;
+struct IscsiTask {
+IscsiLun *iscsilun;
+int status;
+int complete;
+};
+
#define NOP_INTERVAL 5000
#define MAX_NOP_FAILURES 3
@@ -700,6 +706,73 @@ iscsi_getlength(BlockDriverState *bs)
return len;
}
+static void
+iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status,
+void *command_data, void *opaque)
+{
+struct IscsiTask *itask = opaque;
+struct scsi_readcapacity16 *rc16;
+struct scsi_task *task = command_data;
+
+if (status != 0) {
+error_report(iSCSI: Failed to read capacity of iSCSI lun. %s,
+ iscsi_get_error(iscsi));
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+rc16 = scsi_datain_unmarshall(task);
+if (rc16 == NULL) {
+error_report(iSCSI: Failed to unmarshall readcapacity16 data.);
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+itask-iscsilun-block_size = rc16-block_length;
+itask-iscsilun-num_blocks = rc16-returned_lba + 1;
+
+itask-status = 0;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+}
+
+static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
+{
+IscsiLun *iscsilun = bs-opaque;
+struct IscsiTask itask;
+struct scsi_task *task = NULL;
+
+if (iscsilun-type != TYPE_DISK) {
+return -ENOTSUP;
+}
+
+itask.iscsilun = iscsilun;
+itask.status = 0;
+itask.complete = 0;
+
+task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun,
+ iscsi_readcapacity16_cb, itask);
You can use iscsi_readcapacity16_sync. In fact, you probably should
extract code from iscsi_open and reuse it here.
Paolo
+if (task == NULL) {
+ error_report(iSCSI: failed to send readcapacity16 command.);
+ return -EINVAL;
+}
+
+while (!itask.complete) {
+iscsi_set_events(iscsilun);
+qemu_aio_wait();
+}
+
+if (offset iscsi_getlength(bs)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
static int parse_chap(struct iscsi_context *iscsi, const char *target)
{
QemuOptsList *list;
@@ -1093,6 +1166,7 @@ static BlockDriver bdrv_iscsi = {
.create_options = iscsi_create_options,
.bdrv_getlength = iscsi_getlength,
+.bdrv_truncate = iscsi_truncate,
.bdrv_aio_readv = iscsi_aio_readv,
.bdrv_aio_writev = iscsi_aio_writev,
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
On 15.02.2013 12:09, Paolo Bonzini wrote:
Il 15/02/2013 11:58, Peter Lieven ha scritto:
this patch adds iscsi_truncate which effectively allows for online
resizing of iscsi volumes. for this to work you have to resize
the volume on your storage and then call block_resize command
in qemu which will issue a readcapacity16 to update the capacity.
Signed-off-by: Peter Lieven p...@kamp.de
---
block/iscsi.c | 74
+
1 file changed, 74 insertions(+)
diff --git a/block/iscsi.c b/block/iscsi.c
index deb3b68..37f12b3 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -67,6 +67,12 @@ typedef struct IscsiAIOCB {
#endif
} IscsiAIOCB;
+struct IscsiTask {
+IscsiLun *iscsilun;
+int status;
+int complete;
+};
+
#define NOP_INTERVAL 5000
#define MAX_NOP_FAILURES 3
@@ -700,6 +706,73 @@ iscsi_getlength(BlockDriverState *bs)
return len;
}
+static void
+iscsi_readcapacity16_cb(struct iscsi_context *iscsi, int status,
+void *command_data, void *opaque)
+{
+struct IscsiTask *itask = opaque;
+struct scsi_readcapacity16 *rc16;
+struct scsi_task *task = command_data;
+
+if (status != 0) {
+error_report(iSCSI: Failed to read capacity of iSCSI lun. %s,
+ iscsi_get_error(iscsi));
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+rc16 = scsi_datain_unmarshall(task);
+if (rc16 == NULL) {
+error_report(iSCSI: Failed to unmarshall readcapacity16 data.);
+itask-status = 1;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+return;
+}
+
+itask-iscsilun-block_size = rc16-block_length;
+itask-iscsilun-num_blocks = rc16-returned_lba + 1;
+
+itask-status = 0;
+itask-complete = 1;
+scsi_free_scsi_task(task);
+}
+
+static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
+{
+IscsiLun *iscsilun = bs-opaque;
+struct IscsiTask itask;
+struct scsi_task *task = NULL;
+
+if (iscsilun-type != TYPE_DISK) {
+return -ENOTSUP;
+}
+
+itask.iscsilun = iscsilun;
+itask.status = 0;
+itask.complete = 0;
+
+task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun,
+ iscsi_readcapacity16_cb, itask);
You can use iscsi_readcapacity16_sync. In fact, you probably should
extract code from iscsi_open and reuse it here.
Thats not possible afaik. Mixing sync and async commands in libiscsi is
a very bad thing. It will leed to nested event loops.
Peter
Paolo
+if (task == NULL) {
+ error_report(iSCSI: failed to send readcapacity16 command.);
+ return -EINVAL;
+}
+
+while (!itask.complete) {
+iscsi_set_events(iscsilun);
+qemu_aio_wait();
+}
+
+if (offset iscsi_getlength(bs)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
static int parse_chap(struct iscsi_context *iscsi, const char *target)
{
QemuOptsList *list;
@@ -1093,6 +1166,7 @@ static BlockDriver bdrv_iscsi = {
.create_options = iscsi_create_options,
.bdrv_getlength = iscsi_getlength,
+.bdrv_truncate = iscsi_truncate,
.bdrv_aio_readv = iscsi_aio_readv,
.bdrv_aio_writev = iscsi_aio_writev,
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
Antoine, Am 15.02.2013 03:49, schrieb Antoine Mathys: First, the ds1338 code was in a poor state and never handled the 12 hour clock correctly. My first patch failed to fully fix the problem so I had to write a second one, but at no point did Peter or I introduce a regression, quite the opposite. Read closely, I never claimed *you* introduced a regression. What I have rather been observing is a seemingly not-ending stream of bugfix patches on that matter and Peter not making an effort of requesting qtest cases from you or for any new ARM devices elsewhere. And while we're at it, what annoys me personally is that this patch does not have a ds1338: prefix when it doesn't touch anything else. People like me need to go through git logs for potential backports and having that made unnecessarily hard sucks. Peter can hopefully fix that in his arm-devs.next queue. Second, I don't know where you got the idea that I refuse to write test cases. I just didn't have one ready or in the works at the time. From reading the mailing list, obviously: https://bugs.launchpad.net/qemu/+bug/1090558 - closed by Paolo due to lack of test case, no response of yours http://patchwork.ozlabs.org/patch/220390/ Quote: Do you have a testcase? No, but the refactoring makes the code very easy to audit. The expected answer would've been take guest X and do Y to see Z, or better to extend the existing qtest cases to prove something was broken before and fixed afterwards and to avoid the same bug being reintroduced later. qtest has special commands to control time fwiw, so it should be entirely possible to set the time to 0, 12, 23 and anything-in-between hours to verify the register values are as expected. Third, bug 1090558 in mc146818rtc is a good example of a bug which was not due to insufficient testing, but to poorly structured code. There is no point worrying about unit testing if you accept code of such low quality. This goes for the tests too. For instance cmos_get_date_time() in tests/rtc-test.c doesn't work correctly in 12 hour mode. Fourth, I am not interested in the PC architecture, I only wrote a fix for bug 1090558 because Paolo asked me to. It is nice to see that fixing your crappy code makes me not a nice guy who is making things worse. But don't worry, I'll focus on ARM from now on. You seem to be missing the point: My comments have exactly nothing to do with PC vs. ARM or with you making things worse! It's about you a) supplying a bunch of fixes without giving maintainers a preferably automated way to verify they are in fact correct and b) - judging from your replies - being stubborn in grasping that you are not the only one supplying patches to QEMU and that while you may understand the code better now, someone else might not and may well introduce regressions even if you personally didn't - from a maintenance QA point of view it makes no difference who introduces a bug. QEMU has a long history of patch submissions and, as you have noticed on the topic of I2C, our test infrastructure is still new. The code quality doesn't get improved by complaining about it being low though but by improving code (that part you have done) and ensuring that the quality doesn't regress (that's the part this discussion is about). qtest is the most convenient infrastructure since it's integrated into make check and can easily be run by any maintainer or contributor; another option is the external virt-test framework (which didn't seem to have any provisions for ARM last time I looked around). So, my nagging for qtest test cases for ds1338 does not get resolved by saying you'll focus on ARM now and stay out of PC, because if I am not completely mistaken ds1338 is all about ARM. IIUC such an I2C device can be instantiated via -device using the available machine that I added libqos support for (-M n800 or -M n810), to prove that it is easily possible. You can't expect me to write everything for you! Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
A socket may still have references to it in various queues
at the time it is freed, causing memory corruptions.
Signed-off-by: Vitaly Chipounov vitaly.chipou...@epfl.ch
---
slirp/socket.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/slirp/socket.c b/slirp/socket.c
index 77b0c98..8a7adc8 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -55,6 +55,33 @@ socreate(Slirp *slirp)
return(so);
}
+/**
+ * It may happen that a socket is still referenced in various
+ * mbufs at the time it is freed. Clear all references to the
+ * socket here.
+ */
+static void soremovefromqueues(struct socket *so)
+{
+if (!so-so_queued) {
+return;
+}
+
+Slirp *slirp = so-slirp;
+struct mbuf *ifm;
+
+for (ifm = slirp-if_fastq.ifq_next; ifm != slirp-if_fastq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+
+for (ifm = slirp-if_batchq.ifq_next; ifm != slirp-if_batchq; ifm =
ifm-ifq_next) {
+if (ifm-ifq_so == so) {
+ifm-ifq_so = NULL;
+}
+}
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -79,6 +106,8 @@ sofree(struct socket *so)
if(so-so_next so-so_prev)
remque(so); /* crashes if so is not in a queue */
+ soremovefromqueues(so);
+
free(so);
}
--
1.7.10
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
On 15 February 2013 11:24, Andreas Färber afaer...@suse.de wrote: Am 15.02.2013 03:49, schrieb Antoine Mathys: First, the ds1338 code was in a poor state and never handled the 12 hour clock correctly. My first patch failed to fully fix the problem so I had to write a second one, but at no point did Peter or I introduce a regression, quite the opposite. Read closely, I never claimed *you* introduced a regression. What I have rather been observing is a seemingly not-ending stream of bugfix patches One patch is hardly a never-ending stream! on that matter and Peter not making an effort of requesting qtest cases from you or for any new ARM devices elsewhere. If people want to provide test cases, cool; I'm not currently insisting on them. And while we're at it, what annoys me personally is that this patch does not have a ds1338: prefix when it doesn't touch anything else. People like me need to go through git logs for potential backports and having that made unnecessarily hard sucks. Peter can hopefully fix that in his arm-devs.next queue. Yes, and I said so in my review-and-accepted mail yesterday. -- PMM
[Qemu-devel] [PATCH 1/5] sysbus: make SysBusDeviceClass::init optional
Make the SysBusDeviceClass::init optional, for devices which
genuinely don't need to do anything here. In particular, simple
devices which can do all their initialization in their
instance_init method don't need either a DeviceClass::realize
or SysBusDeviceClass::init method.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/sysbus.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index 6d9d1df..e9a16ac 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -118,6 +118,9 @@ static int sysbus_device_init(DeviceState *dev)
SysBusDevice *sd = SYS_BUS_DEVICE(dev);
SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(sd);
+if (!sbc-init) {
+return 0;
+}
return sbc-init(sd);
}
--
1.7.9.5
[Qemu-devel] [PATCH 0/5] Remove sysbus_add_memory and sysbus_del_memory
The functions sysbus_add_memory and sysbus_del_memory are odd wrappers around around memory_region_add/del_subregion, and their presence is an encouragement to devices to try to map their own memory regions into the system address space. Since they're only used in a couple of places in the milkymist and musicpal platforms, rewrite those uses to have the sysbus devices expose the memory regions as sysbus mmio regions, and then have the creator of the device (ie board code) map them in the right places. Then we can remove the functions altogether. The series includes a trivial patch to sysbus to make the init method optional, since (as part of the move towards using only instance_init and realize) it's now possible to have a simple functional device which only needs an instance_init method and no realize or init [the musicpal-misc device introduced in patch 2 being one such example]. Tested on both musicpal and milkymist. I rather suspect sysbus_add_io and sysbus_del_io should also be removed, but since their users are in PPC and x86 platforms I'll let somebody else do that part :-) Peter Maydell (5): sysbus: make SysBusDeviceClass::init optional musicpal: qdevify musicpal-misc milkymist-minimac2: Just expose buffers as a sysbus mmio region milkymist-softusb: Don't map RAM memory regions in the device itself sysbus: Remove sysbus_add_memory and sysbus_del_memory hw/milkymist-hw.h |6 +++--- hw/milkymist-minimac2.c |5 + hw/milkymist-softusb.c | 21 +++-- hw/musicpal.c | 34 +- hw/sysbus.c | 21 +++-- hw/sysbus.h |5 - 6 files changed, 47 insertions(+), 45 deletions(-) -- 1.7.9.5
Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses
On Thu, 14 Feb 2013 17:56:58 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
[...]
+
+}
+
+static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
+{
+X86CPUClass *xcc = X86_CPU_CLASS(oc);
+x86_def_t *def = data;
+int i;
+static const char *versioned_models[] = { qemu32, qemu64,
athlon }; +
+memcpy(xcc-info, def, sizeof(x86_def_t));
+xcc-info.ext_features |= CPUID_EXT_HYPERVISOR;
If this is TCG-specific now, we could make the class match the
reality and mask TCG_FEATURES/TCG_EXT_FEATURES/etc here. That's
what happens in practice, e.g.: -cpu SandyBridge in TCG mode is a
completely different CPU, today.
well, this function is shared between TCG and KVM, I mean, it's common
code for both. Which asks for one more TCG class_init function for TCG
specific behavior.
Having both TCG-specific and KVM-specific subclasses instead of making
the KVM class inherit from the TCG class would make sense to me, as we
may have TCG-specific behavior in other places too.
Or we could do memcpy() again on the KVM class_init function. Or we
could set a different void* pointer on the TCG class, with
already-filtered x86_def_t object. There are multiple possible
solutions.
But could it be a separate patch (i.e. fixing TCG filtering), I think
just moving tcg filtering might cause regression. And need to worked
on separately.
I'm OK with doing that in a separate patch, as it is not a bug fix or
behavior change. But it would be nice to do that before we introduce the
feature properties, to make the reported defaults right since the
beginning.
It's behavior change, if we move filtering from realizefn to class_init,
user would be able to add features not visible now to guest.
ordering now:
class_init, initfn, setting defautls, custom features,
realizefn(filtering)
would be ordering with static properties:
clas_init, static props defaults, global properties(custom features),
x86_cpu_initfn, realizefn
in would be scenario filtering comes before custom features, which is not
necessarily bad and may be nice to consciously add/test features before
enabling them in filter for everyone, but it's behavior change.
We should keep the filtering on realize because of custom -cpu strings,
but if we filter on class_init too, we will make the class introspection
reflect reality more closely. Wasn't this one the main reasons you
argued (and convinced me) for having separate subclasses?
Also, if we do this we will be able to make enforce work for TCG too.
For example: if enforce worked for TCG today, people who want to work
around the existing n270 MOVBE bug could use -cpu n270,+movbe,enforce
to make sure the QEMU version they are using really accepts movbe.
I've thought you proposed to move it, if we duplicate it then it should be
fine.
[...]
+#ifdef CONFIG_KVM
+static void x86_cpu_kvm_def_class_init(ObjectClass *oc, void
*data) +{
+uint32_t eax, ebx, ecx, edx;
+X86CPUClass *xcc = X86_CPU_CLASS(oc);
+
+x86_cpu_def_class_init(oc, data);
+/* sysenter isn't supported in compatibility mode on AMD,
+ * syscall isn't supported in compatibility mode on Intel.
+ * Normally we advertise the actual CPU vendor, but you can
+ * override this using the 'vendor' property if you want to
use
+ * KVM's sysenter/syscall emulation in compatibility mode and
+ * when doing cross vendor migration
+ */
+host_cpuid(0x0, 0, eax, ebx, ecx, edx);
Cool, this is exactly what I was going to suggest, to avoid
depending on the host CPU class and KVM initialization.
I see two options when making the vendor property static, and I
don't know which one is preferable:
One solution is the one in this patch: to call host_cpuid() here in
class_init, and have a different property default depending on the
host CPU.
I prefer this one ^^^.
Another solution is to have default to vendor=host, and have
instance_init understand vendor=host as use the host CPU vendor.
This would make the property default really static (not depending
on the host CPU).
I am inclined for the latter, because I am assuming that the QOM
design expects class_init results to be completely static. This is
not as bad as depending on KVM initialization, but it may be an
unexpected surprise, or even something not allowed by QOM.
That's where I have to disagree :)
You might have a point with 'static' if our goal is for introspection
for source level to work. But we have a number of issues with that:
* model_id is not static for
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
Il 15/02/2013 12:18, Peter Lieven ha scritto: +task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun, + iscsi_readcapacity16_cb, itask); You can use iscsi_readcapacity16_sync. In fact, you probably should extract code from iscsi_open and reuse it here. Thats not possible afaik. Mixing sync and async commands in libiscsi is a very bad thing. It will leed to nested event loops. Ah, I thought qmp_block_resize did a bdrv_drain_all before calling bdrv_truncate. Maybe it should. Kevin, what do you think? Paolo
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
Am 15.02.2013 um 12:54 schrieb Paolo Bonzini pbonz...@redhat.com: Il 15/02/2013 12:18, Peter Lieven ha scritto: +task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun, + iscsi_readcapacity16_cb, itask); You can use iscsi_readcapacity16_sync. In fact, you probably should extract code from iscsi_open and reuse it here. Thats not possible afaik. Mixing sync and async commands in libiscsi is a very bad thing. It will leed to nested event loops. Ah, I thought qmp_block_resize did a bdrv_drain_all before calling bdrv_truncate. Ah ok. In this special case it might be ok. We should ask Ronnie. What must not happen is that the sync task loop calls the async callbacks. Peter Maybe it should. Kevin, what do you think? Paolo
Re: [Qemu-devel] [PATCH] pseries: Implements h_read hcall
On 15.02.2013, at 11:59, Erlon Cruz wrote:
From: Erlon Cruz erlon.c...@br.flextronics.com
This h_call is useful for DLPAR in future amongst other things. Given an index
it fetches the corresponding PTE stored in the htab.
Signed-off-by: Erlon Cruz erlon.c...@br.flextronics.com
Looks good to me, but I'd like an ack from David.
Alex
---
hw/spapr_hcall.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 2889742..1065277 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -323,6 +323,39 @@ static target_ulong h_protect(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
return H_SUCCESS;
}
+static target_ulong h_read(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+target_ulong opcode, target_ulong *args)
+{
+CPUPPCState *env = cpu-env;
+target_ulong flags = args[0];
+target_ulong pte_index = args[1];
+target_ulong v[4], r[4];
+uint8_t *hpte;
+int i, ridx, n_entries = 1;
+
+if ((pte_index * HASH_PTE_SIZE_64) ~env-htab_mask) {
+return H_PARAMETER;
+}
+
+if (flags H_READ_4) {
+/* Clear the two low order bits */
+pte_index = ~(3ULL);
+n_entries = 4;
+}
+
+hpte = env-external_htab + (pte_index * HASH_PTE_SIZE_64);
+
+for (i = 0, ridx = 0; i n_entries; i++) {
+v[i] = ldq_p(hpte);
+r[i] = ldq_p(hpte + (HASH_PTE_SIZE_64/2));
+args[ridx++] = v[i];
+args[ridx++] = r[i];
+hpte += HASH_PTE_SIZE_64;
+}
+
+return H_SUCCESS;
+}
+
static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPREnvironment *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -714,6 +747,7 @@ static void hypercall_register_types(void)
spapr_register_hypercall(H_ENTER, h_enter);
spapr_register_hypercall(H_REMOVE, h_remove);
spapr_register_hypercall(H_PROTECT, h_protect);
+spapr_register_hypercall(H_READ, h_read);
/* hcall-bulk */
spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);
--
1.7.9.5
[Qemu-devel] [PATCH 5/5] sysbus: Remove sysbus_add_memory and sysbus_del_memory
Remove the sysbus_add_memory and sysbus_del_memory functions. These
are trivial wrappers for mapping a memory region into the system
memory space, and have no users now. Sysbus devices should never map
their own memory regions anyway; the correct API for mapping an mmio
region is for the creator of the device to use sysbus_mmio_map.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/sysbus.c | 18 --
hw/sysbus.h |5 -
2 files changed, 23 deletions(-)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index e9a16ac..980f31c 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -217,24 +217,6 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
return g_strdup(path);
}
-void sysbus_add_memory(SysBusDevice *dev, hwaddr addr,
- MemoryRegion *mem)
-{
-memory_region_add_subregion(get_system_memory(), addr, mem);
-}
-
-void sysbus_add_memory_overlap(SysBusDevice *dev, hwaddr addr,
- MemoryRegion *mem, unsigned priority)
-{
-memory_region_add_subregion_overlap(get_system_memory(), addr, mem,
-priority);
-}
-
-void sysbus_del_memory(SysBusDevice *dev, MemoryRegion *mem)
-{
-memory_region_del_subregion(get_system_memory(), mem);
-}
-
void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
MemoryRegion *mem)
{
diff --git a/hw/sysbus.h b/hw/sysbus.h
index a7fcded..25c3ff2 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -56,11 +56,6 @@ void sysbus_init_ioports(SysBusDevice *dev, pio_addr_t
ioport, pio_addr_t size);
void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq);
void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
-void sysbus_add_memory(SysBusDevice *dev, hwaddr addr,
- MemoryRegion *mem);
-void sysbus_add_memory_overlap(SysBusDevice *dev, hwaddr addr,
- MemoryRegion *mem, unsigned priority);
-void sysbus_del_memory(SysBusDevice *dev, MemoryRegion *mem);
void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
MemoryRegion *mem);
void sysbus_del_io(SysBusDevice *dev, MemoryRegion *mem);
--
1.7.9.5
[Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Make musicpal-misc into its own (trivial) qdev device, so we
can get rid of the abuse of sysbus_add_memory().
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/musicpal.c | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 272cb80..819e420 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
#define MP_BOARD_REVISION 0x31
+typedef struct {
+SysBusDevice busdev;
+MemoryRegion iomem;
+} MusicPalMiscState;
+
+typedef SysBusDeviceClass MusicPalMiscClass;
+
+#define TYPE_MUSICPAL_MISC musicpal-misc
+#define MUSICPAL_MISC(obj) \
+ OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_CLASS(klass) \
+ OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC)
+
static uint64_t musicpal_misc_read(void *opaque, hwaddr offset,
unsigned size)
{
@@ -1054,15 +1069,23 @@ static const MemoryRegionOps musicpal_misc_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void musicpal_misc_init(SysBusDevice *dev)
+static void musicpal_misc_init(Object *obj)
{
-MemoryRegion *iomem = g_new(MemoryRegion, 1);
+SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+MusicPalMiscState *s = MUSICPAL_MISC(obj);
-memory_region_init_io(iomem, musicpal_misc_ops, NULL,
+memory_region_init_io(s-iomem, musicpal_misc_ops, NULL,
musicpal-misc, MP_MISC_SIZE);
-sysbus_add_memory(dev, MP_MISC_BASE, iomem);
+sysbus_init_mmio(sd, s-iomem);
}
+static const TypeInfo musicpal_misc_info = {
+.name = TYPE_MUSICPAL_MISC,
+.parent = TYPE_SYS_BUS_DEVICE,
+.instance_init = musicpal_misc_init,
+.instance_size = sizeof(MusicPalMiscState),
+};
+
/* WLAN register offsets */
#define MP_WLAN_MAGIC1 0x11c
#define MP_WLAN_MAGIC2 0x124
@@ -1612,7 +1635,7 @@ static void musicpal_init(QEMUMachineInitArgs *args)
sysbus_create_simple(mv88w8618_wlan, MP_WLAN_BASE, NULL);
-musicpal_misc_init(SYS_BUS_DEVICE(dev));
+sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL);
dev = sysbus_create_simple(musicpal_gpio, MP_GPIO_BASE,
pic[MP_GPIO_IRQ]);
i2c_dev = sysbus_create_simple(gpio_i2c, -1, NULL);
@@ -1692,6 +1715,7 @@ static void musicpal_register_types(void)
type_register_static(musicpal_lcd_info);
type_register_static(musicpal_gpio_info);
type_register_static(musicpal_key_info);
+type_register_static(musicpal_misc_info);
}
type_init(musicpal_register_types)
--
1.7.9.5
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
On Fri, Feb 15, 2013 at 12:54:51PM +0100, Paolo Bonzini wrote: Il 15/02/2013 12:18, Peter Lieven ha scritto: +task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun, + iscsi_readcapacity16_cb, itask); You can use iscsi_readcapacity16_sync. In fact, you probably should extract code from iscsi_open and reuse it here. Thats not possible afaik. Mixing sync and async commands in libiscsi is a very bad thing. It will leed to nested event loops. Ah, I thought qmp_block_resize did a bdrv_drain_all before calling bdrv_truncate. Maybe it should. Kevin, what do you think? Probably. bdrv_truncate() invalidates the bdrv_check_request() result for in-flight requests, so there better be none. Kevin
[Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
device itself. Instead just expose them as sysbus mmio regions which
the device creator can map appropriately. This allows us to drop the
pmem_base and dmem_base properties. Instead of going via
cpu_physical_memory_read/_write when the device wants to access the
RAMs, we just keep a host pointer to the memory and use that.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/milkymist-hw.h |4 ++--
hw/milkymist-softusb.c | 21 +++--
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index 21e202b..47f6d50 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -210,12 +210,12 @@ static inline DeviceState
*milkymist_softusb_create(hwaddr base,
DeviceState *dev;
dev = qdev_create(NULL, milkymist-softusb);
-qdev_prop_set_uint32(dev, pmem_base, pmem_base);
qdev_prop_set_uint32(dev, pmem_size, pmem_size);
-qdev_prop_set_uint32(dev, dmem_base, dmem_base);
qdev_prop_set_uint32(dev, dmem_size, dmem_size);
qdev_init_nofail(dev);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, pmem_base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, dmem_base);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
return dev;
diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
index 01660be..a3e935f 100644
--- a/hw/milkymist-softusb.c
+++ b/hw/milkymist-softusb.c
@@ -54,10 +54,11 @@ struct MilkymistSoftUsbState {
MemoryRegion dmem;
qemu_irq irq;
+void *pmem_ptr;
+void *dmem_ptr;
+
/* device properties */
-uint32_t pmem_base;
uint32_t pmem_size;
-uint32_t dmem_base;
uint32_t dmem_size;
/* device registers */
@@ -134,7 +135,7 @@ static inline void softusb_read_dmem(MilkymistSoftUsbState
*s,
return;
}
-cpu_physical_memory_read(s-dmem_base + offset, buf, len);
+memcpy(buf, s-dmem_ptr + offset, len);
}
static inline void softusb_write_dmem(MilkymistSoftUsbState *s,
@@ -146,7 +147,7 @@ static inline void softusb_write_dmem(MilkymistSoftUsbState
*s,
return;
}
-cpu_physical_memory_write(s-dmem_base + offset, buf, len);
+memcpy(s-dmem_ptr + offset, buf, len);
}
static inline void softusb_read_pmem(MilkymistSoftUsbState *s,
@@ -158,7 +159,7 @@ static inline void softusb_read_pmem(MilkymistSoftUsbState
*s,
return;
}
-cpu_physical_memory_read(s-pmem_base + offset, buf, len);
+memcpy(buf, s-pmem_ptr + offset, len);
}
static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
@@ -170,7 +171,7 @@ static inline void softusb_write_pmem(MilkymistSoftUsbState
*s,
return;
}
-cpu_physical_memory_write(s-pmem_base + offset, buf, len);
+memcpy(s-pmem_ptr + offset, buf, len);
}
static void softusb_mouse_changed(MilkymistSoftUsbState *s)
@@ -270,11 +271,13 @@ static int milkymist_softusb_init(SysBusDevice *dev)
memory_region_init_ram(s-pmem, milkymist-softusb.pmem,
s-pmem_size);
vmstate_register_ram_global(s-pmem);
-sysbus_add_memory(dev, s-pmem_base, s-pmem);
+s-pmem_ptr = memory_region_get_ram_ptr(s-pmem);
+sysbus_init_mmio(dev, s-pmem);
memory_region_init_ram(s-dmem, milkymist-softusb.dmem,
s-dmem_size);
vmstate_register_ram_global(s-dmem);
-sysbus_add_memory(dev, s-dmem_base, s-dmem);
+s-dmem_ptr = memory_region_get_ram_ptr(s-dmem);
+sysbus_init_mmio(dev, s-dmem);
hid_init(s-hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
hid_init(s-hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
@@ -298,9 +301,7 @@ static const VMStateDescription vmstate_milkymist_softusb =
{
};
static Property milkymist_softusb_properties[] = {
-DEFINE_PROP_UINT32(pmem_base, MilkymistSoftUsbState, pmem_base,
0xa000),
DEFINE_PROP_UINT32(pmem_size, MilkymistSoftUsbState, pmem_size,
0x1000),
-DEFINE_PROP_UINT32(dmem_base, MilkymistSoftUsbState, dmem_base,
0xa002),
DEFINE_PROP_UINT32(dmem_size, MilkymistSoftUsbState, dmem_size,
0x2000),
DEFINE_PROP_END_OF_LIST(),
};
--
1.7.9.5
[Qemu-devel] [PATCH 3/5] milkymist-minimac2: Just expose buffers as a sysbus mmio region
Just expose the register buffers memory as a standard sysbus mmio
region which the creator of the device can map, rather than
providing a qdev property which the creator has to set to the
base address and then doing the mapping in the device's own
init function.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/milkymist-hw.h |2 +-
hw/milkymist-minimac2.c |5 +
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index c8bd7e9..21e202b 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -193,10 +193,10 @@ static inline DeviceState
*milkymist_minimac2_create(hwaddr base,
qemu_check_nic_model(nd_table[0], minimac2);
dev = qdev_create(NULL, milkymist-minimac2);
-qdev_prop_set_taddr(dev, buffers_base, buffers_base);
qdev_set_nic_properties(dev, nd_table[0]);
qdev_init_nofail(dev);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, buffers_base);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, rx_irq);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 1, tx_irq);
diff --git a/hw/milkymist-minimac2.c b/hw/milkymist-minimac2.c
index 9992dcc..b462e90 100644
--- a/hw/milkymist-minimac2.c
+++ b/hw/milkymist-minimac2.c
@@ -96,7 +96,6 @@ struct MilkymistMinimac2State {
NICState *nic;
NICConf conf;
char *phy_model;
-hwaddr buffers_base;
MemoryRegion buffers;
MemoryRegion regs_region;
@@ -475,7 +474,7 @@ static int milkymist_minimac2_init(SysBusDevice *dev)
s-rx1_buf = s-rx0_buf + MINIMAC2_BUFFER_SIZE;
s-tx_buf = s-rx1_buf + MINIMAC2_BUFFER_SIZE;
-sysbus_add_memory(dev, s-buffers_base, s-buffers);
+sysbus_init_mmio(dev, s-buffers);
qemu_macaddr_default_if_unset(s-conf.macaddr);
s-nic = qemu_new_nic(net_milkymist_minimac2_info, s-conf,
@@ -517,8 +516,6 @@ static const VMStateDescription vmstate_milkymist_minimac2
= {
};
static Property milkymist_minimac2_properties[] = {
-DEFINE_PROP_TADDR(buffers_base, MilkymistMinimac2State,
-buffers_base, 0),
DEFINE_NIC_PROPERTIES(MilkymistMinimac2State, conf),
DEFINE_PROP_STRING(phy_model, MilkymistMinimac2State, phy_model),
DEFINE_PROP_END_OF_LIST(),
--
1.7.9.5
Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses
I'm happy because the replies are getting shorter! :-)
(But I'm unhappy because my questions about QOM design requirements are
not being answered yet.)
On Fri, Feb 15, 2013 at 12:49:46PM +0100, Igor Mammedov wrote:
On Thu, 14 Feb 2013 17:56:58 -0200
Eduardo Habkost ehabk...@redhat.com wrote:
On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
[...]
+
+}
+
+static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
+{
+X86CPUClass *xcc = X86_CPU_CLASS(oc);
+x86_def_t *def = data;
+int i;
+static const char *versioned_models[] = { qemu32, qemu64,
athlon }; +
+memcpy(xcc-info, def, sizeof(x86_def_t));
+xcc-info.ext_features |= CPUID_EXT_HYPERVISOR;
If this is TCG-specific now, we could make the class match the
reality and mask TCG_FEATURES/TCG_EXT_FEATURES/etc here. That's
what happens in practice, e.g.: -cpu SandyBridge in TCG mode is a
completely different CPU, today.
well, this function is shared between TCG and KVM, I mean, it's common
code for both. Which asks for one more TCG class_init function for TCG
specific behavior.
Having both TCG-specific and KVM-specific subclasses instead of making
the KVM class inherit from the TCG class would make sense to me, as we
may have TCG-specific behavior in other places too.
Or we could do memcpy() again on the KVM class_init function. Or we
could set a different void* pointer on the TCG class, with
already-filtered x86_def_t object. There are multiple possible
solutions.
But could it be a separate patch (i.e. fixing TCG filtering), I think
just moving tcg filtering might cause regression. And need to worked
on separately.
I'm OK with doing that in a separate patch, as it is not a bug fix or
behavior change. But it would be nice to do that before we introduce the
feature properties, to make the reported defaults right since the
beginning.
It's behavior change, if we move filtering from realizefn to class_init,
user would be able to add features not visible now to guest.
ordering now:
class_init, initfn, setting defautls, custom features,
realizefn(filtering)
would be ordering with static properties:
clas_init, static props defaults, global properties(custom features),
x86_cpu_initfn, realizefn
in would be scenario filtering comes before custom features, which is not
necessarily bad and may be nice to consciously add/test features before
enabling them in filter for everyone, but it's behavior change.
We should keep the filtering on realize because of custom -cpu strings,
but if we filter on class_init too, we will make the class introspection
reflect reality more closely. Wasn't this one the main reasons you
argued (and convinced me) for having separate subclasses?
Also, if we do this we will be able to make enforce work for TCG too.
For example: if enforce worked for TCG today, people who want to work
around the existing n270 MOVBE bug could use -cpu n270,+movbe,enforce
to make sure the QEMU version they are using really accepts movbe.
I've thought you proposed to move it, if we duplicate it then it should be
fine.
I was, but then you pointed out that it would change behavior. :-)
[...]
+#ifdef CONFIG_KVM
+static void x86_cpu_kvm_def_class_init(ObjectClass *oc, void
*data) +{
+uint32_t eax, ebx, ecx, edx;
+X86CPUClass *xcc = X86_CPU_CLASS(oc);
+
+x86_cpu_def_class_init(oc, data);
+/* sysenter isn't supported in compatibility mode on AMD,
+ * syscall isn't supported in compatibility mode on Intel.
+ * Normally we advertise the actual CPU vendor, but you can
+ * override this using the 'vendor' property if you want to
use
+ * KVM's sysenter/syscall emulation in compatibility mode and
+ * when doing cross vendor migration
+ */
+host_cpuid(0x0, 0, eax, ebx, ecx, edx);
Cool, this is exactly what I was going to suggest, to avoid
depending on the host CPU class and KVM initialization.
I see two options when making the vendor property static, and I
don't know which one is preferable:
One solution is the one in this patch: to call host_cpuid() here in
class_init, and have a different property default depending on the
host CPU.
I prefer this one ^^^.
Another solution is to have default to vendor=host, and have
instance_init understand vendor=host as use the host CPU vendor.
This would make the property default really static (not depending
on the host CPU).
I am inclined for the latter, because I am assuming that the QOM
design expects class_init
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
Am 15.02.2013 um 13:18 schrieb Kevin Wolf kw...@redhat.com: On Fri, Feb 15, 2013 at 12:54:51PM +0100, Paolo Bonzini wrote: Il 15/02/2013 12:18, Peter Lieven ha scritto: +task = iscsi_readcapacity16_task(iscsilun-iscsi, iscsilun-lun, + iscsi_readcapacity16_cb, itask); You can use iscsi_readcapacity16_sync. In fact, you probably should extract code from iscsi_open and reuse it here. Thats not possible afaik. Mixing sync and async commands in libiscsi is a very bad thing. It will leed to nested event loops. Ah, I thought qmp_block_resize did a bdrv_drain_all before calling bdrv_truncate. Maybe it should. Kevin, what do you think? Probably. bdrv_truncate() invalidates the bdrv_check_request() result for in-flight requests, so there better be none. So you would call brdv_drain_all() before calling brdv_truncate and then use a sync iscsi call? Peter Kevin
Re: [Qemu-devel] [PATCH 1/5] sysbus: make SysBusDeviceClass::init optional
Am 15.02.2013 12:45, schrieb Peter Maydell: Make the SysBusDeviceClass::init optional, for devices which genuinely don't need to do anything here. In particular, simple devices which can do all their initialization in their instance_init method don't need either a DeviceClass::realize or SysBusDeviceClass::init method. Signed-off-by: Peter Maydell peter.mayd...@linaro.org Acked-by: Andreas Färber afaer...@suse.de Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] QOM: stability expectations of introspectable class_init data
Hi, This is an attempt to summarize my main question from the thread: Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses My main unanswered question is about the stability expectations of the introspectable class data (especially property defaults). I am assuming and expecting that the introspectable QOM class data (especially property defaults) should simply reflect capabilities/behavior of the QEMU binary being queried, and would not change depending on the environment QEMU is running (host hardware and host kernel). This way, other components can use class introspection to probe for QEMU capabilities/behavior, and safely expect that the QEMU binary being queried will always have those capabilities/behavior. What Igor is proposing is to break my assumption, and make the default value of the vendor property on the X86CPU subclasses be different depending on the host CPU where QEMU is running. My question is: is that really OK? In another case, we are considering making other properties of a X86CPU subclass have different defaults depending on the capabilities of the host kernel (the host CPU class will have different feature property defaults depending on the capabilities reported by GET_SUPPORTED_CPUID). Would that be OK, too? -- Eduardo
Re: [Qemu-devel] [PATCH] iscsi: add iscsi_truncate support
Il 15/02/2013 13:36, Peter Lieven ha scritto: Ah, I thought qmp_block_resize did a bdrv_drain_all before calling bdrv_truncate. Maybe it should. Kevin, what do you think? Probably. bdrv_truncate() invalidates the bdrv_check_request() result for in-flight requests, so there better be none. So you would call brdv_drain_all() before calling brdv_truncate and then use a sync iscsi call? Yes, please. Paolo
[Qemu-devel] [1.4]: 32-bit framebuffer video regression on qemu-system-ppc
Hi all, I've just updated my QEMU git pull to master in order to do some testing, and I'm noticing a regression with 32-bit framebuffers on PPC. If I load QEMU with the following command line to force a 32-bit framebuffer then the light yellow OpenBIOS background now appears as a bright garish yellow: ./qemu-system-ppc -g 1024x768x32 -vnc :1 Does anyone else see this too? The above executable was built from git master on an amd64 system running Debian Wheezy. Many thanks, Mark.
Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output
Am 15.02.2013 13:51, schrieb Stefan Hajnoczi:
On Fri, Feb 15, 2013 at 10:50:16AM +0100, Andreas Färber wrote:
Am 15.02.2013 10:06, schrieb Stefan Hajnoczi:
In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204
Basically you do DBG(hello world\n) and it gets compiled out by
default using:
if (DBG_LOG) {
printf(hello world\n);
}
Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:
make DEBUG=e1000 # build QEMU with debug printfs only in e1000.o
The iPXE implementation is fancier than this. It supports different log
levels, hex dumping, MD5 hashing, etc.
But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
support that lets you say make DEBUG=e1000,rtl8139,net.
It would be a big step up from the mess we have today.
Thanks for that feedback. If you look at the previous discussion, that's
part of what I did in my RFC, and it was rejected in favor of keeping
#ifdef'able defines. Using inline functions to avoid some nasty
macro-is-not-function issues, there seemed to be no need to combine both
approaches due to the format string being checked one level above.
Redefining debug functions in every file is nasty. I am also not a fan
of modifying a #define, it always need to be reverted before committing
changes.
If you want to convert the code to tracepoints, feel free to go at it!
But that hasn't been done since introducing that infrastructure, so my
hopes are low. ;)
Personally, I think we should use tracing instead of debug printfs
though. Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like. You can mark debug trace events disabled in
./trace-events so they will not be compiled in by default - zero
performance overhead.
This series or patch is not against tracing. It might be an option to
use them for tmp105. But not being the author of the targets and all of
the devices my CPU refactorings potentially touch, it is infeasable for
me to determine an appropriate set of tracepoints everywhere. We'd also
need to decide whether we want per-target tracepoints or per-aspect
tracepoints, since it's a global list. Independent of that question,
some kind of include mechanism (like for the default-configs) would be
nice to decouple adding tracepoints in different areas.
Not sure I understand. You don't need to come up with new trace events
in code you didn't write.
I didn't write those, but I am the one that would like them to work for
my purposes. So it looks like I need to change them or nobody will.
DPRINTF() can be converted mechanically to
trace_foo(arg1, arg). Then we get rid of all the DPRINTF() definitions.
What is foo though and what arg1, arg? That needs to be invented AFAIU.
Following up on Anthony's original thought, the question is whether to
convert a LOG_EXCP macro in target-ppc to ppc_log_excp tracepoint or
whether to homogenize it as log_excp and use that across targets, to
save tracepoint definitions. That's not purely mechanical.
The ./trace-events list is informal and can change at any time. We
don't need to coordinate between different subsystems or targets in
QEMU.
I'm assuming that changes to ppc logging go through ppc-next, changes to
sparc code go through Blue etc. All those potentially conflict since
they're adding to the bottom of the same text file, thus coordination.
It's a long-standing criticism of our centralistic tracing
infrastructure compared to the totally decentral and inhomogeneous
DPRINTFs and what-nots on the other hand.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH qom-cpu-next v5] target-i386: Split command line parsing out of cpu_x86_register()
From: Andreas Färber afaer...@suse.de
In order to instantiate a CPU subtype we will need to know which type,
so move the cpu_model splitting into cpu_x86_init().
Parameters need to be set on the X86CPU instance, so move
cpu_x86_parse_featurestr() into cpu_x86_init() as well.
This leaves cpu_x86_register() operating on the model name only.
Signed-off-by: Andreas Färber afaer...@suse.de
Signed-off-by: Igor Mammedov imamm...@redhat.com
---
v5:
* get error to report from cpu_x86_register()
v4:
* consolidate resource cleanup in when leaving cpu_x86_init(),
to avoid clean code duplication.
* remove unnecessary error message from hw/pc.c
---
hw/pc.c |1 -
target-i386/cpu.c | 80 ++--
2 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 53cc173..07caba7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -876,7 +876,6 @@ void pc_cpus_init(const char *cpu_model)
for (i = 0; i smp_cpus; i++) {
if (!cpu_x86_init(cpu_model)) {
-fprintf(stderr, Unable to find x86 CPU definition\n);
exit(1);
}
}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index dcbed0d..dadb0f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1516,27 +1516,16 @@ static void filter_features_for_kvm(X86CPU *cpu)
}
#endif
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
{
CPUX86State *env = cpu-env;
x86_def_t def1, *def = def1;
-Error *error = NULL;
-char *name, *features;
-gchar **model_pieces;
memset(def, 0, sizeof(*def));
-model_pieces = g_strsplit(cpu_model, ,, 2);
-if (!model_pieces[0]) {
-error_setg(error, Invalid/empty CPU model name);
-goto out;
-}
-name = model_pieces[0];
-features = model_pieces[1];
-
if (cpu_x86_find_by_name(def, name) 0) {
-error_setg(error, Unable to find CPU definition: %s, name);
-goto out;
+error_setg(errp, Unable to find CPU definition: %s, name);
+return;
}
if (kvm_enabled()) {
@@ -1544,58 +1533,69 @@ static int cpu_x86_register(X86CPU *cpu, const char
*cpu_model)
}
def-ext_features |= CPUID_EXT_HYPERVISOR;
-object_property_set_str(OBJECT(cpu), def-vendor, vendor, error);
-object_property_set_int(OBJECT(cpu), def-level, level, error);
-object_property_set_int(OBJECT(cpu), def-family, family, error);
-object_property_set_int(OBJECT(cpu), def-model, model, error);
-object_property_set_int(OBJECT(cpu), def-stepping, stepping, error);
+object_property_set_str(OBJECT(cpu), def-vendor, vendor, errp);
+object_property_set_int(OBJECT(cpu), def-level, level, errp);
+object_property_set_int(OBJECT(cpu), def-family, family, errp);
+object_property_set_int(OBJECT(cpu), def-model, model, errp);
+object_property_set_int(OBJECT(cpu), def-stepping, stepping, errp);
env-cpuid_features = def-features;
env-cpuid_ext_features = def-ext_features;
env-cpuid_ext2_features = def-ext2_features;
env-cpuid_ext3_features = def-ext3_features;
-object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, error);
+object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, errp);
env-cpuid_kvm_features = def-kvm_features;
env-cpuid_svm_features = def-svm_features;
env-cpuid_ext4_features = def-ext4_features;
env-cpuid_7_0_ebx_features = def-cpuid_7_0_ebx_features;
env-cpuid_xlevel2 = def-xlevel2;
-object_property_set_str(OBJECT(cpu), def-model_id, model-id, error);
-if (error) {
-goto out;
-}
-
-cpu_x86_parse_featurestr(cpu, features, error);
-out:
-g_strfreev(model_pieces);
-if (error) {
-fprintf(stderr, %s\n, error_get_pretty(error));
-error_free(error);
-return -1;
-}
-return 0;
+object_property_set_str(OBJECT(cpu), def-model_id, model-id, errp);
}
X86CPU *cpu_x86_init(const char *cpu_model)
{
-X86CPU *cpu;
+X86CPU *cpu = NULL;
CPUX86State *env;
+gchar **model_pieces;
+char *name, *features;
Error *error = NULL;
+model_pieces = g_strsplit(cpu_model, ,, 2);
+if (!model_pieces[0]) {
+error_setg(error, Invalid/empty CPU model name);
+goto out;
+}
+name = model_pieces[0];
+features = model_pieces[1];
+
cpu = X86_CPU(object_new(TYPE_X86_CPU));
env = cpu-env;
env-cpu_model_str = cpu_model;
-if (cpu_x86_register(cpu, cpu_model) 0) {
-object_unref(OBJECT(cpu));
-return NULL;
+cpu_x86_register(cpu, name, error);
+if (error) {
+goto out;
+}
+
+cpu_x86_parse_featurestr(cpu, features, error);
+if (error) {
+goto out;
}
object_property_set_bool(OBJECT(cpu), true, realized, error);
if (error) {
+goto out;
+}
+
+out:
+g_strfreev(model_pieces);
Re: [Qemu-devel] [Qemu-ppc] [1.4]: 32-bit framebuffer video regression on qemu-system-ppc
On 15.02.2013, at 14:01, Mark Cave-Ayland wrote: Hi all, I've just updated my QEMU git pull to master in order to do some testing, and I'm noticing a regression with 32-bit framebuffers on PPC. If I load QEMU with the following command line to force a 32-bit framebuffer then the light yellow OpenBIOS background now appears as a bright garish yellow: ./qemu-system-ppc -g 1024x768x32 -vnc :1 Does anyone else see this too? Yes, I see the same thing on PPC hosts. So far I assumed it was due to my ancient pixman version, but maybe it's not related to that after all. Does it work with SDL? Alex
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Am 15.02.2013 12:45, schrieb Peter Maydell:
Make musicpal-misc into its own (trivial) qdev device, so we
can get rid of the abuse of sysbus_add_memory().
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/musicpal.c | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 272cb80..819e420 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
#define MP_BOARD_REVISION 0x31
+typedef struct {
Anonymous struct
+SysBusDevice busdev;
parent_obj please. :)
+MemoryRegion iomem;
+} MusicPalMiscState;
+
+typedef SysBusDeviceClass MusicPalMiscClass;
Please don't. Either define a struct and use it for .class_size or drop
the typedef.
+
+#define TYPE_MUSICPAL_MISC musicpal-misc
+#define MUSICPAL_MISC(obj) \
+ OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_CLASS(klass) \
+ OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC)
If we don't have such a class so you can just drop these two.
SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed.
+
static uint64_t musicpal_misc_read(void *opaque, hwaddr offset,
unsigned size)
{
@@ -1054,15 +1069,23 @@ static const MemoryRegionOps musicpal_misc_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void musicpal_misc_init(SysBusDevice *dev)
+static void musicpal_misc_init(Object *obj)
{
-MemoryRegion *iomem = g_new(MemoryRegion, 1);
+SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+MusicPalMiscState *s = MUSICPAL_MISC(obj);
-memory_region_init_io(iomem, musicpal_misc_ops, NULL,
+memory_region_init_io(s-iomem, musicpal_misc_ops, NULL,
musicpal-misc, MP_MISC_SIZE);
-sysbus_add_memory(dev, MP_MISC_BASE, iomem);
+sysbus_init_mmio(sd, s-iomem);
}
+static const TypeInfo musicpal_misc_info = {
+.name = TYPE_MUSICPAL_MISC,
+.parent = TYPE_SYS_BUS_DEVICE,
+.instance_init = musicpal_misc_init,
+.instance_size = sizeof(MusicPalMiscState),
+};
+
/* WLAN register offsets */
#define MP_WLAN_MAGIC1 0x11c
#define MP_WLAN_MAGIC2 0x124
@@ -1612,7 +1635,7 @@ static void musicpal_init(QEMUMachineInitArgs *args)
sysbus_create_simple(mv88w8618_wlan, MP_WLAN_BASE, NULL);
-musicpal_misc_init(SYS_BUS_DEVICE(dev));
+sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL);
dev = sysbus_create_simple(musicpal_gpio, MP_GPIO_BASE,
pic[MP_GPIO_IRQ]);
i2c_dev = sysbus_create_simple(gpio_i2c, -1, NULL);
@@ -1692,6 +1715,7 @@ static void musicpal_register_types(void)
type_register_static(musicpal_lcd_info);
type_register_static(musicpal_gpio_info);
type_register_static(musicpal_key_info);
+type_register_static(musicpal_misc_info);
}
type_init(musicpal_register_types)
Otherwise looks very good with instance_init!
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output
On 15.02.2013, at 14:04, Andreas Färber wrote:
Am 15.02.2013 13:51, schrieb Stefan Hajnoczi:
On Fri, Feb 15, 2013 at 10:50:16AM +0100, Andreas Färber wrote:
Am 15.02.2013 10:06, schrieb Stefan Hajnoczi:
In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204
Basically you do DBG(hello world\n) and it gets compiled out by
default using:
if (DBG_LOG) {
printf(hello world\n);
}
Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:
make DEBUG=e1000 # build QEMU with debug printfs only in e1000.o
The iPXE implementation is fancier than this. It supports different log
levels, hex dumping, MD5 hashing, etc.
But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
support that lets you say make DEBUG=e1000,rtl8139,net.
It would be a big step up from the mess we have today.
Thanks for that feedback. If you look at the previous discussion, that's
part of what I did in my RFC, and it was rejected in favor of keeping
#ifdef'able defines. Using inline functions to avoid some nasty
macro-is-not-function issues, there seemed to be no need to combine both
approaches due to the format string being checked one level above.
Redefining debug functions in every file is nasty. I am also not a fan
of modifying a #define, it always need to be reverted before committing
changes.
If you want to convert the code to tracepoints, feel free to go at it!
But that hasn't been done since introducing that infrastructure, so my
hopes are low. ;)
Personally, I think we should use tracing instead of debug printfs
though. Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like. You can mark debug trace events disabled in
./trace-events so they will not be compiled in by default - zero
performance overhead.
This series or patch is not against tracing. It might be an option to
use them for tmp105. But not being the author of the targets and all of
the devices my CPU refactorings potentially touch, it is infeasable for
me to determine an appropriate set of tracepoints everywhere. We'd also
need to decide whether we want per-target tracepoints or per-aspect
tracepoints, since it's a global list. Independent of that question,
some kind of include mechanism (like for the default-configs) would be
nice to decouple adding tracepoints in different areas.
Not sure I understand. You don't need to come up with new trace events
in code you didn't write.
I didn't write those, but I am the one that would like them to work for
my purposes. So it looks like I need to change them or nobody will.
DPRINTF() can be converted mechanically to
trace_foo(arg1, arg). Then we get rid of all the DPRINTF() definitions.
What is foo though and what arg1, arg? That needs to be invented AFAIU.
Following up on Anthony's original thought, the question is whether to
convert a LOG_EXCP macro in target-ppc to ppc_log_excp tracepoint or
whether to homogenize it as log_excp and use that across targets, to
save tracepoint definitions. That's not purely mechanical.
The ./trace-events list is informal and can change at any time. We
don't need to coordinate between different subsystems or targets in
QEMU.
I'm assuming that changes to ppc logging go through ppc-next, changes to
sparc code go through Blue etc. All those potentially conflict since
they're adding to the bottom of the same text file, thus coordination.
It's a long-standing criticism of our centralistic tracing
infrastructure compared to the totally decentral and inhomogeneous
DPRINTFs and what-nots on the other hand.
In parallel to the completely disastrous user experience when using trace
points. Debug printfs are easy and understandable. Tracepoints are not.
However, how about we take this one gradually? If all debug prints in all files
do an
#ifdef DEBUG
static const debug_enabled = 1;
#else
static const debug_enabled = 0;
#endif
then Stefan can probably add a -DDEBUG to a specific c file through Makefile
magic if he wants to do iPXE-style debugging. And if you're - like me - more
happy with local #define DEBUG, then you can do that as well.
I would definitely oppose moving to tracepoints.
Alex
Re: [Qemu-devel] [PATCH 3/5] milkymist-minimac2: Just expose buffers as a sysbus mmio region
Am 15.02.2013 12:45, schrieb Peter Maydell: Just expose the register buffers memory as a standard sysbus mmio region which the creator of the device can map, rather than providing a qdev property which the creator has to set to the base address and then doing the mapping in the device's own init function. Signed-off-by: Peter Maydell peter.mayd...@linaro.org Reviewed-by: Andreas Färber afaer...@suse.de Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] (PAM stuff) reset doesn't work on OVMF + SeaBIOS CSM
(removing edk2-devel, adding Jan) On 02/15/13 08:19, Michael Tokarev wrote: 15.02.2013 07:43, Kevin O'Connor wrote: On Fri, Feb 15, 2013 at 04:10:59AM +0100, Laszlo Ersek wrote: On 02/15/13 02:22, Kevin O'Connor wrote: On Thu, Feb 14, 2013 at 08:16:02PM -0500, Kevin O'Connor wrote: By chance, are you using an older version of kvm? There was a bug in kvm that caused changes to memory mapped at 0xe-0xf to also be reflected in the rom image at 0xfffe-0x. It was my understand that this bug was fixed though. You are great! Disabling KVM for the guest (/domain/@type='qemu') made the reboot work on both the RHEL-6 devel version of qemu and on upstream 1.3.1. (I didn't try suspend/resume yet.) Do you recall the precise commit that fixed the reflection? I've been eyeballing kvm commit messages for a few ten minutes now, but of course in vain. (CC'ing Gleb and Marcelo.) I found this email thread: http://kerneltrap.org/mailarchive/linux-kvm/2010/9/21/6267744 and: http://marc.info/?l=kvm-commitsm=128576215909532 I confirm RHEL-6 qemu-kvm lacks that patch; we still have the FIXME and the return statement that depend on kvm_enabled() in i440fx_update_memory_mappings(). This patch is more than 2 years old and is applied to all more or less recent qemu versions. This does not tell us why disabling kvm (with this patch applied!) makes a difference. I just retested on v1.3.1 + kvm, the problem is still there indeed. (Note that neither Gleb's patch, aa85bd8b support piix PAM registers in KVM, nor the patch that it partially undid: commit d03f4d2defd76f35f46f5418979f3e6d14a11183 Author: Jan Kiszka jan.kis...@web.de Date: Wed Sep 10 21:34:44 2008 +0200 I440fx: do change ISA mappings under KVM As long as KVM does not support remapping or protection state changes of guest memory, do not fiddle with the ISA mappings that QEMU see, confusing both the monitor and the gdbstub. Signed-off-by: Jan Kiszka jan.kis...@web.de Signed-off-by: Avi Kivity a...@qumranet.com made it ever to qemu; these are qemu-kvm commits.) So there must be another (maybe similar) bug somewhere... Maybe there was a concurrent or slightly earlier change to KVM that enabled the userspace fix too?... IOW the KVM fix could be necessary but not sufficient, the KVM fix + the qemu-kvm fix together are sufficient. If I disable KVM, i440fx_update_memory_mappings() probably does the same thing in RHEL-6 qemu-kvm as in upstream qemu v1.3.1. If I enable KVM, then RHEL-6 qemu-kvm breaks immediately in userspace, while upstream 1.3.1 might want to rely on KVM, but runs into a bug (?) on the RHEL-6 host kernel. Thanks, Laszlo
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
Am 15.02.2013 12:45, schrieb Peter Maydell: Don't map the pmem and dmem RAM memory regions in the milkymist-softusb device itself. Instead just expose them as sysbus mmio regions which the device creator can map appropriately. This allows us to drop the pmem_base and dmem_base properties. Instead of going via cpu_physical_memory_read/_write when the device wants to access the RAMs, we just keep a host pointer to the memory and use that. Signed-off-by: Peter Maydell peter.mayd...@linaro.org Reviewed-by: Andreas Färber afaer...@suse.de Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 5/5] sysbus: Remove sysbus_add_memory and sysbus_del_memory
Am 15.02.2013 12:45, schrieb Peter Maydell: Remove the sysbus_add_memory and sysbus_del_memory functions. These are trivial wrappers for mapping a memory region into the system memory space, and have no users now. Sysbus devices should never map their own memory regions anyway; the correct API for mapping an mmio region is for the creator of the device to use sysbus_mmio_map. Signed-off-by: Peter Maydell peter.mayd...@linaro.org Reviewed-by: Andreas Färber afaer...@suse.de Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCHv2] tap: set IFF_ONE_QUEUE per default
On Fri, Feb 15, 2013 at 10:32:31AM +0100, Peter Lieven wrote: historically the kernel queues packets two times. once at the device and second in qdisc. this is believed to cause interface stalls if one of these queues overruns. setting IFF_ONE_QUEUE is the default in kernels = 3.8. the flag is ignored since then. see kernel commit 5d097109257c03a71845729f8db6b5770c4bbedc v2: - do only set the flag on linux as it breaks macvtap - define IFF_ONE_QUEUE in tap-linux.h Signed-off-by: Peter Lieven p...@kamp.de --- net/tap-linux.c |4 net/tap-linux.h |1 + 2 files changed, 5 insertions(+) diff --git a/net/tap-linux.c b/net/tap-linux.c index a953189..d49f2fd 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -51,6 +51,10 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, memset(ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; +#ifdef __linux__ +ifr.ifr_flags |= IFF_ONE_QUEUE; +#endif tap-linux.c --- notice the filename Perhaps the solution is to try with IFF_ONE_QUEUE. If the result is -EINVAL, try without. Stefan
Re: [Qemu-devel] QOM: stability expectations of introspectable class_init data
On Fri, 15 Feb 2013 10:50:16 -0200 Eduardo Habkost ehabk...@redhat.com wrote: Hi, This is an attempt to summarize my main question from the thread: Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses My main unanswered question is about the stability expectations of the introspectable class data (especially property defaults). I am assuming and expecting that the introspectable QOM class data (especially property defaults) should simply reflect capabilities/behavior of the QEMU binary being queried, and would not change depending on the environment QEMU is running (host hardware and host kernel). This way, other components can use class introspection to probe for QEMU capabilities/behavior, and safely expect that the QEMU binary being queried will always have those capabilities/behavior. What Igor is proposing is to break my assumption, and make the default value of the vendor property on the X86CPU subclasses be different depending on the host CPU where QEMU is running. i.e. reflecting actual value of CPUID.vendor of the host. alternative proposed by Eduardo: is to abstract default value of vendor property to host string. My question is: is that really OK? In another case, we are considering making other properties of a X86CPU subclass have different defaults depending on the capabilities of the host kernel (the host CPU class will have different feature property defaults depending on the capabilities reported by GET_SUPPORTED_CPUID). Would that be OK, too?
Re: [Qemu-devel] [RFC PATCH v2 18/23] qcow2: Delay the COW
On Wed, Feb 13, 2013 at 02:22:08PM +0100, Kevin Wolf wrote: /** + * true if the request is sleeping in the COW delay and the coroutine may + * be reentered in order to cancel the timer. + */ +bool sleeping; Does reentering actually cancel the timer...or does it lead to a spurious entry when the timer fires in the future? Do we need anything to really delete the timer in case we re-enter and terminate the coroutine before the timer fires? Stefan
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Am 15.02.2013 14:38, schrieb Peter Maydell:
On 15 February 2013 13:11, Andreas Färber afaer...@suse.de wrote:
Am 15.02.2013 12:45, schrieb Peter Maydell:
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
#define MP_BOARD_REVISION 0x31
+typedef struct {
Anonymous struct
No, it's a typedef. Why would you want to name the struct
particularly when it's an error to then use that name rather
than the typedef? Better to let the compiler make uses of
'struct Foo' rather than 'Foo' a compile failure.
I'm pretty sure it has been requested by Blue and Anthony in the past...
Not sure if it makes a difference for gdb or something.
+SysBusDevice busdev;
parent_obj please. :)
+MemoryRegion iomem;
+} MusicPalMiscState;
+
+typedef SysBusDeviceClass MusicPalMiscClass;
Please don't. Either define a struct and use it for .class_size or drop
the typedef.
So my rationale here was all classes should have a FooClass.
If you have classes which don't have a FooClass then if at
some later point you need to introduce a class struct you
have to go round and locate all the places you wrote
ParentClass and now need to change it to FooClass. If
we always have a typedef everywhere then there is never
a problem.
More generally, I think we should prefer to avoid the kind of
shortcut that the C implementation allows us to take. If we
define a QOM class then that should mean you get the full range
of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and
a FooClass type).
If you prefer we could standardize on
typedef struct {
ParentClass parent;
} FooClass;
rather than typedef ParentClass FooClass;
+
+#define TYPE_MUSICPAL_MISC musicpal-misc
+#define MUSICPAL_MISC(obj) \
+ OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_CLASS(klass) \
+ OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC)
If we don't have such a class so you can just drop these two.
SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed.
Again, this will then require rework if we ever actually need
to put anything in the currently (conceptually) empty class struct.
It may need rework either way. Because aliasing via typedef gives
FooClass fields it will loose once it is turned into a real QOM class.
We had such an issue with i440fx in my PHB series, that's why I'm
sensitive to it. ;)
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 13:11, Andreas Färber afaer...@suse.de wrote:
Am 15.02.2013 12:45, schrieb Peter Maydell:
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
#define MP_BOARD_REVISION 0x31
+typedef struct {
Anonymous struct
No, it's a typedef. Why would you want to name the struct
particularly when it's an error to then use that name rather
than the typedef? Better to let the compiler make uses of
'struct Foo' rather than 'Foo' a compile failure.
+SysBusDevice busdev;
parent_obj please. :)
+MemoryRegion iomem;
+} MusicPalMiscState;
+
+typedef SysBusDeviceClass MusicPalMiscClass;
Please don't. Either define a struct and use it for .class_size or drop
the typedef.
So my rationale here was all classes should have a FooClass.
If you have classes which don't have a FooClass then if at
some later point you need to introduce a class struct you
have to go round and locate all the places you wrote
ParentClass and now need to change it to FooClass. If
we always have a typedef everywhere then there is never
a problem.
More generally, I think we should prefer to avoid the kind of
shortcut that the C implementation allows us to take. If we
define a QOM class then that should mean you get the full range
of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and
a FooClass type).
If you prefer we could standardize on
typedef struct {
ParentClass parent;
} FooClass;
rather than typedef ParentClass FooClass;
+
+#define TYPE_MUSICPAL_MISC musicpal-misc
+#define MUSICPAL_MISC(obj) \
+ OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_CLASS(klass) \
+ OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC)
+#define MUSICPAL_MISC_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC)
If we don't have such a class so you can just drop these two.
SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed.
Again, this will then require rework if we ever actually need
to put anything in the currently (conceptually) empty class struct.
-- PMM
Re: [Qemu-devel] QOM: stability expectations of introspectable class_init data
On Fri, Feb 15, 2013 at 02:35:26PM +0100, Igor Mammedov wrote: On Fri, 15 Feb 2013 10:50:16 -0200 Eduardo Habkost ehabk...@redhat.com wrote: Hi, This is an attempt to summarize my main question from the thread: Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses My main unanswered question is about the stability expectations of the introspectable class data (especially property defaults). I am assuming and expecting that the introspectable QOM class data (especially property defaults) should simply reflect capabilities/behavior of the QEMU binary being queried, and would not change depending on the environment QEMU is running (host hardware and host kernel). This way, other components can use class introspection to probe for QEMU capabilities/behavior, and safely expect that the QEMU binary being queried will always have those capabilities/behavior. What Igor is proposing is to break my assumption, and make the default value of the vendor property on the X86CPU subclasses be different depending on the host CPU where QEMU is running. i.e. reflecting actual value of CPUID.vendor of the host. alternative proposed by Eduardo: is to abstract default value of vendor property to host string. Exactly. Just to be clear: the actual CPUID vendor exposed to the guest _will_ change depending on the host CPU because we need to keep the behavior compatible with previous QEMU versions (that already do that). My question is about how to model that behavior in QOM. My question is: is that really OK? In another case, we are considering making other properties of a X86CPU subclass have different defaults depending on the capabilities of the host kernel (the host CPU class will have different feature property defaults depending on the capabilities reported by GET_SUPPORTED_CPUID). Would that be OK, too? -- Eduardo
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 13:45, Andreas Färber afaer...@suse.de wrote:
Am 15.02.2013 14:38, schrieb Peter Maydell:
On 15 February 2013 13:11, Andreas Färber afaer...@suse.de wrote:
Am 15.02.2013 12:45, schrieb Peter Maydell:
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
#define MP_BOARD_REVISION 0x31
+typedef struct {
Anonymous struct
No, it's a typedef. Why would you want to name the struct
particularly when it's an error to then use that name rather
than the typedef? Better to let the compiler make uses of
'struct Foo' rather than 'Foo' a compile failure.
I'm pretty sure it has been requested by Blue and Anthony in the past...
Not sure if it makes a difference for gdb or something.
I've cc'd them. But unless somebody has an actual good reason
for giving the struct in the typedef a completely unnecessary
name I think leaving it unnamed is better.
(This is distinct from genuinely anonymous structs with no
'struct foo' name or typedef name, which we do have a few of
in the codebase. I agree those are better avoided.)
+typedef SysBusDeviceClass MusicPalMiscClass;
Please don't. Either define a struct and use it for .class_size or drop
the typedef.
So my rationale here was all classes should have a FooClass.
If you have classes which don't have a FooClass then if at
some later point you need to introduce a class struct you
have to go round and locate all the places you wrote
ParentClass and now need to change it to FooClass. If
we always have a typedef everywhere then there is never
a problem.
More generally, I think we should prefer to avoid the kind of
shortcut that the C implementation allows us to take. If we
define a QOM class then that should mean you get the full range
of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and
a FooClass type).
If you prefer we could standardize on
typedef struct {
ParentClass parent;
} FooClass;
rather than typedef ParentClass FooClass;
It may need rework either way. Because aliasing via typedef gives
FooClass fields it will loose once it is turned into a real QOM class.
We had such an issue with i440fx in my PHB series, that's why I'm
sensitive to it. ;)
OK, so that seems like an argument for always defining the
empty-except-for-the-parentclass class struct, or does that
run into problems too?
-- PMM
Re: [Qemu-devel] [PATCH for-next v3 2/5] tmp105: Add debug output
On Fri, Feb 15, 2013 at 10:50:16AM +0100, Andreas Färber wrote:
Am 15.02.2013 10:06, schrieb Stefan Hajnoczi:
In iPXE they use a clever compile-time debug macro:
https://git.ipxe.org/ipxe.git/blob/HEAD:/src/include/compiler.h#l204
Basically you do DBG(hello world\n) and it gets compiled out by
default using:
if (DBG_LOG) {
printf(hello world\n);
}
Here's the really cool part, debugging can be toggled on a per-object
file basis at compile-time:
make DEBUG=e1000 # build QEMU with debug printfs only in e1000.o
The iPXE implementation is fancier than this. It supports different log
levels, hex dumping, MD5 hashing, etc.
But the core idea is:
1. Debug printfs are always if (0 or 1) { printf(...); } so that the
compiler syntax checks them - no more bitrot in debug printfs.
2. A single debug.h header included from qemu-common.h with Makefile
support that lets you say make DEBUG=e1000,rtl8139,net.
It would be a big step up from the mess we have today.
Thanks for that feedback. If you look at the previous discussion, that's
part of what I did in my RFC, and it was rejected in favor of keeping
#ifdef'able defines. Using inline functions to avoid some nasty
macro-is-not-function issues, there seemed to be no need to combine both
approaches due to the format string being checked one level above.
Redefining debug functions in every file is nasty. I am also not a fan
of modifying a #define, it always need to be reverted before committing
changes.
Personally, I think we should use tracing instead of debug printfs
though. Tracing allows you to use not just fprintf(stderr) but also
DTrace, if you like. You can mark debug trace events disabled in
./trace-events so they will not be compiled in by default - zero
performance overhead.
This series or patch is not against tracing. It might be an option to
use them for tmp105. But not being the author of the targets and all of
the devices my CPU refactorings potentially touch, it is infeasable for
me to determine an appropriate set of tracepoints everywhere. We'd also
need to decide whether we want per-target tracepoints or per-aspect
tracepoints, since it's a global list. Independent of that question,
some kind of include mechanism (like for the default-configs) would be
nice to decouple adding tracepoints in different areas.
Not sure I understand. You don't need to come up with new trace events
in code you didn't write. DPRINTF() can be converted mechanically to
trace_foo(arg1, arg). Then we get rid of all the DPRINTF() definitions.
The ./trace-events list is informal and can change at any time. We
don't need to coordinate between different subsystems or targets in
QEMU.
Stefan
Re: [Qemu-devel] [RFC PATCH v2 18/23] qcow2: Delay the COW
On Fri, Feb 15, 2013 at 02:36:37PM +0100, Stefan Hajnoczi wrote: On Wed, Feb 13, 2013 at 02:22:08PM +0100, Kevin Wolf wrote: /** + * true if the request is sleeping in the COW delay and the coroutine may + * be reentered in order to cancel the timer. + */ +bool sleeping; Does reentering actually cancel the timer...or does it lead to a spurious entry when the timer fires in the future? Do we need anything to really delete the timer in case we re-enter and terminate the coroutine before the timer fires? co_sleep_ns() supports this since commit 3ed99025, it cancels and deletes the timer. Block jobs use the same thing when you cancel them. Kevin
[Qemu-devel] [PATCH 1/2] target-arm: Factor out handling of SRS instruction
Factor out the handling of the SRS instruction rather than
duplicating it between the Thumb and ARM decoders. This in
passing fixes two bugs in the Thumb decoder's SRS handling
which didn't exist in the ARM decoder:
* (LP:1079080) storing CPSR rather than SPSR (fixed in the
ARM decoder in commit c67b6b71 in 2009)
* failing to free the 'addr' TCG temp in the writeback case
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
target-arm/translate.c | 136
1 file changed, 69 insertions(+), 67 deletions(-)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index a8893f7..62a4c15 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6561,6 +6561,70 @@ static void gen_store_exclusive(DisasContext *s, int rd,
int rt, int rt2,
}
#endif
+/* gen_srs:
+ * @env: CPUARMState
+ * @s: DisasContext
+ * @mode: mode field from insn (which stack to store to)
+ * @amode: addressing mode (DA/IA/DB/IB), encoded as per P,U bits in ARM insn
+ * @writeback: true if writeback bit set
+ *
+ * Generate code for the SRS (Store Return State) insn.
+ */
+static void gen_srs(DisasContext *s,
+uint32_t mode, uint32_t amode, bool writeback)
+{
+int32_t offset;
+TCGv_i32 addr = tcg_temp_new_i32();
+TCGv_i32 tmp = tcg_const_i32(mode);
+gen_helper_get_r13_banked(addr, cpu_env, tmp);
+tcg_temp_free_i32(tmp);
+switch (amode) {
+case 0: /* DA */
+offset = -4;
+break;
+case 1: /* IA */
+offset = 0;
+break;
+case 2: /* DB */
+offset = -8;
+break;
+case 3: /* IB */
+offset = 4;
+break;
+default:
+abort();
+}
+tcg_gen_addi_i32(addr, addr, offset);
+tmp = load_reg(s, 14);
+gen_st32(tmp, addr, 0);
+tmp = load_cpu_field(spsr);
+tcg_gen_addi_i32(addr, addr, 4);
+gen_st32(tmp, addr, 0);
+if (writeback) {
+switch (amode) {
+case 0:
+offset = -8;
+break;
+case 1:
+offset = 4;
+break;
+case 2:
+offset = -4;
+break;
+case 3:
+offset = 0;
+break;
+default:
+abort();
+}
+tcg_gen_addi_i32(addr, addr, offset);
+tmp = tcg_const_i32(mode);
+gen_helper_set_r13_banked(cpu_env, tmp, addr);
+tcg_temp_free_i32(tmp);
+}
+tcg_temp_free_i32(addr);
+}
+
static void disas_arm_insn(CPUARMState * env, DisasContext *s)
{
unsigned int cond, insn, val, op1, i, shift, rm, rs, rn, rd, sh;
@@ -6653,49 +6717,11 @@ static void disas_arm_insn(CPUARMState * env,
DisasContext *s)
}
} else if ((insn 0x0e5fffe0) == 0x084d0500) {
/* srs */
-int32_t offset;
-if (IS_USER(s))
+if (IS_USER(s)) {
goto illegal_op;
-ARCH(6);
-op1 = (insn 0x1f);
-addr = tcg_temp_new_i32();
-tmp = tcg_const_i32(op1);
-gen_helper_get_r13_banked(addr, cpu_env, tmp);
-tcg_temp_free_i32(tmp);
-i = (insn 23) 3;
-switch (i) {
-case 0: offset = -4; break; /* DA */
-case 1: offset = 0; break; /* IA */
-case 2: offset = -8; break; /* DB */
-case 3: offset = 4; break; /* IB */
-default: abort();
}
-if (offset)
-tcg_gen_addi_i32(addr, addr, offset);
-tmp = load_reg(s, 14);
-gen_st32(tmp, addr, 0);
-tmp = load_cpu_field(spsr);
-tcg_gen_addi_i32(addr, addr, 4);
-gen_st32(tmp, addr, 0);
-if (insn (1 21)) {
-/* Base writeback. */
-switch (i) {
-case 0: offset = -8; break;
-case 1: offset = 4; break;
-case 2: offset = -4; break;
-case 3: offset = 0; break;
-default: abort();
-}
-if (offset)
-tcg_gen_addi_i32(addr, addr, offset);
-tmp = tcg_const_i32(op1);
-gen_helper_set_r13_banked(cpu_env, tmp, addr);
-tcg_temp_free_i32(tmp);
-tcg_temp_free_i32(addr);
-} else {
-tcg_temp_free_i32(addr);
-}
-return;
+ARCH(6);
+gen_srs(s, (insn 0x1f), (insn 23) 3, insn (1 21));
} else if ((insn 0x0e50ffe0) == 0x08100a00) {
/* rfe */
int32_t offset;
@@ -8135,32 +8161,8 @@ static int disas_thumb2_insn(CPUARMState *env,
DisasContext *s, uint16_t insn_hw
gen_rfe(s, tmp, tmp2);
} else {
/* srs */
-op = (insn 0x1f);
-addr = tcg_temp_new_i32();
-tmp = tcg_const_i32(op);
-
[Qemu-devel] [PATCH 2/2] target-arm: Don't decode RFE or SRS on M profile cores
M profile cores do not have the RFE or SRS instructions, so
correctly UNDEF these insn patterns on those cores.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
target-arm/translate.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 62a4c15..9746869 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8135,9 +8135,10 @@ static int disas_thumb2_insn(CPUARMState *env,
DisasContext *s, uint16_t insn_hw
} else {
/* Load/store multiple, RFE, SRS. */
if (((insn 23) 1) == ((insn 24) 1)) {
-/* Not available in user mode. */
-if (IS_USER(s))
+/* RFE, SRS: not available in user mode or on M profile */
+if (IS_USER(s) || IS_M(env)) {
goto illegal_op;
+}
if (insn (1 20)) {
/* rfe */
addr = load_reg(s, rn);
--
1.7.9.5
[Qemu-devel] [PATCH 0/2] target-arm: fix srs bugs
This patchset fixes a couple of bugs in the handling of the SRS instruction in the Thumb decoder, by refactoring the code so that it is shared with the ARM decoder (where those bugs were fixed or never present). The most significant one is LP:1079080, where we were simply not storing the right thing at all (cpsr not spsr). I also noticed that we weren't UNDEFfing these insns on M profile, so patch 2 fixes that. (Yes, despite that fact I combined two near-identical bits of code into one, by the time I fixed the coding style and added a comment it comes out as adding more lines than it deletes...) [Insert srs lolcat joke of your choice here.] Peter Maydell (2): target-arm: Factor out handling of SRS instruction target-arm: Don't decode RFE or SRS on M profile cores target-arm/translate.c | 141 1 file changed, 72 insertions(+), 69 deletions(-) -- 1.7.9.5
Re: [Qemu-devel] [PATCH V22 1/7] Support for TPM command line options
On 02/14/2013 04:43 PM, Stefan Berger wrote:
This patch adds support for TPM command line options.
The command line options supported here are
./qemu-... -tpmdev passthrough,path=path to TPM device,id=id
-device tpm-tis,tpmdev=id
and
./qemu-... -tpmdev help
where the latter works similar to -soundhw ? and shows a list of
available TPM backends (for example 'passthrough').
Using the type parameter, the backend is chosen, i.e., 'passthrough' for the
passthrough driver. The interpretation of the other parameters along
with determining whether enough parameters were provided is pushed into
the backend driver, which needs to implement the interface function
'create' and return a TPMDriver structure if the VM can be started or 'NULL'
if not enough or bad parameters were provided.
Monitor support for 'info tpm' has been added. It for example prints the
following:
(qemu) info tpm
TPM devices:
tpm0: model=tpm-tis
\ tpm0: type=passthrough,path=/dev/tpm0
Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
---
Makefile.objs | 1 +
hmp-commands.hx | 2 +
hmp.c | 44 +
hmp.h | 1 +
include/tpm/tpm.h | 21 +
monitor.c | 8 ++
qapi-schema.json | 83 +
qemu-options.hx | 33 +++
qmp-commands.hx | 18
tpm/Makefile.objs | 1 +
tpm/tpm.c | 272 ++
tpm/tpm_int.h | 79
tpm/tpm_tis.h | 78
vl.c | 37
14 files changed, 678 insertions(+)
create mode 100644 include/tpm/tpm.h
create mode 100644 tpm/Makefile.objs
create mode 100644 tpm/tpm.c
create mode 100644 tpm/tpm_int.h
create mode 100644 tpm/tpm_tis.h
diff --git a/Makefile.objs b/Makefile.objs
index 21e9c91..d52ea49 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -74,6 +74,7 @@ common-obj-y += bt-host.o bt-vhci.o
common-obj-y += dma-helpers.o
common-obj-y += qtest.o
common-obj-y += vl.o
+common-obj-y += tpm/
common-obj-$(CONFIG_SLIRP) += slirp/
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 64008a9..a952fd1 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1642,6 +1642,8 @@ show device tree
show qdev device model list
@item info roms
show roms
+@item info tpm
+show the TPM device
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 2f47a8a..b0a861c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -607,6 +607,50 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
}
}
+void hmp_info_tpm(Monitor *mon, const QDict *qdict)
+{
+TPMInfoList *info_list, *info;
+Error *err = NULL;
+unsigned int c = 0;
+TPMPassthroughOptions *tpo;
+
+info_list = qmp_query_tpm(err);
+if (err) {
+monitor_printf(mon, TPM device not supported\n);
+error_free(err);
+return;
+}
+
+if (info_list) {
+monitor_printf(mon, TPM device:\n);
+}
+
+for (info = info_list; info; info = info-next) {
+TPMInfo *ti = info-value;
+monitor_printf(mon, tpm%d: model=%s\n,
+ c, TpmModel_lookup[ti-model]);
+
+monitor_printf(mon, \\ %s: type=%s,
+ ti-id, TpmType_lookup[ti-type]);
+
+switch (ti-tpm_options-kind) {
+case TPM_TYPE_OPTIONS_KIND_TPM_PASSTHROUGH_OPTIONS:
+tpo = ti-tpm_options-tpm_passthrough_options;
+monitor_printf(mon, %s%s%s%s,
+ tpo-has_path ? ,path= : ,
+ tpo-has_path ? tpo-path : ,
+ tpo-has_cancel_path ? ,cancel-path= : ,
+ tpo-has_cancel_path ? tpo-cancel_path : );
+break;
+case TPM_TYPE_OPTIONS_KIND_MAX:
+break;
+}
+monitor_printf(mon, \n);
+c++;
+}
+qapi_free_TPMInfoList(info_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 30b3c20..95fe76e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
+void hmp_info_tpm(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/include/tpm/tpm.h b/include/tpm/tpm.h
new file mode 100644
index 000..cc8f20e
--- /dev/null
+++ b/include/tpm/tpm.h
@@ -0,0 +1,21 @@
+/*
+ * Public TPM functions
+ *
+ * Copyright (C) 2011-2013 IBM Corporation
+ *
+ * Authors:
+ * Stefan Bergerstef...@us.ibm.com
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QEMU_TPM_H
+#define
[Qemu-devel] [PATCH qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()
Since we still need env for ppc-specific fields, obtain it via the new
env_ptr fields to avoid cpu name conflicts between CPUState and
PowerPCCPU for now.
This fixes a potential issue with env being NULL at the end of the loop
but cpu still being a valid pointer corresponding to a previous env.
Signed-off-by: Andreas Färber afaer...@suse.de
---
Alex, this is a rebased and slimmed-down version of yesterday's paste.
Can you ack please? Thanks, /-F
hw/ppc/e500.c | 11 +++
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b7474c0..451682c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -240,20 +240,15 @@ static int ppce500_load_device_tree(CPUPPCState *env,
/* We need to generate the cpu nodes in reverse order, so Linux can pick
the first node as boot node and be happy */
for (i = smp_cpus - 1; i = 0; i--) {
-CPUState *cpu = NULL;
+CPUState *cpu;
char cpu_name[128];
uint64_t cpu_release_addr = MPC8544_SPIN_BASE + (i * 0x20);
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = ENV_GET_CPU(env);
-if (cpu-cpu_index == i) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(i);
if (cpu == NULL) {
continue;
}
+env = cpu-env_ptr;
snprintf(cpu_name, sizeof(cpu_name), /cpus/PowerPC,8544@%x,
cpu-cpu_index);
--
1.7.10.4
Re: [Qemu-devel] [PATCHv2] tap: set IFF_ONE_QUEUE per default
Am 15.02.2013 um 14:32 schrieb Stefan Hajnoczi stefa...@gmail.com: On Fri, Feb 15, 2013 at 10:32:31AM +0100, Peter Lieven wrote: historically the kernel queues packets two times. once at the device and second in qdisc. this is believed to cause interface stalls if one of these queues overruns. setting IFF_ONE_QUEUE is the default in kernels = 3.8. the flag is ignored since then. see kernel commit 5d097109257c03a71845729f8db6b5770c4bbedc v2: - do only set the flag on linux as it breaks macvtap - define IFF_ONE_QUEUE in tap-linux.h Signed-off-by: Peter Lieven p...@kamp.de --- net/tap-linux.c |4 net/tap-linux.h |1 + 2 files changed, 5 insertions(+) diff --git a/net/tap-linux.c b/net/tap-linux.c index a953189..d49f2fd 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -51,6 +51,10 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, memset(ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; +#ifdef __linux__ +ifr.ifr_flags |= IFF_ONE_QUEUE; +#endif tap-linux.c --- notice the filename i know. is qemu really using it on MACs? Peter Perhaps the solution is to try with IFF_ONE_QUEUE. If the result is -EINVAL, try without. Stefan
Re: [Qemu-devel] [PATCH qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()
On 15.02.2013, at 15:25, Andreas Färber wrote:
Since we still need env for ppc-specific fields, obtain it via the new
env_ptr fields to avoid cpu name conflicts between CPUState and
PowerPCCPU for now.
This fixes a potential issue with env being NULL at the end of the loop
but cpu still being a valid pointer corresponding to a previous env.
Signed-off-by: Andreas Färber afaer...@suse.de
---
Alex, this is a rebased and slimmed-down version of yesterday's paste.
Can you ack please? Thanks, /-F
In your QOM conversion you also converted other places like this. Have you
verified that these don't suffer from the same problem?
Alex
hw/ppc/e500.c | 11 +++
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b7474c0..451682c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -240,20 +240,15 @@ static int ppce500_load_device_tree(CPUPPCState *env,
/* We need to generate the cpu nodes in reverse order, so Linux can pick
the first node as boot node and be happy */
for (i = smp_cpus - 1; i = 0; i--) {
-CPUState *cpu = NULL;
+CPUState *cpu;
char cpu_name[128];
uint64_t cpu_release_addr = MPC8544_SPIN_BASE + (i * 0x20);
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = ENV_GET_CPU(env);
-if (cpu-cpu_index == i) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(i);
if (cpu == NULL) {
continue;
}
+env = cpu-env_ptr;
snprintf(cpu_name, sizeof(cpu_name), /cpus/PowerPC,8544@%x,
cpu-cpu_index);
--
1.7.10.4
Re: [Qemu-devel] [PATCHv2] tap: set IFF_ONE_QUEUE per default
On 15/02/13 15:27, Peter Lieven wrote: Am 15.02.2013 um 14:32 schrieb Stefan Hajnoczi stefa...@gmail.com: On Fri, Feb 15, 2013 at 10:32:31AM +0100, Peter Lieven wrote: historically the kernel queues packets two times. once at the device and second in qdisc. this is believed to cause interface stalls if one of these queues overruns. setting IFF_ONE_QUEUE is the default in kernels = 3.8. the flag is ignored since then. see kernel commit 5d097109257c03a71845729f8db6b5770c4bbedc v2: - do only set the flag on linux as it breaks macvtap - define IFF_ONE_QUEUE in tap-linux.h Signed-off-by: Peter Lieven p...@kamp.de --- net/tap-linux.c |4 net/tap-linux.h |1 + 2 files changed, 5 insertions(+) diff --git a/net/tap-linux.c b/net/tap-linux.c index a953189..d49f2fd 100644 --- a/net/tap-linux.c +++ b/net/tap-linux.c @@ -51,6 +51,10 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr, memset(ifr, 0, sizeof(ifr)); ifr.ifr_flags = IFF_TAP | IFF_NO_PI; +#ifdef __linux__ +ifr.ifr_flags |= IFF_ONE_QUEUE; +#endif tap-linux.c --- notice the filename i know. is qemu really using it on MACs? It has nothing todo with Apple products. Its about a linux kernel driver that provides a tap like interface that can be attached to an network interface with MAC-address filtering. http://virt.kernelnewbies.org/MacVTap Christian
[Qemu-devel] [PATCH qom-cpu-next] cpus: Replace open-coded CPU loop in qmp_memsave() with qemu_get_cpu()
No functional change, just less usages of first_cpu and next_cpu fields.
Signed-off-by: Andreas Färber afaer...@suse.de
---
cpus.c | 11 +++
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/cpus.c b/cpus.c
index 41779eb..845e915 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1262,18 +1262,13 @@ void qmp_memsave(int64_t addr, int64_t size, const char
*filename,
cpu_index = 0;
}
-for (env = first_cpu; env; env = env-next_cpu) {
-cpu = ENV_GET_CPU(env);
-if (cpu_index == cpu-cpu_index) {
-break;
-}
-}
-
-if (env == NULL) {
+cpu = qemu_get_cpu(cpu_index);
+if (cpu == NULL) {
error_set(errp, QERR_INVALID_PARAMETER_VALUE, cpu-index,
a CPU number);
return;
}
+env = cpu-env_ptr;
f = fopen(filename, wb);
if (!f) {
--
1.7.10.4
Re: [Qemu-devel] QOM: stability expectations of introspectable class_init data
Eduardo Habkost ehabk...@redhat.com writes: Hi, This is an attempt to summarize my main question from the thread: Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses My main unanswered question is about the stability expectations of the introspectable class data (especially property defaults). I am assuming and expecting that the introspectable QOM class data (especially property defaults) should simply reflect capabilities/behavior of the QEMU binary being queried, and would not change depending on the environment QEMU is running (host hardware and host kernel). This way, other components can use class introspection to probe for QEMU capabilities/behavior, and safely expect that the QEMU binary being queried will always have those capabilities/behavior. What Igor is proposing is to break my assumption, and make the default value of the vendor property on the X86CPU subclasses be different depending on the host CPU where QEMU is running. My question is: is that really OK? The rules are: 1) We can add and remove types but we can never change the semantics of the type. If the 'serial' device is an ISA UART16650A, it must stay that. 2) We can add and remove properties but never change their semantics. Removing properties should be done in a limited fashion of course. 3) Whenever using a '-M' parameter, a given command line syntax must produce an identical system from the guest's point of view on a newer QEMU version when all other things are equal. So generally speaking, we can, and do, change default values but we also have to add compat properties to make sure of (3). CPU vendor is a special case. It's not reasonable to expose a general vendor string when using KVM. Hence the 'all other things are equal' clause. In another case, we are considering making other properties of a X86CPU subclass have different defaults depending on the capabilities of the host kernel (the host CPU class will have different feature property defaults depending on the capabilities reported by GET_SUPPORTED_CPUID). Would that be OK, too? Given 'all other things are equal', yes, it would be. FWIW, I agree with the suggestion elsewhere in the thread that instead of changing default values, an auto detect value is a more user friendly way of having this behavior. Regards, Anthony Liguori -- Eduardo
Re: [Qemu-devel] [PATCH V22 6/7] Add support for cancelling of a TPM command
On 02/14/2013 04:43 PM, Stefan Berger wrote:
This patch adds support for cancelling an executing TPM command.
In Linux for example a user can cancel a command through the TPM's
sysfs 'cancel' entry using
echo 1 /sysfs/class/misc/tpm0/device/cancel
This patch propagates the cancellation of a command inside a VM
to the host TPM's sysfs entry.
It also uses the possibility to cancel the command before QEMU VM
shutdown or reboot, which helps in preventing QEMU from hanging while
waiting for the completion of the command.
To relieve higher layers or users from having to determine the TPM's
cancel sysfs entry, the driver searches for the entry in well known
locations.
Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
---
qemu-options.hx | 13 +++-
tpm/tpm_passthrough.c | 166 ++
vl.c | 5 ++
3 files changed, 168 insertions(+), 16 deletions(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index ede6d94..410b7fa 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2193,8 +2193,10 @@ DEFHEADING()
DEFHEADING(TPM device options:)
DEF(tpmdev, HAS_ARG, QEMU_OPTION_tpmdev, \
--tpmdev passthrough,id=id[,path=path]\n
-use path to provide path to a character device; default is
/dev/tpm0\n,
+-tpmdev passthrough,id=id[,path=path][,cancel-path=path]\n
+use path to provide path to a character device; default is
/dev/tpm0\n
+use cancel-path to provide path to TPM's cancel sysfs entry;
if\n
+not provided it will be searched for in
/sys/class/misc/tpm?/device\n,
QEMU_ARCH_ALL)
STEXI
@@ -2216,7 +2218,7 @@ Use 'help' to print all available TPM backend types.
qemu -tpmdev help
@end example
-@item -tpmdev passthrough, id=@var{id}, path=@var{path}
+@item -tpmdev passthrough, id=@var{id}, path=@var{path}, path=@var{cancel-path}
(Linux-host only) Enable access to the host's TPM using the passthrough
driver.
@@ -2225,6 +2227,11 @@ driver.
a Linux host this would be @code{/dev/tpm0}.
@option{path} is optional and by default @code{/dev/tpm0} is used.
+@option{cancel-path} specifies the path to the host TPM device's sysfs
+entry allowing for cancellation of an ongoing TPM command.
+@option{cancel-path} is optional and by default QEMU will search for the
+sysfs entry to use.
+
Some notes about using the host's TPM with the passthrough driver:
The TPM device accessed by the passthrough driver must not be
diff --git a/tpm/tpm_passthrough.c b/tpm/tpm_passthrough.c
index 7d5de8e..300575b 100644
--- a/tpm/tpm_passthrough.c
+++ b/tpm/tpm_passthrough.c
@@ -22,6 +22,8 @@
* License along with this library; if not, see http://www.gnu.org/licenses/
*/
+#include dirent.h
+
#include qemu-common.h
#include qapi/error.h
#include qemu/sockets.h
@@ -57,11 +59,18 @@ struct TPMPassthruState {
char *tpm_dev;
int tpm_fd;
+bool tpm_executing;
+bool tpm_op_canceled;
+int cancel_fd;
bool had_startup_error;
};
#define TPM_PASSTHROUGH_DEFAULT_DEVICE /dev/tpm0
+/* functions */
+
+static void tpm_passthrough_cancel_cmd(TPMBackend *tb);
+
static int tpm_passthrough_unix_write(int fd, const uint8_t *buf, uint32_t
len)
{
return send_all(fd, buf, len);
@@ -79,25 +88,36 @@ static uint32_t tpm_passthrough_get_size_from_buffer(const
uint8_t *buf)
return be32_to_cpu(resp-len);
}
-static int tpm_passthrough_unix_tx_bufs(int tpm_fd,
+static int tpm_passthrough_unix_tx_bufs(TPMPassthruState *tpm_pt,
const uint8_t *in, uint32_t in_len,
uint8_t *out, uint32_t out_len)
{
int ret;
-ret = tpm_passthrough_unix_write(tpm_fd, in, in_len);
+tpm_pt-tpm_op_canceled = false;
+tpm_pt-tpm_executing = true;
+
+ret = tpm_passthrough_unix_write(tpm_pt-tpm_fd, in, in_len);
if (ret != in_len) {
-error_report(tpm_passthrough: error while transmitting data
- to TPM: %s (%i)\n,
- strerror(errno), errno);
+if (!tpm_pt-tpm_op_canceled ||
+(tpm_pt-tpm_op_canceled errno != ECANCELED)) {
+error_report(tpm_passthrough: error while transmitting data
+ to TPM: %s (%i)\n,
+ strerror(errno), errno);
+}
goto err_exit;
}
-ret = tpm_passthrough_unix_read(tpm_fd, out, out_len);
+tpm_pt-tpm_executing = false;
+
+ret = tpm_passthrough_unix_read(tpm_pt-tpm_fd, out, out_len);
if (ret 0) {
-error_report(tpm_passthrough: error while reading data from
- TPM: %s (%i)\n,
- strerror(errno), errno);
+if (!tpm_pt-tpm_op_canceled ||
+(tpm_pt-tpm_op_canceled errno != ECANCELED)) {
+error_report(tpm_passthrough: error while reading data from
+ TPM: %s
Re: [Qemu-devel] [PATCH V22 1/7] Support for TPM command line options
On 02/14/2013 04:43 PM, Stefan Berger wrote: +/* + * Parse the TPM configuration options. + * To display all available TPM backends the user may use '-tpmdev ?' This comment is out of date. + */ +int tpm_config_parse(QemuOptsList *opts_list, const char *optarg) -- Regards, Corey Bryant
Re: [Qemu-devel] [PATCH qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()
Am 15.02.2013 15:29, schrieb Alexander Graf: On 15.02.2013, at 15:25, Andreas Färber wrote: Since we still need env for ppc-specific fields, obtain it via the new env_ptr fields to avoid cpu name conflicts between CPUState and PowerPCCPU for now. This fixes a potential issue with env being NULL at the end of the loop but cpu still being a valid pointer corresponding to a previous env. Signed-off-by: Andreas Färber afaer...@suse.de --- Alex, this is a rebased and slimmed-down version of yesterday's paste. Can you ack please? Thanks, /-F In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem? Not yet. Not all callers are using this pattern though, so solutions would differ and be per-case patches. At some point I obviously intend to change first_cpu and -next_cpu to CPUState and have been wondering whether we can use QTAILQ macros or anything else more standardized, with better iteration support. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCHv2] iscsi: add iscsi_truncate support
this patch adds iscsi_truncate which effectively allows for online
resizing of iscsi volumes. for this to work you have to resize
the volume on your storage and then call block_resize command
in qemu which will issue a readcapacity16 to update the capacity.
v2:
- add a general bdrv_drain_all() before bdrv_truncate() to avoid
in-flight AIOs while the device is truncated
- since no AIOs are in flight we can use a sync libiscsi call
to re-read the capacity
- factor out the readcapacity16 logic as it is redundant
to iscsi_open() and iscsi_truncate().
Signed-off-by: Peter Lieven p...@kamp.de
---
block.c |4
block/iscsi.c | 65 +
2 files changed, 56 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index 50dab8e..d8880e3 100644
--- a/block.c
+++ b/block.c
@@ -2427,6 +2427,10 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
return -EACCES;
if (bdrv_in_use(bs))
return -EBUSY;
+
+/* there should be better no AIOs in flight when we truncate the device */
+bdrv_drain_all();
+
ret = drv-bdrv_truncate(bs, offset);
if (ret == 0) {
ret = refresh_total_sectors(bs, offset BDRV_SECTOR_BITS);
diff --git a/block/iscsi.c b/block/iscsi.c
index deb3b68..dd41943 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -823,6 +823,35 @@ static void iscsi_nop_timed_event(void *opaque)
}
#endif
+static int iscsi_disk_readcapacity16_sync(IscsiLun *iscsilun) {
+struct scsi_task *task = NULL;
+struct scsi_readcapacity16 *rc16 = NULL;
+
+task = iscsi_readcapacity16_sync(iscsilun-iscsi, iscsilun-lun);
+if (task == NULL) {
+error_report(iSCSI: failed to send readcapacity16 command.);
+return -EINVAL;
+}
+if (task-status != SCSI_STATUS_GOOD) {
+error_report(iSCSI: failed to send readcapacity16 command.);
+scsi_free_scsi_task(task);
+return -EINVAL;
+}
+rc16 = scsi_datain_unmarshall(task);
+if (rc16 == NULL) {
+error_report(iSCSI: Failed to unmarshall readcapacity16 data.);
+scsi_free_scsi_task(task);
+return -EINVAL;
+}
+
+iscsilun-block_size = rc16-block_length;
+iscsilun-num_blocks = rc16-returned_lba + 1;
+
+scsi_free_scsi_task(task);
+
+return 0;
+}
+
/*
* We support iscsi url's on the form
* iscsi://[username%password@]host[:port]/targetname/lun
@@ -835,7 +864,6 @@ static int iscsi_open(BlockDriverState *bs, const char
*filename, int flags)
struct scsi_task *task = NULL;
struct scsi_inquiry_standard *inq = NULL;
struct scsi_readcapacity10 *rc10 = NULL;
-struct scsi_readcapacity16 *rc16 = NULL;
char *initiator_name = NULL;
int ret;
@@ -926,23 +954,13 @@ static int iscsi_open(BlockDriverState *bs, const char
*filename, int flags)
iscsilun-type = inq-periperal_device_type;
scsi_free_scsi_task(task);
+task = NULL;
switch (iscsilun-type) {
case TYPE_DISK:
-task = iscsi_readcapacity16_sync(iscsi, iscsilun-lun);
-if (task == NULL || task-status != SCSI_STATUS_GOOD) {
-error_report(iSCSI: failed to send readcapacity16 command.);
-ret = -EINVAL;
+if ((ret = iscsi_disk_readcapacity16_sync(iscsilun))) {
goto out;
}
-rc16 = scsi_datain_unmarshall(task);
-if (rc16 == NULL) {
-error_report(iSCSI: Failed to unmarshall readcapacity16 data.);
-ret = -EINVAL;
-goto out;
-}
-iscsilun-block_size = rc16-block_length;
-iscsilun-num_blocks = rc16-returned_lba + 1;
break;
case TYPE_ROM:
task = iscsi_readcapacity10_sync(iscsi, iscsilun-lun, 0, 0);
@@ -1023,6 +1041,26 @@ static void iscsi_close(BlockDriverState *bs)
memset(iscsilun, 0, sizeof(IscsiLun));
}
+static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
+{
+IscsiLun *iscsilun = bs-opaque;
+int ret = 0;
+
+if (iscsilun-type != TYPE_DISK) {
+return -ENOTSUP;
+}
+
+if ((ret = iscsi_disk_readcapacity16_sync(iscsilun))) {
+return ret;
+}
+
+if (offset iscsi_getlength(bs)) {
+return -EINVAL;
+}
+
+return 0;
+}
+
static int iscsi_has_zero_init(BlockDriverState *bs)
{
return 0;
@@ -1093,6 +1131,7 @@ static BlockDriver bdrv_iscsi = {
.create_options = iscsi_create_options,
.bdrv_getlength = iscsi_getlength,
+.bdrv_truncate = iscsi_truncate,
.bdrv_aio_readv = iscsi_aio_readv,
.bdrv_aio_writev = iscsi_aio_writev,
--
1.7.9.5
Re: [Qemu-devel] QOM: stability expectations of introspectable class_init data
On Fri, Feb 15, 2013 at 08:22:31AM -0600, Anthony Liguori wrote: Eduardo Habkost ehabk...@redhat.com writes: Hi, This is an attempt to summarize my main question from the thread: Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses My main unanswered question is about the stability expectations of the introspectable class data (especially property defaults). I am assuming and expecting that the introspectable QOM class data (especially property defaults) should simply reflect capabilities/behavior of the QEMU binary being queried, and would not change depending on the environment QEMU is running (host hardware and host kernel). This way, other components can use class introspection to probe for QEMU capabilities/behavior, and safely expect that the QEMU binary being queried will always have those capabilities/behavior. What Igor is proposing is to break my assumption, and make the default value of the vendor property on the X86CPU subclasses be different depending on the host CPU where QEMU is running. My question is: is that really OK? The rules are: 1) We can add and remove types but we can never change the semantics of the type. If the 'serial' device is an ISA UART16650A, it must stay that. 2) We can add and remove properties but never change their semantics. Removing properties should be done in a limited fashion of course. 3) Whenever using a '-M' parameter, a given command line syntax must produce an identical system from the guest's point of view on a newer QEMU version when all other things are equal. So generally speaking, we can, and do, change default values but we also have to add compat properties to make sure of (3). OK, thanks for the explanation. CPU vendor is a special case. It's not reasonable to expose a general vendor string when using KVM. Hence the 'all other things are equal' clause. True, CPU vendor is special. And the tricky part is the all other things are equal clause. e.g.: we try to keep all guest-visible features the same when using -M even if the host CPU or the host kernel changes. So the host kernel and the host CPU aren't included in the all other things are equal clause, but the host CPU vendor is. In other words: we don't support cross-CPU-vendor migration out of the box (and if somebody wants to try to make it work, it will require an explicit CPu vendor to be always set on the command-line). In another case, we are considering making other properties of a X86CPU subclass have different defaults depending on the capabilities of the host kernel (the host CPU class will have different feature property defaults depending on the capabilities reported by GET_SUPPORTED_CPUID). Would that be OK, too? Given 'all other things are equal', yes, it would be. -cpu host is known and expected to _really_ stretch the all other things are equal part of rule (3). Other CPU models are kept compatible even if the host CPU or host kernel changes (as long as the host CPU has the same vendor). host keeps rule (3) if and only if the host kernel CPUID capabilities and the host CPU are exactly the same. I would go further and simply declare -cpu host as not following rule (3) at all, instead of trying to define the specific conditions where it would follow it. FWIW, I agree with the suggestion elsewhere in the thread that instead of changing default values, an auto detect value is a more user friendly way of having this behavior. Are you talking about the suggestion of having vendor default to host, and having instance_init translate vendor=host to the host CPU vendor? My main argument for it was the possibility of not being allowed to change the defaults based on host CPU. But if we are allowed to change the default, do we still want to use the vendor=host solution? (My opinion is now divided, but I am inclined towards it). -- Eduardo
Re: [Qemu-devel] [PATCH qom-cpu-next v5] target-i386: Split command line parsing out of cpu_x86_register()
On Fri, Feb 15, 2013 at 02:06:56PM +0100, Igor Mammedov wrote:
From: Andreas Färber afaer...@suse.de
In order to instantiate a CPU subtype we will need to know which type,
so move the cpu_model splitting into cpu_x86_init().
Parameters need to be set on the X86CPU instance, so move
cpu_x86_parse_featurestr() into cpu_x86_init() as well.
This leaves cpu_x86_register() operating on the model name only.
Signed-off-by: Andreas Färber afaer...@suse.de
Signed-off-by: Igor Mammedov imamm...@redhat.com
Reviewed-by: Eduardo Habkost ehabk...@redhat.com
---
v5:
* get error to report from cpu_x86_register()
v4:
* consolidate resource cleanup in when leaving cpu_x86_init(),
to avoid clean code duplication.
* remove unnecessary error message from hw/pc.c
---
hw/pc.c |1 -
target-i386/cpu.c | 80 ++--
2 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 53cc173..07caba7 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -876,7 +876,6 @@ void pc_cpus_init(const char *cpu_model)
for (i = 0; i smp_cpus; i++) {
if (!cpu_x86_init(cpu_model)) {
-fprintf(stderr, Unable to find x86 CPU definition\n);
exit(1);
}
}
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index dcbed0d..dadb0f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1516,27 +1516,16 @@ static void filter_features_for_kvm(X86CPU *cpu)
}
#endif
-static int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp)
{
CPUX86State *env = cpu-env;
x86_def_t def1, *def = def1;
-Error *error = NULL;
-char *name, *features;
-gchar **model_pieces;
memset(def, 0, sizeof(*def));
-model_pieces = g_strsplit(cpu_model, ,, 2);
-if (!model_pieces[0]) {
-error_setg(error, Invalid/empty CPU model name);
-goto out;
-}
-name = model_pieces[0];
-features = model_pieces[1];
-
if (cpu_x86_find_by_name(def, name) 0) {
-error_setg(error, Unable to find CPU definition: %s, name);
-goto out;
+error_setg(errp, Unable to find CPU definition: %s, name);
+return;
}
if (kvm_enabled()) {
@@ -1544,58 +1533,69 @@ static int cpu_x86_register(X86CPU *cpu, const char
*cpu_model)
}
def-ext_features |= CPUID_EXT_HYPERVISOR;
-object_property_set_str(OBJECT(cpu), def-vendor, vendor, error);
-object_property_set_int(OBJECT(cpu), def-level, level, error);
-object_property_set_int(OBJECT(cpu), def-family, family, error);
-object_property_set_int(OBJECT(cpu), def-model, model, error);
-object_property_set_int(OBJECT(cpu), def-stepping, stepping, error);
+object_property_set_str(OBJECT(cpu), def-vendor, vendor, errp);
+object_property_set_int(OBJECT(cpu), def-level, level, errp);
+object_property_set_int(OBJECT(cpu), def-family, family, errp);
+object_property_set_int(OBJECT(cpu), def-model, model, errp);
+object_property_set_int(OBJECT(cpu), def-stepping, stepping, errp);
env-cpuid_features = def-features;
env-cpuid_ext_features = def-ext_features;
env-cpuid_ext2_features = def-ext2_features;
env-cpuid_ext3_features = def-ext3_features;
-object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, error);
+object_property_set_int(OBJECT(cpu), def-xlevel, xlevel, errp);
env-cpuid_kvm_features = def-kvm_features;
env-cpuid_svm_features = def-svm_features;
env-cpuid_ext4_features = def-ext4_features;
env-cpuid_7_0_ebx_features = def-cpuid_7_0_ebx_features;
env-cpuid_xlevel2 = def-xlevel2;
-object_property_set_str(OBJECT(cpu), def-model_id, model-id, error);
-if (error) {
-goto out;
-}
-
-cpu_x86_parse_featurestr(cpu, features, error);
-out:
-g_strfreev(model_pieces);
-if (error) {
-fprintf(stderr, %s\n, error_get_pretty(error));
-error_free(error);
-return -1;
-}
-return 0;
+object_property_set_str(OBJECT(cpu), def-model_id, model-id, errp);
}
X86CPU *cpu_x86_init(const char *cpu_model)
{
-X86CPU *cpu;
+X86CPU *cpu = NULL;
CPUX86State *env;
+gchar **model_pieces;
+char *name, *features;
Error *error = NULL;
+model_pieces = g_strsplit(cpu_model, ,, 2);
+if (!model_pieces[0]) {
+error_setg(error, Invalid/empty CPU model name);
+goto out;
+}
+name = model_pieces[0];
+features = model_pieces[1];
+
cpu = X86_CPU(object_new(TYPE_X86_CPU));
env = cpu-env;
env-cpu_model_str = cpu_model;
-if (cpu_x86_register(cpu, cpu_model) 0) {
-object_unref(OBJECT(cpu));
-return NULL;
+cpu_x86_register(cpu, name, error);
+if (error) {
+goto out;
[Qemu-devel] [Bug 1079080] Re: ARM instruction srs wrong behaviour
Thanks -- I've submitted a patch which fixes this: http://patchwork.ozlabs.org/patch/220748/ If you'd like to give me a name/email [format Full Name em...@wherever.com] I can credit you in a Reported-by: tag in the commit message... -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1079080 Title: ARM instruction srs wrong behaviour Status in QEMU: Confirmed Bug description: Quote from ARM Architecture Reference Manual ARMv7-A and ARMv7-R : Store Return State stores the LR and SPSR of the current mode to the stack of a specified mode Problem: When executing this instruction, the register stored is CPSR instead of SPSR. Context: Using QEMU 1.2.0 to simulate a Zynq application (processor Cortex-a9 mpcore) with the following command line: qemu-system-arm -M xilinx-zynq-a9 -m 512 -serial null -serial mon:stdio -dtb /home/vcesson/workspace/xilinx_zynq.dtb -kernel install/tests/io/serial/current/tests/serial2 -S -s -nographic To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1079080/+subscriptions
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Il 15/02/2013 14:48, Peter Maydell ha scritto:
If you prefer we could standardize on
typedef struct {
ParentClass parent;
} FooClass;
rather than typedef ParentClass FooClass;
It may need rework either way. Because aliasing via typedef gives
FooClass fields it will loose once it is turned into a real QOM class.
We had such an issue with i440fx in my PHB series, that's why I'm
sensitive to it. ;)
OK, so that seems like an argument for always defining the
empty-except-for-the-parentclass class struct, or does that
run into problems too?
I like the empty-except-for-parentclass. OTOH, if you have no fields
you will not use FOO_CLASS. You will only use PARENT_CLASS, and those
uses will be fine even after you start having a FooClass.
So, having no typedef and no _CLASS macros at all is simple and should
be acceptable.
But if you have a typedef, you should a) make it a struct, b) add the
_CLASS macros.
Paolo
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
Il 15/02/2013 12:45, Peter Maydell ha scritto:
Don't map the pmem and dmem RAM memory regions in the milkymist-softusb
device itself. Instead just expose them as sysbus mmio regions which
the device creator can map appropriately. This allows us to drop the
pmem_base and dmem_base properties. Instead of going via
cpu_physical_memory_read/_write when the device wants to access the
RAMs, we just keep a host pointer to the memory and use that.
I think it's best to avoid storing long-lived host pointers. Ideally we
should have no long-lived host pointers anytime the thread is quiescent
(for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event;
for I/O that means select/poll).
Can you call memory_region_get_ram_ptr from the read/write functions?
Paolo
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
hw/milkymist-hw.h |4 ++--
hw/milkymist-softusb.c | 21 +++--
2 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/hw/milkymist-hw.h b/hw/milkymist-hw.h
index 21e202b..47f6d50 100644
--- a/hw/milkymist-hw.h
+++ b/hw/milkymist-hw.h
@@ -210,12 +210,12 @@ static inline DeviceState
*milkymist_softusb_create(hwaddr base,
DeviceState *dev;
dev = qdev_create(NULL, milkymist-softusb);
-qdev_prop_set_uint32(dev, pmem_base, pmem_base);
qdev_prop_set_uint32(dev, pmem_size, pmem_size);
-qdev_prop_set_uint32(dev, dmem_base, dmem_base);
qdev_prop_set_uint32(dev, dmem_size, dmem_size);
qdev_init_nofail(dev);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, pmem_base);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, dmem_base);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
return dev;
diff --git a/hw/milkymist-softusb.c b/hw/milkymist-softusb.c
index 01660be..a3e935f 100644
--- a/hw/milkymist-softusb.c
+++ b/hw/milkymist-softusb.c
@@ -54,10 +54,11 @@ struct MilkymistSoftUsbState {
MemoryRegion dmem;
qemu_irq irq;
+void *pmem_ptr;
+void *dmem_ptr;
+
/* device properties */
-uint32_t pmem_base;
uint32_t pmem_size;
-uint32_t dmem_base;
uint32_t dmem_size;
/* device registers */
@@ -134,7 +135,7 @@ static inline void
softusb_read_dmem(MilkymistSoftUsbState *s,
return;
}
-cpu_physical_memory_read(s-dmem_base + offset, buf, len);
+memcpy(buf, s-dmem_ptr + offset, len);
}
static inline void softusb_write_dmem(MilkymistSoftUsbState *s,
@@ -146,7 +147,7 @@ static inline void
softusb_write_dmem(MilkymistSoftUsbState *s,
return;
}
-cpu_physical_memory_write(s-dmem_base + offset, buf, len);
+memcpy(s-dmem_ptr + offset, buf, len);
}
static inline void softusb_read_pmem(MilkymistSoftUsbState *s,
@@ -158,7 +159,7 @@ static inline void
softusb_read_pmem(MilkymistSoftUsbState *s,
return;
}
-cpu_physical_memory_read(s-pmem_base + offset, buf, len);
+memcpy(buf, s-pmem_ptr + offset, len);
}
static inline void softusb_write_pmem(MilkymistSoftUsbState *s,
@@ -170,7 +171,7 @@ static inline void
softusb_write_pmem(MilkymistSoftUsbState *s,
return;
}
-cpu_physical_memory_write(s-pmem_base + offset, buf, len);
+memcpy(s-pmem_ptr + offset, buf, len);
}
static void softusb_mouse_changed(MilkymistSoftUsbState *s)
@@ -270,11 +271,13 @@ static int milkymist_softusb_init(SysBusDevice *dev)
memory_region_init_ram(s-pmem, milkymist-softusb.pmem,
s-pmem_size);
vmstate_register_ram_global(s-pmem);
-sysbus_add_memory(dev, s-pmem_base, s-pmem);
+s-pmem_ptr = memory_region_get_ram_ptr(s-pmem);
+sysbus_init_mmio(dev, s-pmem);
memory_region_init_ram(s-dmem, milkymist-softusb.dmem,
s-dmem_size);
vmstate_register_ram_global(s-dmem);
-sysbus_add_memory(dev, s-dmem_base, s-dmem);
+s-dmem_ptr = memory_region_get_ram_ptr(s-dmem);
+sysbus_init_mmio(dev, s-dmem);
hid_init(s-hid_kbd, HID_KEYBOARD, softusb_kbd_hid_datain);
hid_init(s-hid_mouse, HID_MOUSE, softusb_mouse_hid_datain);
@@ -298,9 +301,7 @@ static const VMStateDescription vmstate_milkymist_softusb
= {
};
static Property milkymist_softusb_properties[] = {
-DEFINE_PROP_UINT32(pmem_base, MilkymistSoftUsbState, pmem_base,
0xa000),
DEFINE_PROP_UINT32(pmem_size, MilkymistSoftUsbState, pmem_size,
0x1000),
-DEFINE_PROP_UINT32(dmem_base, MilkymistSoftUsbState, dmem_base,
0xa002),
DEFINE_PROP_UINT32(dmem_size, MilkymistSoftUsbState, dmem_size,
0x2000),
DEFINE_PROP_END_OF_LIST(),
};
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
On 15 February 2013 15:21, Paolo Bonzini pbonz...@redhat.com wrote: Il 15/02/2013 12:45, Peter Maydell ha scritto: Don't map the pmem and dmem RAM memory regions in the milkymist-softusb device itself. Instead just expose them as sysbus mmio regions which the device creator can map appropriately. This allows us to drop the pmem_base and dmem_base properties. Instead of going via cpu_physical_memory_read/_write when the device wants to access the RAMs, we just keep a host pointer to the memory and use that. I think it's best to avoid storing long-lived host pointers. Ideally we should have no long-lived host pointers anytime the thread is quiescent (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event; for I/O that means select/poll). Can you call memory_region_get_ram_ptr from the read/write functions? I could, but does it buy us anything in particular? (alternatively, what's the reason why we should avoid storing the host pointers? We do it in a number of other devices...) -- PMM
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Am 15.02.2013 16:14, schrieb Paolo Bonzini:
Il 15/02/2013 14:48, Peter Maydell ha scritto:
If you prefer we could standardize on
typedef struct {
ParentClass parent;
} FooClass;
rather than typedef ParentClass FooClass;
It may need rework either way. Because aliasing via typedef gives
FooClass fields it will loose once it is turned into a real QOM class.
We had such an issue with i440fx in my PHB series, that's why I'm
sensitive to it. ;)
OK, so that seems like an argument for always defining the
empty-except-for-the-parentclass class struct, or does that
run into problems too?
I like the empty-except-for-parentclass. OTOH, if you have no fields
you will not use FOO_CLASS. You will only use PARENT_CLASS, and those
uses will be fine even after you start having a FooClass.
So, having no typedef and no _CLASS macros at all is simple and should
be acceptable.
But if you have a typedef, you should a) make it a struct, b) add the
_CLASS macros.
c) use it in TypeInfo, otherwise it's moot. :)
A related topic where having classes prepared may make sense is in
converting less-simple-than-SysBusDevice types to QOM realize. Device
Foo may need to call its parent's realize function then, so should save
it in its class, which it may not have yet.
However since this is IMO (compared to what other people have already
complained about) unnecessary boilerplate code, I'm more in favor of an
as-needed approach. I.e. if we don't need a struct, don't require to
define it nor macros to access it. But surely there's nothing wrong with
adding more structs/macros on a voluntary basis as long as they are
consistent.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
On 02/15/2013 12:24 PM, Andreas Färber wrote: The expected answer would've been take guest X and do Y to see Z, or better to extend the existing qtest cases to prove something was broken before and fixed afterwards and to avoid the same bug being reintroduced later. If we are talking about adding a test case in order to have some guarantee that what works after a fix keeps working in the future, that's fine. And I am willing to add such tests for the DS1338 implementation (once it is finished). But demanding a test case that the code passes with the fix but fails without, in order to prove that something was broken before, is only reasonable if the bug was found through testing in the first place. It is inappropriate for a bug found in a code review. Not only do you not need a test case to prove the bug exists, but reverse-engineering a test-case can be a significant undertaking. Paolo tried to do that unsuccessfully in the case of bug 1090558 and I had no reason to think I could do better. This does not strike me as a very productive use of developer time anyway. And your suggestion that it is better to leave a known bug unpatched until someone can conjure up a test case is ridiculous. I don't see how that attitude help users, in the short or long term. If you don't nuance your position you are only going to discourage much needed code reviews. I don't see what good can come of that.
[Qemu-devel] [PATCH qom-cpu-next] spapr_hcall: Replace open-coded CPU loop with qemu_get_cpu()
The helper functions all access ppc-specific fields only so don't bother
to change arguments to PowerPCCPU and use env_ptr instead.
No functional change.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/spapr_hcall.c | 11 +++
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index af1db6e..7b89594 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -469,16 +469,11 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
CPUPPCState *tenv;
CPUState *tcpu;
-for (tenv = first_cpu; tenv; tenv = tenv-next_cpu) {
-tcpu = CPU(ppc_env_get_cpu(tenv));
-if (tcpu-cpu_index == procno) {
-break;
-}
-}
-
-if (!tenv) {
+tcpu = qemu_get_cpu(procno);
+if (!tcpu) {
return H_PARAMETER;
}
+tenv = tcpu-env_ptr;
switch (flags) {
case FLAGS_REGISTER_VPA:
--
1.7.10.4
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
Il 15/02/2013 16:35, Peter Maydell ha scritto: I think it's best to avoid storing long-lived host pointers. Ideally we should have no long-lived host pointers anytime the thread is quiescent (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event; for I/O that means select/poll). Can you call memory_region_get_ram_ptr from the read/write functions? I could, but does it buy us anything in particular? (alternatively, what's the reason why we should avoid storing the host pointers? We do it in a number of other devices...) I find it more complex to reason about finer-grained locking (particularly regarding the lifetimes) when you have object A storing something into object B. In practice it should be handled just fine by reference counting, but I still find it harder to wrap my head around it. Paolo
[Qemu-devel] [PATCH qom-cpu-next] monitor: Use qemu_get_cpu() in monitor_set_cpu()
No functional change, just a reduction of CPU loops.
The mon_cpu field is left untouched for now since changing that requires
a number of larger prerequisites, including cpu_synchronize_state() and
mon_get_cpu().
Signed-off-by: Andreas Färber afaer...@suse.de
---
monitor.c | 13 +
1 Datei geändert, 5 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/monitor.c b/monitor.c
index 20bd19b..cae33c4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -855,17 +855,14 @@ EventInfoList *qmp_query_events(Error **errp)
/* set the current CPU defined by the user */
int monitor_set_cpu(int cpu_index)
{
-CPUArchState *env;
CPUState *cpu;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = ENV_GET_CPU(env);
-if (cpu-cpu_index == cpu_index) {
-cur_mon-mon_cpu = env;
-return 0;
-}
+cpu = qemu_get_cpu(cpu_index);
+if (cpu == NULL) {
+return -1;
}
-return -1;
+cur_mon-mon_cpu = cpu-env_ptr;
+return 0;
}
static CPUArchState *mon_get_cpu(void)
--
1.7.10.4
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
Il 15/02/2013 16:41, Antoine Mathys ha scritto: On 02/15/2013 12:24 PM, Andreas Färber wrote: The expected answer would've been take guest X and do Y to see Z, or better to extend the existing qtest cases to prove something was broken before and fixed afterwards and to avoid the same bug being reintroduced later. If we are talking about adding a test case in order to have some guarantee that what works after a fix keeps working in the future, that's fine. And I am willing to add such tests for the DS1338 implementation (once it is finished). But demanding a test case that the code passes with the fix but fails without, in order to prove that something was broken before, is only reasonable if the bug was found through testing in the first place. It depends. For example, the mc146818rtc model was rewritten almost completely last year. Testcases helped a lot, and more testcases would have helped even more. It does not matter if they came from past bugs, or from code review, or from blackbox testing. Not only do you not need a test case to prove the bug exists, but reverse-engineering a test-case can be a significant undertaking. That's true. But... Paolo tried to do that unsuccessfully in the case of bug 1090558 and I had no reason to think I could do better. This does not strike me as a very productive use of developer time anyway. ... at least trying to do that _is_ a productive use of developer time. Of course, insisting at all costs is not. So a reviewer is right in asking for a testcase and complaining of a lack of testcases. He should be okay with I couldn't find one just as well, especially if it comes with a patch that actually adds testcases. My effort to reproduce bug 1090558 did produce such a patch. As an aside ds1338 is a much simpler model than mc146818rtc (the capture behavior is much nicer than mc146818rtc's UIP, and ds1338 also has no alarm and no interrupts), the bug should be almost trivial to test for. And your suggestion that it is better to leave a known bug unpatched until someone can conjure up a test case is ridiculous. I don't see how that attitude help users, in the short or long term. Of course some common sense is in order, as usual. Leaving bugs unpatched is bad, but prodding contributors and other maintainers is good. Paolo
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
On 15 February 2013 16:06, Paolo Bonzini pbonz...@redhat.com wrote: Il 15/02/2013 16:35, Peter Maydell ha scritto: I think it's best to avoid storing long-lived host pointers. Ideally we should have no long-lived host pointers anytime the thread is quiescent (for CPUs that means kvm_cpu_exec or the end of qemu_tcg_wait_io_event; for I/O that means select/poll). Can you call memory_region_get_ram_ptr from the read/write functions? I could, but does it buy us anything in particular? (alternatively, what's the reason why we should avoid storing the host pointers? We do it in a number of other devices...) I find it more complex to reason about finer-grained locking (particularly regarding the lifetimes) when you have object A storing something into object B. In practice it should be handled just fine by reference counting, but I still find it harder to wrap my head around it. But these memory regions belong to this device -- we own them and they won't go away until the device is destroyed [which is never, as it happens, for this device.] More generally, if it's valid for us to hold a MemoryRegion* and call memory_region_get_ram_ptr() in the read/write function, it's just as valid to keep the ram pointer: the two have exactly matching lifetimes, unless I'm missing something. (as an aside, memory_region_destroy() doesn't free the memory that memory_region_init_ram() allocates.) -- PMM
Re: [Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself
Il 15/02/2013 17:17, Peter Maydell ha scritto: But these memory regions belong to this device -- we own them and they won't go away until the device is destroyed [which is never, as it happens, for this device.] More generally, if it's valid for us to hold a MemoryRegion* and call memory_region_get_ram_ptr() in the read/write function, it's just as valid to keep the ram pointer: the two have exactly matching lifetimes, unless I'm missing something. No, you're not: In practice it should be handled just fine by reference counting, but I still find it harder to wrap my head around it. (as an aside, memory_region_destroy() doesn't free the memory that memory_region_init_ram() allocates.) Paolo
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 15:14, Paolo Bonzini pbonz...@redhat.com wrote:
I like the empty-except-for-parentclass. OTOH, if you have no fields
you will not use FOO_CLASS. You will only use PARENT_CLASS, and those
uses will be fine even after you start having a FooClass.
OK, that makes sense I think, except there is one case where you
do need a FooClassc even if you have no class fields, which is
when you can yourself be subclassed. You want to provide the subclasses
with all the types etc they need so they don't change if you have
to add a class field yourself in future.
I wrote this up for the wiki page: http://wiki.qemu.org/QOMConventions
===begin===
If your class (a) will be subclassed or (b) has member fields it needs
to put in its class struct then you should write all of:
* a codeFOO_CLASS/code macro
* a codeFOO_GET_CLASS/code macro
* a FooClass structure definition containing at least the parent class field:
typedef struct {
/* private */
MyParentClass parent_class;
/* public */
[any fields you need]
} FooClass;
* and your codeTypeInfo/code for this class should set the
code.class_size/code field to codesizeof(FooClass)/code.
These ensure that nothing in future should need changing if new
fields are added to your class struct, and that any subclasses
have the correct typenames available so they won't need to change
either even if your implementation changes.
If your class meets neither of the above requirements (ie it is a
simple leaf class) then:
* don't provide codeFOO_CLASS/code or codeFOO_GET_CLASS/code
* don't provide a FooClass structure
* leave the codeTypeInfo/code's code.class_size/code field unset.
If a change means a class which didn't provide these macros/types
now needs to provide them, then your change should add all of them
(ie move the class from the latter category to the former).
===endit===
If that has some agreement I'll take the 'caution' label off it :-)
and update this patch to match, ie remove the class macros and
typedef.
-- PMM
Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.
Am 15.02.2013 12:41, schrieb Peter Maydell: On 15 February 2013 11:24, Andreas Färber afaer...@suse.de wrote: Am 15.02.2013 03:49, schrieb Antoine Mathys: First, the ds1338 code was in a poor state and never handled the 12 hour clock correctly. My first patch failed to fully fix the problem so I had to write a second one, but at no point did Peter or I introduce a regression, quite the opposite. Read closely, I never claimed *you* introduced a regression. What I have rather been observing is a seemingly not-ending stream of bugfix patches One patch is hardly a never-ending stream! I was referring to the previous six as well, where I first brought up the topic of qtest (before going on to implement I2C libqos after seeing another I2C'ish bugfix which did turn out to be half-broken). Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu-next] spapr_hcall: Replace open-coded CPU loop with qemu_get_cpu()
On 15.02.2013, at 16:49, Andreas Färber wrote:
The helper functions all access ppc-specific fields only so don't bother
to change arguments to PowerPCCPU and use env_ptr instead.
No functional change.
Signed-off-by: Andreas Färber afaer...@suse.de
Acked-by: Alexander Graf ag...@suse.de
Alex
---
hw/spapr_hcall.c | 11 +++
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index af1db6e..7b89594 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -469,16 +469,11 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu,
sPAPREnvironment *spapr,
CPUPPCState *tenv;
CPUState *tcpu;
-for (tenv = first_cpu; tenv; tenv = tenv-next_cpu) {
-tcpu = CPU(ppc_env_get_cpu(tenv));
-if (tcpu-cpu_index == procno) {
-break;
-}
-}
-
-if (!tenv) {
+tcpu = qemu_get_cpu(procno);
+if (!tcpu) {
return H_PARAMETER;
}
+tenv = tcpu-env_ptr;
switch (flags) {
case FLAGS_REGISTER_VPA:
--
1.7.10.4
[Qemu-devel] [PATCH qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.
Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/ppce500_spin.c | 15 ---
1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
{
SpinState *s = opaque;
int env_idx = addr / sizeof(SpinInfo);
-CPUPPCState *env;
-CPUState *cpu = NULL;
+CPUState *cpu;
SpinInfo *curspin = s-spin[env_idx];
uint8_t *curspin_p = (uint8_t*)curspin;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = CPU(ppc_env_get_cpu(env));
-if (cpu-cpu_index == env_idx) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(env_idx);
if (cpu == NULL) {
/* Unknown CPU */
return;
@@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
if (!(ldq_p(curspin-addr) 1)) {
/* run CPU */
SpinKick kick = {
-.cpu = ppc_env_get_cpu(env),
+.cpu = POWERPC_CPU(cpu),
.spin = curspin,
};
-run_on_cpu(CPU(kick.cpu), spin_kick, kick);
+run_on_cpu(cpu, spin_kick, kick);
}
}
--
1.7.10.4
Re: [Qemu-devel] [PATCH qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()
On 15.02.2013, at 17:51, Andreas Färber wrote:
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.
Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/ppce500_spin.c | 15 ---
1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
{
SpinState *s = opaque;
int env_idx = addr / sizeof(SpinInfo);
-CPUPPCState *env;
-CPUState *cpu = NULL;
+CPUState *cpu;
SpinInfo *curspin = s-spin[env_idx];
uint8_t *curspin_p = (uint8_t*)curspin;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = CPU(ppc_env_get_cpu(env));
-if (cpu-cpu_index == env_idx) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(env_idx);
if (cpu == NULL) {
/* Unknown CPU */
return;
@@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
if (!(ldq_p(curspin-addr) 1)) {
/* run CPU */
SpinKick kick = {
-.cpu = ppc_env_get_cpu(env),
+.cpu = POWERPC_CPU(cpu),
Why not just cpu?
Alex
.spin = curspin,
};
-run_on_cpu(CPU(kick.cpu), spin_kick, kick);
+run_on_cpu(cpu, spin_kick, kick);
}
}
--
1.7.10.4
Re: [Qemu-devel] [PATCH qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()
On 15.02.2013, at 15:35, Andreas Färber wrote: Am 15.02.2013 15:29, schrieb Alexander Graf: On 15.02.2013, at 15:25, Andreas Färber wrote: Since we still need env for ppc-specific fields, obtain it via the new env_ptr fields to avoid cpu name conflicts between CPUState and PowerPCCPU for now. This fixes a potential issue with env being NULL at the end of the loop but cpu still being a valid pointer corresponding to a previous env. Signed-off-by: Andreas Färber afaer...@suse.de --- Alex, this is a rebased and slimmed-down version of yesterday's paste. Can you ack please? Thanks, /-F In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem? Not yet. Not all callers are using this pattern though, so solutions would differ and be per-case patches. This specific patch is: Acked-by: Alexander Graf ag...@suse.de However, please make sure to fix the other places too :) Alex At some point I obviously intend to change first_cpu and -next_cpu to CPUState and have been wondering whether we can use QTAILQ macros or anything else more standardized, with better iteration support. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()
Am 15.02.2013 17:54, schrieb Alexander Graf:
On 15.02.2013, at 17:51, Andreas Färber wrote:
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.
Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/ppce500_spin.c | 15 ---
1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
{
SpinState *s = opaque;
int env_idx = addr / sizeof(SpinInfo);
-CPUPPCState *env;
-CPUState *cpu = NULL;
+CPUState *cpu;
SpinInfo *curspin = s-spin[env_idx];
uint8_t *curspin_p = (uint8_t*)curspin;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = CPU(ppc_env_get_cpu(env));
-if (cpu-cpu_index == env_idx) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(env_idx);
if (cpu == NULL) {
/* Unknown CPU */
return;
@@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
if (!(ldq_p(curspin-addr) 1)) {
/* run CPU */
SpinKick kick = {
-.cpu = ppc_env_get_cpu(env),
+.cpu = POWERPC_CPU(cpu),
Why not just cpu?
PowerPCCPU vs. CPUState type.
Having the specific type in ppc code allows direct access to -env.
If you see a performance issue, we could also use (PowerPCCPU *)cpu.
Andreas
.spin = curspin,
};
-run_on_cpu(CPU(kick.cpu), spin_kick, kick);
+run_on_cpu(cpu, spin_kick, kick);
}
}
--
1.7.10.4
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()
On 15.02.2013, at 17:58, Andreas Färber wrote:
Am 15.02.2013 17:54, schrieb Alexander Graf:
On 15.02.2013, at 17:51, Andreas Färber wrote:
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.
Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/ppce500_spin.c | 15 ---
1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
{
SpinState *s = opaque;
int env_idx = addr / sizeof(SpinInfo);
-CPUPPCState *env;
-CPUState *cpu = NULL;
+CPUState *cpu;
SpinInfo *curspin = s-spin[env_idx];
uint8_t *curspin_p = (uint8_t*)curspin;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = CPU(ppc_env_get_cpu(env));
-if (cpu-cpu_index == env_idx) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(env_idx);
if (cpu == NULL) {
/* Unknown CPU */
return;
@@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
if (!(ldq_p(curspin-addr) 1)) {
/* run CPU */
SpinKick kick = {
-.cpu = ppc_env_get_cpu(env),
+.cpu = POWERPC_CPU(cpu),
Why not just cpu?
PowerPCCPU vs. CPUState type.
Having the specific type in ppc code allows direct access to -env.
If you see a performance issue, we could also use (PowerPCCPU *)cpu.
There are no performance issues in the spin code :). By definition. This thing
could be written in bash and still be fast enough.
I was mostly wondering whether ppc_env_get_cpu(env) returns a PowerPCCPU *. But
apparently it does. Ok then :)
Acked-by: Alexander Graf ag...@suse.de
Alex
Andreas
.spin = curspin,
};
-run_on_cpu(CPU(kick.cpu), spin_kick, kick);
+run_on_cpu(cpu, spin_kick, kick);
}
}
--
1.7.10.4
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu-next 0/6] QOM CPUState, part 8: CPU_COMMON continued
Am 14.02.2013 18:49, schrieb Andreas Färber: Am 01.02.2013 13:38, schrieb Andreas Färber: Hello, This series moves more fields from CPU_COMMON / CPU*State to CPUState, allowing access from target-independent code. The final patch in this series will help solve some issues (in particular avoid a dependency on CPU_COMMON TLB refactoring for now) but opens a can of worms: Since it is initialized in derived instance_init functions, functions cannot randomly be changed to operate on CPUState and be called from CPUState's instance_init or they will crash due to NULL env_ptr. The questionable patch in this series has been acked by rth, so if nobody objects, I'll queue it on qom-cpu-next tonight, [...] Applied: https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next Andreas For those of you that may have been following the CPU refactorings closely, I have now split off part of former qom-cpu-8 branch into qom-cpu-9. This series thereby applies directly to qom-cpu-next, whereas qom-cpu-9 depends on the pending s390x pull, my m68k cleanups and may be changed for VMState changes cooking elsewhere to keep i386 v5 compat. Available for testing at: git://github.com/afaerber/qemu-cpu.git qom-cpu-8.v1 https://github.com/afaerber/qemu-cpu/commits/qom-cpu-8.v1 Regards, Andreas Changes from previews: * Drop #ifdefs for user-only CPUState fields. * Defer interrupt-related changes to part 9. Andreas Färber (6): cpu: Move host_tid field to CPUState cpu: Move running field to CPUState cpu: Move exit_request field to CPUState cpu: Move current_tb field to CPUState cputlb: Pass CPUState to cpu_unlink_tb() cpu: Add CPUArchState pointer to CPUState cpu-exec.c | 21 - cputlb.c|6 -- dump.c |8 ++-- exec.c |6 -- gdbstub.c | 14 +- hw/apic_common.c|2 +- hw/apic_internal.h |2 +- hw/kvmvapic.c | 13 - hw/spapr_hcall.c|5 +++-- include/exec/cpu-defs.h |5 - include/exec/exec-all.h |4 +++- include/exec/gdbstub.h |5 ++--- include/qom/cpu.h | 11 +++ kvm-all.c |6 +++--- linux-user/main.c | 37 ++--- linux-user/syscall.c|4 +++- qom/cpu.c |2 ++ target-alpha/cpu.c |2 ++ target-arm/cpu.c|2 ++ target-cris/cpu.c |2 ++ target-i386/cpu.c |1 + target-i386/kvm.c |4 ++-- target-lm32/cpu.c |2 ++ target-m68k/cpu.c |2 ++ target-microblaze/cpu.c |2 ++ target-mips/cpu.c |2 ++ target-openrisc/cpu.c |2 ++ target-ppc/translate_init.c |2 ++ target-s390x/cpu.c |2 ++ target-sh4/cpu.c|2 ++ target-sparc/cpu.c |2 ++ target-unicore32/cpu.c |2 ++ target-xtensa/cpu.c |2 ++ translate-all.c | 36 +++- translate-all.h |2 +- 35 Dateien geändert, 149 Zeilen hinzugefügt(+), 73 Zeilen entfernt(-) -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] fixing qemu busy wait
when debugging with qemu(user mode), qemu waits in infinite loop to read a signal from gdb (when it waits on breakpoint for example). I added sleeps to reduce the cpu usage from 100% to about ~0%. qemu-busy-wait.patch Description: Binary data
[Qemu-devel] [Bug 1126369] [NEW] qemu-img snapshot -c is unreasonably slow
Public bug reported: Something fishy is going on with qcow2 internal snapshot creation times. I don't know if this is a regression because I haven't used internal snapshots in the past. QEMU 1.4-rc2: $ qemu-img create -f qcow2 test.qcow2 -o size=50G,preallocation=metadata $ time qemu-img snapshot -c new test.qcow2 real3m39.147s user0m10.748s sys 0m26.165s (This is on an SSD) I expect snapshot creation to take under 1 second. ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1126369 Title: qemu-img snapshot -c is unreasonably slow Status in QEMU: New Bug description: Something fishy is going on with qcow2 internal snapshot creation times. I don't know if this is a regression because I haven't used internal snapshots in the past. QEMU 1.4-rc2: $ qemu-img create -f qcow2 test.qcow2 -o size=50G,preallocation=metadata $ time qemu-img snapshot -c new test.qcow2 real 3m39.147s user 0m10.748s sys 0m26.165s (This is on an SSD) I expect snapshot creation to take under 1 second. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1126369/+subscriptions
Re: [Qemu-devel] [PATCH qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()
Am 15.02.2013 17:45, schrieb Alexander Graf: On 15.02.2013, at 15:35, Andreas Färber wrote: Am 15.02.2013 15:29, schrieb Alexander Graf: On 15.02.2013, at 15:25, Andreas Färber wrote: Since we still need env for ppc-specific fields, obtain it via the new env_ptr fields to avoid cpu name conflicts between CPUState and PowerPCCPU for now. This fixes a potential issue with env being NULL at the end of the loop but cpu still being a valid pointer corresponding to a previous env. Signed-off-by: Andreas Färber afaer...@suse.de --- Alex, this is a rebased and slimmed-down version of yesterday's paste. Can you ack please? Thanks, /-F In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem? Not yet. Not all callers are using this pattern though, so solutions would differ and be per-case patches. This specific patch is: Acked-by: Alexander Graf ag...@suse.de Thanks, applied to qom-cpu-next queue: https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next However, please make sure to fix the other places too :) In whole there's two e500 patches fixing a potential bug, plus non-functional cleanups for sPAPR, monitor and QMP code. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()
Am 15.02.2013 18:04, schrieb Alexander Graf:
On 15.02.2013, at 17:58, Andreas Färber wrote:
Am 15.02.2013 17:54, schrieb Alexander Graf:
On 15.02.2013, at 17:51, Andreas Färber wrote:
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.
Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
Signed-off-by: Andreas Färber afaer...@suse.de
---
hw/ppce500_spin.c | 15 ---
1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
{
SpinState *s = opaque;
int env_idx = addr / sizeof(SpinInfo);
-CPUPPCState *env;
-CPUState *cpu = NULL;
+CPUState *cpu;
SpinInfo *curspin = s-spin[env_idx];
uint8_t *curspin_p = (uint8_t*)curspin;
-for (env = first_cpu; env != NULL; env = env-next_cpu) {
-cpu = CPU(ppc_env_get_cpu(env));
-if (cpu-cpu_index == env_idx) {
-break;
-}
-}
-
+cpu = qemu_get_cpu(env_idx);
if (cpu == NULL) {
/* Unknown CPU */
return;
@@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr,
uint64_t value,
if (!(ldq_p(curspin-addr) 1)) {
/* run CPU */
SpinKick kick = {
-.cpu = ppc_env_get_cpu(env),
+.cpu = POWERPC_CPU(cpu),
Why not just cpu?
PowerPCCPU vs. CPUState type.
Having the specific type in ppc code allows direct access to -env.
If you see a performance issue, we could also use (PowerPCCPU *)cpu.
There are no performance issues in the spin code :). By definition. This
thing could be written in bash and still be fast enough.
I was mostly wondering whether ppc_env_get_cpu(env) returns a PowerPCCPU *.
But apparently it does. Ok then :)
Acked-by: Alexander Graf ag...@suse.de
Thanks, applied to qom-cpu-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
/-F
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
