Re: [Qemu-devel] [PATCH for-2.7] vnc: fix qemu crash because of SIGSEGV
Hi Gerd, Can you pls pick up this patch? thanks Regards, -Gonglei > -Original Message- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Friday, September 02, 2016 8:39 PM > To: Marc-André Lureau > Cc: Gonglei (Arei); QEMU Developers; Huangweidong (C); Gerd Hoffmann > Subject: Re: [Qemu-devel] [PATCH for-2.7] vnc: fix qemu crash because of > SIGSEGV > > On 2 September 2016 at 13:34, Marc-André Lureau > wrote: > > Hi > > > > On Fri, Sep 2, 2016 at 3:04 PM Gonglei wrote: > > > >> > >> > It looks like this is not a regression from 2.7, perhaps it should be > >> post-poned? > >> > > >> Yes, it's not a regression from 2.7, but it indeed is a serious bug and > >> the fix is harmless. :) > >> > >> > > The timing is bad. Unless Gerd or a maintainer sends a pull request today > > with it, it's probably not going to make it in 2.7 (due today according to > > planning). > > For a non-regression this would have had to be sent at least a > week ago to have had a chance of getting into 2.7. I would only accep > anything into 2.7 now if it was an absolute release-breaker > (eg "crashes on startup for 50% of users"); this is a long way from > that. 2.8 and cc qemu-stable, please. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH v2 2/2] e1000: fix buliding complaint
Who can pick up this patch? Dmitry or Jason? Thanks!
Regards,
-Gonglei
> -Original Message-
> From: Dmitry Fleytman [mailto:dmi...@daynix.com]
> Sent: Wednesday, August 31, 2016 8:59 PM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; berra...@redhat.com
> Subject: Re: [PATCH v2 2/2] e1000: fix buliding complaint
>
> Reviewed-by: Dmitry Fleytman
>
> > On 30 Aug 2016, at 07:10 AM, Gonglei wrote:
> >
> > hw/net/e1000e_core.c:56: warning: e1000e_set_interrupt_cause declared
> inline after being called
> > hw/net/e1000e_core.c:56: warning: previous declaration of
> e1000e_set_interrupt_cause was here
> >
> > Signed-off-by: Gonglei
> > ---
> > hw/net/e1000e_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index badb1fe..825e169 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -2168,7 +2168,7 @@ e1000e_update_interrupt_state(E1000ECore
> *core)
> > }
> > }
> >
> > -static inline void
> > +static void
> > e1000e_set_interrupt_cause(E1000ECore *core, uint32_t val)
> > {
> > trace_e1000e_irq_set_cause_entry(val, core->mac[ICR]);
> > --
> > 1.7.12.4
> >
> >
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 09 Sep 2016 08:38:13 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Thu, 8 Sep 2016 18:19:27 +0300 > > "Michael S. Tsirkin" wrote: > > > >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > >> > On Thu, 8 Sep 2016 18:00:28 +0300 > >> > "Michael S. Tsirkin" wrote: > >> > > >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > >> > > > On Thu, 8 Sep 2016 10:59:26 +0200 > >> > > > Cornelia Huck wrote: > >> > > > > >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200 > >> > > > > Greg Kurz wrote: > >> > > > > > >> > > > > > Calling assert() really makes sense when hitting a genuine bug, > >> > > > > > which calls > >> > > > > > for a fix in QEMU. However, when something goes wrong because > >> > > > > > the guest > >> > > > > > sends a malformed message, it is better to write down a more > >> > > > > > meaningul > >> > > > > > error message and exit. > >> > > > > > > >> > > > > > Signed-off-by: Greg Kurz > >> > > > > > --- > >> > > > > > hw/9pfs/virtio-9p-device.c | 20 ++-- > >> > > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > >> > > > > > >> > > > > While this is an improvement over the current state, I don't think > >> > > > > the > >> > > > > guest should be able to kill qemu just by doing something stupid. > >> > > > > > >> > > > > >> > > > Hi Connie, > >> > > > > >> > > > I'm glad you're pointing this out... this was also my impression, but > >> > > > since there are a bunch of sanity checks in the virtio code that > >> > > > cause > >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare > >> > > > stand up :) > >> > > > >> > > It's true that it's broken in many places but we should just > >> > > fix them all. > >> > > > >> > > > >> > > A separate question is how to log such hardware/guest bugs generally. > >> > > People already complained about disk filling up because of us printing > >> > > errors on each such bug. Maybe print each message only N times, and > >> > > then set a flag to skip the log until management tells us to restart > >> > > logging again. > >> > > >> > I'd expect to get the message just once per device if we set the device > >> > to broken (unless the guess continuously resets it again...) > >> > >> Which it can do, so we should limit that anyway. > >> > >> > Do we have > >> > a generic print/log ratelimit infrastructure in qemu? > >> > >> There are actually two kinds of errors > >> host side ones and ones triggered by guests. > >> > >> We should distinguish between them API-wise, then > >> we will be able to limit the logging of those > >> that guest can trigger. > >> > > > > FWIW it makes sense to use error_report() if QEMU exits. > > exit(STATUS) with STATUS != 0 without printing a message is always > wrong. > I fully agree. > > If it continues > > execution, this means we're expecting the guest or the host to do something > > to fix the error condition. This requires QEMU to emit an event of some > > sort, but not necessarily to log an error message in a file. I guess this > > depends if QEMU is run by some tooling, or by a human. > > error_report() normally goes to stderr. Tooling or humans can of course > make it go to a file instead. > > error_report() is indeed a sub-par way to send an "attention" signal to > the host, because recognizing such a signal reliably is unnecessary hard > for management applications. QMP events are much easier. > My wording was poor but yes, that was my point. :) > Both are useless when the signal needs to go to the guest. Signalling > the guest is a device model job. > I also agree with that. In the case of virtio, this is explained in section 2.1.2 of the spec. > error_report() without exit() has its uses. Error conditions in need of > fixing aren't the only reason to call error_report(). But when you add > a call, ask yourself whether management application or guest would like > to respond to it. In the case of the present patch, we currently have BUG_ON() which generates a cryptic and unusable message. It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is correct since it is now [1] impossible to hit this according to the code (see virtqueue_pop() and virtqueue_map_desc()). The second one (len != sizeof out) though matches a potential guest originated error. If I do as suggested by Connie, then the error_report() isn't needed anymore. Cheers. -- Greg [1] sending an empty buffer was sufficient before commit 1e7aed70144b4 as said in my previous answer
Re: [Qemu-devel] [PULL 34/38] qmp: add QMP interface "query-cpu-model-comparison"
On 09/08/2016 04:12 PM, Eric Blake wrote: > On 09/06/2016 02:47 AM, Cornelia Huck wrote: >> From: David Hildenbrand >> >> Let's provide a standardized interface to compare two CPU models. >> "query-cpu-model-compare" takes two models and returns how they compare >> in a specific configuration. >> > >> +## >> +# @CpuModelCompareResult: >> +# >> +# An enumeration of CPU model comparation results. The result is usually >> +# calcualted using e.g. CPU features or CPU generations. > > s/calcualted/calculated/ We will send a fixup for all typos/whitespaces. Thank you for reviewing.
[Qemu-devel] [PATCH] usb-host: fix streams detection in usb_host_speed_compat
The companion descriptor is present on all usb3 devices, not only
those with streams support. We need to check attributes to see
whenever the device uses streams or not.
Signed-off-by: Gerd Hoffmann
---
hw/usb/host-libusb.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index e94672c..bd81d71 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -743,10 +743,13 @@ static void usb_host_speed_compat(USBHostDevice *s)
rc = libusb_get_ss_endpoint_companion_descriptor
(ctx, endp, &endp_ss_comp);
if (rc == LIBUSB_SUCCESS) {
+int streams = endp_ss_comp->bmAttributes & 0x1f;
+if (streams) {
+compat_full = false;
+compat_high = false;
+}
libusb_free_ss_endpoint_companion_descriptor
(endp_ss_comp);
-compat_full = false;
-compat_high = false;
}
#endif
break;
--
1.8.3.1
[Qemu-devel] [PATCH 0/3] tests: more test cases for virtio-9p
As with other virtio-* qtests, PC platform is assumed. --- Greg Kurz (3): tests: virtio-9p: introduce start/stop functions tests: virtio-9p: add basic configuration test tests: virtio-9p: add basic transaction test tests/Makefile.include |2 - tests/virtio-9p-test.c | 179 2 files changed, 164 insertions(+), 17 deletions(-) -- Greg
[Qemu-devel] [PATCH 1/3] tests: virtio-9p: introduce start/stop functions
First step to be able to run several functional steps.
Signed-off-by: Greg Kurz
---
tests/virtio-9p-test.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 1e39335a7945..ed53f21bc31f 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -11,33 +11,40 @@
#include "libqtest.h"
#include "qemu-common.h"
-/* Tests only initialization so far. TODO: Replace with functional tests */
-static void pci_nop(void)
-{
-}
+static char *test_share;
-static char test_share[] = "/tmp/qtest.XX";
-
-int main(int argc, char **argv)
+static void qvirtio_9p_start(void)
{
char *args;
-int ret;
-g_test_init(&argc, &argv, NULL);
-qtest_add_func("/virtio/9p/pci/nop", pci_nop);
-
-g_assert(mkdtemp(test_share));
+test_share = g_strdup("/tmp/qtest.XX");
+g_assert_nonnull(mkdtemp(test_share));
args = g_strdup_printf("-fsdev local,id=fsdev0,security_model=none,path=%s
"
"-device
virtio-9p-pci,fsdev=fsdev0,mount_tag=qtest",
test_share);
+
qtest_start(args);
g_free(args);
+}
-ret = g_test_run();
-
+static void qvirtio_9p_stop(void)
+{
qtest_end();
rmdir(test_share);
+g_free(test_share);
+}
+
+static void pci_nop(void)
+{
+qvirtio_9p_start();
+qvirtio_9p_stop();
+}
+
+int main(int argc, char **argv)
+{
+g_test_init(&argc, &argv, NULL);
+qtest_add_func("/virtio/9p/pci/nop", pci_nop);
-return ret;
+return g_test_run();
}
[Qemu-devel] [PATCH 2/3] tests: virtio-9p: add basic configuration test
This adds PCI init code and a basic test that checks the device config
matches what is passed on the command line.
Signed-off-by: Greg Kurz
---
tests/Makefile.include |2 +
tests/virtio-9p-test.c | 82 +++-
2 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 03382b5fe7b8..a9dce206fbf6 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -600,7 +600,7 @@ tests/virtio-blk-test$(EXESUF): tests/virtio-blk-test.o
$(libqos-virtio-obj-y)
tests/virtio-net-test$(EXESUF): tests/virtio-net-test.o $(libqos-pc-obj-y)
$(libqos-virtio-obj-y)
tests/virtio-rng-test$(EXESUF): tests/virtio-rng-test.o $(libqos-pc-obj-y)
tests/virtio-scsi-test$(EXESUF): tests/virtio-scsi-test.o
$(libqos-virtio-obj-y)
-tests/virtio-9p-test$(EXESUF): tests/virtio-9p-test.o
+tests/virtio-9p-test$(EXESUF): tests/virtio-9p-test.o $(libqos-virtio-obj-y)
tests/virtio-serial-test$(EXESUF): tests/virtio-serial-test.o
tests/virtio-console-test$(EXESUF): tests/virtio-console-test.o
tests/tpci200-test$(EXESUF): tests/tpci200-test.o
diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index ed53f21bc31f..d7d508481bc0 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -10,7 +10,15 @@
#include "qemu/osdep.h"
#include "libqtest.h"
#include "qemu-common.h"
+#include "libqos/pci-pc.h"
+#include "libqos/virtio.h"
+#include "libqos/virtio-pci.h"
+#include "libqos/malloc.h"
+#include "libqos/malloc-pc.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_pci.h"
+static const char mount_tag[] = "qtest";
static char *test_share;
static void qvirtio_9p_start(void)
@@ -21,8 +29,8 @@ static void qvirtio_9p_start(void)
g_assert_nonnull(mkdtemp(test_share));
args = g_strdup_printf("-fsdev local,id=fsdev0,security_model=none,path=%s
"
- "-device
virtio-9p-pci,fsdev=fsdev0,mount_tag=qtest",
- test_share);
+ "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s",
+ test_share, mount_tag);
qtest_start(args);
g_free(args);
@@ -41,10 +49,80 @@ static void pci_nop(void)
qvirtio_9p_stop();
}
+typedef struct {
+QVirtioDevice *dev;
+QGuestAllocator *alloc;
+QPCIBus *bus;
+QVirtQueue *vq;
+} QVirtIO9P;
+
+static QVirtIO9P *qvirtio_9p_pci_init(void)
+{
+QVirtIO9P *v9p;
+QVirtioPCIDevice *dev;
+
+v9p = g_new0(QVirtIO9P, 1);
+v9p->alloc = pc_alloc_init();
+v9p->bus = qpci_init_pc();
+
+dev = qvirtio_pci_device_find(v9p->bus, VIRTIO_ID_9P);
+g_assert_nonnull(dev);
+g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_9P);
+v9p->dev = (QVirtioDevice *) dev;
+
+qvirtio_pci_device_enable(dev);
+qvirtio_reset(&qvirtio_pci, v9p->dev);
+qvirtio_set_acknowledge(&qvirtio_pci, v9p->dev);
+qvirtio_set_driver(&qvirtio_pci, v9p->dev);
+
+v9p->vq = qvirtqueue_setup(&qvirtio_pci, v9p->dev, v9p->alloc, 0);
+return v9p;
+}
+
+static void qvirtio_9p_pci_free(QVirtIO9P *v9p)
+{
+qvirtqueue_cleanup(&qvirtio_pci, v9p->vq, v9p->alloc);
+pc_alloc_uninit(v9p->alloc);
+qvirtio_pci_device_disable(container_of(v9p->dev, QVirtioPCIDevice, vdev));
+g_free(v9p->dev);
+qpci_free_pc(v9p->bus);
+g_free(v9p);
+}
+
+static void pci_basic_config(void)
+{
+QVirtIO9P *v9p;
+void *addr;
+size_t tag_len;
+char* tag;
+int i;
+
+qvirtio_9p_start();
+v9p = qvirtio_9p_pci_init();
+
+addr = ((QVirtioPCIDevice *) v9p->dev)->addr +
VIRTIO_PCI_CONFIG_OFF(false);
+tag_len = qvirtio_config_readw(&qvirtio_pci, v9p->dev,
+ (uint64_t)(uintptr_t)addr);
+g_assert_cmpint(tag_len, ==, strlen(mount_tag));
+addr += sizeof(uint16_t);
+
+tag = g_malloc(tag_len);
+for (i = 0; i < tag_len; i++) {
+tag[i] = qvirtio_config_readb(&qvirtio_pci, v9p->dev,
+ (uint64_t)(uintptr_t)addr + i);
+}
+g_assert_cmpmem(tag, tag_len, mount_tag, tag_len);
+g_free(tag);
+
+qvirtio_9p_pci_free(v9p);
+qvirtio_9p_stop();
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
qtest_add_func("/virtio/9p/pci/nop", pci_nop);
+qtest_add_func("/virtio/9p/pci/basic/configuration", pci_basic_config);
return g_test_run();
}
[Qemu-devel] [PATCH 3/3] tests: virtio-9p: add basic transaction test
This adds a simple test to validate the device is functional: it transmits
a request with type Terror, which is not used by the 9P protocol [1], and
expects QEMU to return a reply with type Rerror and the "Operation not
supported" error string.
[1] http://lxr.free-electrons.com/source/include/net/9p/9p.h#L121
Signed-off-by: Greg Kurz
---
tests/virtio-9p-test.c | 62
1 file changed, 62 insertions(+)
diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index d7d508481bc0..d13a30cf0dee 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -8,6 +8,7 @@
*/
#include "qemu/osdep.h"
+#include
#include "libqtest.h"
#include "qemu-common.h"
#include "libqos/pci-pc.h"
@@ -17,6 +18,9 @@
#include "libqos/malloc-pc.h"
#include "standard-headers/linux/virtio_ids.h"
#include "standard-headers/linux/virtio_pci.h"
+#include "hw/9pfs/9p.h"
+
+#define QVIRTIO_9P_TIMEOUT_US (1 * 1000 * 1000)
static const char mount_tag[] = "qtest";
static char *test_share;
@@ -118,11 +122,69 @@ static void pci_basic_config(void)
qvirtio_9p_stop();
}
+typedef struct VirtIO9PHdr {
+uint32_t size;
+uint8_t id;
+uint16_t tag;
+} QEMU_PACKED VirtIO9PHdr;
+
+typedef struct VirtIO9PMsgRError {
+uint16_t error_len;
+char error[0];
+} QEMU_PACKED VirtIO9PMsgRError;
+
+#define P9_MAX_SIZE 8192
+
+static void pci_basic_transaction(void)
+{
+QVirtIO9P *v9p;
+VirtIO9PHdr hdr;
+VirtIO9PMsgRError *resp;
+uint64_t req_addr, resp_addr;
+uint32_t free_head;
+char *expected_error = strerror(ENOTSUP);
+
+qvirtio_9p_start();
+v9p = qvirtio_9p_pci_init();
+
+hdr.size = sizeof(hdr);
+hdr.id = P9_TERROR;
+hdr.tag = 12345;
+
+req_addr = guest_alloc(v9p->alloc, hdr.size);
+memwrite(req_addr, &hdr, sizeof(hdr));
+free_head = qvirtqueue_add(v9p->vq, req_addr, hdr.size, false, true);
+
+resp_addr = guest_alloc(v9p->alloc, P9_MAX_SIZE);
+qvirtqueue_add(v9p->vq, resp_addr, P9_MAX_SIZE, true, false);
+
+qvirtqueue_kick(&qvirtio_pci, v9p->dev, v9p->vq, free_head);
+guest_free(v9p->alloc, req_addr);
+qvirtio_wait_queue_isr(&qvirtio_pci, v9p->dev, v9p->vq,
+ QVIRTIO_9P_TIMEOUT_US);
+
+memread(resp_addr, &hdr, sizeof(hdr));
+g_assert_cmpint(hdr.size, <, (uint32_t) P9_MAX_SIZE);
+g_assert_cmpint(hdr.id, ==, (uint8_t) P9_RERROR);
+g_assert_cmpint(hdr.tag, ==, (uint16_t) 12345);
+
+resp = g_malloc(hdr.size);
+memread(resp_addr + sizeof(hdr), resp, hdr.size - sizeof(hdr));
+guest_free(v9p->alloc, resp_addr);
+g_assert_cmpmem(resp->error, resp->error_len, expected_error,
+strlen(expected_error));
+g_free(resp);
+
+qvirtio_9p_pci_free(v9p);
+qvirtio_9p_stop();
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
qtest_add_func("/virtio/9p/pci/nop", pci_nop);
qtest_add_func("/virtio/9p/pci/basic/configuration", pci_basic_config);
+qtest_add_func("/virtio/9p/pci/basic/transaction", pci_basic_transaction);
return g_test_run();
}
[Qemu-devel] [Bug 842290] Re: MIPS Malta mini-bootloader print function has bad jump instruction
Looks like this has been finally fixed by this commit here: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=7f81dbb9a0e89b53 ** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/842290 Title: MIPS Malta mini-bootloader print function has bad jump instruction Status in QEMU: Fix Released Bug description: One of the hardcoded bootloader library instructions in the MIPS Malta mini-bootloader's print function is: stl_raw(p++, 0x08000205); /* j 814 */ Since this function is loaded at 0xbfc00808, this jump jumps to the middle of nowhere. The properly-encoded instruction is: stl_raw(p++, 0x0bf00205); /* j 814 */ With this patch, the print function behaves as expected. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/842290/+subscriptions
Re: [Qemu-devel] [PATCH 0/3] tests: more test cases for virtio-9p
Hi, Your series seems to have some coding style problems. See output below for more information: Type: series Message-id: 147340827736.6462.8546871953640244651.stgit@bahia Subject: [Qemu-devel] [PATCH 0/3] tests: more test cases for virtio-9p === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/147340827736.6462.8546871953640244651.stgit@bahia -> patchew/147340827736.6462.8546871953640244651.stgit@bahia Switched to a new branch 'test' 04c0f1e tests: virtio-9p: add basic transaction test 5194780 tests: virtio-9p: add basic configuration test e762ebb tests: virtio-9p: introduce start/stop functions === OUTPUT BEGIN === Checking PATCH 1/3: tests: virtio-9p: introduce start/stop functions... Checking PATCH 2/3: tests: virtio-9p: add basic configuration test... ERROR: "foo* bar" should be "foo *bar" #105: FILE: tests/virtio-9p-test.c:97: +char* tag; total: 1 errors, 0 warnings, 113 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 3/3: tests: virtio-9p: add basic transaction test... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH 1/3] migrate: Fix cpu-throttle-increment regression in HMP
On Fri, Sep 9, 2016 at 7:16 AM Eric Blake wrote: > Commit 69ef1f3 accidentally broke migrate_set_parameter's ability > to set the cpu-throttle-increment to anything other than the > default, because it forgot to parse the user's string into an > integer. > > CC: qemu-sta...@nongnu.org > Signed-off-by: Eric Blake > Reviewed-by: Marc-André Lureau --- > hmp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hmp.c b/hmp.c > index ad33b44..d6c6c01 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1286,6 +1286,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const > QDict *qdict) > break; > case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT: > has_cpu_throttle_increment = true; > +use_int_value = true; > break; > case MIGRATION_PARAMETER_TLS_CREDS: > has_tls_creds = true; > -- > 2.7.4 > > > -- Marc-André Lureau
Re: [Qemu-devel] [PATCH for-2.7] vnc: fix qemu crash because of SIGSEGV
On Fr, 2016-09-09 at 07:13 +, Gonglei (Arei) wrote: > Hi Gerd, > > Can you pls pick up this patch? thanks Added to UI queue, thanks. cheers, Gerd
Re: [Qemu-devel] [PATCH v9 1/2] virtio-crypto: Add virtio crypto device specification
On Thursday, September 08, 2016 6:05 PM, Gonglei Wrote:
> +The below AEAD algorithms are defined currently:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_AEAD 0
> +#define VIRTIO_CRYPTO_AEAD_GCM1
> +#define VIRTIO_CRYPTO_AEAD_CCM2
> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
> +\end{lstlisting}
> +
> +\devicenormative{\subsection}{Device Requirements: Device configuration
> layout}\label{sec:Device Types / Crypto Device / Device configuration layout /
> Device Requirements: Device configuration layout}
Xelatex complains " Argument of \label has an extra } ", need fix.
Same complaints below when using devicenormative and label.
> +
> +\begin{itemize*}
> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
> inclusive.
> +\item The device SHOULD set \field{status} according to the status of the
> hardware-backed implementation.
> +\item The device MUST set \field{crypto_services} according to the crypto
> services which the device offered.
> +\item The device MUST set detailed algorithms mask according to
> \field{crypto_services} field.
> +\end{itemize*}
> +
> +\drivernormative{\subsection}{Driver Requirements: Device configuration
> layout}\label{sec:Device Types / Crypto Device / Device configuration layout /
> Driver Requirements: Device configuration layout}
> +
> +\begin{itemize*}
> +\item The driver MUST read the ready \field{status} from the bottom bit of
> status to
> + check whether the hardware-backed implementation is ready or not.
> +\item The driver MAY read \field{max_dataqueues} field to discover how
> many data queues the device supports.
> +\item The driver MUST read \field{crypto_services} field to discover which
> services the device is able to offer.
> +\item The driver MUST read the detailed \field{algorithms} field according to
> \field{crypto_services} field.
> +\end{itemize*}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
> Device Initialization}
> +
> +\subsubsection{Driver Requirements: Device Initialization}\label{sec:Device
> Types / Crypto Device / Device Initialization / Driver Requirements: Device
> Initialization}
> +
> +\begin{itemize*}
> +\item The driver MUST identify and initialize up to \field{max_dataqueues}
> data virtqueues.
> +\item The driver MUST identify the control virtqueue.
> +\item The driver MUST identify the ready status of hardware-backend from
> \field{status} field.
> +\item The driver MUST read the supported crypto services from bits of
> \field{crypto_servies}.
> +\item The driver MUST read the supported algorithms according to
> \field{crypto_services} field.
> +\end{itemize*}
> +
> +\subsubsection{Device Requirements: Device Initialization}\label{sec:Device
> Types / Crypto Device / Device Initialization / Device Requirements: Device
> Initialization}
> +
> +\begin{itemize*}
> +\item The device MUST be configured at least one accelerator which executes
> real crypto operations.
> +\item The device MUST write the \field{crypto_services} field according to
> the
> capacities of the backend accelerator.
> +\end{itemize*}
> +
> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> Device Operation}
> +
> +Packets can be transmitted by placing them in both the controlq and dataq.
> +Packets consist of a generic header and a service-specific request.
> +Where 'general header' is for all crypto requests, 'service specific
> requests'
> +are composed of operation parameter + output data + input data in general.
> +Operation parameters are algorithm-specific parameters, output data is the
> +data should be operated, input data is the "operation result + result
> buffer".
> +The general header of controlq:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_OPCODE(service, op) ((service << 8) | (op))
> +
> +struct virtio_crypto_ctrl_header {
> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION \
> + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> +__virtio32 opcode;
> +__virtio32 algo;
> +__virtio32 flag;
> +/* data virtqueue id */
> +__virtio32 queue_id;
> +};
> +\end{lstlisting}
> +
> +The general header of dataq:
> +
> +\begin{lstlisting}
> +
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Thu, 8 Sep 2016 19:55:16 +0300 "Michael S. Tsirkin" wrote: > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote: > > On Thu, 8 Sep 2016 18:19:27 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > > > > On Thu, 8 Sep 2016 18:00:28 +0300 > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > > If it continues > > execution, this means we're expecting the guest or the host to do something > > to fix the error condition. This requires QEMU to emit an event of some > > sort, but not necessarily to log an error message in a file. I guess this > > depends if QEMU is run by some tooling, or by a human. > > I'm not sure we need an event if tools are not expected to > do anything with it. If we limit # of times error > is printed, tools will need to reset this counter, > so we will need an event on overflow. If the device goes into a broken state, it should be discoverable from outside. I'm not sure we need an actual event signalling this if this happens due to the guest doing something wrong: That would be a task for tools monitoring _inside_ the guest. For tools monitoring the health of the machine (from the host perspective), the discovery interface would probably be enough?
[Qemu-devel] [PATCH] MAINTAINERS: add virtio-* tests
Except virtio-9p, all virtio-* tests are orphan. This patch tries to fix it, according to the following logic: - when the related subsystem has its own section in MAINTAINERS, the test is added there - otherwise it is added to the "parent" section (aka. SCSI, Network devices, virtio) Signed-off-by: Greg Kurz --- MAINTAINERS |7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index b6fb84e8267c..cc6d1cdc9165 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -826,6 +826,7 @@ Network devices M: Jason Wang S: Odd Fixes F: hw/net/ +F: tests/virtio-net-test.c T: git git://github.com/jasowang/qemu.git net SCSI @@ -833,6 +834,7 @@ M: Paolo Bonzini S: Supported F: include/hw/scsi* F: hw/scsi/* +F: tests/virtio-scsi-test.c T: git git://github.com/bonzini/qemu.git scsi-next LSI53C895A @@ -885,6 +887,7 @@ S: Supported F: hw/*/virtio* F: net/vhost-user.c F: include/hw/virtio/ +F: tests/virtio-balloon-test.c virtio-9p M: Aneesh Kumar K.V @@ -903,6 +906,7 @@ S: Supported F: hw/block/virtio-blk.c F: hw/block/dataplane/* F: hw/virtio/dataplane/* +F: tests/virtio-blk-test.c T: git git://github.com/stefanha/qemu.git block virtio-ccw @@ -925,6 +929,8 @@ S: Supported F: hw/char/virtio-serial-bus.c F: hw/char/virtio-console.c F: include/hw/virtio/virtio-serial.h +F: tests/virtio-console-test.c +F: tests/virtio-serial-test.c virtio-rng M: Amit Shah @@ -933,6 +939,7 @@ F: hw/virtio/virtio-rng.c F: include/hw/virtio/virtio-rng.h F: include/sysemu/rng*.h F: backends/rng*.c +F: tests/virtio-rng-test.c nvme M: Keith Busch
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 9 Sep 2016 10:30:53 +0200 Cornelia Huck wrote: > On Thu, 8 Sep 2016 19:55:16 +0300 > "Michael S. Tsirkin" wrote: > > > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote: > > > On Thu, 8 Sep 2016 18:19:27 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > > > > > On Thu, 8 Sep 2016 18:00:28 +0300 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > > > > If it continues > > > execution, this means we're expecting the guest or the host to do > > > something > > > to fix the error condition. This requires QEMU to emit an event of some > > > sort, but not necessarily to log an error message in a file. I guess this > > > depends if QEMU is run by some tooling, or by a human. > > > > I'm not sure we need an event if tools are not expected to > > do anything with it. If we limit # of times error > > is printed, tools will need to reset this counter, > > so we will need an event on overflow. > > If the device goes into a broken state, it should be discoverable from > outside. I'm not sure we need an actual event signalling this if this > happens due to the guest doing something wrong: That would be a task > for tools monitoring _inside_ the guest. Well, in case of a virtio device being broken, section 2.1.2 in the spec suggests to set the status to DEVICE_NEEDS_RESET and to notify it to the guest (aka. event signalling). I'll send a patch shortly. > For tools monitoring the > health of the machine (from the host perspective), the discovery > interface would probably be enough? > Yeah, probably. Cheers. -- Greg
Re: [Qemu-devel] [PATCH] iothread: Stop threads before main() quits
> > Reviewed-by: Paolo Bonzini > > Thanks! Is there a coming PULL from you that this patch could sneak in? :) There is (next week), but iothread.c is one of those grey area of maintenance that I wouldn't mind passing to someone else. Such as the owner of block/io.c. :) Paolo
Re: [Qemu-devel] [PATCH v2] target-mips: generate fences
On Thu, Sep 08, 2016 at 10:04:05AM -0700, Richard Henderson wrote:
> > +static void gen_sync(int stype)
> > +{
> > +TCGOrder tcg_mo = TCG_BAR_SC;
> > +
> > +switch (stype) {
> > +case 0x4: /* SYNC_WMB */
> > +tcg_mo |= TCG_MO_ST_ST;
> > +break;
> > +case 0x10: /* SYNC_MB */
> > +tcg_mo |= TCG_MO_ALL;
> > +break;
> > +case 0x11: /* SYNC_ACQUIRE */
> > +tcg_mo |= TCG_MO_LD_LD | TCG_MO_LD_ST;
> > +break;
> > +case 0x12: /* SYNC_RELEASE */
> > +tcg_mo |= TCG_MO_ST_ST | TCG_MO_LD_ST;
> > +break;
> > +case 0x13: /* SYNC_RMB */
> > +tcg_mo |= TCG_MO_LD_LD;
> > +break;
> > +default:
> > +tcg_mo |= TCG_MO_ALL;
> > +break;
> > +}
>
> See git://github.com/rth7680/qemu.git tcg-next wherein this suggests that some
> enhancement is desired in tcg/mips/ as well. Not that I have a mips32r6 board
> with which to test. Would you or James Hogan be able to improve that?
Actually the lightweight variants of SYNC exist also in earlier revisions of
MIPS architecture; therefore this can be tested for example on mips32r2.
According to manuals the support is optional, and if given CPU doesn't
implement lightweight SYNCs (i.e. with stype != 0) then they are supposed
to behave in the same way as SYNC 0. (which also means I simplified that here
and always interpret the stype to take advantage of weaker ordering barriers).
Thanks,
Leon
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 9 Sep 2016 10:46:25 +0200 Greg Kurz wrote: > On Fri, 9 Sep 2016 10:30:53 +0200 > Cornelia Huck wrote: > > > On Thu, 8 Sep 2016 19:55:16 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote: > > > > On Thu, 8 Sep 2016 18:19:27 +0300 > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > > > > > > On Thu, 8 Sep 2016 18:00:28 +0300 > > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > > > > > > If it continues > > > > execution, this means we're expecting the guest or the host to do > > > > something > > > > to fix the error condition. This requires QEMU to emit an event of some > > > > sort, but not necessarily to log an error message in a file. I guess > > > > this > > > > depends if QEMU is run by some tooling, or by a human. > > > > > > I'm not sure we need an event if tools are not expected to > > > do anything with it. If we limit # of times error > > > is printed, tools will need to reset this counter, > > > so we will need an event on overflow. > > > > If the device goes into a broken state, it should be discoverable from > > outside. I'm not sure we need an actual event signalling this if this > > happens due to the guest doing something wrong: That would be a task > > for tools monitoring _inside_ the guest. > > Well, in case of a virtio device being broken, section 2.1.2 in the spec > suggests to set the status to DEVICE_NEEDS_RESET and to notify it to > the guest (aka. event signalling). I'll send a patch shortly. Stefan had already sent <1460467534-29147-4-git-send-email-stefa...@redhat.com> ages ago, but it has not yet made it anywhere... Anyhow, I was concerned with host signalling (sorry for being unclear), and I still do not think we need to alert host monitoring software to guest stupidity. > > > For tools monitoring the > > health of the machine (from the host perspective), the discovery > > interface would probably be enough? > > > > Yeah, probably. > > Cheers. > > -- > Greg >
[Qemu-devel] [PATCH v2] virtio-pci: error out when both legacy and modern modes are disabled
From: Greg Kurz
Without presuming if we got there because of a user mistake or some
more subtle bug in the tooling, it really does not make sense to
implement a non-functional device.
Signed-off-by: Greg Kurz
Reviewed-by: Marcel Apfelbaum
Reviewed-by: Cornelia Huck
Signed-off-by: Greg Kurz
---
v2: - moved the check to virtio_pci_realize() to ensure disable_legacy is
either ON or OFF, but not AUTO (this would cause the check to fail
before we have a chance to know if the device can actually work).
I've kept the R-b tags, please raise a flag if this is inapropriate.
I've dropped the other patch about virtio-9p: the issue will be addressed
in a separate series.
---
hw/virtio/virtio-pci.c |8
1 file changed, 8 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 268fd8ebb219..551c4346bf4b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1770,6 +1770,14 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error
**errp)
proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
+if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
+error_setg(errp, "device cannot work as neither modern nor legacy mode"
+ " is enabled");
+error_append_hint(errp, "Set either disable-modern or disable-legacy"
+ " to off\n");
+return;
+}
+
if (pcie_port && pci_is_express(pci_dev)) {
int pos;
Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct
On Fri, Sep 9, 2016 at 7:19 AM Eric Blake wrote:
> It is rather verbose, and slightly error-prone, to repeat
> the same set of parameters for input (migrate-set-parameters)
> as for output (query-migrate-parameters), where the only
> difference is whether the members are optional. We can just
> document that the optional members will always be present
> on output, and then share a common struct between both
> commands. The next patch can then reduce the amount of
> code needed on input.
>
> Also, we made a mistake in qemu 2.7 of returning an empty
> string during 'query-migrate-parameters' when there is no
> TLS, rather than omitting TLS details entirely. Technically,
> this change risks breaking any 2.7 client that is hard-coded
> to expect the parameter's existence; on the other hand, clients
> that are portable to 2.6 already must be prepared for those
> members to not be present.
>
> And this gets rid of yet one more place where the QMP output
> visitor is silently converting a NULL string into "" (which
> is a hack I ultimately want to kill off).
>
> Signed-off-by: Eric Blake
>
Reviewed-by: Marc-André Lureau
> ---
> qapi-schema.json | 116
> +++---
> hmp.c | 9 +++-
> migration/migration.c | 7 +++
> 3 files changed, 57 insertions(+), 75 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c4f3674..4a51e5b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -647,40 +647,53 @@
> #
> # @migrate-set-parameters
> #
> -# Set the following migration parameters
> -#
> -# @compress-level: compression level
> -#
> -# @compress-threads: compression thread count
> -#
> -# @decompress-threads: decompression thread count
> -#
> -# @cpu-throttle-initial: Initial percentage of time guest cpus are
> throttled
> -#when migration auto-converge is activated. The
> -#default value is 20. (Since 2.7)
> -#
> -# @cpu-throttle-increment: throttle percentage increase each time
> -# auto-converge detects that migration is not
> making
> -# progress. The default value is 10. (Since 2.7)
> -#
> -# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> -# establishing a TLS connection over the migration data
> channel.
> -# On the outgoing side of the migration, the credentials must
> -# be for a 'client' endpoint, while for the incoming side the
> -# credentials must be for a 'server' endpoint. Setting this
> -# will enable TLS for all migrations. The default is unset,
> -# resulting in unsecured migration at the QEMU level. (Since
> 2.7)
> -#
> -# @tls-hostname: hostname of the target host for the migration. This is
> -#required when using x509 based TLS credentials and the
> -#migration URI does not already include a hostname. For
> -#example if using fd: or exec: based migration, the
> -#hostname must be provided so that the server's x509
> -#certificate identity can be validated. (Since 2.7)
> +# Set various migration parameters. See MigrationParameters for details.
> #
> # Since: 2.4
> ##
> { 'command': 'migrate-set-parameters',
> + 'data': 'MigrationParameters' }
> +
> +#
> +# @MigrationParameters
> +#
> +# Optional members can be omitted on input ('migrate-set-parameters')
> +# but most members will always be present on output
> +# ('query-migrate-parameters'), with the exception of tls-creds and
> +# tls-hostname.
> +#
> +# @compress-level: #optional compression level
> +#
> +# @compress-threads: #optional compression thread count
> +#
> +# @decompress-threads: #optional decompression thread count
> +#
> +# @cpu-throttle-initial: #optional Initial percentage of time guest cpus
> are
> +#throttledwhen migration auto-converge is
> activated.
> +#The default value is 20. (Since 2.7)
> +#
> +# @cpu-throttle-increment: #optional throttle percentage increase each
> time
> +# auto-converge detects that migration is not
> making
> +# progress. The default value is 10. (Since 2.7)
> +#
> +# @tls-creds: #optional ID of the 'tls-creds' object that provides
> credentials
> +# for establishing a TLS connection over the migration data
> +# channel. On the outgoing side of the migration, the
> credentials
> +# must be for a 'client' endpoint, while for the incoming
> side the
> +# credentials must be for a 'server' endpoint. Setting this
> +# will enable TLS for all migrations. The default is unset,
> +# resulting in unsecured migration at the QEMU level. (Since
> 2.7)
> +#
> +# @tls-hostname: #optional hostname of the target host for the migration.
> This
> +#is required
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
Greg Kurz writes: > On Fri, 09 Sep 2016 08:38:13 +0200 > Markus Armbruster wrote: > >> Greg Kurz writes: >> >> > On Thu, 8 Sep 2016 18:19:27 +0300 >> > "Michael S. Tsirkin" wrote: >> > >> >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: >> >> > On Thu, 8 Sep 2016 18:00:28 +0300 >> >> > "Michael S. Tsirkin" wrote: >> >> > >> >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: >> >> > > > On Thu, 8 Sep 2016 10:59:26 +0200 >> >> > > > Cornelia Huck wrote: >> >> > > > >> >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200 >> >> > > > > Greg Kurz wrote: >> >> > > > > >> >> > > > > > Calling assert() really makes sense when hitting a genuine bug, >> >> > > > > > which calls >> >> > > > > > for a fix in QEMU. However, when something goes wrong because >> >> > > > > > the guest >> >> > > > > > sends a malformed message, it is better to write down a more >> >> > > > > > meaningul >> >> > > > > > error message and exit. >> >> > > > > > >> >> > > > > > Signed-off-by: Greg Kurz >> >> > > > > > --- >> >> > > > > > hw/9pfs/virtio-9p-device.c | 20 ++-- >> >> > > > > > 1 file changed, 18 insertions(+), 2 deletions(-) >> >> > > > > >> >> > > > > While this is an improvement over the current state, I don't >> >> > > > > think the >> >> > > > > guest should be able to kill qemu just by doing something stupid. >> >> > > > > >> >> > > > >> >> > > > Hi Connie, >> >> > > > >> >> > > > I'm glad you're pointing this out... this was also my impression, >> >> > > > but >> >> > > > since there are a bunch of sanity checks in the virtio code that >> >> > > > cause >> >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not dare >> >> > > > stand up :) >> >> > > >> >> > > It's true that it's broken in many places but we should just >> >> > > fix them all. >> >> > > >> >> > > >> >> > > A separate question is how to log such hardware/guest bugs generally. >> >> > > People already complained about disk filling up because of us printing >> >> > > errors on each such bug. Maybe print each message only N times, and >> >> > > then set a flag to skip the log until management tells us to restart >> >> > > logging again. >> >> > >> >> > I'd expect to get the message just once per device if we set the device >> >> > to broken (unless the guess continuously resets it again...) >> >> >> >> Which it can do, so we should limit that anyway. >> >> >> >> > Do we have >> >> > a generic print/log ratelimit infrastructure in qemu? >> >> >> >> There are actually two kinds of errors >> >> host side ones and ones triggered by guests. >> >> >> >> We should distinguish between them API-wise, then >> >> we will be able to limit the logging of those >> >> that guest can trigger. >> >> >> > >> > FWIW it makes sense to use error_report() if QEMU exits. >> >> exit(STATUS) with STATUS != 0 without printing a message is always >> wrong. >> > > I fully agree. > >> > If it continues >> > execution, this means we're expecting the guest or the host to do something >> > to fix the error condition. This requires QEMU to emit an event of some >> > sort, but not necessarily to log an error message in a file. I guess this >> > depends if QEMU is run by some tooling, or by a human. >> >> error_report() normally goes to stderr. Tooling or humans can of course >> make it go to a file instead. >> >> error_report() is indeed a sub-par way to send an "attention" signal to >> the host, because recognizing such a signal reliably is unnecessary hard >> for management applications. QMP events are much easier. >> > > My wording was poor but yes, that was my point. :) > >> Both are useless when the signal needs to go to the guest. Signalling >> the guest is a device model job. >> > > I also agree with that. In the case of virtio, this is explained in section > 2.1.2 of the spec. > >> error_report() without exit() has its uses. Error conditions in need of >> fixing aren't the only reason to call error_report(). But when you add >> a call, ask yourself whether management application or guest would like >> to respond to it. > > In the case of the present patch, we currently have BUG_ON() which generates > a cryptic and unusable message. > > It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is > correct since it is now [1] impossible to hit this according to the code (see > virtqueue_pop() and virtqueue_map_desc()). > > The second one (len != sizeof out) though matches a potential guest originated > error. If I do as suggested by Connie, then the error_report() isn't needed > anymore. I dive into the details of your analysis right now, only make high-level recommendations: * Issues common to all virtio devices should be addressed in the virtio core. If that's not feasible, they should be addressed in all devices consistently. * Guest misbehavior sh
Re: [Qemu-devel] [PATCH 3/3] migrate: Use boxed qapi for migrate-set-parameters
On Fri, Sep 9, 2016 at 7:16 AM Eric Blake wrote:
> Now that QAPI makes it easy to pass a struct around, we don't
> have to declare as many parameters or local variables.
>
> Signed-off-by: Eric Blake
>
Reviewed-by: Marc-André Lureau
> ---
> qapi-schema.json | 2 +-
> hmp.c | 40 ++-
> migration/migration.c | 65
> +--
> 3 files changed, 46 insertions(+), 61 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 4a51e5b..88b4888 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -651,7 +651,7 @@
> #
> # Since: 2.4
> ##
> -{ 'command': 'migrate-set-parameters',
> +{ 'command': 'migrate-set-parameters', 'boxed': true,
>'data': 'MigrationParameters' }
>
> #
> diff --git a/hmp.c b/hmp.c
> index 1e4094a..0893b8e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1260,44 +1260,40 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
> const char *valuestr = qdict_get_str(qdict, "value");
> long valueint = 0;
> Error *err = NULL;
> -bool has_compress_level = false;
> -bool has_compress_threads = false;
> -bool has_decompress_threads = false;
> -bool has_cpu_throttle_initial = false;
> -bool has_cpu_throttle_increment = false;
> -bool has_tls_creds = false;
> -bool has_tls_hostname = false;
> bool use_int_value = false;
> int i;
>
> for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
> if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
> +MigrationParameters p = { 0 };
> switch (i) {
> case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> -has_compress_level = true;
> +p.has_compress_level = true;
> use_int_value = true;
> break;
> case MIGRATION_PARAMETER_COMPRESS_THREADS:
> -has_compress_threads = true;
> +p.has_compress_threads = true;
> use_int_value = true;
> break;
> case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
> -has_decompress_threads = true;
> +p.has_decompress_threads = true;
> use_int_value = true;
> break;
> case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
> -has_cpu_throttle_initial = true;
> +p.has_cpu_throttle_initial = true;
> use_int_value = true;
> break;
> case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
> -has_cpu_throttle_increment = true;
> +p.has_cpu_throttle_increment = true;
> use_int_value = true;
> break;
> case MIGRATION_PARAMETER_TLS_CREDS:
> -has_tls_creds = true;
> +p.has_tls_creds = true;
> +p.tls_creds = (char *) valuestr;
> break;
> case MIGRATION_PARAMETER_TLS_HOSTNAME:
> -has_tls_hostname = true;
> +p.has_tls_hostname = true;
> +p.tls_hostname = (char *) valuestr;
> break;
> }
>
> @@ -1307,16 +1303,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
> valuestr);
> goto cleanup;
> }
> +/* Set all integers; only one has_FOO will be set, and
> + * the code ignores the remaining values */
> +p.compress_level = valueint;
> +p.compress_threads = valueint;
> +p.decompress_threads = valueint;
> +p.cpu_throttle_initial = valueint;
> +p.cpu_throttle_increment = valueint;
> }
>
> -qmp_migrate_set_parameters(has_compress_level, valueint,
> - has_compress_threads, valueint,
> - has_decompress_threads, valueint,
> - has_cpu_throttle_initial, valueint,
> - has_cpu_throttle_increment,
> valueint,
> - has_tls_creds, valuestr,
> - has_tls_hostname, valuestr,
> - &err);
> +qmp_migrate_set_parameters(&p, &err);
> break;
> }
> }
> diff --git a/migration/migration.c b/migration/migration.c
> index 1a8f26b..42336e3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -766,78 +766,67 @@ void
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> }
> }
>
> -void qmp_migrate_set_parameters(bool has_compress_level,
> -int64_t compress_level,
> -bool has_compress_threads,
> -
[Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Uses throttling APIs to limit I/O bandwidth and number of operations on the
devices which use 9p-local driver.
Signed-off-by: Pradeep Jagadeesh
---
fsdev/file-op-9p.h | 3 +
fsdev/qemu-fsdev-opts.c | 52 +
hw/9pfs/9p-local.c | 18 -
hw/9pfs/9p-throttle.c | 201
hw/9pfs/9p-throttle.h | 46 +++
hw/9pfs/9p.c| 7 ++
hw/9pfs/Makefile.objs | 1 +
7 files changed, 326 insertions(+), 2 deletions(-)
create mode 100644 hw/9pfs/9p-throttle.c
create mode 100644 hw/9pfs/9p-throttle.h
This adds the support for the 9p-local driver.
For now this functionality can be enabled only through qemu cli options.
QMP interface and support to other drivers need further extensions.
To make it simple for other drivers, the throttle code has been put in
separate files.
v1 -> v2:
-Fixed FsContext redeclaration issue
-Removed couple of function declarations from 9p-throttle.h
-Fixed some of the .help messages
diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
index 6db9fea..e86b91a 100644
--- a/fsdev/file-op-9p.h
+++ b/fsdev/file-op-9p.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include "hw/9pfs/9p-throttle.h"
#define SM_LOCAL_MODE_BITS0600
#define SM_LOCAL_DIR_MODE_BITS0700
@@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
char *path;
int export_flags;
FileOperations *ops;
+FsThrottle fst;
} FsDriverEntry;
typedef struct FsContext
@@ -83,6 +85,7 @@ typedef struct FsContext
int export_flags;
struct xattr_operations **xops;
struct extended_ops exops;
+FsThrottle *fst;
/* fs driver specific data */
void *private;
} FsContext;
diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
index 1dd8c7a..2774855 100644
--- a/fsdev/qemu-fsdev-opts.c
+++ b/fsdev/qemu-fsdev-opts.c
@@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
}, {
.name = "sock_fd",
.type = QEMU_OPT_NUMBER,
+}, {
+.name = "bps",
+.type = QEMU_OPT_NUMBER,
+.help = "limit total bytes per second",
+}, {
+.name = "bps_rd",
+.type = QEMU_OPT_NUMBER,
+.help = "limit read bytes per second",
+}, {
+.name = "bps_wr",
+.type = QEMU_OPT_NUMBER,
+.help = "limit write bytes per second",
+}, {
+.name = "iops",
+.type = QEMU_OPT_NUMBER,
+.help = "limit total io operations per second",
+}, {
+.name = "iops_rd",
+.type = QEMU_OPT_NUMBER,
+.help = "limit read operations per second ",
+}, {
+.name = "iops_wr",
+.type = QEMU_OPT_NUMBER,
+.help = "limit write operations per second",
+}, {
+.name = "bps_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum bytes burst",
+}, {
+.name = "bps_rd_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum bytes read burst",
+}, {
+.name = "bps_wr_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum bytes write burst",
+}, {
+.name = "iops_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum io operations burst",
+}, {
+.name = "iops_rd_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum io operations read burst",
+}, {
+.name = "iops_wr_max",
+.type = QEMU_OPT_NUMBER,
+.help = "maximum io operations write burst",
+}, {
+.name = "iops_size",
+.type = QEMU_OPT_NUMBER,
+.help = "io iops-size",
},
{ /*End of list */ }
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3f271fc..49c2819 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx,
V9fsFidOpenState *fs,
const struct iovec *iov,
int iovcnt, off_t offset)
{
+throttle9p_request(ctx->fst, false, iov->iov_len);
+
#ifdef CONFIG_PREADV
return preadv(fs->fd, iov, iovcnt, offset);
#else
@@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx,
V9fsFidOpenState *fs,
const struct iovec *iov,
int iovcnt, off_t offset)
{
-ssize_t ret
-;
+ssize_t ret;
+
+throttle9p_request(ctx->fst, true, iov->iov_len);
+
#ifdef CONFIG_PREADV
ret = pwritev(fs->fd, iov, iovcnt, offset);
#else
@@ -1213,6 +1217,9 @@ static int local_parse_opts(QemuOpts *opts, struct
FsDriverEntry *fse)
const char *sec_model = qemu_opt_get(opts, "security_model");
const char *path = qemu_opt_get(opts, "path");
+/* get the throttle structure */
+FsThrottle *fst = &fse->fst;
+
if (!sec_model) {
Re: [Qemu-devel] DAX can not work on virtual nvdimm device
On Thu 08-09-16 14:47:08, Ross Zwisler wrote: > On Tue, Sep 06, 2016 at 05:06:20PM +0200, Jan Kara wrote: > > On Thu 01-09-16 20:57:38, Ross Zwisler wrote: > > > On Wed, Aug 31, 2016 at 04:44:47PM +0800, Xiao Guangrong wrote: > > > > On 08/31/2016 01:09 AM, Dan Williams wrote: > > > > > > > > > > Can you post your exact reproduction steps? This test is not failing > > > > > for me. > > > > > > > > > > > > > Sure. > > > > > > > > 1. make the guest kernel based on your tree, the top commit is > > > >10d7902fa0e82b (dax: unmap/truncate on device shutdown) and > > > >the config file can be found in this thread. > > > > > > > > 2. add guest kernel command line: memmap=6G!10G > > > > > > > > 3: start the guest: > > > >x86_64-softmmu/qemu-system-x86_64 -machine pc,nvdimm --enable-kvm \ > > > >-smp 16 -m 32G,maxmem=100G,slots=100 /other/VMs/centos6.img -monitor > > > > stdio > > > > > > > > 4: in guest: > > > >mkfs.ext4 /dev/pmem0 > > > >mount -o dax /dev/pmem0 /mnt/pmem/ > > > >echo > /mnt/pmem/xxx > > > >./mmap /mnt/pmem/xxx > > > >./read /mnt/pmem/xxx > > > > > > > > The source code of mmap and read has been attached in this mail. > > > > > > > > Hopefully, you can detect the error triggered by read test. > > > > > > > > Thanks! > > > > > > Okay, I think I've isolated this issue. Xiao's VM was an old CentOS 6 > > > system, > > > and for some reason ext4+DAX with the old tools found in that VM fails. > > > I was > > > able to reproduce this failure with a freshly installed CentOS 6.8 VM. > > > > > > You can see the failure with his tests, or perhaps more easily with this > > > series of commands: > > > > > > # mkfs.ext4 /dev/pmem0 > > > # mount -o dax /dev/pmem0 /mnt/pmem/ > > > # touch /mnt/pmem/x > > > # md5sum /mnt/pmem/x > > > md5sum: /mnt/pmem/x: Bad address > > > > > > This sequence of commands works fine in the old CentOS 6 system if you > > > use XFS > > > instead of ext4, and it works fine with both ext4 and XFS in CentOS 7 and > > > with recent versions of Fedora. > > > > > > I've added the ext4 folks to this mail in case they care, but my guess is > > > that > > > the tools in CentOS 6 are so old that it's not worth worrying about. For > > > reference, the kernel in CentOS 6 is based on 2.6.32. :) DAX was > > > introduced > > > in v4.0. > > > > Hum, can you post 'dumpe2fs -h /dev/pmem0' output from that system when the > > md5sum fails? Because the only idea I have is that mkfs.ext4 in CentOS 6 > > creates the filesystem with a different set of features than more recent > > e2fsprogs and so we hit some untested path... > > Sure, here's the output: > > # dumpe2fs -h /dev/pmem0 > dumpe2fs 1.41.12 (17-May-2010) > Filesystem volume name: > Last mounted on: /mnt/pmem > Filesystem UUID: 4cd8a836-cc54-4c59-ae0a-4a26bab0f8bc > Filesystem magic number: 0xEF53 > Filesystem revision #:1 (dynamic) > Filesystem features: has_journal ext_attr resize_inode dir_index filetype > needs_recovery extent flex_bg sparse_super large_file huge_file uninit_bg > dir_nlink extra_isize > Filesystem flags: signed_directory_hash > Default mount options:(none) > Filesystem state: clean > Errors behavior: Continue > Filesystem OS type: Linux > Inode count: 1048576 > Block count: 4194304 > Reserved block count: 209715 > Free blocks: 4084463 > Free inodes: 1048565 > First block: 0 > Block size: 4096 > Fragment size:4096 > Reserved GDT blocks: 1023 > Blocks per group: 32768 > Fragments per group: 32768 > Inodes per group: 8192 > Inode blocks per group: 512 > RAID stride: 1 > Flex block group size:16 > Filesystem created: Thu Sep 8 14:45:31 2016 > Last mount time: Thu Sep 8 14:45:39 2016 > Last write time: Thu Sep 8 14:45:39 2016 > Mount count: 1 > Maximum mount count: 21 > Last checked: Thu Sep 8 14:45:31 2016 > Check interval: 15552000 (6 months) > Next check after: Tue Mar 7 13:45:31 2017 > Lifetime writes: 388 MB > Reserved blocks uid: 0 (user root) > Reserved blocks gid: 0 (group root) > First inode: 11 > Inode size: 256 > Required extra isize: 28 > Desired extra isize: 28 > Journal inode:8 > Default directory hash: half_md4 > Directory Hash Seed: 19cad581-c46a-4212-bfa0-d527ff55db49 > Journal backup: inode blocks > Journal features: (none) > Journal size: 128M > Journal length: 32768 > Journal sequence: 0x0002 > Journal start:1 Hum, nothing unusual in there. I've tried reproducing on a local SLE11 SP3 machine (which is from about the same time) but everything works as expected there. Shrug...
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 9 Sep 2016 10:53:05 +0200 Cornelia Huck wrote: > On Fri, 9 Sep 2016 10:46:25 +0200 > Greg Kurz wrote: > > > On Fri, 9 Sep 2016 10:30:53 +0200 > > Cornelia Huck wrote: > > > > > On Thu, 8 Sep 2016 19:55:16 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Thu, Sep 08, 2016 at 06:26:52PM +0200, Greg Kurz wrote: > > > > > On Thu, 8 Sep 2016 18:19:27 +0300 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > > > > > > > On Thu, 8 Sep 2016 18:00:28 +0300 > > > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > > > > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > > > > > > > > If it continues > > > > > execution, this means we're expecting the guest or the host to do > > > > > something > > > > > to fix the error condition. This requires QEMU to emit an event of > > > > > some > > > > > sort, but not necessarily to log an error message in a file. I guess > > > > > this > > > > > depends if QEMU is run by some tooling, or by a human. > > > > > > > > I'm not sure we need an event if tools are not expected to > > > > do anything with it. If we limit # of times error > > > > is printed, tools will need to reset this counter, > > > > so we will need an event on overflow. > > > > > > If the device goes into a broken state, it should be discoverable from > > > outside. I'm not sure we need an actual event signalling this if this > > > happens due to the guest doing something wrong: That would be a task > > > for tools monitoring _inside_ the guest. > > > > Well, in case of a virtio device being broken, section 2.1.2 in the spec > > suggests to set the status to DEVICE_NEEDS_RESET and to notify it to > > the guest (aka. event signalling). I'll send a patch shortly. > > Stefan had already sent > <1460467534-29147-4-git-send-email-stefa...@redhat.com> ages ago, but > it has not yet made it anywhere... > I don't know what to do with this message-id :\ > Anyhow, I was concerned with host signalling (sorry for being unclear), > and I still do not think we need to alert host monitoring software to > guest stupidity. > I agree. Sorry if my poor wording made you (and others) think I was suggesting that :) My point was that if QEMU exits because of guest stupidity, you are forced to error_report() something to the host, but this is really suboptimal (even if BUG_ON is worse)... then there was that discussion about log files getting to big, but I don't even know how we came there, as it does not really make sense when QEMU exits. > > > > > For tools monitoring the > > > health of the machine (from the host perspective), the discovery > > > interface would probably be enough? > > > > > > > Yeah, probably. > > > > Cheers. > > > > -- > > Greg > > >
Re: [Qemu-devel] [PATCH 6/6] crypto: support more hash algorithms for pbkdf
On Thu, Sep 08, 2016 at 12:57:38PM -0500, Eric Blake wrote:
> On 09/08/2016 11:27 AM, Daniel P. Berrange wrote:
> > Currently pbkdf is only supported with SHA1 and SHA256. Expand
> > this to support all algorithms known to QEMU.
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > crypto/pbkdf-gcrypt.c | 12 +-
> > crypto/pbkdf-nettle.c | 61
> > ---
> > tests/test-crypto-pbkdf.c | 53 +++-
> > 3 files changed, 115 insertions(+), 11 deletions(-)
> >
>
> >
> > if (hash >= G_N_ELEMENTS(hash_map) ||
> > hash_map[hash] == GCRY_MD_NONE) {
> > -error_setg(errp, "Unexpected hash algorithm %d", hash);
> > +error_setg_errno(errp, ENOSYS,
> > + "PBKDF does not support hash algorithm %s",
> > + QCryptoHashAlgorithm_lookup[hash]);
>
> Can this access beyond bounds if hash > G_N_ELEMENTS(hash_map)?
I'm relying on fact that 'hash' came from QAPIs string -> enum
convertor - nothing in QEMU lets users provide raw integer enum
values - so will be guaranteed to be a valid enum value.
> > +++ b/crypto/pbkdf-nettle.c
>
> >
> > default:
> > error_setg_errno(errp, ENOSYS,
> > - "PBKDF does not support hash algorithm %d", hash);
> > + "PBKDF does not support hash algorithm %s",
> > + QCryptoHashAlgorithm_lookup[hash]);
>
> and again
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 1/6] crypto: make PBKDF iterations configurable for LUKS format
On Thu, Sep 08, 2016 at 12:44:55PM -0500, Eric Blake wrote:
> On 09/08/2016 11:27 AM, Daniel P. Berrange wrote:
> > As protection against bruteforcing passphrases, the PBKDF
> > algorithm is tuned by counting the number of iterations
> > needed to produce 1 second of running time. If the machine
> > that the image will be used on is much faster than the
> > machine where the image is created, it can be desirable
> > to raise the number of limits. This adds a new 'iter-time'
>
> s/limits/iterations/ ?
>
> > property that allows the user to choose the iteration
> > wallclock time.
> >
> > Signed-off-by: Daniel P. Berrange
> > ---
> > block/crypto.c | 6 ++
> > crypto/block-luks.c | 32 +++-
> > qapi/crypto.json| 6 +-
> > 3 files changed, 34 insertions(+), 10 deletions(-)
> >
>
> > +++ b/crypto/block-luks.c
> > @@ -917,8 +917,12 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > const char *hash_alg;
> > char *cipher_mode_spec = NULL;
> > QCryptoCipherAlgorithm ivcipheralg = 0;
> > +uint64_t iters;
> >
> > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
> > +if (!luks_opts.has_iter_time) {
> > +luks_opts.iter_time = 1000;
> > +}
> > if (!luks_opts.has_cipher_alg) {
> > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
> > }
> > @@ -1064,7 +1068,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > /* Determine how many iterations we need to hash the master
> > * key, in order to have 1 second of compute time used
> > */
> > -luks->header.master_key_iterations =
> > +iters = luks_opts.iter_time *
> > qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
>
> luks_opts.iter_time is a user-provided 64-bit value, so this
> multiplication can overflow...
Oh doh, there I was thinkig it was just a 32bit int...
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 2/6] crypto: clear out buffer after timing pbkdf algorithm
On Thu, Sep 08, 2016 at 12:47:43PM -0500, Eric Blake wrote: > On 09/08/2016 11:27 AM, Daniel P. Berrange wrote: > > The 'out' buffer will hold a key derived from master > > password, so it is best practice to clear this buffer > > when no longer required. > > > > Signed-off-by: Daniel P. Berrange > > --- > > crypto/pbkdf.c | 15 ++- > > 1 file changed, 10 insertions(+), 5 deletions(-) > > Reviewed-by: Eric Blake > > It still doesn't prevent the memory from being copied elsewhere (such as > the stack being paged out), unless we go to extraordinary lengths to > explicitly request volatile memory that can't be paged out. I don't > know if we need to worry about that, though. Do any of our crypto > libraries provide APIs for allocating local-use-only memory for > sensitive data? AFAICT, while gcrypt uses such APIs internally, it doesn't expose them to users. Nettle avoids malloc entirely in its API. So if we wanted that we'd basically need to roll our own. I don't think this is a big deal, but I just wanted to add the memset() as a sanity check really. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 9 Sep 2016 11:26:17 +0200 Greg Kurz wrote: > > > > Stefan had already sent > > <1460467534-29147-4-git-send-email-stefa...@redhat.com> ages ago, but > > it has not yet made it anywhere... > > > > I don't know what to do with this message-id :\ I finally found :) +message-id:<1460467534-29147-4-git-send-email-stefa...@redhat.com> in the search box at https://lists.nongnu.org/archive/html/qemu-devel/ Cheers. -- Greg
Re: [Qemu-devel] [PATCH v9 1/2] virtio-crypto: Add virtio crypto device specification
Hi Xin,
Will fix in the next version, thanks!
Regards,
-Gonglei
> -Original Message-
> From: Zeng, Xin [mailto:xin.z...@intel.com]
> Sent: Friday, September 09, 2016 4:26 PM
> To: Gonglei (Arei); qemu-devel@nongnu.org; virtio-...@lists.oasis-open.org
> Cc: Huangpeng (Peter); Luonengjun; m...@redhat.com;
> cornelia.h...@de.ibm.com; stefa...@redhat.com;
> denglin...@chinamobile.com; Jani Kokkonen; ola.liljed...@arm.com;
> varun.se...@freescale.com; Keating, Brian A; Ma, Liang J; Griffin, John;
> Hanweidong (Randy); Huangweidong (C); mike.cara...@nxp.com;
> ag...@suse.de; Claudio Fontana
> Subject: RE: [PATCH v9 1/2] virtio-crypto: Add virtio crypto device
> specification
>
> On Thursday, September 08, 2016 6:05 PM, Gonglei Wrote:
>
> > +The below AEAD algorithms are defined currently:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_NO_AEAD 0
> > +#define VIRTIO_CRYPTO_AEAD_GCM1
> > +#define VIRTIO_CRYPTO_AEAD_CCM2
> > +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
> > +\end{lstlisting}
> > +
> > +\devicenormative{\subsection}{Device Requirements: Device configuration
> > layout}\label{sec:Device Types / Crypto Device / Device configuration
> > layout /
> > Device Requirements: Device configuration layout}
>
> Xelatex complains " Argument of \label has an extra } ", need fix.
> Same complaints below when using devicenormative and label.
>
> > +
> > +\begin{itemize*}
> > +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
> > inclusive.
> > +\item The device SHOULD set \field{status} according to the status of the
> > hardware-backed implementation.
> > +\item The device MUST set \field{crypto_services} according to the crypto
> > services which the device offered.
> > +\item The device MUST set detailed algorithms mask according to
> > \field{crypto_services} field.
> > +\end{itemize*}
> > +
> > +\drivernormative{\subsection}{Driver Requirements: Device configuration
> > layout}\label{sec:Device Types / Crypto Device / Device configuration
> > layout /
> > Driver Requirements: Device configuration layout}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST read the ready \field{status} from the bottom bit of
> > status to
> > + check whether the hardware-backed implementation is ready or not.
> > +\item The driver MAY read \field{max_dataqueues} field to discover how
> > many data queues the device supports.
> > +\item The driver MUST read \field{crypto_services} field to discover which
> > services the device is able to offer.
> > +\item The driver MUST read the detailed \field{algorithms} field according
> > to
> > \field{crypto_services} field.
> > +\end{itemize*}
> > +
> > +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device /
> > Device Initialization}
> > +
> > +\subsubsection{Driver Requirements: Device Initialization}\label{sec:Device
> > Types / Crypto Device / Device Initialization / Driver Requirements: Device
> > Initialization}
> > +
> > +\begin{itemize*}
> > +\item The driver MUST identify and initialize up to \field{max_dataqueues}
> > data virtqueues.
> > +\item The driver MUST identify the control virtqueue.
> > +\item The driver MUST identify the ready status of hardware-backend from
> > \field{status} field.
> > +\item The driver MUST read the supported crypto services from bits of
> > \field{crypto_servies}.
> > +\item The driver MUST read the supported algorithms according to
> > \field{crypto_services} field.
> > +\end{itemize*}
> > +
> > +\subsubsection{Device Requirements: Device Initialization}\label{sec:Device
> > Types / Crypto Device / Device Initialization / Device Requirements: Device
> > Initialization}
> > +
> > +\begin{itemize*}
> > +\item The device MUST be configured at least one accelerator which
> executes
> > real crypto operations.
> > +\item The device MUST write the \field{crypto_services} field according to
> the
> > capacities of the backend accelerator.
> > +\end{itemize*}
> > +
> > +\subsection{Device Operation}\label{sec:Device Types / Crypto Device /
> > Device Operation}
> > +
> > +Packets can be transmitted by placing them in both the controlq and dataq.
> > +Packets consist of a generic header and a service-specific request.
> > +Where 'general header' is for all crypto requests, 'service specific
> > requests'
> > +are composed of operation parameter + output data + input data in general.
> > +Operation parameters are algorithm-specific parameters, output data is the
> > +data should be operated, input data is the "operation result + result
> > buffer".
> > +The general header of controlq:
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_CRYPTO_OPCODE(service, op) ((service << 8) | (op))
> > +
> > +struct virtio_crypto_ctrl_header {
> > +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION \
> > + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER,
> 0x02)
> > +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION \
> > + VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIP
Re: [Qemu-devel] [PATCH 1/2] virtio-9p: print error message and exit instead of BUG_ON()
On Fri, 09 Sep 2016 11:08:56 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Fri, 09 Sep 2016 08:38:13 +0200 > > Markus Armbruster wrote: > > > >> Greg Kurz writes: > >> > >> > On Thu, 8 Sep 2016 18:19:27 +0300 > >> > "Michael S. Tsirkin" wrote: > >> > > >> >> On Thu, Sep 08, 2016 at 05:04:47PM +0200, Cornelia Huck wrote: > >> >> > On Thu, 8 Sep 2016 18:00:28 +0300 > >> >> > "Michael S. Tsirkin" wrote: > >> >> > > >> >> > > On Thu, Sep 08, 2016 at 11:12:16AM +0200, Greg Kurz wrote: > >> >> > > > On Thu, 8 Sep 2016 10:59:26 +0200 > >> >> > > > Cornelia Huck wrote: > >> >> > > > > >> >> > > > > On Wed, 07 Sep 2016 19:19:24 +0200 > >> >> > > > > Greg Kurz wrote: > >> >> > > > > > >> >> > > > > > Calling assert() really makes sense when hitting a genuine > >> >> > > > > > bug, which calls > >> >> > > > > > for a fix in QEMU. However, when something goes wrong because > >> >> > > > > > the guest > >> >> > > > > > sends a malformed message, it is better to write down a more > >> >> > > > > > meaningul > >> >> > > > > > error message and exit. > >> >> > > > > > > >> >> > > > > > Signed-off-by: Greg Kurz > >> >> > > > > > --- > >> >> > > > > > hw/9pfs/virtio-9p-device.c | 20 ++-- > >> >> > > > > > 1 file changed, 18 insertions(+), 2 deletions(-) > >> >> > > > > > >> >> > > > > While this is an improvement over the current state, I don't > >> >> > > > > think the > >> >> > > > > guest should be able to kill qemu just by doing something > >> >> > > > > stupid. > >> >> > > > > > >> >> > > > > >> >> > > > Hi Connie, > >> >> > > > > >> >> > > > I'm glad you're pointing this out... this was also my impression, > >> >> > > > but > >> >> > > > since there are a bunch of sanity checks in the virtio code that > >> >> > > > cause > >> >> > > > QEMU to exit (even recently added like 1e7aed70144b), I did not > >> >> > > > dare > >> >> > > > stand up :) > >> >> > > > >> >> > > It's true that it's broken in many places but we should just > >> >> > > fix them all. > >> >> > > > >> >> > > > >> >> > > A separate question is how to log such hardware/guest bugs > >> >> > > generally. > >> >> > > People already complained about disk filling up because of us > >> >> > > printing > >> >> > > errors on each such bug. Maybe print each message only N times, and > >> >> > > then set a flag to skip the log until management tells us to restart > >> >> > > logging again. > >> >> > > >> >> > I'd expect to get the message just once per device if we set the > >> >> > device > >> >> > to broken (unless the guess continuously resets it again...) > >> >> > >> >> Which it can do, so we should limit that anyway. > >> >> > >> >> > Do we have > >> >> > a generic print/log ratelimit infrastructure in qemu? > >> >> > >> >> There are actually two kinds of errors > >> >> host side ones and ones triggered by guests. > >> >> > >> >> We should distinguish between them API-wise, then > >> >> we will be able to limit the logging of those > >> >> that guest can trigger. > >> >> > >> > > >> > FWIW it makes sense to use error_report() if QEMU exits. > >> > >> exit(STATUS) with STATUS != 0 without printing a message is always > >> wrong. > >> > > > > I fully agree. > > > >> > If it continues > >> > execution, this means we're expecting the guest or the host to do > >> > something > >> > to fix the error condition. This requires QEMU to emit an event of some > >> > sort, but not necessarily to log an error message in a file. I guess this > >> > depends if QEMU is run by some tooling, or by a human. > >> > >> error_report() normally goes to stderr. Tooling or humans can of course > >> make it go to a file instead. > >> > >> error_report() is indeed a sub-par way to send an "attention" signal to > >> the host, because recognizing such a signal reliably is unnecessary hard > >> for management applications. QMP events are much easier. > >> > > > > My wording was poor but yes, that was my point. :) > > > >> Both are useless when the signal needs to go to the guest. Signalling > >> the guest is a device model job. > >> > > > > I also agree with that. In the case of virtio, this is explained in section > > 2.1.2 of the spec. > > > >> error_report() without exit() has its uses. Error conditions in need of > >> fixing aren't the only reason to call error_report(). But when you add > >> a call, ask yourself whether management application or guest would like > >> to respond to it. > > > > In the case of the present patch, we currently have BUG_ON() which generates > > a cryptic and unusable message. > > > > It turns out that the first one (elem->out_num == 0 || elem->in_num == 0) is > > correct since it is now [1] impossible to hit this according to the code > > (see > > virtqueue_pop() and virtqueue_map_desc()). > > > > The second one
[Qemu-devel] [PATCH RFC] exec: remove duplicate madvise to QEMU_MADV_DONTFORK
kvm_setup_guest_memory() also just does "madivse to QEMU_MADV_DONTFORK".
Signed-off-by: Cao jin
---
Though I don't understand these code well, tt seems is duplicate code to me.
But I am not sure whether I am doing the right thing, so, RFC.
exec.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/exec.c b/exec.c
index 80398b0..819f7cd 100644
--- a/exec.c
+++ b/exec.c
@@ -1623,9 +1623,6 @@ static void ram_block_add(RAMBlock *new_block, Error
**errp)
qemu_ram_setup_dump(new_block->host, new_block->max_length);
qemu_madvise(new_block->host, new_block->max_length,
QEMU_MADV_HUGEPAGE);
qemu_madvise(new_block->host, new_block->max_length,
QEMU_MADV_DONTFORK);
-if (kvm_enabled()) {
-kvm_setup_guest_memory(new_block->host, new_block->max_length);
-}
}
}
--
2.1.0
Re: [Qemu-devel] [PULLv2 00/25] Leak patches
On 8 September 2016 at 15:16, Marc-André Lureau wrote: > The following changes since commit 59351d9b40b1de0fb77e1ff3e53faa04c995c707: > > Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.8-20160907' > into staging (2016-09-08 11:28:12 +0100) > > are available in the git repository at: > > g...@github.com:elmarco/qemu.git tags/leak-pull-request > > for you to fetch changes up to e2dd21e510ed66daeb4c5d58638450c1fb8c6fea: > > tests: fix postcopy-test leaks (2016-09-08 18:05:22 +0400) > > > Pull request > > v2: > - dropped "tests: fix small leak in test-io-channel-command" that Daniel > Berrange will pick > - fixed "tests: add qtest_add_data_func_full" to work with glib < 2.26 > > > Applied, thanks. -- PMM
[Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
This patch adds virtio_test_backend_feature() function to
enable checking a backend feature before the negociation
takes place.
It may be used, for example, to check whether the backend
supports VIRTIO_F_VERSION_1 before enabling modern
capabilities.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio.c | 14 ++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..7ab91a1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
size_t size)
virtio_save(VIRTIO_DEVICE(opaque), f);
}
+bool virtio_test_backend_feature(VirtIODevice *vdev,
+ unsigned int fbit, Error **errp)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+uint64_t feature;
+
+virtio_add_feature(&feature, fbit);
+
+assert(k->get_features != NULL);
+feature = k->get_features(vdev, feature, errp);
+
+return virtio_has_feature(feature, fbit);
+}
+
static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d2490c1..5fb74c8 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -235,6 +235,8 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
+bool virtio_test_backend_feature(VirtIODevice *vdev,
+ unsigned int fbit, Error **errp);
/* Base devices. */
typedef struct VirtIOBlkConf VirtIOBlkConf;
--
2.7.4
[Qemu-devel] [PATCH 0/2] virtio-pci: Improve device plugging whith legacy backends
This series makes device plugging more robust, to avoid guest to be confused when the backend doesn't support VIRTIO_F_VERSION_1. The problem is seen with Linux guests running mainline kernels, when backend doesn't support the feature: virtio_net virtio0: virtio: device uses modern interface but does not have VIRTIO_F_VERSION_1. When it happens, the modern device probe returns -EINVAL, whereas its caller expects -ENODEV being returned to switch to legacy device probing. We need to make QEMU more robust to ensure the guest won't be confused, so this series exposes modern interface only when backend support it. It has been tested with vhost-net and vhost-user backends in client and server modes. Maxime Coquelin (2): virtio: Add backend feature testing functionnality virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 hw/virtio/virtio-pci.c | 15 +++ hw/virtio/virtio-pci.h | 5 + hw/virtio/virtio.c | 14 ++ include/hw/virtio/virtio.h | 2 ++ 4 files changed, 36 insertions(+) -- 2.7.4
[Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio-pci.c | 15 +++
hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 755f921..0e5d59c 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d,
Error **errp)
uint32_t size;
VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+/*
+ * Virtio capabilities present without
+ * VIRTIO_F_VERSION_1 confuses guests
+ */
+if (!virtio_test_backend_feature(vdev, VIRTIO_F_VERSION_1, errp)) {
+virtio_pci_disable_modern(proxy);
+}
+
+legacy = virtio_pci_legacy(proxy);
+modern = virtio_pci_modern(proxy);
+if (!legacy && !modern) {
+error_setg(errp, "PCI device is neither legacy nor modern.");
+return;
+}
+
config = proxy->pci_dev.config;
if (proxy->class_code) {
pci_config_set_class(config, proxy->class_code);
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 25fbf8a..4e976b3 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -172,6 +172,11 @@ static inline void
virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
proxy->disable_legacy = ON_OFF_AUTO_ON;
}
+static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
+{
+proxy->disable_modern = true;
+}
+
/*
* virtio-scsi-pci: This extends VirtioPCIProxy.
*/
--
2.7.4
[Qemu-devel] [Bug 965327] Re: virtio-pci: can't reserve io 0x0000-0x001f
Closing according to comment #3 ** Changed in: qemu Status: New => Fix Released -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/965327 Title: virtio-pci: can't reserve io 0x-0x001f Status in QEMU: Fix Released Bug description: Before 2012-03-05 I was able to successfully enable a virtio-pci block device from a sPAPR pseries ppc64 Linux guest. With the current git master branch after this date I get the following error: virtio-pci :00:00.0: device not available (can't reserve [io 0x-0x001f]) virtio-pci: probe of :00:00.0 failed with error -22 virtio-pci :00:01.0: device not available (can't reserve [io 0x-0x003f]) virtio-pci: probe of :00:01.0 failed with error -22 Full details: - command line: - ./testing/qemu/ppc64-softmmu/qemu-system-ppc64 \ -L ./testing/qemu/pc-bios \ -M pseries \ -m 1024 \ -rtc base=localtime \ -parallel none \ -netdev type=user,id=mynet0,hostfwd=tcp:127.0.0.1:9011-10.0.2.11:22 \ -device virtio-net-pci,netdev=mynet0 \ -drive file=images/suse-ppc.img,if=virtio,index=0,media=disk,cache=unsafe \ -kernel images/iso/suseboot/vmlinux \ -append "root=/dev/mapper/system-root ro audit=0 selinux=0 apparmor=0 console=tty0 console=ttyPZ0" \ -initrd images/iso/suseboot/initrd.img \ -gdb tcp::1234 -- BEFORE virtio-pci "bug/user error?" introduced: -- sPAPR memory map: RTAS : 0x3fff..3fff0013 FDT : 0x3ffe..3ffe Kernel : 0x0040..01abad7b Ramdisk : 0x01ad..02053df7 Firmware load: 0x..000d6ec0 Firmware runtime : 0x3d7e..3ffe sPAPR reset SLOF ** QEMU Starting Build Date = Mar 3 2012 21:46:40 FW Version = git-440e662879c4fc3c Press "s" to enter Open Firmware. Populating /vdevice methods Populating /vdevice/v-scsi@2000 VSCSI: Initializing VSCSI: Looking for disks SCSI ID 2 CD-ROM : "QEMU QEMU CD-ROM 1.0." Populating /vdevice/vty@3000 Populating /pci@0,0 Adapters on 00 (D) : 1af4 1000virtio [ net ] 00 0800 (D) : 1af4 1001virtio [ block ] No NVRAM common partition, re-initializing... Using default console: /vdevice/vty@3000 Detected RAM kernel at 40 (16bad7c bytes) Welcome to Open Firmware Copyright (c) 2004, 2011 IBM Corporation All rights reserved. This program and the accompanying materials are made available under the terms of the BSD License available at http://www.opensource.org/licenses/bsd-license.php Booting from memory... OF stdout device is: /vdevice/vty@3000 Preparing to boot Linux version 3.2.0-2-ppc64 (geeko@buildhost) (gcc version 4.6.2 20111212 [gcc-4_6-branch revision 18] (SUSE Linux) ) #1 SMP Wed Jan 25 10:51:08 UTC 2012 (2206a5c) Detected machine type: 0101 Max number of cores passed to firmware: 1024 (NR_CPUS = 1024) Calling ibm,client-architecture-support... not implemented couldn't open /packages/elf-loader command line: root=/dev/mapper/system-root ro audit=0 selinux=0 apparmor=0 console=tty0 console=ttyPZ0 memory layout at init: memory_limit : (16 MB aligned) alloc_bottom : 01ad alloc_top: 3000 alloc_top_hi : 4000 rmo_top : 3000 ram_top : 4000 instantiating rtas at 0x2fff... done Querying for OPAL presence... not there. boot cpu hw idx 0 copying OF device tree... Building dt strings... Building dt structure... Device tree strings 0x020e -> 0x020e0635 Device tree struct 0x020f -> 0x0210 Calling quiesce... returning from prom_init Using pSeries machine description Using 1TB segments Found initrd at 0xc1ad:0xc2053df8 bootconsole [udbg0] enabled CPU maps initialized for 1 thread per core Starting Linux PPC64 #1 SMP Wed Jan 25 10:51:08 UTC 2012 (2206a5c) - ppc64_pft_size= 0x18 physicalMemorySize= 0x4000 htab_hash_mask= 0x1 - Initializing cgroup subsys cpuset Initializing cgroup subsys cpu Linux version 3.2.0-2
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On Fri, 9 Sep 2016 12:14:31 +0200
Maxime Coquelin wrote:
> This patch adds virtio_test_backend_feature() function to
> enable checking a backend feature before the negociation
> takes place.
>
> It may be used, for example, to check whether the backend
> supports VIRTIO_F_VERSION_1 before enabling modern
> capabilities.
>
> Cc: Marcel Apfelbaum
> Cc: Michael S. Tsirkin
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Maxime Coquelin
> ---
> hw/virtio/virtio.c | 14 ++
> include/hw/virtio/virtio.h | 2 ++
> 2 files changed, 16 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 74c085c..7ab91a1 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
> size_t size)
> virtio_save(VIRTIO_DEVICE(opaque), f);
> }
>
> +bool virtio_test_backend_feature(VirtIODevice *vdev,
> + unsigned int fbit, Error **errp)
> +{
> +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +uint64_t feature;
> +
> +virtio_add_feature(&feature, fbit);
> +
> +assert(k->get_features != NULL);
> +feature = k->get_features(vdev, feature, errp);
> +
> +return virtio_has_feature(feature, fbit);
> +}
> +
> static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
> {
> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
What happens if you want to test for features that depend upon each
other? The backend may support your feature, but it may withdraw the
feature bit if a dependency is not fullfilled.
You'll probably want to run validation on the whole feature set; but
that is hard if you're too early in the setup process.
Re: [Qemu-devel] [PULL v2 0/4] target-arm queue
On 9 September 2016 at 07:50, Cédric Le Goater wrote: > Hello Peter, > > I have a little patchset fixing what you have asked for > regarding the RAM setting in Aspeed, but it depends on > the ast2500. So I suppose I should wait for the next > merge before sending ? I dropped those patches, so you'll need to respin them anyway. You can include the ram setting stuff in that patchset (squashed in or as an extra patch as you see fit). thanks -- PMM
Re: [Qemu-devel] vxhs caching behaviour (was: [PATCH v4 RFC] block/vxhs: Initial commit to add) Veritas HyperScale VxHS block device support
Am 08.09.2016 um 22:46 hat ashish mittal geschrieben:
> Hi Kevin,
>
> By design, our writeback cache is on non-volatile SSD device. We do
> async writes to this cache and also maintain a persistent index map of
> the data written. This gives us the capability to recover write-back
> cache if needed.
So your server application uses something like O_SYNC to write data to
the cache SSD, in order to avoid that data is sitting only in the kernel
page cache or in a volatile write cache of the SSD?
Kevin
> On Thu, Sep 8, 2016 at 7:20 AM, Kevin Wolf wrote:
> > Am 08.09.2016 um 16:00 hat Jeff Cody geschrieben:
> >> > >> +/*
> >> > >> + * This is called by QEMU when a flush gets triggered from within
> >> > >> + * a guest at the block layer, either for IDE or SCSI disks.
> >> > >> + */
> >> > >> +int vxhs_co_flush(BlockDriverState *bs)
> >> > >> +{
> >> > >> +BDRVVXHSState *s = bs->opaque;
> >> > >> +uint64_t size = 0;
> >> > >> +int ret = 0;
> >> > >> +
> >> > >> +ret = qemu_iio_ioctl(s->qnio_ctx,
> >> > >> +s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd,
> >> > >> +VDISK_AIO_FLUSH, &size, NULL, IIO_FLAG_SYNC);
> >> > >> +
> >> > >> +if (ret < 0) {
> >> > >> +/*
> >> > >> + * Currently not handling the flush ioctl
> >> > >> + * failure because of network connection
> >> > >> + * disconnect. Since all the writes are
> >> > >> + * commited into persistent storage hence
> >> > >> + * this flush call is noop and we can safely
> >> > >> + * return success status to the caller.
> >> > >
> >> > > I'm not sure I understand here. Are you saying the qemu_iio_ioctl()
> >> > > call
> >> > > above is a noop?
> >> > >
> >> >
> >> > Yes, qemu_iio_ioctl(VDISK_AIO_FLUSH) is only a place-holder at present
> >> > in case we later want to add some functionality to it. I have now
> >> > added a comment to this affect to avoid any confusion.
> >> >
> >>
> >> The problem is you don't know which version of the qnio library any given
> >> QEMU binary will be using, since it is a shared library. Future versions
> >> may implement the flush ioctl as expressed above, in which case we may hide
> >> a valid error.
> >>
> >> Am I correct in assuming that this call suppresses errors because an error
> >> is returned for an unknown ioctl operation of VDISK_AIO_FLUSH? If so, and
> >> you want a placeholder here for flushing, you should go all the way and
> >> stub
> >> out the underlying ioctl call to return success. Then QEMU can at least
> >> rely on the error return from the flush operation.
> >
> > So what's the story behind the missing flush command?
> >
> > Does the server always use something like O_SYNC, i.e. all potential
> > write caches in the stack operate in a writethrough mode? So each write
> > request is only completed successfully if it is ensured that the data is
> > safe on disk rather than in a volatile writeback cache?
> >
> > As soon as any writeback cache can be involved (e.g. the kernel page
> > cache or a volatile disk cache) and there is no flush command (a real
> > one, not just stubbed), the driver is not operating correctly and
> > therefore not ready for inclusion.
> >
> > So Ashish, can you tell us something about caching behaviour across the
> > storage stack when vxhs is involved?
> >
> > Kevin
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On Fri, 9 Sep 2016 12:14:32 +0200
Maxime Coquelin wrote:
> This patch makes pci devices plugging more robust, by not confusing
> guest with modern interface when the backend doesn't support
> VIRTIO_F_VERSION_1.
>
> Cc: Marcel Apfelbaum
> Cc: Michael S. Tsirkin
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Maxime Coquelin
> ---
> hw/virtio/virtio-pci.c | 15 +++
> hw/virtio/virtio-pci.h | 5 +
> 2 files changed, 20 insertions(+)
Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
ccw") fixes this issue for ccw via the introduction of a
->post_plugged() callback. Unfortunately, we did not find a good way to
make it work for pci back then.
Two comments:
- ->post_plugged() handles the dependencies (as noticed for your first
patch) - and this is due to it being called after the plugging is
already done.
- I don't really like pci and ccw being too different. We have probably
more flexibility with the handling for ccw, so I could probably convert
ccw to the same mechanism that pci uses. Maybe there are other uses for
->post_plugged()?
[Qemu-devel] [PATCH RFC v1 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag
Signed-off-by: Nikunj A Dadhania
---
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 2 +-
target-ppc/mmu-hash64.c | 4 ++--
target-ppc/mmu_helper.c | 6 +++---
4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 1e808c8..7dc 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1009,6 +1009,7 @@ struct CPUPPCState {
bool tlb_dirty; /* Set to non-zero when modifying TLB */
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active*/
uint32_t tlb_need_flush; /* Delayed flush needed */
+#define TLB_NEED_LOCAL_FLUSH 0x1
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 3d279f1..4457a30 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -157,7 +157,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
static inline void check_tlb_flush(CPUPPCState *env)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
-if (env->tlb_need_flush) {
+if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) == TLB_NEED_LOCAL_FLUSH) {
env->tlb_need_flush = 0;
tlb_flush(cs, 1);
}
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 8118143..4c7ceef 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -110,7 +110,7 @@ void helper_slbia(CPUPPCState *env)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
}
}
}
@@ -132,7 +132,7 @@ void helper_slbie(CPUPPCState *env, target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
}
}
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 696bb03..249 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
target_ulong addr)
* we just mark the TLB to be flushed later (context synchronizing
* event or sync instruction on 32-bit).
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
break;
#if defined(TARGET_PPC64)
case POWERPC_MMU_64B:
@@ -1979,7 +1979,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask) in QEMU,
* we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
break;
#endif /* defined(TARGET_PPC64) */
default:
@@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong
srnum, target_ulong value)
}
}
#else
-env->tlb_need_flush = 1;
+env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
#endif
}
}
--
2.7.4
[Qemu-devel] [PATCH RFC v1 3/3] target-ppc: tlbie should have global effect
tlbie (H_REMOVE, H_PROTECT and H_BULK_REMOVE for pseries) should have a
global effect.
Introduces TLB_NEED_GLOBAL_FLUSH flag. During delayed flush, once taking
care of local flush, check broadcast flush(ptesync, tlbsync, etc) is
needed. Depending on the bitmask state of the tlb_need_flush, tlb is
flushed from other cpus if needed and the flags are cleared.
Suggested-by: Benjamin Herrenschmidt
Signed-off-by: Nikunj A Dadhania
---
hw/ppc/spapr_hcall.c | 3 +++
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 23 ++-
target-ppc/mmu-hash64.c | 2 +-
target-ppc/mmu_helper.c | 10 +++---
target-ppc/translate.c | 6 ++
6 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ef12ea0..19fbae6 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -282,6 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
}
}
exit:
+env->tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
check_tlb_flush(env, 1);
return rc;
@@ -319,6 +320,8 @@ static target_ulong h_protect(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
ppc_hash64_store_hpte(cpu, pte_index,
(v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
+/* Flush the tlb */
+check_tlb_flush(env, 1);
/* Don't need a memory barrier, due to qemu's global lock */
ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
return H_SUCCESS;
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 7dc..50fe0f5 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1010,6 +1010,7 @@ struct CPUPPCState {
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active*/
uint32_t tlb_need_flush; /* Delayed flush needed */
#define TLB_NEED_LOCAL_FLUSH 0x1
+#define TLB_NEED_GLOBAL_FLUSH 0x2
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index fe20870..f18b2ff 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,13 +154,34 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
+static inline void tlb_clear_flag(CPUState *cs)
+{
+PowerPCCPU *cpu = POWERPC_CPU(cs);
+CPUPPCState *env = &cpu->env;
+
+env->tlb_need_flush = 0;
+}
+
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
+CPUState *other_cs;
+
if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) == TLB_NEED_LOCAL_FLUSH) {
-env->tlb_need_flush = 0;
tlb_flush(cs, 1);
}
+
+if (global &&
+(env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH) == TLB_NEED_GLOBAL_FLUSH)
+{
+CPU_FOREACH(other_cs) {
+if (other_cs != cs) {
+tlb_clear_flag(other_cs);
+tlb_flush(other_cs, 1);
+}
+}
+}
+env->tlb_need_flush = 0;
}
#else
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 4c7ceef..f582efe 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -912,7 +912,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
* invalidate, and we still don't have a tlb_flush_mask(env, n,
* mask) in QEMU, we just invalidate all TLBs
*/
-tlb_flush(CPU(cpu), 1);
+cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
}
void ppc_hash64_update_rmls(CPUPPCState *env)
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 59dea8f..aee272f 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2757,7 +2757,7 @@ static inline void booke206_invalidate_ea_tlb(CPUPPCState
*env, int tlbn,
void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
{
-PowerPCCPU *cpu = ppc_env_get_cpu(env);
+CPUState *cs;
if (address & 0x4) {
/* flush all entries */
@@ -2774,11 +2774,15 @@ void helper_booke206_tlbivax(CPUPPCState *env,
target_ulong address)
if (address & 0x8) {
/* flush TLB1 entries */
booke206_invalidate_ea_tlb(env, 1, address);
-tlb_flush(CPU(cpu), 1);
+CPU_FOREACH(cs) {
+tlb_flush(cs, 1);
+}
} else {
/* flush TLB0 entries */
booke206_invalidate_ea_tlb(env, 0, address);
-tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
+CPU_FOREACH(cs) {
+tlb_flush_page(cs, address & MAS2_EPN_MASK);
+}
}
}
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 5f12c41..d3cfbc8 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4443,6 +4443,7 @@ static void gen_tlbie(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
GEN_PRIV;
#else
+TCGv_i32 t1;
CHK_H
[Qemu-devel] [PATCH RFC v1 2/3] target-ppc: add flag in chech_tlb_flush()
The flag will be used to indicate whether broadcast tlb flush is needed
or not.
Signed-off-by: Nikunj A Dadhania
---
hw/ppc/spapr_hcall.c | 4 ++--
target-ppc/excp_helper.c | 4 ++--
target-ppc/helper.h | 2 +-
target-ppc/helper_regs.h | 4 ++--
target-ppc/mmu_helper.c | 4 ++--
target-ppc/translate.c | 13 +++--
6 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 73af112..ef12ea0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
switch (ret) {
case REMOVE_SUCCESS:
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
return H_SUCCESS;
case REMOVE_NOT_FOUND:
@@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
}
}
exit:
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
return rc;
}
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 04ed4da..09947e4 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
excp_model, int excp)
/* Any interrupt is context synchronizing, check if TCG TLB
* needs a delayed flush on ppc64
*/
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
}
void ppc_cpu_do_interrupt(CPUState *cs)
@@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env, target_ulong
nip, target_ulong msr)
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
/* Context synchronizing: check if TCG TLB needs flush */
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
}
void helper_rfi(CPUPPCState *env)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index dcf3f95..a86e184 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
DEF_HELPER_1(hrfid, void, env)
DEF_HELPER_2(store_lpcr, void, env, tl)
#endif
-DEF_HELPER_1(check_tlb_flush, void, env)
+DEF_HELPER_2(check_tlb_flush, void, env, i32)
#endif
DEF_HELPER_3(lmw, void, env, tl, i32)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 4457a30..fe20870 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
-static inline void check_tlb_flush(CPUPPCState *env)
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) == TLB_NEED_LOCAL_FLUSH) {
@@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState *env)
}
}
#else
-static inline void check_tlb_flush(CPUPPCState *env) { }
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
#endif
#endif /* HELPER_REGS_H */
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 249..59dea8f 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState *env,
target_ulong type)
}
-void helper_check_tlb_flush(CPUPPCState *env)
+void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
{
-check_tlb_flush(env);
+check_tlb_flush(env, global);
}
/*/
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 618334a..5f12c41 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3064,7 +3064,7 @@ static void gen_eieio(DisasContext *ctx)
}
#if !defined(CONFIG_USER_ONLY)
-static inline void gen_check_tlb_flush(DisasContext *ctx)
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global)
{
TCGv_i32 t;
TCGLabel *l;
@@ -3076,12 +3076,13 @@ static inline void gen_check_tlb_flush(DisasContext
*ctx)
t = tcg_temp_new_i32();
tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
-gen_helper_check_tlb_flush(cpu_env);
+tcg_gen_movi_i32(t, global);
+gen_helper_check_tlb_flush(cpu_env, t);
gen_set_label(l);
tcg_temp_free_i32(t);
}
#else
-static inline void gen_check_tlb_flush(DisasContext *ctx) { }
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global) { }
#endif
/* isync */
@@ -3092,7 +3093,7 @@ static void gen_isync(DisasContext *ctx)
* kernel mode however so check MSR_PR
*/
if (!ctx->pr) {
-gen_check_tlb_flush(ctx);
+gen_check_tlb_flush(ctx, 0);
}
gen_stop_exception(ctx);
}
@@ -3257,7 +3258,7 @@ static void gen_sync(DisasContext *ctx)
* check MSR_PR as well.
*/
if (((l == 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) {
-gen_check_tlb_flush(ctx);
+gen_check_tlb_flush(ctx, 1)
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On 09/09/2016 01:33 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 12:14:31 +0200
Maxime Coquelin wrote:
This patch adds virtio_test_backend_feature() function to
enable checking a backend feature before the negociation
takes place.
It may be used, for example, to check whether the backend
supports VIRTIO_F_VERSION_1 before enabling modern
capabilities.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio.c | 14 ++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..7ab91a1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
size_t size)
virtio_save(VIRTIO_DEVICE(opaque), f);
}
+bool virtio_test_backend_feature(VirtIODevice *vdev,
+ unsigned int fbit, Error **errp)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+uint64_t feature;
+
+virtio_add_feature(&feature, fbit);
+
+assert(k->get_features != NULL);
+feature = k->get_features(vdev, feature, errp);
+
+return virtio_has_feature(feature, fbit);
+}
+
static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
What happens if you want to test for features that depend upon each
other? The backend may support your feature, but it may withdraw the
feature bit if a dependency is not fullfilled.
You'll probably want to run validation on the whole feature set; but
that is hard if you're too early in the setup process.
While I agree with the feature dependency issue , would the negation be ok?
What I mean is: if the backend does not support feature X, no
matter what the depending features are, it will still not support it after the
negotiation.
Changing the function to virtio_backend_unsupported_feature(x) would be better?
Thanks,
Marcel
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On Fri, 9 Sep 2016 13:48:00 +0300
Marcel Apfelbaum wrote:
> On 09/09/2016 01:33 PM, Cornelia Huck wrote:
> > On Fri, 9 Sep 2016 12:14:31 +0200
> > Maxime Coquelin wrote:
> >
> >> This patch adds virtio_test_backend_feature() function to
> >> enable checking a backend feature before the negociation
> >> takes place.
> >>
> >> It may be used, for example, to check whether the backend
> >> supports VIRTIO_F_VERSION_1 before enabling modern
> >> capabilities.
> >>
> >> Cc: Marcel Apfelbaum
> >> Cc: Michael S. Tsirkin
> >> Cc: qemu-sta...@nongnu.org
> >> Signed-off-by: Maxime Coquelin
> >> ---
> >> hw/virtio/virtio.c | 14 ++
> >> include/hw/virtio/virtio.h | 2 ++
> >> 2 files changed, 16 insertions(+)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 74c085c..7ab91a1 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
> >> size_t size)
> >> virtio_save(VIRTIO_DEVICE(opaque), f);
> >> }
> >>
> >> +bool virtio_test_backend_feature(VirtIODevice *vdev,
> >> + unsigned int fbit, Error **errp)
> >> +{
> >> +VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >> +uint64_t feature;
> >> +
> >> +virtio_add_feature(&feature, fbit);
> >> +
> >> +assert(k->get_features != NULL);
> >> +feature = k->get_features(vdev, feature, errp);
> >> +
> >> +return virtio_has_feature(feature, fbit);
> >> +}
> >> +
> >> static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
> >> {
> >> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > What happens if you want to test for features that depend upon each
> > other? The backend may support your feature, but it may withdraw the
> > feature bit if a dependency is not fullfilled.
> >
> > You'll probably want to run validation on the whole feature set; but
> > that is hard if you're too early in the setup process.
> >
>
> While I agree with the feature dependency issue , would the negation be ok?
> What I mean is: if the backend does not support feature X, no
> matter what the depending features are, it will still not support it after
> the negotiation.
>
> Changing the function to virtio_backend_unsupported_feature(x) would be
> better?
I think yes, although that would mean we need a new query function that
pokes through all the layers, no?
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On 09/09/2016 01:55 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 13:48:00 +0300
Marcel Apfelbaum wrote:
On 09/09/2016 01:33 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 12:14:31 +0200
Maxime Coquelin wrote:
This patch adds virtio_test_backend_feature() function to
enable checking a backend feature before the negociation
takes place.
It may be used, for example, to check whether the backend
supports VIRTIO_F_VERSION_1 before enabling modern
capabilities.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio.c | 14 ++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 16 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..7ab91a1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,20 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
size_t size)
virtio_save(VIRTIO_DEVICE(opaque), f);
}
+bool virtio_test_backend_feature(VirtIODevice *vdev,
+ unsigned int fbit, Error **errp)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+uint64_t feature;
+
+virtio_add_feature(&feature, fbit);
+
+assert(k->get_features != NULL);
+feature = k->get_features(vdev, feature, errp);
+
+return virtio_has_feature(feature, fbit);
+}
+
static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
What happens if you want to test for features that depend upon each
other? The backend may support your feature, but it may withdraw the
feature bit if a dependency is not fullfilled.
You'll probably want to run validation on the whole feature set; but
that is hard if you're too early in the setup process.
While I agree with the feature dependency issue , would the negation be ok?
What I mean is: if the backend does not support feature X, no
matter what the depending features are, it will still not support it after the
negotiation.
Changing the function to virtio_backend_unsupported_feature(x) would be better?
I think yes, although that would mean we need a new query function that
pokes through all the layers, no?
I was thinking to keep the same function proposed by Maxime and change it to
negate things:
/*
* A missing feature before all negotiations finished will still be missing at
the end.
*/
bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
unsigned int fbit, Error **errp)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
uint64_t feature;
virtio_add_feature(&feature, fbit);
assert(k->get_features != NULL);
feature = k->get_features(vdev, feature, errp);
return !virtio_has_feature(feature, fbit);
}
We only check if the feature was not there from the start.
Thanks,
Marcel
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>>
>> > I previously split the global trace-events file up into one file
>> > per-subdirectory to avoid merge conflict hell.
>> [...]
>>
>> Sorry, I could not find the message where the infrastructure is modified to
>> provide this. But I think there's a more efficient way to provide modular
>> auto-generated tracing code without the hierarchical indexing you proposed.
> [snip]
>> struct TraceEvent ___trace_events[] = {
>> {
>> .name = "eventname",
>> .sstate = 1,
>> .dstate = ___trace_eventname_dstate;
>> }
>> }
>>
>> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
> Life would be simpler if we had the 'bool dstate' as part of the
> TraceEvent struct, but doing so would essentially be reverting this
> previous change:
> commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
> Author: Paolo Bonzini
> Date: Wed Oct 28 07:06:27 2015 +0100
> trace: track enabled events in a separate array
> This is more cache friendly on the fast path, where we already have
> the event id available.
> I asked Paolo about this previously and he indicated it was a notable
> performance improvement, so we can't put dstate back into the TraceEvent
> struct :-(
Sorry, there was a typo in my example code that led to this misunderstanding.
I meant this for the global auto-generated .c:
struct TraceEvent {
/* ... */
bool *dstate;
};
bool ___TRACE_EVENTNAME_DSTATE;
struct TraceEvent ___trace_events[] = {
{
.name = "eventname",
.sstate = 1,
.dstate = &___TRACE_EVENTNAME_DSTATE;
}
}
TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
If you look at the modified macro I pasted for "trace/control-internal.h":
#define trace_event_get_state_dynamic_by_id(id) \
(unlikely(trace_events_enabled_count) && \
(___ ## id ## _DSTATE))
We're retaining the fast-path performance of the seggregated boolean dstate
array, while the event descriptor contains a pointer to the per-event dstate
global boolean variable in case you want to get/set the dstate based on an event
pointer.
Cheers,
Lluis
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On 09/09/2016 01:40 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 12:14:32 +0200
Maxime Coquelin wrote:
This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio-pci.c | 15 +++
hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
ccw") fixes this issue for ccw via the introduction of a
->post_plugged() callback. Unfortunately, we did not find a good way to
make it work for pci back then.
It seems that for ccw is enough to rewind dev->rev_max,
sadly for pci we need to rewind a lot of settings/resources.
Thanks,
Marcel
Two comments:
- ->post_plugged() handles the dependencies (as noticed for your first
patch) - and this is due to it being called after the plugging is
already done.
- I don't really like pci and ccw being too different. We have probably
more flexibility with the handling for ccw, so I could probably convert
ccw to the same mechanism that pci uses. Maybe there are other uses for
->post_plugged()?
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On Fri, 9 Sep 2016 14:02:17 +0300
Marcel Apfelbaum wrote:
> I was thinking to keep the same function proposed by Maxime and change it to
> negate things:
>
> /*
> * A missing feature before all negotiations finished will still be missing
> at the end.
> */
> bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
> unsigned int fbit, Error **errp)
> {
> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> uint64_t feature;
>
> virtio_add_feature(&feature, fbit);
>
> assert(k->get_features != NULL);
> feature = k->get_features(vdev, feature, errp);
>
> return !virtio_has_feature(feature, fbit);
> }
>
> We only check if the feature was not there from the start.
I think you'll still end up with the same problem (overindicating
unsupportedness), as you start out with an otherwise empty feature
set, causing features with dependencies to be removed. I fear that
anything starting with an incomplete feature list will have that
problem.
Maybe it would be better to limit this to the one bit we currently want
to test (VERSION_1)? We know the semantics of that one. Less general,
but also less headaches.
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On Fri, 9 Sep 2016 14:04:55 +0300
Marcel Apfelbaum wrote:
> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> > On Fri, 9 Sep 2016 12:14:32 +0200
> > Maxime Coquelin wrote:
> >
> >> This patch makes pci devices plugging more robust, by not confusing
> >> guest with modern interface when the backend doesn't support
> >> VIRTIO_F_VERSION_1.
> >>
> >> Cc: Marcel Apfelbaum
> >> Cc: Michael S. Tsirkin
> >> Cc: qemu-sta...@nongnu.org
> >> Signed-off-by: Maxime Coquelin
> >> ---
> >> hw/virtio/virtio-pci.c | 15 +++
> >> hw/virtio/virtio-pci.h | 5 +
> >> 2 files changed, 20 insertions(+)
> >
> > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> > ccw") fixes this issue for ccw via the introduction of a
> > ->post_plugged() callback. Unfortunately, we did not find a good way to
> > make it work for pci back then.
>
> It seems that for ccw is enough to rewind dev->rev_max,
> sadly for pci we need to rewind a lot of settings/resources.
Yes, that what I meant with 'more flexibility for ccw'.
>
> Thanks,
> Marcel
>
> >
> > Two comments:
> > - ->post_plugged() handles the dependencies (as noticed for your first
> > patch) - and this is due to it being called after the plugging is
> > already done.
> > - I don't really like pci and ccw being too different. We have probably
> > more flexibility with the handling for ccw, so I could probably convert
> > ccw to the same mechanism that pci uses. Maybe there are other uses for
> > ->post_plugged()?
> >
>
Re: [Qemu-devel] [PATCH RFC v1 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag
On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote:
> > Signed-off-by: Nikunj A Dadhania
> ---
> target-ppc/cpu.h | 1 +
> target-ppc/helper_regs.h | 2 +-
> target-ppc/mmu-hash64.c | 4 ++--
> target-ppc/mmu_helper.c | 6 +++---
> 4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1e808c8..7dc 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1009,6 +1009,7 @@ struct CPUPPCState {
> bool tlb_dirty; /* Set to non-zero when modifying TLB
> */
> bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active
> */
> uint32_t tlb_need_flush; /* Delayed flush needed */
> +#define TLB_NEED_LOCAL_FLUSH 0x1
> #endif
>
> /* Other registers */
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 3d279f1..4457a30 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -157,7 +157,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
> target_ulong value,
> static inline void check_tlb_flush(CPUPPCState *env)
> {
> CPUState *cs = CPU(ppc_env_get_cpu(env));
> -if (env->tlb_need_flush) {
> +if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) ==
> TLB_NEED_LOCAL_FLUSH) {
> env->tlb_need_flush = 0;
> tlb_flush(cs, 1);
> }
No. This should be
if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
tlb_flush(cs, 1);
env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
}
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 8118143..4c7ceef 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -110,7 +110,7 @@ void helper_slbia(CPUPPCState *env)
> * and we still don't have a tlb_flush_mask(env, n, mask)
> * in QEMU, we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> }
> }
> }
Should be
env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
> @@ -132,7 +132,7 @@ void helper_slbie(CPUPPCState *env, target_ulong addr)
> * and we still don't have a tlb_flush_mask(env, n, mask)
> * in QEMU, we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> }
> }
ditto.
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 696bb03..249 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
> * we just mark the TLB to be flushed later (context synchronizing
> * event or sync instruction on 32-bit).
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> break;
again.
> #if defined(TARGET_PPC64)
> case POWERPC_MMU_64B:
> @@ -1979,7 +1979,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
> * and we still don't have a tlb_flush_mask(env, n, mask) in
> QEMU,
> * we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> break;
again.
> #endif /* defined(TARGET_PPC64) */
> default:
> @@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong
> srnum, target_ulong value)
> }
> }
> #else
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> #endif
and one more.
> }
> }
Re: [Qemu-devel] [PATCH RFC v1 2/3] target-ppc: add flag in chech_tlb_flush()
On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote:
> The flag will be used to indicate whether broadcast tlb flush is
> needed
> or not.
>
> Signed-off-by: Nikunj A Dadhania
> ---
> hw/ppc/spapr_hcall.c | 4 ++--
> target-ppc/excp_helper.c | 4 ++--
> target-ppc/helper.h | 2 +-
> target-ppc/helper_regs.h | 4 ++--
> target-ppc/mmu_helper.c | 4 ++--
> target-ppc/translate.c | 13 +++--
> 6 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 73af112..ef12ea0 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
>
> switch (ret) {
> case REMOVE_SUCCESS:
> -check_tlb_flush(env);
> +check_tlb_flush(env, 1);
> return H_SUCCESS;
>
> case REMOVE_NOT_FOUND:
> @@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU
> *cpu, sPAPRMachineState *spapr,
> }
> }
> exit:
> -check_tlb_flush(env);
> +check_tlb_flush(env, 1);
>
> return rc;
> }
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 04ed4da..09947e4 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu,
> int excp_model, int excp)
> /* Any interrupt is context synchronizing, check if TCG TLB
> * needs a delayed flush on ppc64
> */
> -check_tlb_flush(env);
> +check_tlb_flush(env, 1);
No that one is local
> }
>
> void ppc_cpu_do_interrupt(CPUState *cs)
> @@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env,
> target_ulong nip, target_ulong msr)
> cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>
> /* Context synchronizing: check if TCG TLB needs flush */
> -check_tlb_flush(env);
> +check_tlb_flush(env, 1);
> }
Same
> void helper_rfi(CPUPPCState *env)
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index dcf3f95..a86e184 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
> DEF_HELPER_1(hrfid, void, env)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> #endif
> -DEF_HELPER_1(check_tlb_flush, void, env)
> +DEF_HELPER_2(check_tlb_flush, void, env, i32)
> #endif
>
> DEF_HELPER_3(lmw, void, env, tl, i32)
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 4457a30..fe20870 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState
> *env, target_ulong value,
> }
>
> #if !defined(CONFIG_USER_ONLY)
> -static inline void check_tlb_flush(CPUPPCState *env)
> +static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> global)
> {
> CPUState *cs = CPU(ppc_env_get_cpu(env));
> if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) ==
> TLB_NEED_LOCAL_FLUSH) {
> @@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState
> *env)
> }
> }
> #else
> -static inline void check_tlb_flush(CPUPPCState *env) { }
> +static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> global) { }
> #endif
>
> #endif /* HELPER_REGS_H */
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 249..59dea8f 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState *env,
> target_ulong type)
> }
>
>
> -void helper_check_tlb_flush(CPUPPCState *env)
> +void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
> {
> -check_tlb_flush(env);
> +check_tlb_flush(env, global);
> }
>
> /***
> **/
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 618334a..5f12c41 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -3064,7 +3064,7 @@ static void gen_eieio(DisasContext *ctx)
> }
>
> #if !defined(CONFIG_USER_ONLY)
> -static inline void gen_check_tlb_flush(DisasContext *ctx)
> +static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t
> global)
> {
> TCGv_i32 t;
> TCGLabel *l;
> @@ -3076,12 +3076,13 @@ static inline void
> gen_check_tlb_flush(DisasContext *ctx)
> t = tcg_temp_new_i32();
> tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState,
> tlb_need_flush));
> tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
> -gen_helper_check_tlb_flush(cpu_env);
> +tcg_gen_movi_i32(t, global);
> +gen_helper_check_tlb_flush(cpu_env, t);
> gen_set_label(l);
> tcg_temp_free_i32(t);
> }
> #else
> -static inline void gen_check_tlb_flush(DisasContext *ctx) { }
> +static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t
> global) { }
> #endif
>
> /* isync */
> @@ -3092,7 +3093,7 @@ static void gen_isync(DisasContext *ctx)
> * kernel mode however so check
[Qemu-devel] [PATCH 1/3] tests: add /vhost-user/connect-fail test
Check early connection failure and resume.
Signed-off-by: Marc-André Lureau
---
tests/vhost-user-test.c | 39 +++
1 file changed, 39 insertions(+)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index b89a551..ab91e16 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -135,6 +135,7 @@ typedef struct TestServer {
CompatGCond data_cond;
int log_fd;
uint64_t rings;
+bool test_fail;
} TestServer;
static const char *tmpfs;
@@ -249,6 +250,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
uint8_t *p = (uint8_t *) &msg;
int fd;
+if (s->test_fail) {
+qemu_chr_disconnect(chr);
+/* now switch to non-failure */
+s->test_fail = false;
+}
+
if (size != VHOST_USER_HDR_SIZE) {
g_test_message("Wrong message size received %d\n", size);
return;
@@ -715,6 +722,35 @@ static void test_reconnect(void)
g_test_trap_assert_passed();
g_free(path);
}
+
+static void test_connect_fail_subprocess(void)
+{
+TestServer *s = test_server_new("connect-fail");
+char *cmd;
+
+s->test_fail = true;
+g_thread_new("connect", connect_thread, s);
+cmd = GET_QEMU_CMDE(s, 2, ",server", "");
+qtest_start(cmd);
+g_free(cmd);
+
+init_virtio_dev(s);
+wait_for_fds(s);
+wait_for_rings_started(s, 2);
+
+qtest_end();
+test_server_free(s);
+}
+
+static void test_connect_fail(void)
+{
+gchar *path = g_strdup_printf("/%s/vhost-user/connect-fail/subprocess",
+ qtest_get_arch());
+g_test_trap_subprocess(path, 0, 0);
+g_test_trap_assert_passed();
+g_free(path);
+}
+
#endif
int main(int argc, char **argv)
@@ -766,6 +802,9 @@ int main(int argc, char **argv)
qtest_add_func("/vhost-user/reconnect/subprocess",
test_reconnect_subprocess);
qtest_add_func("/vhost-user/reconnect", test_reconnect);
+qtest_add_func("/vhost-user/connect-fail/subprocess",
+ test_connect_fail_subprocess);
+qtest_add_func("/vhost-user/connect-fail", test_connect_fail);
#endif
ret = g_test_run();
--
2.10.0
[Qemu-devel] [PATCH 0/3] vhost-user tests
Hi, The following tests have been post-poned for after 2.7 from the vhost-user-reconnect series. They have been rebased and fixed to work with last changes. Marc-André Lureau (3): tests: add /vhost-user/connect-fail test tests: add a simple /vhost-user/multiqueue test tests: add /vhost-user/flags-mismatch test tests/vhost-user-test.c | 208 +++- tests/Makefile.include | 2 +- 2 files changed, 205 insertions(+), 5 deletions(-) -- 2.10.0
[Qemu-devel] [PATCH 2/3] tests: add a simple /vhost-user/multiqueue test
This test just checks that 2 virtio-net queues can be setup over
vhost-user and waits for them to be started.
Signed-off-by: Marc-André Lureau
---
tests/vhost-user-test.c | 109 ++--
tests/Makefile.include | 2 +-
2 files changed, 107 insertions(+), 4 deletions(-)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index ab91e16..ffdd398 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -20,6 +20,11 @@
#include "libqos/pci-pc.h"
#include "libqos/virtio-pci.h"
+#include "libqos/pci-pc.h"
+#include "libqos/virtio-pci.h"
+#include "libqos/malloc-pc.h"
+#include "hw/virtio/virtio-net.h"
+
#include
#include
#include
@@ -50,6 +55,7 @@
#define VHOST_MEMORY_MAX_NREGIONS8
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_F_MQ 0
#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
#define VHOST_LOG_PAGE 0x1000
@@ -72,6 +78,7 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_VRING_ERR = 14,
VHOST_USER_GET_PROTOCOL_FEATURES = 15,
VHOST_USER_SET_PROTOCOL_FEATURES = 16,
+VHOST_USER_GET_QUEUE_NUM = 17,
VHOST_USER_SET_VRING_ENABLE = 18,
VHOST_USER_MAX
} VhostUserRequest;
@@ -136,6 +143,7 @@ typedef struct TestServer {
int log_fd;
uint64_t rings;
bool test_fail;
+int queues;
} TestServer;
static const char *tmpfs;
@@ -281,6 +289,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
msg.size = sizeof(m.payload.u64);
msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+if (s->queues > 1) {
+msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
+}
p = (uint8_t *) &msg;
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
@@ -295,6 +306,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
msg.flags |= VHOST_USER_REPLY_MASK;
msg.size = sizeof(m.payload.u64);
msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
+if (s->queues > 1) {
+msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
+}
p = (uint8_t *) &msg;
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
@@ -307,7 +321,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
p = (uint8_t *) &msg;
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
-assert(msg.payload.state.index < 2);
+assert(msg.payload.state.index < s->queues * 2);
s->rings &= ~(0x1ULL << msg.payload.state.index);
break;
@@ -347,10 +361,18 @@ static void chr_read(void *opaque, const uint8_t *buf,
int size)
break;
case VHOST_USER_SET_VRING_BASE:
-assert(msg.payload.state.index < 2);
+assert(msg.payload.state.index < s->queues * 2);
s->rings |= 0x1ULL << msg.payload.state.index;
break;
+case VHOST_USER_GET_QUEUE_NUM:
+msg.flags |= VHOST_USER_REPLY_MASK;
+msg.size = sizeof(m.payload.u64);
+msg.payload.u64 = s->queues;
+p = (uint8_t *) &msg;
+qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+break;
+
default:
break;
}
@@ -397,6 +419,7 @@ static TestServer *test_server_new(const gchar *name)
g_cond_init(&server->data_cond);
server->log_fd = -1;
+server->queues = 1;
return server;
}
@@ -648,7 +671,6 @@ static void test_migrate(void)
global_qtest = global;
}
-#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
static void wait_for_rings_started(TestServer *s, size_t count)
{
gint64 end_time;
@@ -666,6 +688,7 @@ static void wait_for_rings_started(TestServer *s, size_t
count)
g_mutex_unlock(&s->data_mutex);
}
+#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
static gboolean
reconnect_cb(gpointer user_data)
{
@@ -753,6 +776,85 @@ static void test_connect_fail(void)
#endif
+static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
+{
+QVirtioPCIDevice *dev;
+
+dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
+g_assert(dev != NULL);
+g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_NET);
+
+qvirtio_pci_device_enable(dev);
+qvirtio_reset(&qvirtio_pci, &dev->vdev);
+qvirtio_set_acknowledge(&qvirtio_pci, &dev->vdev);
+qvirtio_set_driver(&qvirtio_pci, &dev->vdev);
+
+return dev;
+}
+
+static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev)
+{
+uint32_t features;
+
+features = qvirtio_get_features(bus, dev);
+features = features & ~(QVIRTIO_F_BAD_FEATURE |
+(1u << VIRTIO_RING_F_INDIRECT_DESC) |
+(1u << VIRTIO_RING_F_EVENT_IDX));
+qvirtio_set_features(bus, dev, features);
+
+qvirtio_set_driver_ok(bus, dev);
+}
+
+#define PCI_SLOT0x04
+
+static void test_multiqueue(void)
+{
+const int queues
[Qemu-devel] [PATCH 3/3] tests: add /vhost-user/flags-mismatch test
Check that qemu disconnects the backend that doesn't have the previously
acked features.
Signed-off-by: Marc-André Lureau
---
tests/vhost-user-test.c | 60 -
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index ffdd398..a39846e 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -130,6 +130,13 @@ static VhostUserMsg m __attribute__ ((unused));
#define VHOST_USER_VERSION(0x1)
/*/
+enum {
+TEST_FLAGS_OK,
+TEST_FLAGS_DISCONNECT,
+TEST_FLAGS_BAD,
+TEST_FLAGS_END,
+};
+
typedef struct TestServer {
gchar *socket_path;
gchar *mig_path;
@@ -143,6 +150,7 @@ typedef struct TestServer {
int log_fd;
uint64_t rings;
bool test_fail;
+int test_flags;
int queues;
} TestServer;
@@ -292,6 +300,10 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
if (s->queues > 1) {
msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
}
+if (s->test_flags >= TEST_FLAGS_BAD) {
+msg.payload.u64 = 0;
+s->test_flags = TEST_FLAGS_END;
+}
p = (uint8_t *) &msg;
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
@@ -299,6 +311,10 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
case VHOST_USER_SET_FEATURES:
g_assert_cmpint(msg.payload.u64 & (0x1ULL <<
VHOST_USER_F_PROTOCOL_FEATURES),
!=, 0ULL);
+if (s->test_flags == TEST_FLAGS_DISCONNECT) {
+qemu_chr_disconnect(chr);
+s->test_flags = TEST_FLAGS_BAD;
+}
break;
case VHOST_USER_GET_PROTOCOL_FEATURES:
@@ -424,6 +440,16 @@ static TestServer *test_server_new(const gchar *name)
return server;
}
+static void chr_event(void *opaque, int event)
+{
+TestServer *s = opaque;
+
+if (s->test_flags == TEST_FLAGS_END &&
+event == CHR_EVENT_CLOSED) {
+s->test_flags = TEST_FLAGS_OK;
+}
+}
+
static void test_server_create_chr(TestServer *server, const gchar *opt)
{
gchar *chr_path;
@@ -432,7 +458,8 @@ static void test_server_create_chr(TestServer *server,
const gchar *opt)
server->chr = qemu_chr_new(server->chr_name, chr_path, NULL);
g_free(chr_path);
-qemu_chr_add_handlers(server->chr, chr_can_read, chr_read, NULL, server);
+qemu_chr_add_handlers(server->chr, chr_can_read, chr_read,
+ chr_event, server);
}
static void test_server_listen(TestServer *server)
@@ -774,6 +801,34 @@ static void test_connect_fail(void)
g_free(path);
}
+static void test_flags_mismatch_subprocess(void)
+{
+TestServer *s = test_server_new("flags-mismatch");
+char *cmd;
+
+s->test_flags = TEST_FLAGS_DISCONNECT;
+g_thread_new("connect", connect_thread, s);
+cmd = GET_QEMU_CMDE(s, 2, ",server", "");
+qtest_start(cmd);
+g_free(cmd);
+
+init_virtio_dev(s);
+wait_for_fds(s);
+wait_for_rings_started(s, 2);
+
+qtest_end();
+test_server_free(s);
+}
+
+static void test_flags_mismatch(void)
+{
+gchar *path = g_strdup_printf("/%s/vhost-user/flags-mismatch/subprocess",
+ qtest_get_arch());
+g_test_trap_subprocess(path, 0, 0);
+g_test_trap_assert_passed();
+g_free(path);
+}
+
#endif
static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
@@ -908,6 +963,9 @@ int main(int argc, char **argv)
qtest_add_func("/vhost-user/connect-fail/subprocess",
test_connect_fail_subprocess);
qtest_add_func("/vhost-user/connect-fail", test_connect_fail);
+qtest_add_func("/vhost-user/flags-mismatch/subprocess",
+ test_flags_mismatch_subprocess);
+qtest_add_func("/vhost-user/flags-mismatch", test_flags_mismatch);
#endif
ret = g_test_run();
--
2.10.0
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On 09/09/2016 01:20 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 14:02:17 +0300
Marcel Apfelbaum wrote:
I was thinking to keep the same function proposed by Maxime and change it to
negate things:
/*
* A missing feature before all negotiations finished will still be missing at
the end.
*/
bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
unsigned int fbit, Error **errp)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
uint64_t feature;
virtio_add_feature(&feature, fbit);
assert(k->get_features != NULL);
feature = k->get_features(vdev, feature, errp);
return !virtio_has_feature(feature, fbit);
}
We only check if the feature was not there from the start.
I think you'll still end up with the same problem (overindicating
unsupportedness), as you start out with an otherwise empty feature
set, causing features with dependencies to be removed. I fear that
anything starting with an incomplete feature list will have that
problem.
Maybe it would be better to limit this to the one bit we currently want
to test (VERSION_1)? We know the semantics of that one. Less general,
but also less headaches.
Yes, that could be the solution.
I agree that making it too generic might create confusion
with some features.
Thanks,
Maxime
Re: [Qemu-devel] [PATCH RFC v1 3/3] target-ppc: tlbie should have global effect
On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote:
>
> +env->tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
> check_tlb_flush(env th, 1);
>
Hrm... how did that work bore ? IE. check_tlb_flush won't do anything
if tlb_need_flush is 0, isn't it already set elsewhere ? If not, that's a
bug in the existing bug that I'd suggest you fix separately. If it is, then
this only needs to OR-in TLB_NEED_GLOBAL_FLUSH and only if the hash entry
was found, it, at the point where tlb_need_flush was originally set
> return r
> @@ -319,6 +320,8 @@ static target_ulong h_protect(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> ppc_hash64_store_hpte(cpu, pte_index,
> (v & ~HPTE64_V_VALID) | H.PTE64_V_HPTE_DIRTY, 0);
> ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> +/* Flush the tlb */
> +check_tlb_flush(env, 1);
> /* Don't need a memory barrier, due to qemu's global lock */
> ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
> return H_SUCCESS;
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 7dc..50fe0f5 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1010,6 +1010,7 @@ struct CPUPPCState {
> bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active
> */
> uint32_t tlb_need_flush; /* Delayed flush needed */
> #define TLB_NEED_LOCAL_FLUSH 0x1
> +#define TLB_NEED_GLOBAL_FLUSH 0x2
> #endif
>
> /* Other registers */
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index fe20870..f18b2ff 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -154,13 +154,34 @@ static inline int hreg_store_msr(CPUPPCState *env,
> target_ulong value,
> }
>
> #if !defined(CONFIG_USER_ONLY)
> +static inline void tlb_clear_flag(CPUState *cs)
> +{
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +CPUPPCState *env = &cpu->env;
> +
> +env->tlb_need_flush = 0;
> +}
> +
> static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
> {
> CPUState *cs = CPU(ppc_env_get_cpu(env));
> +CPUState *other_cs;
> +
> if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) ==
> TLB_NEED_LOCAL_FLUSH) {
> -env->tlb_need_flush = 0;
> tlb_flush(cs, 1);
> }
> +
> +if (global &&
> +(env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH) ==
> TLB_NEED_GLOBAL_FLUSH)
> +{
> +CPU_FOREACH(other_cs) {
> +if (other_cs != cs) {
> +tlb_clear_flag(other_cs);
> +tlb_flush(other_cs, 1);
> +}
> +}
> +}
> +env->tlb_need_flush = 0;
> }
> #else
> static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 4c7ceef..f582efe 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -912,7 +912,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
> * invalidate, and we still don't have a tlb_flush_mask(env, n,
> * mask) in QEMU, we just invalidate all TLBs
> */
> -tlb_flush(CPU(cpu), 1);
> +cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
> }
>
> void ppc_hash64_update_rmls(CPUPPCState *env)
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 59dea8f..aee272f 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2757,7 +2757,7 @@ static inline void
> booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn,
>
> void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
> {
> -PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +CPUState *cs;
>
> if (address & 0x4) {
> /* flush all entries */
> @@ -2774,11 +2774,15 @@ void helper_booke206_tlbivax(CPUPPCState *env,
> target_ulong address)
> if (address & 0x8) {
> /* flush TLB1 entries */
> booke206_invalidate_ea_tlb(env, 1, address);
> -tlb_flush(CPU(cpu), 1);
> +CPU_FOREACH(cs) {
> +tlb_flush(cs, 1);
> +}
> } else {
> /* flush TLB0 entries */
> booke206_invalidate_ea_tlb(env, 0, address);
> -tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
> +CPU_FOREACH(cs) {
> +tlb_flush_page(cs, address & MAS2_EPN_MASK);
> +}
> }
> }
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 5f12c41..d3cfbc8 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4443,6 +4443,7 @@ static void gen_tlbie(DisasContext *ctx)
> #if defined(CONFIG_USER_ONLY)
> GEN_PRIV;
> #else
> +TCGv_i32 t1;
> CHK_HV;
>
> if (NARROW_MODE(ctx)) {
> @@ -4453,6 +4454,11 @@ static void gen_tlbie(DisasContext *ctx)
> } else {
> gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> }
> +t1 = tcg_temp_new_i32();
> +tcg_gen_ld_i32(t1, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
> +tcg_ge
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On 09/09/2016 01:20 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 14:04:55 +0300
Marcel Apfelbaum wrote:
On 09/09/2016 01:40 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 12:14:32 +0200
Maxime Coquelin wrote:
This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio-pci.c | 15 +++
hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
ccw") fixes this issue for ccw via the introduction of a
->post_plugged() callback. Unfortunately, we did not find a good way to
make it work for pci back then.
It seems that for ccw is enough to rewind dev->rev_max,
sadly for pci we need to rewind a lot of settings/resources.
Yes, that what I meant with 'more flexibility for ccw'.
Maybe we could replace post_plugged with a pre_plugged approach?
In ->pre_plugged(), cww and pci would specify which features it can
support using virtio_add_feature().
Then we could call get_features() before ->device_plugged().
Doing this, both ccw and pci would have the needed information without
having to rewind any settings.
Does that make sense?
But for now, I think it would be better to merge something in the spirit
of this series (taking into account to remarks).
Indeed, I think we want this fixed in stable, but the above proposal
would be too huge for stable.
Thanks,
Maxime
Re: [Qemu-devel] [PATCH 1/2] virtio: Add backend feature testing functionnality
On 09/09/2016 02:36 PM, Maxime Coquelin wrote:
On 09/09/2016 01:20 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 14:02:17 +0300
Marcel Apfelbaum wrote:
I was thinking to keep the same function proposed by Maxime and change it to
negate things:
/*
* A missing feature before all negotiations finished will still be missing at
the end.
*/
bool virtio_test_backend_unsupported_feature(VirtIODevice *vdev,
unsigned int fbit, Error **errp)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
uint64_t feature;
virtio_add_feature(&feature, fbit);
assert(k->get_features != NULL);
feature = k->get_features(vdev, feature, errp);
return !virtio_has_feature(feature, fbit);
}
We only check if the feature was not there from the start.
I think you'll still end up with the same problem (overindicating
unsupportedness), as you start out with an otherwise empty feature
set, causing features with dependencies to be removed. I fear that
anything starting with an incomplete feature list will have that
problem.
Maybe it would be better to limit this to the one bit we currently want
to test (VERSION_1)? We know the semantics of that one. Less general,
but also less headaches.
Yes, that could be the solution.
I agree that making it too generic might create confusion
with some features.
Sounds good to me. Let's go with it and we'll re-think it if
we'll find other feature negotiation issues later.
Thanks,
Marcel
Thanks,
Maxime
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On Fri, 9 Sep 2016 13:44:35 +0200
Maxime Coquelin wrote:
> On 09/09/2016 01:20 PM, Cornelia Huck wrote:
> > On Fri, 9 Sep 2016 14:04:55 +0300
> > Marcel Apfelbaum wrote:
> >
> >> On 09/09/2016 01:40 PM, Cornelia Huck wrote:
> >>> On Fri, 9 Sep 2016 12:14:32 +0200
> >>> Maxime Coquelin wrote:
> >>>
> This patch makes pci devices plugging more robust, by not confusing
> guest with modern interface when the backend doesn't support
> VIRTIO_F_VERSION_1.
>
> Cc: Marcel Apfelbaum
> Cc: Michael S. Tsirkin
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Maxime Coquelin
> ---
> hw/virtio/virtio-pci.c | 15 +++
> hw/virtio/virtio-pci.h | 5 +
> 2 files changed, 20 insertions(+)
> >>>
> >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
> >>> ccw") fixes this issue for ccw via the introduction of a
> >>> ->post_plugged() callback. Unfortunately, we did not find a good way to
> >>> make it work for pci back then.
> >>
> >> It seems that for ccw is enough to rewind dev->rev_max,
> >> sadly for pci we need to rewind a lot of settings/resources.
> >
> > Yes, that what I meant with 'more flexibility for ccw'.
> Maybe we could replace post_plugged with a pre_plugged approach?
>
> In ->pre_plugged(), cww and pci would specify which features it can
> support using virtio_add_feature().
> Then we could call get_features() before ->device_plugged().
I think that would work for ccw (haven't looked at pci).
>
> Doing this, both ccw and pci would have the needed information without
> having to rewind any settings.
>
> Does that make sense?
>
> But for now, I think it would be better to merge something in the spirit
> of this series (taking into account to remarks).
> Indeed, I think we want this fixed in stable, but the above proposal
> would be too huge for stable.
A 'just check for VERSION_1' approach would probably be best for stable.
Re: [Qemu-devel] [PATCH for-2.7] qtest.c: Allow zero size in memset qtest commands
On 8 September 2016 at 15:37, Eric Blake wrote:
> On 08/05/2016 05:43 AM, Peter Maydell wrote:
>> Some tests use the qtest protocol "memset" command with a zero
>> size, expecting it to do nothing. However in the current code this
>> will result in calling memset() with a NULL pointer, which is
>> undefined behaviour. Detect and specially handle zero sizes to
>> avoid this.
>>
>> Signed-off-by: Peter Maydell
>> ---
>> Looking at the code for the other commands that take a size
>> ('read', 'write', 'b64read' and 'b64write' they all assume a
>> non-zero size. I've left those alone though, somebody else can
>> make them do nothing on zero size if they feel it's important.)
>
> I obviously missed reviewing this in time for 2.7, but looks reasonable
> to me.
>
> Reviewed-by: Eric Blake
Applied to master, thanks.
-- PMM
Re: [Qemu-devel] [PATCH RFC v1 3/3] target-ppc: tlbie should have global effect
Benjamin Herrenschmidt writes: > On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote: >> >> +env->tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH; >> check_tlb_flush(env th, 1); >> > > Hrm... how did that work bore ? IE. check_tlb_flush won't do anything > if tlb_need_flush is 0, isn't it already set elsewhere ? Error of my judgement, it is set in remove_hpte, so the above condition is not required. Good catch. Regards Nikunj
Re: [Qemu-devel] [PATCH 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
On 09/09/2016 01:49 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 13:44:35 +0200
Maxime Coquelin wrote:
On 09/09/2016 01:20 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 14:04:55 +0300
Marcel Apfelbaum wrote:
On 09/09/2016 01:40 PM, Cornelia Huck wrote:
On Fri, 9 Sep 2016 12:14:32 +0200
Maxime Coquelin wrote:
This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio-pci.c | 15 +++
hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for
ccw") fixes this issue for ccw via the introduction of a
->post_plugged() callback. Unfortunately, we did not find a good way to
make it work for pci back then.
It seems that for ccw is enough to rewind dev->rev_max,
sadly for pci we need to rewind a lot of settings/resources.
Yes, that what I meant with 'more flexibility for ccw'.
Maybe we could replace post_plugged with a pre_plugged approach?
In ->pre_plugged(), cww and pci would specify which features it can
support using virtio_add_feature().
Then we could call get_features() before ->device_plugged().
I think that would work for ccw (haven't looked at pci).
Good, once quick fix accepted, I'll try this solution.
Doing this, both ccw and pci would have the needed information without
having to rewind any settings.
Does that make sense?
But for now, I think it would be better to merge something in the spirit
of this series (taking into account to remarks).
Indeed, I think we want this fixed in stable, but the above proposal
would be too huge for stable.
A 'just check for VERSION_1' approach would probably be best for stable.
Ok, thanks.
I will send a v2 replacing the generic function with a VERISON_1
specfic:
bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
Maxime
Re: [Qemu-devel] [Libguestfs] Extracting files from OVA is bad
On Fri, Sep 09, 2016 at 01:03:49PM +0200, Tomáš Golembiovský wrote: > Hi, > > recently we (oVirt) have started discussing whether the way virt-v2v > handles import from OVA files is good. And I would be interested in > ideas how it can be improved. It is likely somebody already gave some > thought to this problem. > > TL;DR: Extracting the OVA before import is a problem for large VMs (in > sizes of TBs). Can we change something to prevent the extraction and > work directly over OVA? Specifically virt-v2v needs to do: qemu-img create -b -f qcow2 overlay.qcow2 qemu-img convert overlay.qcow2 output > What we consider a huge shortcoming is the fact that whole OVA is > extracted prior to the import into a temporary directory and processed > afterwards. Under normal situation user can have up to three copies of > the VM on his drive at the end of import: > > * original OVA, > * temporary extracted files (will be deleted when virt-v2v terminates, > * converted VM. > > > This is not a good idea for large VMs that have hunderds of GBs or even > TBs in size. The requirements on the necessary storage space can be > lessened with proper partitioning. I.e. source OVA and converted VM > don't end up on the same drive and TMPDIR is set to put even temporary > files somewhere else. But this is not a general solution. And sometimes > the necessary space may not be available at all. > > > The question is how to change the import path so that virt-v2v doesn't > have to extract the OVA. I can see the following solutions: > > 1) Solve it virt-v2v: create a layer for directly accessing the files > in the archive. > > 2) Solve it in QEMU: create backing method that would allow creating > qemu disk backed by the archive. As long as the tar file is not compressed, accessing a file within it should be trivial. I asked Kevin if there is a way to get qemu to access a disk image at an offset within another file, but there is no such feature at the moment. It's possible with `losetup', but that requires root :-( (At this point I would normally grumble about how easy this would be with a microkernel, but I won't do that now.) David Gilbert suggested looking at qemu-nbd which has an --offset option, allowing a particular offset with another file to be accessed. If we wanted to do it entirely within virt-v2v, I think this would be the way to go - the complex logic could be hidden inside v2v/input_ova.ml The second problem is to work out the right offset to use. I suspect this is something that http://www.libarchive.org/ can do, and that package is also in RHEL. We could even imagine a qemu block backend based on libarchive. > 3) Solve it on oVirt side: use some FUSE-based tool to provide > access to the archive and pass the OVA to virt-v2v not as a file but > as directory. http://www.cybernoia.de/software/archivemount/ is one such tool which can do this. It's not in RHEL, but it seems to be based on libarchive. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: [Qemu-devel] [PATCH v2] 9pfs: add support for IO limits to 9p-local driver
Hi Pradeep,
two comment below:
On 09.09.2016 11:10, Pradeep Jagadeesh wrote:
> Uses throttling APIs to limit I/O bandwidth and number of operations on the
> devices which use 9p-local driver.
>
> Signed-off-by: Pradeep Jagadeesh
> ---
> fsdev/file-op-9p.h | 3 +
> fsdev/qemu-fsdev-opts.c | 52 +
> hw/9pfs/9p-local.c | 18 -
> hw/9pfs/9p-throttle.c | 201
>
> hw/9pfs/9p-throttle.h | 46 +++
> hw/9pfs/9p.c| 7 ++
> hw/9pfs/Makefile.objs | 1 +
> 7 files changed, 326 insertions(+), 2 deletions(-)
> create mode 100644 hw/9pfs/9p-throttle.c
> create mode 100644 hw/9pfs/9p-throttle.h
>
> This adds the support for the 9p-local driver.
> For now this functionality can be enabled only through qemu cli options.
> QMP interface and support to other drivers need further extensions.
> To make it simple for other drivers, the throttle code has been put in
> separate files.
>
> v1 -> v2:
>
> -Fixed FsContext redeclaration issue
> -Removed couple of function declarations from 9p-throttle.h
> -Fixed some of the .help messages
>
> diff --git a/fsdev/file-op-9p.h b/fsdev/file-op-9p.h
> index 6db9fea..e86b91a 100644
> --- a/fsdev/file-op-9p.h
> +++ b/fsdev/file-op-9p.h
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include "hw/9pfs/9p-throttle.h"
>
> #define SM_LOCAL_MODE_BITS0600
> #define SM_LOCAL_DIR_MODE_BITS0700
> @@ -74,6 +75,7 @@ typedef struct FsDriverEntry {
> char *path;
> int export_flags;
> FileOperations *ops;
> +FsThrottle fst;
> } FsDriverEntry;
>
> typedef struct FsContext
> @@ -83,6 +85,7 @@ typedef struct FsContext
> int export_flags;
> struct xattr_operations **xops;
> struct extended_ops exops;
> +FsThrottle *fst;
> /* fs driver specific data */
> void *private;
> } FsContext;
> diff --git a/fsdev/qemu-fsdev-opts.c b/fsdev/qemu-fsdev-opts.c
> index 1dd8c7a..2774855 100644
> --- a/fsdev/qemu-fsdev-opts.c
> +++ b/fsdev/qemu-fsdev-opts.c
> @@ -37,6 +37,58 @@ static QemuOptsList qemu_fsdev_opts = {
> }, {
> .name = "sock_fd",
> .type = QEMU_OPT_NUMBER,
> +}, {
> +.name = "bps",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit total bytes per second",
> +}, {
> +.name = "bps_rd",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit read bytes per second",
> +}, {
> +.name = "bps_wr",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit write bytes per second",
> +}, {
> +.name = "iops",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit total io operations per second",
> +}, {
> +.name = "iops_rd",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit read operations per second ",
> +}, {
> +.name = "iops_wr",
> +.type = QEMU_OPT_NUMBER,
> +.help = "limit write operations per second",
> +}, {
> +.name = "bps_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum bytes burst",
> +}, {
> +.name = "bps_rd_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum bytes read burst",
> +}, {
> +.name = "bps_wr_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum bytes write burst",
> +}, {
> +.name = "iops_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum io operations burst",
> +}, {
> +.name = "iops_rd_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum io operations read burst",
> +}, {
> +.name = "iops_wr_max",
> +.type = QEMU_OPT_NUMBER,
> +.help = "maximum io operations write burst",
> +}, {
> +.name = "iops_size",
> +.type = QEMU_OPT_NUMBER,
> +.help = "io iops-size",
> },
>
> { /*End of list */ }
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 3f271fc..49c2819 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -420,6 +420,8 @@ static ssize_t local_preadv(FsContext *ctx,
> V9fsFidOpenState *fs,
> const struct iovec *iov,
> int iovcnt, off_t offset)
> {
> +throttle9p_request(ctx->fst, false, iov->iov_len);
> +
> #ifdef CONFIG_PREADV
> return preadv(fs->fd, iov, iovcnt, offset);
> #else
> @@ -436,8 +438,10 @@ static ssize_t local_pwritev(FsContext *ctx,
> V9fsFidOpenState *fs,
> const struct iovec *iov,
> int iovcnt, off_t offset)
> {
> -ssize_t ret
> -;
> +ssize_t ret;
> +
> +throttle9p_request(ctx->fst, true, iov->iov_len);
> +
> #ifdef CONFIG_PREADV
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
On Fri, Sep 09, 2016 at 01:03:51PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
>
> > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
> >> Daniel P Berrange writes:
> >>
> >> > I previously split the global trace-events file up into one file
> >> > per-subdirectory to avoid merge conflict hell.
> >> [...]
> >>
> >> Sorry, I could not find the message where the infrastructure is modified to
> >> provide this. But I think there's a more efficient way to provide modular
> >> auto-generated tracing code without the hierarchical indexing you proposed.
>
> > [snip]
>
> >> struct TraceEvent ___trace_events[] = {
> >> {
> >> .name = "eventname",
> >> .sstate = 1,
> >> .dstate = ___trace_eventname_dstate;
> >> }
> >> }
> >>
> >> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>
> > Life would be simpler if we had the 'bool dstate' as part of the
> > TraceEvent struct, but doing so would essentially be reverting this
> > previous change:
>
> > commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
> > Author: Paolo Bonzini
> > Date: Wed Oct 28 07:06:27 2015 +0100
>
> > trace: track enabled events in a separate array
>
> > This is more cache friendly on the fast path, where we already have
> > the event id available.
>
> > I asked Paolo about this previously and he indicated it was a notable
> > performance improvement, so we can't put dstate back into the TraceEvent
> > struct :-(
>
> Sorry, there was a typo in my example code that led to this misunderstanding.
>
> I meant this for the global auto-generated .c:
>
> struct TraceEvent {
> /* ... */
> bool *dstate;
> };
>
> bool ___TRACE_EVENTNAME_DSTATE;
>
> struct TraceEvent ___trace_events[] = {
> {
> .name = "eventname",
> .sstate = 1,
> .dstate = &___TRACE_EVENTNAME_DSTATE;
> }
> }
>
> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>
>
> If you look at the modified macro I pasted for "trace/control-internal.h":
>
>
> #define trace_event_get_state_dynamic_by_id(id) \
> (unlikely(trace_events_enabled_count) && \
>(___ ## id ## _DSTATE))
>
> We're retaining the fast-path performance of the seggregated boolean dstate
> array, while the event descriptor contains a pointer to the per-event dstate
> global boolean variable in case you want to get/set the dstate based on an
> event
> pointer.
The various _DSTATE variables are still arbitrarily scattered in
memory, as opposed to in a contiguous cache friendly array, which
is one of the goals of Paolo's original change.
That said, I'm unclear on how much of the performance win from
Paolo's change came from eliminating the struct field de-reference,
vs having the contiguous array. I'm guessing most of the win is
from the former, the latter only being important if we hit multiple
related tracepoints on close succession.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PULL 0/9] Docker patches
On 9 September 2016 at 06:52, Fam Zheng wrote: > The following changes since commit 2926375cffce464fde6b4dabaed1e133d549af39: > > Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging > (2016-09-06 17:18:17 +0100) > > are available in the git repository at: > > g...@github.com:famz/qemu tags/docker-pull-request > > for you to fetch changes up to f8042deafa724924d03c44c7bd21e269ccb5190b: > > docker: silence debootstrap when --quiet is given (2016-09-08 19:56:34 > +0800) > > > > This includes Sascha Silbe's improvements on the debian-bootstrap image and > the > new min-glib image. > > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v4 2/3] tests: make pc_alloc_init/init_flags/uninit generic
On Thu, 8 Sep 2016 09:50:31 +0200
Laurent Vivier wrote:
> On 08/09/2016 04:04, David Gibson wrote:
> > On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:
> >> And add support for ppc64.
> >>
> >> Signed-off-by: Laurent Vivier
> >
> > Some of my coments may be obsoleted by the discussion with Greg.
> >
> >> ---
> >> v2:
> >> - remove useless parenthesis, inline
> [...]
> >> +
> >> +QGuestAllocator *machine_alloc_init(void)
> >> +{
> >> +const char *arch = qtest_get_arch();
> >
> > Maybe we need to add a qtest_get_machine_type().
>
> I'm working on that...
>
The problem is that qtest only knows about archs, based on $(TARGETS).
Maybe the machine type could be the default one for a given arch ?
Cheers.
--
Greg
Re: [Qemu-devel] [PATCH v4 2/3] tests: make pc_alloc_init/init_flags/uninit generic
On 09/09/2016 14:25, Greg Kurz wrote:
> On Thu, 8 Sep 2016 09:50:31 +0200
> Laurent Vivier wrote:
>
>> On 08/09/2016 04:04, David Gibson wrote:
>>> On Tue, Sep 06, 2016 at 03:17:56PM +0200, Laurent Vivier wrote:
And add support for ppc64.
Signed-off-by: Laurent Vivier
>>>
>>> Some of my coments may be obsoleted by the discussion with Greg.
>>>
---
v2:
- remove useless parenthesis, inline
>> [...]
+
+QGuestAllocator *machine_alloc_init(void)
+{
+const char *arch = qtest_get_arch();
>>>
>>> Maybe we need to add a qtest_get_machine_type().
>>
>> I'm working on that...
>>
>
> The problem is that qtest only knows about archs, based on $(TARGETS).
> Maybe the machine type could be the default one for a given arch ?
Once the machine is started we can use QMP[1] to ask the machine type
(for instance, "pseries-2.7-machine").
So what we could do is a generic qtest_machine_vboot() which ask the
machine type and configure the qtest framework accordingly.
Laurent
[1] { 'execute': 'qom-get', 'arguments': { 'path': '/machine',
'property': 'type' } }
[Qemu-devel] [PATCH 2/2] mirror: fix improperly filled copy_bitmap for mirror block job
bdrv_is_allocated_above() returns true in the case even for completel
zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.
The patch stops using bdrv_is_allocated_above() wrapper and switches to
bdrv_get_block_status_above() to distinguish zeroed areas and areas with
data to avoid extra IO operations if possible.
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
CC: Fam Zheng
CC: Kevin Wolf
CC: Max Reitz
CC: Jeff Cody
---
block/mirror.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index e0b3f41..da55375 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -548,14 +548,15 @@ static void mirror_throttle(MirrorBlockJob *s)
static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
{
-int64_t sector_num, end;
+int64_t sector_num, end, alloc_mask;
BlockDriverState *base = s->base;
BlockDriverState *bs = blk_bs(s->common.blk);
BlockDriverState *target_bs = blk_bs(s->target);
-int ret, n;
+int n;
end = s->bdev_length / BDRV_SECTOR_SIZE;
+alloc_mask = BDRV_BLOCK_ALLOCATED;
if (base == NULL && !bdrv_has_zero_init(target_bs)) {
if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
@@ -583,6 +584,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
}
mirror_drain(s);
+
+alloc_mask = BDRV_BLOCK_DATA;
}
/* First part, loop on the sectors and initialize the dirty bitmap. */
@@ -590,6 +593,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
/* Just to make sure we are not exceeding int limit. */
int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
end - sector_num);
+int64_t status;
+BlockDriverState *file;
mirror_throttle(s);
@@ -597,13 +602,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob
*s)
return 0;
}
-ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
-if (ret < 0) {
-return ret;
+status = bdrv_get_block_status_above(bs, base, sector_num,
+ nb_sectors, &n, &file);
+if (status < 0) {
+return status;
}
assert(n > 0);
-if (ret == 1) {
+if (status & alloc_mask) {
bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
}
sector_num += n;
--
2.7.4
[Qemu-devel] [PATCH v2 0/2] mirror: fix improperly filled copy_bitmap for mirror block job
bdrv_is_allocated_above() returns true in the case even for completel zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases. The patch stops using bdrv_is_allocated_above() wrapper and switches to bdrv_get_block_status_above() to distinguish zeroed areas and areas with data to avoid extra IO operations if possible. Though this change requires some preparations in bdrv_get_block_status_above() performed in the patch (1). Changes from v1: - fixed assert in 041 test case (added patch 1) - fixed commit message - fixed status check to be on the safe side Signed-off-by: Denis V. Lunev CC: Stefan Hajnoczi CC: Fam Zheng CC: Kevin Wolf CC: Max Reitz CC: Jeff Cody Denis V. Lunev (2): block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above() mirror: fix improperly filled copy_bitmap for mirror block job block/io.c | 26 -- block/mirror.c | 18 -- 2 files changed, 32 insertions(+), 12 deletions(-) -- 2.7.4
[Qemu-devel] [PATCH 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()
They should work very similar, covering same areas if backing store is
shorter than the image. This change is necessary for the followup patch
switching to bdrv_get_block_status_above() in mirror to avoid assert
in check_block.
Signed-off-by: Denis V. Lunev
CC: Stefan Hajnoczi
CC: Fam Zheng
CC: Kevin Wolf
CC: Max Reitz
CC: Jeff Cody
---
block/io.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/block/io.c b/block/io.c
index 420944d..0422123 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1745,14 +1745,28 @@ static int64_t coroutine_fn
bdrv_co_get_block_status_above(BlockDriverState *bs,
assert(bs != base);
for (p = bs; p != base; p = backing_bs(p)) {
-ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
-if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
-break;
+int sc;
+ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file);
+if (ret < 0) {
+return ret;
+} else if (ret & BDRV_BLOCK_ALLOCATED) {
+*pnum = sc;
+return ret;
+}
+
+/*
+ * [sector_num, nb_sectors] is unallocated on top but intermediate
+ * might have
+ *
+ * [sector_num+x, nr_sectors] allocated.
+ */
+if (nb_sectors > sc &&
+(p == bs || sector_num + sc < p->total_sectors)) {
+nb_sectors = sc;
}
-/* [sector_num, pnum] unallocated on this layer, which could be only
- * the first part of [sector_num, nb_sectors]. */
-nb_sectors = MIN(nb_sectors, *pnum);
}
+
+*pnum = nb_sectors;
return ret;
}
--
2.7.4
Re: [Qemu-devel] [PATCH v6 2/2] qdisk - hw/block/xen_disk: grant copy implementation
On 09/08/2016 12:00 AM, Paulina Szubarczyk wrote:
On 09/07/2016 10:56 PM, Stefano Stabellini wrote:
On Wed, 7 Sep 2016, Paulina Szubarczyk wrote:
Copy data operated on during request from/to local buffers to/from
the grant references.
Before grant copy operation local buffers must be allocated what is
done by calling ioreq_init_copy_buffers. For the 'read' operation,
first, the qemu device invokes the read operation on local buffers
and on the completion grant copy is called and buffers are freed.
For the 'write' operation grant copy is performed before invoking
write by qemu device.
A new value 'feature_grant_copy' is added to recognize when the
grant copy operation is supported by a guest.
Signed-off-by: Paulina Szubarczyk
---
Changes since v5:
-added checking of every interface in the configure file. Based on
the Roger's comment that xengnttab_map_grant_ref was added prior
xengnttab_grant_copy, thus do not need to be check again here
I dropped this check.
Thank you Paulina, the patch is good. Thanks for your work! Sorry for
coming in late in the review; I have a couple of minor suggestions
below.
It had not been possible without all the help I have got. I am very
grateful for it.
In addition to Anthony's ack, it would be also nice to have Roger's ack
on this patch.
Roger, will you need more time to take a look at the patch or may I send
a new version with applied Stefano comments?
configure | 55
hw/block/xen_disk.c | 157
++--
include/hw/xen/xen_common.h | 14
3 files changed, 221 insertions(+), 5 deletions(-)
diff --git a/configure b/configure
index 4b808f9..3f44d38 100755
--- a/configure
+++ b/configure
@@ -1956,6 +1956,61 @@ EOF
/*
* If we have stable libs the we don't want the libxc compat
* layers, regardless of what CFLAGS we may have been given.
+ *
+ * Also, check if xengnttab_grant_copy_segment_t is defined and
+ * grant copy operation is implemented.
+ */
+#undef XC_WANT_COMPAT_EVTCHN_API
+#undef XC_WANT_COMPAT_GNTTAB_API
+#undef XC_WANT_COMPAT_MAP_FOREIGN_API
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#if !defined(HVM_MAX_VCPUS)
+# error HVM_MAX_VCPUS not defined
+#endif
+int main(void) {
+ xc_interface *xc = NULL;
+ xenforeignmemory_handle *xfmem;
+ xenevtchn_handle *xe;
+ xengnttab_handle *xg;
+ xen_domain_handle_t handle;
+ xengnttab_grant_copy_segment_t* seg = NULL;
+
+ xs_daemon_open();
+
+ xc = xc_interface_open(0, 0, 0);
+ xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+ xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
+ xc_hvm_inject_msi(xc, 0, 0xf000, 0x);
+ xc_hvm_create_ioreq_server(xc, 0, HVM_IOREQSRV_BUFIOREQ_ATOMIC,
NULL);
+ xc_domain_create(xc, 0, handle, 0, NULL, NULL);
+
+ xfmem = xenforeignmemory_open(0, 0);
+ xenforeignmemory_map(xfmem, 0, 0, 0, 0, 0);
+
+ xe = xenevtchn_open(0, 0);
+ xenevtchn_fd(xe);
+
+ xg = xengnttab_open(0, 0);
+ xengnttab_grant_copy(xg, 0, seg);
+
+ return 0;
+}
+EOF
+ compile_prog "" "$xen_libs $xen_stable_libs"
+then
+xen_ctrl_version=480
+xen=yes
+ elif
+ cat > $TMPC <= 480
In general I prefer to avoid this kind of #ifdef in xen_disk.c, but I
see that Anthony suggested it for a good reason. The only alternartive I
can think of would be to introduce two static inline functions in
xen_common.h to set a xengnttab_grant_copy_segment_t seg. But this is
also OK.
The functions take as parameters pointers to struct ioreq defined in
xen_disk.c which members are used to fill the
xengnttab_grant_copy_segment_t. There would be some overhead to move the
functions to the header.
+static void free_buffers(struct ioreq *ioreq)
Please name this function ioreq_free_copy_buffers to make it clearer
that has to do with the same buffers initialized below.
+{
+int i;
+
+for (i = 0; i < ioreq->v.niov; i++) {
+ioreq->page[i] = NULL;
+}
+
+qemu_vfree(ioreq->pages);
+}
+
+static int ioreq_init_copy_buffers(struct ioreq *ioreq)
+{
+int i;
+
+if (ioreq->v.niov == 0) {
+return 0;
+}
+
+ioreq->pages = qemu_memalign(XC_PAGE_SIZE, ioreq->v.niov *
XC_PAGE_SIZE);
+
+for (i = 0; i < ioreq->v.niov; i++) {
+ioreq->page[i] = ioreq->pages + i * XC_PAGE_SIZE;
+ioreq->v.iov[i].iov_base = ioreq->page[i];
+}
+
+return 0;
+}
+
+static int ioreq_copy(struct ioreq *ioreq)
Please name this function in a way that makes it clear that it has
something to do with grant copies. Like for example ioreq_grant_copy.
I will change the names of both functions and remove the blank line
pointed below.
+{
+xengnttab_handle *gnt = ioreq->blkdev->xendev.gnttabdev;
+xengnttab_grant_copy_segment_t
segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+int i, count, rc;
+int64_t file_blk = ioreq->blkdev->file_blk;
+
+if (ioreq->v.niov == 0) {
+return 0;
+}
+
+count = iore
Re: [Qemu-devel] [PATCH v5 20/20] Replace qmp-commands.hx by doc/qmp-commands.txt
s,doc/,docs/ in subject. Can touch up on commit.
Re: [Qemu-devel] [PATCH v3 05/34] int128: Add int128_make128
On Sat, Sep 03, 2016 at 09:39:33PM +0100, Richard Henderson wrote:
> Allows Int128 to be used more generally, rather than having to
> begin with 64-bit inputs and accumulate.
>
> Signed-off-by: Richard Henderson
> ---
> include/qemu/int128.h | 20 +++-
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index 08f1db1..67440fa 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -10,6 +10,11 @@ static inline Int128 int128_make64(uint64_t a)
> return a;
> }
>
> +static inline Int128 int128_make128(uint64_t lo, uint64_t hi)
> +{
> +return (unsigned __int128)hi << 64 | lo;
> +}
> +
This causes build failures for me on CentOS6.5:
/user/lea/dev/qemu/include/qemu/int128.h:7: error: expected ‘=’, ‘,’, ‘;’,
‘asm’ or ‘__attribute__’ before ‘Int128’
/user/lea/dev/qemu/include/qemu/int128.h:9: error: expected ‘=’, ‘,’, ‘;’,
‘asm’ or ‘__attribute__’ before ‘int128_make64’
/user/lea/dev/qemu/include/qemu/int128.h:14: error: expected ‘=’, ‘,’, ‘;’,
‘asm’ or ‘__attribute__’ before ‘int128_make128’
(...)
This is because CONFIG_INT128 is set if test for __int128_t succeeds, not
__int128. The following change on top of patches 4 and 5 in this series
fixes the problem for me:
diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 261b55f..5c9890d 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -4,7 +4,7 @@
#ifdef CONFIG_INT128
#include "qemu/bswap.h"
-typedef __int128 Int128;
+typedef __int128_t Int128;
static inline Int128 int128_make64(uint64_t a)
{
@@ -13,7 +13,7 @@ static inline Int128 int128_make64(uint64_t a)
static inline Int128 int128_make128(uint64_t lo, uint64_t hi)
{
-return (unsigned __int128)hi << 64 | lo;
+return (__uint128_t)hi << 64 | lo;
}
[Qemu-devel] [PATCH v2 2/6] queue: Add macro for incremental traversal
Adds macro QTAILQ_FOREACH_CONTINUE to support incremental list
traversal.
Signed-off-by: Lluís Vilanova
---
include/qemu/queue.h |5 +
1 file changed, 5 insertions(+)
diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index c2b6c81..4d57166 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -414,6 +414,11 @@ struct {
\
(var); \
(var) = ((var)->field.tqe_next))
+#define QTAILQ_FOREACH_CONTINUE(var, field) \
+for ((var) = ((var)->field.tqe_next); \
+(var); \
+(var) = ((var)->field.tqe_next))
+
#define QTAILQ_FOREACH_SAFE(var, head, field, next_var) \
for ((var) = ((head)->tqh_first); \
(var) && ((next_var) = ((var)->field.tqe_next), 1); \
[Qemu-devel] [RFC PATCH v2 0/6] translate: [tcg] Generic translation framework
This series proposes a generic (target-agnostic) instruction translation framework. It basically provides a generic main loop for instruction disassembly, which calls target-specific functions when necessary. This generalization makes inserting new code in the main loop easier, and helps in keeping all targets in synch as to the contents of it. I've only ported i386 as an example to get some feedback, but I'm planning on porting ARM next to see how well it fits into the current organization. Signed-off-by: Lluís Vilanova --- Changes in v2 = * Port ARM and AARCH64 targets. * Fold single-stepping checks into "max_insns" [Richard Henderson]. * Move instruction start marks to target code [Richard Henderson]. * Add target hook for TB start. * Check for TCG temporary leaks. * Move instruction disassembly into a target hook. * Make breakpoint_hit() return an enum to accomodate target's needs (ARM). Lluís Vilanova (6): Pass generic CPUState to gen_intermediate_code() queue: Add macro for incremental traversal target: [tcg] Add generic translation framework target: [tcg] Redefine DISAS_* onto the generic translation framework (DJ_*) target: [tcg,i386] Port to generic translation framework target: [tcg,arm] Port to generic translation framework include/exec/exec-all.h | 13 - include/exec/gen-icount.h |2 include/exec/translate-all_template.h | 76 +++ include/qemu/queue.h |5 include/qom/cpu.h | 21 + target-alpha/translate.c | 11 - target-arm/translate-a64.c| 342 target-arm/translate.c| 718 + target-arm/translate.h| 41 +- target-cris/translate.c | 20 - target-i386/translate.c | 307 +++--- target-lm32/translate.c | 22 + target-m68k/translate.c | 18 - target-microblaze/translate.c | 24 + target-mips/translate.c | 15 - target-moxie/translate.c | 14 - target-openrisc/translate.c | 24 + target-ppc/translate.c| 15 - target-s390x/translate.c | 16 - target-sh4/translate.c| 15 - target-sparc/translate.c | 11 - target-tilegx/translate.c |7 target-tricore/translate.c|9 target-unicore32/translate.c | 20 - target-xtensa/translate.c | 13 - translate-all.c |2 translate-all_template.h | 200 + 27 files changed, 1130 insertions(+), 851 deletions(-) create mode 100644 include/exec/translate-all_template.h create mode 100644 translate-all_template.h To: qemu-devel@nongnu.org Cc: Paolo Bonzini Cc: Peter Crosthwaite Cc: Richard Henderson
[Qemu-devel] [PATCH v2 3/6] target: [tcg] Add generic translation framework
Signed-off-by: Lluís Vilanova
---
include/exec/gen-icount.h |2
include/exec/translate-all_template.h | 76 +
include/qom/cpu.h | 21 +++
translate-all_template.h | 200 +
4 files changed, 298 insertions(+), 1 deletion(-)
create mode 100644 include/exec/translate-all_template.h
create mode 100644 translate-all_template.h
diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 050de59..c91ac95 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -45,7 +45,7 @@ static inline void gen_tb_start(TranslationBlock *tb)
tcg_temp_free_i32(count);
}
-static void gen_tb_end(TranslationBlock *tb, int num_insns)
+static inline void gen_tb_end(TranslationBlock *tb, int num_insns)
{
gen_set_label(exitreq_label);
tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_REQUESTED);
diff --git a/include/exec/translate-all_template.h
b/include/exec/translate-all_template.h
new file mode 100644
index 000..a552db0
--- /dev/null
+++ b/include/exec/translate-all_template.h
@@ -0,0 +1,76 @@
+/*
+ * Generic intermediate code generation.
+ *
+ * Copyright (C) 2016 Lluís Vilanova
+ *
+ * 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 EXEC__TRANSLATE_ALL_TEMPLATE_H
+#define EXEC__TRANSLATE_ALL_TEMPLATE_H
+
+/*
+ * Include this header from a target-specific file, and add a
+ *
+ * DisasContextBase base;
+ *
+ * member in your target-specific DisasContext.
+ */
+
+
+#include "exec/exec-all.h"
+
+
+/**
+ * BreakpointHitType:
+ * @BH_MISS: No hit
+ * @BH_HIT_INSN: Hit, but continue translating instruction
+ * @BH_HIT_TB: Hit, stop translating TB
+ *
+ * How to react to a breakpoint hit.
+ */
+typedef enum BreakpointHitType
+{
+BH_MISS,
+BH_HIT_INSN,
+BH_HIT_TB,
+} BreakpointHitType;
+
+/**
+ * DisasJumpType:
+ * @DJ_NEXT: Next instruction in program order
+ * @DJ_TOO_MANY: Too many instructions executed
+ * @DJ_TARGET: Start of target-specific conditions
+ *
+ * What instruction to disassemble next.
+ */
+typedef enum DisasJumpType
+{
+DJ_NEXT,
+DJ_TOO_MANY,
+DJ_TARGET,
+} DisasJumpType;
+
+/**
+ * DisasContextBase:
+ * @tb: Translation block for this disassembly.
+ * @singlestep_enabled: "Hardware" single stepping enabled.
+ * @pc_first: Address of first guest instruction in this TB.
+ * @pc_next: Address of next guest instruction in this TB (current during
+ * disassembly).
+ * @num_insns: Number of translated instructions (including current).
+ *
+ * Architecture-agnostic disassembly context.
+ */
+typedef struct DisasContextBase
+{
+TranslationBlock *tb;
+bool singlestep_enabled;
+target_ulong pc_first;
+target_ulong pc_next;
+DisasJumpType jmp_type;
+unsigned int num_insns;
+} DisasContextBase;
+
+#endif /* EXEC__TRANSLATE_ALL_TEMPLATE_H */
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index ce0c406..68b72b9 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -848,6 +848,27 @@ static inline bool cpu_breakpoint_test(CPUState *cpu,
vaddr pc, int mask)
return false;
}
+/* Get first breakpoint matching a PC */
+static inline CPUBreakpoint *cpu_breakpoint_get(CPUState *cpu, vaddr pc,
CPUBreakpoint *bp)
+{
+if (likely(bp == NULL)) {
+if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
+QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
+if (bp->pc == pc) {
+return bp;
+}
+}
+}
+} else {
+QTAILQ_FOREACH_CONTINUE(bp, entry) {
+if (bp->pc == pc) {
+return bp;
+}
+}
+}
+return NULL;
+}
+
int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
int flags, CPUWatchpoint **watchpoint);
int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
diff --git a/translate-all_template.h b/translate-all_template.h
new file mode 100644
index 000..f1e4d89
--- /dev/null
+++ b/translate-all_template.h
@@ -0,0 +1,200 @@
+/*
+ * Generic intermediate code generation.
+ *
+ * Copyright (C) 2016 Lluís Vilanova
+ *
+ * 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 TRANSLATE_ALL_TEMPLATE_H
+#define TRANSLATE_ALL_TEMPLATE_H
+
+/*
+ * Include this header from a target-specific file, which must define the
+ * target-specific functions declared below.
+ *
+ * These must be paired with instructions in "exec/translate-all_template.h".
+ */
+
+
+#include "cpu.h"
+#include "qemu/error-report.h"
+
+
+static void gen_intermediate_code_target_init_disas_context(
+DisasContext * restrict dc, CPUArchState * restrict env);
+
+static void gen_intermediate_code_target_init_globals(
+DisasContext * restrict dc, CPUArchState * restrict env);
+
+static void gen_inte
[Qemu-devel] [PATCH v2 5/6] target: [tcg, i386] Port to generic translation framework
Signed-off-by: Lluís Vilanova
---
target-i386/translate.c | 306 ++-
1 file changed, 142 insertions(+), 164 deletions(-)
diff --git a/target-i386/translate.c b/target-i386/translate.c
index bfd9aed..a02398d 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -69,6 +69,10 @@
case (2 << 6) | (OP << 3) | 0 ... (2 << 6) | (OP << 3) | 7: \
case (3 << 6) | (OP << 3) | 0 ... (3 << 6) | (OP << 3) | 7
+#include "exec/translate-all_template.h"
+#define DJ_JUMP (DJ_TARGET+0) /* end of block due to call/jump */
+#define DJ_MISC (DJ_TARGET+1) /* some other reason */
+
//#define MACRO_TEST 1
/* global register indexes */
@@ -94,7 +98,11 @@ static TCGv_i64 cpu_tmp1_i64;
static int x86_64_hregs;
#endif
-typedef struct DisasContext {
+
+typedef struct DisasContext
+{
+DisasContextBase base;
+
/* current insn context */
int override; /* -1 if no override */
int prefix;
@@ -102,8 +110,6 @@ typedef struct DisasContext {
TCGMemOp dflag;
target_ulong pc_start;
target_ulong pc; /* pc = eip + cs_base */
-int is_jmp; /* 1 = means jump (stop translation), 2 means CPU
- static state change (stop translation) */
/* current block context */
target_ulong cs_base; /* base of CS segment */
int pe; /* protected mode */
@@ -124,12 +130,10 @@ typedef struct DisasContext {
int cpl;
int iopl;
int tf; /* TF cpu flag */
-int singlestep_enabled; /* "hardware" single step enabled */
int jmp_opt; /* use direct block chaining for direct jumps */
int repz_opt; /* optimize jumps within repz instructions */
int mem_index; /* select memory access functions */
uint64_t flags; /* all execution flags */
-struct TranslationBlock *tb;
int popl_esp_hack; /* for correct popl with esp base handling */
int rip_offset; /* only used in x86_64, but left for simplicity */
int cpuid_features;
@@ -140,6 +144,8 @@ typedef struct DisasContext {
int cpuid_xsave_features;
} DisasContext;
+#include "translate-all_template.h"
+
static void gen_eob(DisasContext *s);
static void gen_jmp(DisasContext *s, target_ulong eip);
static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num);
@@ -1113,7 +1119,7 @@ static void gen_bpt_io(DisasContext *s, TCGv_i32 t_port,
int ot)
static inline void gen_ins(DisasContext *s, TCGMemOp ot)
{
-if (s->tb->cflags & CF_USE_ICOUNT) {
+if (s->base.tb->cflags & CF_USE_ICOUNT) {
gen_io_start();
}
gen_string_movl_A0_EDI(s);
@@ -1128,14 +1134,14 @@ static inline void gen_ins(DisasContext *s, TCGMemOp ot)
gen_op_movl_T0_Dshift(ot);
gen_op_add_reg_T0(s->aflag, R_EDI);
gen_bpt_io(s, cpu_tmp2_i32, ot);
-if (s->tb->cflags & CF_USE_ICOUNT) {
+if (s->base.tb->cflags & CF_USE_ICOUNT) {
gen_io_end();
}
}
static inline void gen_outs(DisasContext *s, TCGMemOp ot)
{
-if (s->tb->cflags & CF_USE_ICOUNT) {
+if (s->base.tb->cflags & CF_USE_ICOUNT) {
gen_io_start();
}
gen_string_movl_A0_ESI(s);
@@ -1148,7 +1154,7 @@ static inline void gen_outs(DisasContext *s, TCGMemOp ot)
gen_op_movl_T0_Dshift(ot);
gen_op_add_reg_T0(s->aflag, R_ESI);
gen_bpt_io(s, cpu_tmp2_i32, ot);
-if (s->tb->cflags & CF_USE_ICOUNT) {
+if (s->base.tb->cflags & CF_USE_ICOUNT) {
gen_io_end();
}
}
@@ -2089,7 +2095,7 @@ static inline int insn_const_size(TCGMemOp ot)
static inline bool use_goto_tb(DisasContext *s, target_ulong pc)
{
#ifndef CONFIG_USER_ONLY
-return (pc & TARGET_PAGE_MASK) == (s->tb->pc & TARGET_PAGE_MASK) ||
+return (pc & TARGET_PAGE_MASK) == (s->base.tb->pc & TARGET_PAGE_MASK) ||
(pc & TARGET_PAGE_MASK) == (s->pc_start & TARGET_PAGE_MASK);
#else
return true;
@@ -2104,7 +2110,7 @@ static inline void gen_goto_tb(DisasContext *s, int
tb_num, target_ulong eip)
/* jump to same page: we can use a direct jump */
tcg_gen_goto_tb(tb_num);
gen_jmp_im(eip);
-tcg_gen_exit_tb((uintptr_t)s->tb + tb_num);
+tcg_gen_exit_tb((uintptr_t)s->base.tb + tb_num);
} else {
/* jump to another page: currently not optimized */
gen_jmp_im(eip);
@@ -2125,7 +2131,7 @@ static inline void gen_jcc(DisasContext *s, int b,
gen_set_label(l1);
gen_goto_tb(s, 1, val);
-s->is_jmp = DISAS_TB_JUMP;
+s->base.jmp_type = DJ_JUMP;
} else {
l1 = gen_new_label();
l2 = gen_new_label();
@@ -2196,11 +2202,11 @@ static void gen_movl_seg_T0(DisasContext *s, int
seg_reg)
stop as a special handling must be done to disable hardware
interrupts for the next instruction */
if (seg_reg == R_SS || (s->code32 && seg_reg < R_FS))
-s->is_jmp = DISAS_TB_JUMP;
+s->base.jmp_type = DJ_JUMP;
} else {
gen_op_movl_seg_T0_vm(seg_reg);
[Qemu-devel] [PATCH v2 4/6] target: [tcg] Redefine DISAS_* onto the generic translation framework (DJ_*)
Temporarily redefine DISAS_* values based on DJ_TARGET. They should
disappear as targets get ported to the generic framework.
Signed-off-by: Lluís Vilanova
---
include/exec/exec-all.h | 11 +++
target-arm/translate.h | 15 ---
target-cris/translate.c |3 ++-
target-m68k/translate.c |3 ++-
target-s390x/translate.c |3 ++-
target-unicore32/translate.c |3 ++-
6 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 17659e6..54dc4ad 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -36,10 +36,13 @@ typedef ram_addr_t tb_page_addr_t;
#endif
/* is_jmp field values */
-#define DISAS_NEXT0 /* next instruction can be analyzed */
-#define DISAS_JUMP1 /* only pc was modified dynamically */
-#define DISAS_UPDATE 2 /* cpu state was modified dynamically */
-#define DISAS_TB_JUMP 3 /* only pc was modified statically */
+/* TODO: delete after all targets are transitioned to generic translation */
+#include "exec/translate-all_template.h"
+#define DISAS_NEXTDJ_NEXT /* next instruction can be analyzed */
+#define DISAS_JUMP(DJ_TARGET+0) /* only pc was modified dynamically */
+#define DISAS_UPDATE (DJ_TARGET+1) /* cpu state was modified dynamically
*/
+#define DISAS_TB_JUMP (DJ_TARGET+2) /* only pc was modified statically */
+#define DISAS_TARGET (DJ_TARGET+3) /* base for target-specific values */
#include "qemu/log.h"
diff --git a/target-arm/translate.h b/target-arm/translate.h
index dbd7ac8..602763c 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -107,21 +107,22 @@ static inline int default_exception_el(DisasContext *s)
}
/* target-specific extra values for is_jmp */
+/* TODO: rename as DJ_* when transitioning this target to generic translation
*/
/* These instructions trap after executing, so the A32/T32 decoder must
* defer them until after the conditional execution state has been updated.
* WFI also needs special handling when single-stepping.
*/
-#define DISAS_WFI 4
-#define DISAS_SWI 5
+#define DISAS_WFI DISAS_TARGET + 0
+#define DISAS_SWI DISAS_TARGET + 1
/* For instructions which unconditionally cause an exception we can skip
* emitting unreachable code at the end of the TB in the A64 decoder
*/
-#define DISAS_EXC 6
+#define DISAS_EXC DISAS_TARGET + 2
/* WFE */
-#define DISAS_WFE 7
-#define DISAS_HVC 8
-#define DISAS_SMC 9
-#define DISAS_YIELD 10
+#define DISAS_WFE DISAS_TARGET + 3
+#define DISAS_HVC DISAS_TARGET + 4
+#define DISAS_SMC DISAS_TARGET + 5
+#define DISAS_YIELD DISAS_TARGET + 6
#ifdef TARGET_AARCH64
void a64_translate_init(void);
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 284543a..ab48679 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -50,7 +50,8 @@
#define BUG() (gen_BUG(dc, __FILE__, __LINE__))
#define BUG_ON(x) ({if (x) BUG();})
-#define DISAS_SWI 5
+/* TODO: rename as DJ_* when transitioning this target to generic translation
*/
+#define DISAS_SWI DISAS_TARGET + 0
/* Used by the decoder. */
#define EXTRACT_FIELD(src, start, end) \
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index f7d131f..75ffcc3 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -141,7 +141,8 @@ typedef struct DisasContext {
int done_mac;
} DisasContext;
-#define DISAS_JUMP_NEXT 4
+/* TODO: rename as DJ_* when transitioning this target to generic translation
*/
+#define DISAS_JUMP_NEXT DISAS_TARGET + 0
#if defined(CONFIG_USER_ONLY)
#define IS_USER(s) 1
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 468531a..69ca717 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -74,7 +74,8 @@ typedef struct {
} u;
} DisasCompare;
-#define DISAS_EXCP 4
+/* TODO: rename as DJ_* when transitioning this target to generic translation
*/
+#define DISAS_EXCP DISAS_TARGET + 0
#ifdef DEBUG_INLINE_BRANCHES
static uint64_t inline_branch_hit[CC_OP_MAX];
diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
index 7499748..1cd2dab 100644
--- a/target-unicore32/translate.c
+++ b/target-unicore32/translate.c
@@ -45,9 +45,10 @@ typedef struct DisasContext {
#define IS_USER(s) 1
#endif
+/* TODO: rename as DJ_* when transitioning this target to generic translation
*/
/* These instructions trap after executing, so defer them until after the
conditional executions state has been updated. */
-#define DISAS_SYSCALL 5
+#define DISAS_SYSCALL DISAS_TARGET + 0
static TCGv_env cpu_env;
static TCGv_i32 cpu_R[32];
[Qemu-devel] [PATCH v2 1/6] Pass generic CPUState to gen_intermediate_code()
Needed to implement a target-agnostic gen_intermediate_code() in the
future.
Signed-off-by: Lluís Vilanova
Reviewed-by: David Gibson
---
include/exec/exec-all.h |2 +-
target-alpha/translate.c | 11 +--
target-arm/translate.c| 24
target-cris/translate.c | 17 -
target-i386/translate.c | 13 ++---
target-lm32/translate.c | 22 +++---
target-m68k/translate.c | 15 +++
target-microblaze/translate.c | 24
target-mips/translate.c | 15 +++
target-moxie/translate.c | 14 +++---
target-openrisc/translate.c | 24
target-ppc/translate.c| 15 +++
target-s390x/translate.c | 13 ++---
target-sh4/translate.c| 15 +++
target-sparc/translate.c | 11 +--
target-tilegx/translate.c |7 +++
target-tricore/translate.c|9 -
target-unicore32/translate.c | 17 -
target-xtensa/translate.c | 13 ++---
translate-all.c |2 +-
20 files changed, 135 insertions(+), 148 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d008296..17659e6 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -43,7 +43,7 @@ typedef ram_addr_t tb_page_addr_t;
#include "qemu/log.h"
-void gen_intermediate_code(CPUArchState *env, struct TranslationBlock *tb);
+void gen_intermediate_code(CPUState *env, struct TranslationBlock *tb);
void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
target_ulong *data);
diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 0ea0e6e..5f77467 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -2869,10 +2869,9 @@ static ExitStatus translate_one(DisasContext *ctx,
uint32_t insn)
return ret;
}
-void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb)
+void gen_intermediate_code(CPUState *cpu, struct TranslationBlock *tb)
{
-AlphaCPU *cpu = alpha_env_get_cpu(env);
-CPUState *cs = CPU(cpu);
+CPUAlphaState *env = cpu->env_ptr;
DisasContext ctx, *ctxp = &ctx;
target_ulong pc_start;
target_ulong pc_mask;
@@ -2887,7 +2886,7 @@ void gen_intermediate_code(CPUAlphaState *env, struct
TranslationBlock *tb)
ctx.pc = pc_start;
ctx.mem_idx = cpu_mmu_index(env, false);
ctx.implver = env->implver;
-ctx.singlestep_enabled = cs->singlestep_enabled;
+ctx.singlestep_enabled = cpu->singlestep_enabled;
#ifdef CONFIG_USER_ONLY
ctx.ir = cpu_std_ir;
@@ -2926,7 +2925,7 @@ void gen_intermediate_code(CPUAlphaState *env, struct
TranslationBlock *tb)
tcg_gen_insn_start(ctx.pc);
num_insns++;
-if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
+if (unlikely(cpu_breakpoint_test(cpu, ctx.pc, BP_ANY))) {
ret = gen_excp(&ctx, EXCP_DEBUG, 0);
/* The address covered by the breakpoint must be included in
[tb->pc, tb->pc + tb->size) in order to for it to be
@@ -3001,7 +3000,7 @@ void gen_intermediate_code(CPUAlphaState *env, struct
TranslationBlock *tb)
if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
&& qemu_log_in_addr_range(pc_start)) {
qemu_log("IN: %s\n", lookup_symbol(pc_start));
-log_target_disas(cs, pc_start, ctx.pc - pc_start, 1);
+log_target_disas(cpu, pc_start, ctx.pc - pc_start, 1);
qemu_log("\n");
}
#endif
diff --git a/target-arm/translate.c b/target-arm/translate.c
index bd5d5cb..721a3d4 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11624,10 +11624,10 @@ static bool insn_crosses_page(CPUARMState *env,
DisasContext *s)
}
/* generate intermediate code for basic block 'tb'. */
-void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
+void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb)
{
-ARMCPU *cpu = arm_env_get_cpu(env);
-CPUState *cs = CPU(cpu);
+CPUARMState *env = cpu->env_ptr;
+ARMCPU *arm_cpu = arm_env_get_cpu(env);
DisasContext dc1, *dc = &dc1;
target_ulong pc_start;
target_ulong next_page_start;
@@ -11641,7 +11641,7 @@ void gen_intermediate_code(CPUARMState *env,
TranslationBlock *tb)
* the A32/T32 complexity to do with conditional execution/IT blocks/etc.
*/
if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) {
-gen_intermediate_code_a64(cpu, tb);
+gen_intermediate_code_a64(arm_cpu, tb);
return;
}
@@ -11651,7 +11651,7 @@ void gen_intermediate_code(CPUARMState *env,
TranslationBlock *tb)
dc->is_jmp = DISAS_NEXT;
dc->pc = pc_start;
-dc->singlestep_enabled = cs->singlestep_enabled;
+dc->singlestep_enabled = cpu->singlestep_enabled;
dc->condjmp = 0;
dc->aarch64
[Qemu-devel] [PATCH v2 0/2] virtio-pci: Improve device plugging whith legacy backends
This series makes device plugging more robust, to avoid guest to be confused when the backend doesn't support VIRTIO_F_VERSION_1. The problem is seen with Linux guests running mainline kernels, when backend doesn't support the feature: virtio_net virtio0: virtio: device uses modern interface but does not have VIRTIO_F_VERSION_1. When it happens, the modern device probe returns -EINVAL, whereas its caller expects -ENODEV being returned to switch to legacy device probing. We need to make QEMU more robust to ensure the guest won't be confused, so this series exposes modern interface only when backend support it. It has been tested with vhost-net and vhost-user backends in client and server modes. Changes since v1: - - Make the backend feature check function specialized to only VIRTIO_1 Maxime Coquelin (2): virtio: Add function to check whether backend supports VIRTIO_1 virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1 hw/virtio/virtio-pci.c | 15 +++ hw/virtio/virtio-pci.h | 5 + hw/virtio/virtio.c | 13 + include/hw/virtio/virtio.h | 1 + 4 files changed, 34 insertions(+) -- 2.7.4
[Qemu-devel] [PATCH v2 1/2] virtio: Add function to check whether backend supports VIRTIO_1
This patch adds virtio_test_backend_virtio_1() function to
check whether backend supports VIRTIO_F_VERSION_1 before the
negociation takes place.
Cc: Cornelia Huck
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio.c | 13 +
include/hw/virtio/virtio.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..8b30b69 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1481,6 +1481,19 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque,
size_t size)
virtio_save(VIRTIO_DEVICE(opaque), f);
}
+bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp)
+{
+VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+uint64_t feature;
+
+virtio_add_feature(&feature, VIRTIO_F_VERSION_1);
+
+assert(k->get_features != NULL);
+feature = k->get_features(vdev, feature, errp);
+
+return virtio_has_feature(feature, VIRTIO_F_VERSION_1);
+}
+
static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d2490c1..3a31754 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -235,6 +235,7 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_update_irq(VirtIODevice *vdev);
int virtio_set_features(VirtIODevice *vdev, uint64_t val);
+bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp);
/* Base devices. */
typedef struct VirtIOBlkConf VirtIOBlkConf;
--
2.7.4
[Qemu-devel] [PATCH v2 6/6] target: [tcg, arm] Port to generic translation framework
Signed-off-by: Lluís Vilanova
---
target-arm/translate-a64.c | 342 ++---
target-arm/translate.c | 718 ++--
target-arm/translate.h | 42 ++-
3 files changed, 553 insertions(+), 549 deletions(-)
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index f5e29d2..6ce4dff 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -225,17 +225,17 @@ static void gen_exception(int excp, uint32_t syndrome,
uint32_t target_el)
static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
{
-gen_a64_set_pc_im(s->pc - offset);
+gen_a64_set_pc_im(s->base.pc_next - offset);
gen_exception_internal(excp);
-s->is_jmp = DISAS_EXC;
+s->base.jmp_type = DJ_EXC;
}
static void gen_exception_insn(DisasContext *s, int offset, int excp,
uint32_t syndrome, uint32_t target_el)
{
-gen_a64_set_pc_im(s->pc - offset);
+gen_a64_set_pc_im(s->base.pc_next - offset);
gen_exception(excp, syndrome, target_el);
-s->is_jmp = DISAS_EXC;
+s->base.jmp_type = DJ_EXC;
}
static void gen_ss_advance(DisasContext *s)
@@ -263,7 +263,7 @@ static void gen_step_complete_exception(DisasContext *s)
gen_ss_advance(s);
gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
default_exception_el(s));
-s->is_jmp = DISAS_EXC;
+s->base.jmp_type = DJ_EXC;
}
static inline bool use_goto_tb(DisasContext *s, int n, uint64_t dest)
@@ -271,13 +271,13 @@ static inline bool use_goto_tb(DisasContext *s, int n,
uint64_t dest)
/* No direct tb linking with singlestep (either QEMU's or the ARM
* debug architecture kind) or deterministic io
*/
-if (s->singlestep_enabled || s->ss_active || (s->tb->cflags & CF_LAST_IO))
{
+if (s->base.singlestep_enabled || s->ss_active || (s->base.tb->cflags &
CF_LAST_IO)) {
return false;
}
#ifndef CONFIG_USER_ONLY
/* Only link tbs from inside the same guest page */
-if ((s->tb->pc & TARGET_PAGE_MASK) != (dest & TARGET_PAGE_MASK)) {
+if ((s->base.tb->pc & TARGET_PAGE_MASK) != (dest & TARGET_PAGE_MASK)) {
return false;
}
#endif
@@ -289,21 +289,21 @@ static inline void gen_goto_tb(DisasContext *s, int n,
uint64_t dest)
{
TranslationBlock *tb;
-tb = s->tb;
+tb = s->base.tb;
if (use_goto_tb(s, n, dest)) {
tcg_gen_goto_tb(n);
gen_a64_set_pc_im(dest);
tcg_gen_exit_tb((intptr_t)tb + n);
-s->is_jmp = DISAS_TB_JUMP;
+s->base.jmp_type = DJ_TB_JUMP;
} else {
gen_a64_set_pc_im(dest);
if (s->ss_active) {
gen_step_complete_exception(s);
-} else if (s->singlestep_enabled) {
+} else if (s->base.singlestep_enabled) {
gen_exception_internal(EXCP_DEBUG);
} else {
tcg_gen_exit_tb(0);
-s->is_jmp = DISAS_TB_JUMP;
+s->base.jmp_type = DJ_TB_JUMP;
}
}
}
@@ -334,11 +334,11 @@ static void unallocated_encoding(DisasContext *s)
qemu_log_mask(LOG_UNIMP, \
"%s:%d: unsupported instruction encoding 0x%08x " \
"at pc=%016" PRIx64 "\n", \
- __FILE__, __LINE__, insn, s->pc - 4); \
+ __FILE__, __LINE__, insn, s->base.pc_next - 4);
\
unallocated_encoding(s); \
} while (0);
-static void init_tmp_a64_array(DisasContext *s)
+void init_tmp_a64_array(DisasContext *s)
{
#ifdef CONFIG_DEBUG_TCG
int i;
@@ -1152,11 +1152,11 @@ static inline AArch64DecodeFn *lookup_disas_fn(const
AArch64DecodeTable *table,
*/
static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
{
-uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
+uint64_t addr = s->base.pc_next + sextract32(insn, 0, 26) * 4 - 4;
if (insn & (1U << 31)) {
/* C5.6.26 BL Branch with link */
-tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
+tcg_gen_movi_i64(cpu_reg(s, 30), s->base.pc_next);
}
/* C5.6.20 B Branch / C5.6.26 BL Branch with link */
@@ -1179,7 +1179,7 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t
insn)
sf = extract32(insn, 31, 1);
op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
rt = extract32(insn, 0, 5);
-addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
+addr = s->base.pc_next + sextract32(insn, 5, 19) * 4 - 4;
tcg_cmp = read_cpu_reg(s, rt, sf);
label_match = gen_new_label();
@@ -1187,7 +1187,7 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t
insn)
tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
tcg_cmp, 0, label_match);
-gen_goto_tb(s, 0, s->pc);
+gen_goto_tb(s, 0, s->base.pc_next);
gen_set_label(l
[Qemu-devel] [PATCH v2 2/2] virtio-pci: Disable modern interface if backend without VIRTIO_F_VERSION_1
This patch makes pci devices plugging more robust, by not confusing
guest with modern interface when the backend doesn't support
VIRTIO_F_VERSION_1.
Cc: Cornelia Huck
Cc: Marcel Apfelbaum
Cc: Michael S. Tsirkin
Cc: qemu-sta...@nongnu.org
Signed-off-by: Maxime Coquelin
---
hw/virtio/virtio-pci.c | 15 +++
hw/virtio/virtio-pci.h | 5 +
2 files changed, 20 insertions(+)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 755f921..9e88d7b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d,
Error **errp)
uint32_t size;
VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+/*
+ * Virtio capabilities present without
+ * VIRTIO_F_VERSION_1 confuses guests
+ */
+if (!virtio_test_backend_virtio_1(vdev, errp)) {
+virtio_pci_disable_modern(proxy);
+}
+
+legacy = virtio_pci_legacy(proxy);
+modern = virtio_pci_modern(proxy);
+if (!legacy && !modern) {
+error_setg(errp, "PCI device is neither legacy nor modern.");
+return;
+}
+
config = proxy->pci_dev.config;
if (proxy->class_code) {
pci_config_set_class(config, proxy->class_code);
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 25fbf8a..4e976b3 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -172,6 +172,11 @@ static inline void
virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
proxy->disable_legacy = ON_OFF_AUTO_ON;
}
+static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
+{
+proxy->disable_modern = true;
+}
+
/*
* virtio-scsi-pci: This extends VirtioPCIProxy.
*/
--
2.7.4
Re: [Qemu-devel] [PATCH v3 13/34] tcg: Add atomic helpers
> +#define GEN_ATOMIC_HELPER(NAME, OP, NEW)\
> +static void * const table_##NAME[16] = {\
> +[MO_8] = gen_helper_atomic_##NAME##b, \
> +[MO_16 | MO_LE] = gen_helper_atomic_##NAME##w_le, \
> +[MO_16 | MO_BE] = gen_helper_atomic_##NAME##w_be, \
> +[MO_32 | MO_LE] = gen_helper_atomic_##NAME##l_le, \
> +[MO_32 | MO_BE] = gen_helper_atomic_##NAME##l_be, \
> +[MO_64 | MO_LE] = gen_helper_atomic_##NAME##q_le, \
> +[MO_64 | MO_BE] = gen_helper_atomic_##NAME##q_be, \
> +}; \
> +void tcg_gen_atomic_##NAME##_i32\
> +(TCGv_i32 ret, TCGv addr, TCGv_i32 val, TCGArg idx, TCGMemOp memop) \
> +{ \
> +if (parallel_cpus) {\
> +do_atomic_op_i32(ret, addr, val, idx, memop, table_##NAME); \
> +} else {\
> +do_nonatomic_op_i32(ret, addr, val, idx, memop, NEW,\
> +tcg_gen_##OP##_i32);\
> +} \
> +} \
> +void tcg_gen_atomic_##NAME##_i64\
> +(TCGv_i64 ret, TCGv addr, TCGv_i64 val, TCGArg idx, TCGMemOp memop) \
> +{ \
> +if (parallel_cpus) {\
> +do_atomic_op_i64(ret, addr, val, idx, memop, table_##NAME); \
> +} else {\
> +do_nonatomic_op_i64(ret, addr, val, idx, memop, NEW,\
> +tcg_gen_##OP##_i64);\
> +} \
> +}
> +
> +GEN_ATOMIC_HELPER(fetch_add, add, 0)
> +GEN_ATOMIC_HELPER(fetch_and, and, 0)
> +GEN_ATOMIC_HELPER(fetch_or, or, 0)
> +GEN_ATOMIC_HELPER(fetch_xor, xor, 0)
> +
> +GEN_ATOMIC_HELPER(add_fetch, add, 1)
> +GEN_ATOMIC_HELPER(and_fetch, and, 1)
> +GEN_ATOMIC_HELPER(or_fetch, or, 1)
> +GEN_ATOMIC_HELPER(xor_fetch, xor, 1)
This causes build errors on CentOS6.5 (where __ATOMIC_RELAXED is undefined):
/user/lea/dev/qemu/tcg/tcg-op.c:2280: error: ‘gen_helper_atomic_fetch_addb’
undeclared here (not in a function)
/user/lea/dev/qemu/tcg/tcg-op.c:2280: error: ‘gen_helper_atomic_fetch_addw_le’
undeclared here (not in a function)
/user/lea/dev/qemu/tcg/tcg-op.c:2280: error: ‘gen_helper_atomic_fetch_addw_be’
undeclared here (not in a function)
(...)
The fix I've been using locally while working on enabling MTTCG for MIPS:
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 5542416..818a5a4 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -409,22 +409,22 @@
/* Provide shorter names for GCC atomic builtins. */
#define atomic_fetch_inc(ptr) __sync_fetch_and_add(ptr, 1)
#define atomic_fetch_dec(ptr) __sync_fetch_and_add(ptr, -1)
-#define atomic_fetch_add __sync_fetch_and_add
-#define atomic_fetch_sub __sync_fetch_and_sub
-#define atomic_fetch_and __sync_fetch_and_and
-#define atomic_fetch_or__sync_fetch_and_or
-#define atomic_fetch_xor __sync_fetch_and_xor
+#define atomic_fetch_add(ptr, n) __sync_fetch_and_add(ptr, n)
+#define atomic_fetch_sub(ptr, n) __sync_fetch_and_sub(ptr, n)
+#define atomic_fetch_and(ptr, n) __sync_fetch_and_and(ptr, n)
+#define atomic_fetch_or(ptr, n) __sync_fetch_and_or(ptr, n)
+#define atomic_fetch_xor(ptr, n) __sync_fetch_and_xor(ptr, n)
#define atomic_inc_fetch(ptr) __sync_add_and_fetch(ptr, 1)
#define atomic_dec_fetch(ptr) __sync_add_and_fetch(ptr, -1)
-#define atomic_add_fetch __sync_add_and_fetch
-#define atomic_sub_fetch __sync_sub_and_fetch
-#define atomic_and_fetch __sync_and_and_fetch
-#define atomic_or_fetch__sync_or_and_fetch
-#define atomic_xor_fetch __sync_xor_and_fetch
-
-#define atomic_cmpxchg __sync_val_compare_and_swap
-#define atomic_cmpxchg__nocheck atomic_cmpxchg
+#define atomic_add_fetch(ptr, n) __sync_add_and_fetch(ptr, n)
+#define atomic_sub_fetch(ptr, n) __sync_sub_and_fetch(ptr, n)
+#define atomic_and_fetch(ptr, n) __sync_and_and_fetch(ptr, n)
+#define atomic_or_fetch(ptr, n) __sync_or_and_fetch(ptr, n)
+#define atomic_xor_fetch(ptr, n) __sync_xor_and_fetch(ptr, n)
+
+#define atomic_cmpxchg(ptr, old, new) __sync_val_compare_and_swap(ptr, old,
new)
+#define atomic_cmpxchg__nocheck(ptr, old, new) atomic_cmpxchg(ptr, old, new)
/* And even shorter n
Re: [Qemu-devel] [PATCH for-2.8 v1 00/60] Modular build of trace event files
Daniel P Berrange writes:
> On Fri, Sep 09, 2016 at 01:03:51PM +0200, Lluís Vilanova wrote:
>> Daniel P Berrange writes:
>>
>> > On Thu, Sep 08, 2016 at 03:23:26PM +0200, Lluís Vilanova wrote:
>> >> Daniel P Berrange writes:
>> >>
>> >> > I previously split the global trace-events file up into one file
>> >> > per-subdirectory to avoid merge conflict hell.
>> >> [...]
>> >>
>> >> Sorry, I could not find the message where the infrastructure is modified
>> >> to
>> >> provide this. But I think there's a more efficient way to provide modular
>> >> auto-generated tracing code without the hierarchical indexing you
>> >> proposed.
>>
>> > [snip]
>>
>> >> struct TraceEvent ___trace_events[] = {
>> >> {
>> >> .name = "eventname",
>> >> .sstate = 1,
>> >> .dstate = ___trace_eventname_dstate;
>> >> }
>> >> }
>> >>
>> >> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>>
>> > Life would be simpler if we had the 'bool dstate' as part of the
>> > TraceEvent struct, but doing so would essentially be reverting this
>> > previous change:
>>
>> > commit 585ec7273e6fdab902b2128bc6c2a8136aafef04
>> > Author: Paolo Bonzini
>> > Date: Wed Oct 28 07:06:27 2015 +0100
>>
>> > trace: track enabled events in a separate array
>>
>> > This is more cache friendly on the fast path, where we already have
>> > the event id available.
>>
>> > I asked Paolo about this previously and he indicated it was a notable
>> > performance improvement, so we can't put dstate back into the TraceEvent
>> > struct :-(
>>
>> Sorry, there was a typo in my example code that led to this misunderstanding.
>>
>> I meant this for the global auto-generated .c:
>>
>> struct TraceEvent {
>> /* ... */
>> bool *dstate;
>> };
>>
>> bool ___TRACE_EVENTNAME_DSTATE;
>>
>> struct TraceEvent ___trace_events[] = {
>> {
>> .name = "eventname",
>> .sstate = 1,
>> .dstate = &___TRACE_EVENTNAME_DSTATE;
>> }
>> }
>>
>> TraceEvent *TRACE_EVENTNAME = &___trace_events[...];
>>
>>
>> If you look at the modified macro I pasted for "trace/control-internal.h":
>>
>>
>> #define trace_event_get_state_dynamic_by_id(id) \
>> (unlikely(trace_events_enabled_count) && \
>> (___ ## id ## _DSTATE))
>>
>> We're retaining the fast-path performance of the seggregated boolean dstate
>> array, while the event descriptor contains a pointer to the per-event dstate
>> global boolean variable in case you want to get/set the dstate based on an
>> event
>> pointer.
> The various _DSTATE variables are still arbitrarily scattered in
> memory, as opposed to in a contiguous cache friendly array, which
> is one of the goals of Paolo's original change.
> That said, I'm unclear on how much of the performance win from
> Paolo's change came from eliminating the struct field de-reference,
> vs having the contiguous array. I'm guessing most of the win is
> from the former, the latter only being important if we hit multiple
> related tracepoints on close succession.
The latter can also be achieved by packing them all together in a single
section, but I don't know if that's acceptable within portable QEMU code. For
example:
bool ___TRACE_EVENTNAME_DSTATE __attribute__((section("dstate_array")))
Cheers,
Lluis
[Qemu-devel] [PATCH v2 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag
Signed-off-by: Nikunj A Dadhania
---
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 4 ++--
target-ppc/mmu-hash64.c | 4 ++--
target-ppc/mmu_helper.c | 6 +++---
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 1e808c8..7dc 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1009,6 +1009,7 @@ struct CPUPPCState {
bool tlb_dirty; /* Set to non-zero when modifying TLB */
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active*/
uint32_t tlb_need_flush; /* Delayed flush needed */
+#define TLB_NEED_LOCAL_FLUSH 0x1
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 3d279f1..69204a5 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -157,9 +157,9 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
static inline void check_tlb_flush(CPUPPCState *env)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
-if (env->tlb_need_flush) {
-env->tlb_need_flush = 0;
+if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
tlb_flush(cs, 1);
+env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
}
}
#else
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 8118143..1f52b64 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -110,7 +110,7 @@ void helper_slbia(CPUPPCState *env)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
}
}
@@ -132,7 +132,7 @@ void helper_slbie(CPUPPCState *env, target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
}
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 696bb03..d59d2f8 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
target_ulong addr)
* we just mark the TLB to be flushed later (context synchronizing
* event or sync instruction on 32-bit).
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
break;
#if defined(TARGET_PPC64)
case POWERPC_MMU_64B:
@@ -1979,7 +1979,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask) in QEMU,
* we just invalidate all TLBs
*/
-env->tlb_need_flush = 1;
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
break;
#endif /* defined(TARGET_PPC64) */
default:
@@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong
srnum, target_ulong value)
}
}
#else
-env->tlb_need_flush = 1;
+env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
#endif
}
}
--
2.7.4
[Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect
tlbie (H_REMOVE, H_PROTECT and H_BULK_REMOVE for pseries) should have a
global effect.
Introduces TLB_NEED_GLOBAL_FLUSH flag. During delayed flush, once taking
care of local flush, check broadcast flush(ptesync, tlbsync, etc) is
needed. Depending on the bitmask state of the tlb_need_flush, tlb is
flushed from other cpus if needed and the flags are cleared.
Suggested-by: Benjamin Herrenschmidt
Signed-off-by: Nikunj A Dadhania
---
hw/ppc/spapr_hcall.c | 2 ++
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 19 +++
target-ppc/mmu-hash64.c | 2 +-
target-ppc/mmu_helper.c | 10 +++---
target-ppc/translate.c | 6 ++
6 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ef12ea0..6144e17 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -319,6 +319,8 @@ static target_ulong h_protect(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
ppc_hash64_store_hpte(cpu, pte_index,
(v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
+/* Flush the tlb */
+check_tlb_flush(env, 1);
/* Don't need a memory barrier, due to qemu's global lock */
ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
return H_SUCCESS;
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 7dc..50fe0f5 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1010,6 +1010,7 @@ struct CPUPPCState {
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active*/
uint32_t tlb_need_flush; /* Delayed flush needed */
#define TLB_NEED_LOCAL_FLUSH 0x1
+#define TLB_NEED_GLOBAL_FLUSH 0x2
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index bcf65ce..24872ca 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,6 +154,14 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
+static inline void tlb_clear_flag(CPUState *cs)
+{
+PowerPCCPU *cpu = POWERPC_CPU(cs);
+CPUPPCState *env = &cpu->env;
+
+env->tlb_need_flush = 0;
+}
+
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
@@ -161,6 +169,17 @@ static inline void check_tlb_flush(CPUPPCState *env,
uint32_t global)
tlb_flush(cs, 1);
env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
}
+
+if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
+CPUState *other_cs;
+CPU_FOREACH(other_cs) {
+if (other_cs != cs) {
+tlb_clear_flag(other_cs);
+tlb_flush(other_cs, 1);
+}
+}
+env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
+}
}
#else
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 1f52b64..fdb7a78 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -912,7 +912,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
* invalidate, and we still don't have a tlb_flush_mask(env, n,
* mask) in QEMU, we just invalidate all TLBs
*/
-tlb_flush(CPU(cpu), 1);
+cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
}
void ppc_hash64_update_rmls(CPUPPCState *env)
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index bf9f329..1dd057a 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2757,7 +2757,7 @@ static inline void booke206_invalidate_ea_tlb(CPUPPCState
*env, int tlbn,
void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
{
-PowerPCCPU *cpu = ppc_env_get_cpu(env);
+CPUState *cs;
if (address & 0x4) {
/* flush all entries */
@@ -2774,11 +2774,15 @@ void helper_booke206_tlbivax(CPUPPCState *env,
target_ulong address)
if (address & 0x8) {
/* flush TLB1 entries */
booke206_invalidate_ea_tlb(env, 1, address);
-tlb_flush(CPU(cpu), 1);
+CPU_FOREACH(cs) {
+tlb_flush(cs, 1);
+}
} else {
/* flush TLB0 entries */
booke206_invalidate_ea_tlb(env, 0, address);
-tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
+CPU_FOREACH(cs) {
+tlb_flush_page(cs, address & MAS2_EPN_MASK);
+}
}
}
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 77bb312..2aae43b 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4443,6 +4443,7 @@ static void gen_tlbie(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
GEN_PRIV;
#else
+TCGv_i32 t1;
CHK_HV;
if (NARROW_MODE(ctx)) {
@@ -4453,6 +4454,11 @@ static void gen_tlbie(DisasContext *ctx)
} else {
gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
}
+t1 = tcg_temp_new_i32();
+tcg_gen_ld_i32
[Qemu-devel] [PATCH v2 2/3] target-ppc: add flag in chech_tlb_flush()
The flag will be used to indicate whether broadcast tlb flush is needed
or not.
Moreover, BookS does both ptesync and tlbsync, so make that a nop for
the server and tlbsync would generate a check flush for BookE
Signed-off-by: Nikunj A Dadhania
---
hw/ppc/spapr_hcall.c | 4 ++--
target-ppc/excp_helper.c | 4 ++--
target-ppc/helper.h | 2 +-
target-ppc/helper_regs.h | 4 ++--
target-ppc/mmu_helper.c | 4 ++--
target-ppc/translate.c | 20 ++--
6 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 73af112..ef12ea0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
switch (ret) {
case REMOVE_SUCCESS:
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
return H_SUCCESS;
case REMOVE_NOT_FOUND:
@@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu,
sPAPRMachineState *spapr,
}
}
exit:
-check_tlb_flush(env);
+check_tlb_flush(env, 1);
return rc;
}
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 04ed4da..3b78126 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
excp_model, int excp)
/* Any interrupt is context synchronizing, check if TCG TLB
* needs a delayed flush on ppc64
*/
-check_tlb_flush(env);
+check_tlb_flush(env, 0);
}
void ppc_cpu_do_interrupt(CPUState *cs)
@@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env, target_ulong
nip, target_ulong msr)
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
/* Context synchronizing: check if TCG TLB needs flush */
-check_tlb_flush(env);
+check_tlb_flush(env, 0);
}
void helper_rfi(CPUPPCState *env)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index dcf3f95..a86e184 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
DEF_HELPER_1(hrfid, void, env)
DEF_HELPER_2(store_lpcr, void, env, tl)
#endif
-DEF_HELPER_1(check_tlb_flush, void, env)
+DEF_HELPER_2(check_tlb_flush, void, env, i32)
#endif
DEF_HELPER_3(lmw, void, env, tl, i32)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 69204a5..bcf65ce 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
-static inline void check_tlb_flush(CPUPPCState *env)
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
@@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState *env)
}
}
#else
-static inline void check_tlb_flush(CPUPPCState *env) { }
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
#endif
#endif /* HELPER_REGS_H */
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index d59d2f8..bf9f329 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState *env,
target_ulong type)
}
-void helper_check_tlb_flush(CPUPPCState *env)
+void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
{
-check_tlb_flush(env);
+check_tlb_flush(env, global);
}
/*/
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 618334a..77bb312 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3064,7 +3064,7 @@ static void gen_eieio(DisasContext *ctx)
}
#if !defined(CONFIG_USER_ONLY)
-static inline void gen_check_tlb_flush(DisasContext *ctx)
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global)
{
TCGv_i32 t;
TCGLabel *l;
@@ -3076,12 +3076,13 @@ static inline void gen_check_tlb_flush(DisasContext
*ctx)
t = tcg_temp_new_i32();
tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
-gen_helper_check_tlb_flush(cpu_env);
+tcg_gen_movi_i32(t, global);
+gen_helper_check_tlb_flush(cpu_env, t);
gen_set_label(l);
tcg_temp_free_i32(t);
}
#else
-static inline void gen_check_tlb_flush(DisasContext *ctx) { }
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global) { }
#endif
/* isync */
@@ -3092,7 +3093,7 @@ static void gen_isync(DisasContext *ctx)
* kernel mode however so check MSR_PR
*/
if (!ctx->pr) {
-gen_check_tlb_flush(ctx);
+gen_check_tlb_flush(ctx, 0);
}
gen_stop_exception(ctx);
}
@@ -3257,7 +3258,7 @@ static void gen_sync(DisasContext *ctx)
* check MSR_PR as well.
*/
if (((l == 2) || !(c
Re: [Qemu-devel] [PATCH v2 4/6] hypertrace: [softmmu] Add QEMU-side proxy to "guest_hypertrace" event
Michael S Tsirkin writes: > On Mon, Sep 05, 2016 at 08:56:44PM +0200, Lluís Vilanova wrote: >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index 929ec2f..8973f57 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -80,6 +80,8 @@ >> #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005 >> #define PCI_DEVICE_ID_VIRTIO_9P 0x1009 >> >> +#define PCI_DEVICE_ID_HYPERTRACE 0x10f0 >> + >> #define PCI_VENDOR_ID_REDHAT 0x1b36 >> #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001 >> #define PCI_DEVICE_ID_REDHAT_SERIAL 0x0002 > There's no need to add IDs to this header. > We should probably drop the ones we currently have in pci.h, too. You mean these should simply be a define inside each device's code? Thanks, Lluis
Re: [Qemu-devel] [PATCH v4 06/17] monitor: unregister conditional commands
Marc-André Lureau writes:
> Hi
>
> - Original Message -
>> marcandre.lur...@redhat.com writes:
>>
>> > From: Marc-André Lureau
>> >
>> > The current monitor dispatch codes doesn't know commands that have been
>> > filtered out during qmp-commands.hx preprocessing. query-commands
>> > doesn't list them either. However, qapi generator doesn't filter them
>> > out, and they are listed in the command list.
>> >
>> > For now, disable the commands that aren't avaible to avoid introducing a
>> > regression there when switching to qmp_dispatch() or listing commands
>> > from the generated qapi code.
>> >
>> > Signed-off-by: Marc-André Lureau
>> > ---
>> > monitor.c | 23 +++
>> > 1 file changed, 23 insertions(+)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index b7ae552..ef946ad 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -1008,6 +1008,26 @@ static void qmp_query_qmp_schema(QDict *qdict,
>> > QObject **ret_data,
>> > *ret_data = qobject_from_json(qmp_schema_json);
>> > }
>> >
>> > +/*
>> > + * Those commands are registered unconditionnally by generated
>> > + * qmp files. FIXME: Educate the QAPI schema on #ifdef commands.
>> > + */
>> > +static void qmp_disable_marshal(void)
>> > +{
>> > +#ifndef CONFIG_SPICE
>> > +qmp_disable_command("query-spice");
>> > +#endif
>> > +#ifndef TARGET_I386
>> > +qmp_disable_command("rtc-reset-reinjection");
>> > +#endif
>> > +#ifndef TARGET_S390X
>> > +qmp_disable_command("dump-skeys");
>> > +#endif
>> > +#ifndef TARGET_ARM
>> > +qmp_disable_command("query-gic-capabilities");
>> > +#endif
>> > +}
>> > +
>> > static void qmp_init_marshal(void)
>> > {
>> > qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
>> > @@ -1016,6 +1036,9 @@ static void qmp_init_marshal(void)
>> > QCO_NO_OPTIONS);
>> > qmp_register_command("netdev_add", qmp_netdev_add,
>> > QCO_NO_OPTIONS);
>> > +
>> > +/* call it after the rest of qapi_init() */
>> > +register_module_init(qmp_disable_marshal, MODULE_INIT_QAPI);
>> > }
>> >
>> > qapi_init(qmp_init_marshal);
>>
>> Let's see whether I understand this patch...
>>
>> qmp_disable_command() sets a flag that can be tested directly or with
>> qmp_command_is_enabled(). do_qmp_dispatch() tests it, and fails the
>> command with an "The command FOO has been disabled for this instance"
>> error. It's called from qmp_dispatch(), which we're not yet using for
>> QMP at this point of the series.
>>
>> QGA uses this to implement its "freeze whitelist", i.e. to disable all
>> commands that aren't known to be safe while filesystems are frozen.
>>
>> qmp_disable_command() does nothing when the command doesn't exist, which
>> I think is the case at this point of the series.
>>
>> So this patch does exactly nothing by itself. When you later flip
>> dispatch to use qmp_dispatch(), it becomes active, and the error for
>> these commands changes from
>>
>> {"error": {"class": "CommandNotFound", "desc": "The command FOO has not
>> been found"}}
>>
>> to
>>
>> {"error": {"class": "GenericError", "desc": "The command FOO has been
>> disabled for this instance"}}
>>
>> which is fine with me.
Unfortunately, it looks like it's not fine with libvirt.
qemuMonitorJSONGetGICCapabilities() in src/qemu/qemu_monitor_json.c:
int ret = -1;
[...]
/* If the 'query-gic-capabilities' QMP command was not available
* we simply successfully return zero capabilities.
* This is the case for QEMU <2.6 and all non-ARM architectures */
if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
ret = 0;
goto cleanup;
}
if (qemuMonitorJSONCheckError(cmd, reply) < 0)
goto cleanup;
Ideally, libvirt would use query-commands instead of ErrorClass
CommandNotFound. But we need to play the hand we've been dealt. That
means preserving ErrorClass CommandNotFound.
>> Is this basically correct?
>
> I think it's correct, I haven't played much with this as I focused on the
> conditional build instead, which is actually not so difficult, see:
[...]
Re: [Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp
On Fri, Sep 9, 2016 at 8:41 AM, Eric Blake wrote:
> On 09/08/2016 10:59 AM, Ashijeet Acharya wrote:
>> Mark old-commands for speed and downtime as deprecated.
>> Move max-bandwidth and downtime-limit into migrate-set-parameters for
>> setting maximum migration speed and expected downtime limit parameters
>> respectively.
>> Change downtime units to milliseconds (only for new-command) and update
>> the query part in both hmp and qmp qemu control interfaces.
>>
>> Signed-off-by: Ashijeet Acharya
>> ---
>
>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>> +{
>> +bool has_compress_level = false;
>> +bool has_compress_threads = false;
>> +bool has_decompress_threads = false;
>> +bool has_cpu_throttle_initial = false;
>> +bool has_cpu_throttle_increment = false;
>> +bool has_tls_creds = false;
>> +bool has_tls_hostname = false;
>> +bool has_max_bandwidth = true;
>> +bool has_downtime_limit = false;
>> +const char *valuestr = NULL;
>> +long valueint = 0;
>> +Error *err = NULL;
>> +
>> +qmp_migrate_set_parameters(has_compress_level, valueint,
>> + has_compress_threads, valueint,
>> + has_decompress_threads, valueint,
>> + has_cpu_throttle_initial, valueint,
>> + has_cpu_throttle_increment, valueint,
>> + has_tls_creds, valuestr,
>> + has_tls_hostname, valuestr,
>> + has_max_bandwidth, valuebw,
>> + has_downtime_limit, valueint,
>> + &err);
>> +
>> +}
>
> Oh, and I was so caught up on the grossness of this method that I failed
> to overlook that you are silently ignoring and leaking any errors.
> Since you aren't making any further decisions, you don't need the 'Error
> *err' declaration, and can just pass 'errp' instead of '&err' to
> qmp_migrate_set_parameters(). At any rate, I'm also posting my proposal
> to make this patch have a simpler back-compat method, by using qapi
> boxed parameters of my series.
>
Should I send v5 in reply to your series since it uses your new qapi
boxed parameters method or as an independent new thread?
Ashijeet
> --
> Eric Blake eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] DAX can not work on virtual nvdimm device
On Fri, Sep 09, 2016 at 11:19:25AM +0200, Jan Kara wrote: > > > > > 1. make the guest kernel based on your tree, the top commit is > > > > >10d7902fa0e82b (dax: unmap/truncate on device shutdown) and > > > > >the config file can be found in this thread. What git tree is 10d7902fa0e82b from? > Hum, nothing unusual in there. I've tried reproducing on a local SLE11 SP3 > machine (which is from about the same time) but everything works as > expected there. Shrug... I just tried reproducing it via "gce-xfstests --pmem-device shell" with a downgraded e2fsprogs to stock upstream 1.42.12, and I can't reproduce it with the ext4.git tree. I'm not sure whether the git commit is critical, though. Is this a regression that was working before? Or it's possible that Red Hat did something weird with the CentOS 6 e2fsprogs - Ted
Re: [Qemu-devel] [PATCH RFC v1 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag
Nikunj A Dadhania writes:
I think we need a little more detail here. In fact when you post the
next version of the series could you please include a cover letter to
cover what the series is trying to achieve?
> Signed-off-by: Nikunj A Dadhania
> ---
> target-ppc/cpu.h | 1 +
> target-ppc/helper_regs.h | 2 +-
> target-ppc/mmu-hash64.c | 4 ++--
> target-ppc/mmu_helper.c | 6 +++---
> 4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1e808c8..7dc 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1009,6 +1009,7 @@ struct CPUPPCState {
> bool tlb_dirty; /* Set to non-zero when modifying TLB
> */
> bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active
> */
> uint32_t tlb_need_flush; /* Delayed flush needed */
> +#define TLB_NEED_LOCAL_FLUSH 0x1
> #endif
>
> /* Other registers */
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 3d279f1..4457a30 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -157,7 +157,7 @@ static inline int hreg_store_msr(CPUPPCState *env,
> target_ulong value,
> static inline void check_tlb_flush(CPUPPCState *env)
> {
> CPUState *cs = CPU(ppc_env_get_cpu(env));
> -if (env->tlb_need_flush) {
> +if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) ==
> TLB_NEED_LOCAL_FLUSH) {
> env->tlb_need_flush = 0;
> tlb_flush(cs, 1);
> }
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 8118143..4c7ceef 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -110,7 +110,7 @@ void helper_slbia(CPUPPCState *env)
> * and we still don't have a tlb_flush_mask(env, n, mask)
> * in QEMU, we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
I'm not sure what we gain here versus just using a straight bool for the flag.
> }
> }
> }
> @@ -132,7 +132,7 @@ void helper_slbie(CPUPPCState *env, target_ulong addr)
> * and we still don't have a tlb_flush_mask(env, n, mask)
> * in QEMU, we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> }
> }
>
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 696bb03..249 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
> * we just mark the TLB to be flushed later (context synchronizing
> * event or sync instruction on 32-bit).
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> break;
> #if defined(TARGET_PPC64)
> case POWERPC_MMU_64B:
> @@ -1979,7 +1979,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
> * and we still don't have a tlb_flush_mask(env, n, mask) in
> QEMU,
> * we just invalidate all TLBs
> */
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> break;
> #endif /* defined(TARGET_PPC64) */
> default:
> @@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong
> srnum, target_ulong value)
> }
> }
> #else
> -env->tlb_need_flush = 1;
> +env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
> #endif
> }
> }
--
Alex Bennée
Re: [Qemu-devel] [Libguestfs] Extracting files from OVA is bad
> On 09 Sep 2016, at 14:02, Richard W.M. Jones wrote: > > On Fri, Sep 09, 2016 at 01:03:49PM +0200, Tomáš Golembiovský wrote: >> Hi, >> >> recently we (oVirt) have started discussing whether the way virt-v2v >> handles import from OVA files is good. And I would be interested in >> ideas how it can be improved. It is likely somebody already gave some >> thought to this problem. >> >> TL;DR: Extracting the OVA before import is a problem for large VMs (in >> sizes of TBs). Can we change something to prevent the extraction and >> work directly over OVA? > > Specifically virt-v2v needs to do: > > qemu-img create -b -f qcow2 overlay.qcow2 > qemu-img convert overlay.qcow2 output > >> What we consider a huge shortcoming is the fact that whole OVA is >> extracted prior to the import into a temporary directory and processed >> afterwards. Under normal situation user can have up to three copies of >> the VM on his drive at the end of import: >> >> * original OVA, >> * temporary extracted files (will be deleted when virt-v2v terminates, >> * converted VM. >> >> >> This is not a good idea for large VMs that have hunderds of GBs or even >> TBs in size. The requirements on the necessary storage space can be >> lessened with proper partitioning. I.e. source OVA and converted VM >> don't end up on the same drive and TMPDIR is set to put even temporary >> files somewhere else. But this is not a general solution. And sometimes >> the necessary space may not be available at all. >> >> >> The question is how to change the import path so that virt-v2v doesn't >> have to extract the OVA. I can see the following solutions: >> >> 1) Solve it virt-v2v: create a layer for directly accessing the files >>in the archive. >> >> 2) Solve it in QEMU: create backing method that would allow creating >>qemu disk backed by the archive. > > As long as the tar file is not compressed, accessing a file within it > should be trivial. The OVA standard [1] talks about compression. But it looks like it’s meant only for individual disks inside the archive. It doesn’t seem to be clear about it. The OVF xml is guaranteed to be at the beginning so it doesn’t need to read a lot until really reading it whole even if it would be compressed. Well, I guess it’s a reasonable to start with plain tar regardless. Thanks, michal [1] http://www.dmtf.org/sites/default/files/standards/documents/DSP0243_2.1.1.pdf > > I asked Kevin if there is a way to get qemu to access a disk image at > an offset within another file, but there is no such feature at the > moment. It's possible with `losetup', but that requires root :-( > > (At this point I would normally grumble about how easy this would be > with a microkernel, but I won't do that now.) > > David Gilbert suggested looking at qemu-nbd which has an --offset > option, allowing a particular offset with another file to be accessed. > If we wanted to do it entirely within virt-v2v, I think this would be > the way to go - the complex logic could be hidden inside v2v/input_ova.ml > > The second problem is to work out the right offset to use. I suspect > this is something that http://www.libarchive.org/ can do, and that > package is also in RHEL. > > We could even imagine a qemu block backend based on libarchive. > >> 3) Solve it on oVirt side: use some FUSE-based tool to provide >>access to the archive and pass the OVA to virt-v2v not as a file but >>as directory. > > http://www.cybernoia.de/software/archivemount/ is one such tool which > can do this. It's not in RHEL, but it seems to be based on > libarchive. > > Rich. > > -- > Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones > Read my programming and virtualization blog: http://rwmj.wordpress.com > libguestfs lets you edit virtual machines. Supports shell scripting, > bindings from many languages. http://libguestfs.org
