Re: [Qemu-devel] [PATCH] qcow2: allocate cluster_cache/cluster_data on demand
On 19/08/17 12:46, Alexey Kardashevskiy wrote: > On 19/08/17 01:18, Eric Blake wrote: >> On 08/18/2017 08:31 AM, Stefan Hajnoczi wrote: >>> Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1) >>> * cluster_size + 512 bytes upfront. Allocate s->cluster_cache and >>> s->cluster_data when the first read operation is performance on a >>> compressed cluster. >>> >>> The buffers are freed in .bdrv_close(). .bdrv_open() no longer has any >>> code paths that can allocate these buffers, so remove the free functions >>> in the error code path. >>> >>> Reported-by: Alexey Kardashevskiy >>> Cc: Kevin Wolf >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> Alexey: Does this improve your memory profiling results? >> >> Is this a regression from earlier versions? > > Hm, I have not thought about this. > > So. I did bisect and this started happening from > 9a4c0e220d8a4f82b5665d0ee95ef94d8e1509d5 > "hw/virtio-pci: fix virtio behaviour" > > Before that, the very same command line would take less than 1GB of > resident memory. That thing basically enforces virtio-1.0 for QEMU <=2.6 > which means that upstream with "-machine pseries-2.6" works fine (less than > 1GB), "-machine pseries-2.7" does not (close to 7GB, sometime even 9GB). > > Then I tried bisecting again, with > "scsi=off,disable-modern=off,disable-legacy=on" on my 150 virtio-block > devices, started from > e266d421490e0 "virtio-pci: add flags to enable/disable legacy/modern" (it > added the disable-modern switch) which uses 2GB of memory. > > I ended up with ada434cd0b44 "virtio-pci: implement cfg capability". > > Then I removed proxy->modern_as on v2.10.0-rc3 (see below) and got 1.5GB of > used memory (yay!) > > I do not really know how to reinterpret all of this, do you? > > > Note: 1GB..9GB numbers from below are the peak values from valgrind's s/from below/from above/ , sorry, bad cut-n-paste :) -- Alexey signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] qcow2: allocate cluster_cache/cluster_data on demand
On 19/08/17 01:18, Eric Blake wrote:
> On 08/18/2017 08:31 AM, Stefan Hajnoczi wrote:
>> Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1)
>> * cluster_size + 512 bytes upfront. Allocate s->cluster_cache and
>> s->cluster_data when the first read operation is performance on a
>> compressed cluster.
>>
>> The buffers are freed in .bdrv_close(). .bdrv_open() no longer has any
>> code paths that can allocate these buffers, so remove the free functions
>> in the error code path.
>>
>> Reported-by: Alexey Kardashevskiy
>> Cc: Kevin Wolf
>> Signed-off-by: Stefan Hajnoczi
>> ---
>> Alexey: Does this improve your memory profiling results?
>
> Is this a regression from earlier versions?
Hm, I have not thought about this.
So. I did bisect and this started happening from
9a4c0e220d8a4f82b5665d0ee95ef94d8e1509d5
"hw/virtio-pci: fix virtio behaviour"
Before that, the very same command line would take less than 1GB of
resident memory. That thing basically enforces virtio-1.0 for QEMU <=2.6
which means that upstream with "-machine pseries-2.6" works fine (less than
1GB), "-machine pseries-2.7" does not (close to 7GB, sometime even 9GB).
Then I tried bisecting again, with
"scsi=off,disable-modern=off,disable-legacy=on" on my 150 virtio-block
devices, started from
e266d421490e0 "virtio-pci: add flags to enable/disable legacy/modern" (it
added the disable-modern switch) which uses 2GB of memory.
I ended up with ada434cd0b44 "virtio-pci: implement cfg capability".
Then I removed proxy->modern_as on v2.10.0-rc3 (see below) and got 1.5GB of
used memory (yay!)
I do not really know how to reinterpret all of this, do you?
Note: 1GB..9GB numbers from below are the peak values from valgrind's
massif. This is pretty much resident memory used by QEMU process. In my
testing I did not enable KVM and I did not start the guest (i.e. used -S).
150 virtio-block devices, 2GB RAM for the guest.
Also, while bisecting, I only paid attention if it is 1..2GB or 6..9GB -
all tests did fit these 2 ranges, for any given sha1 the amount of memory
would be stable but among "good" commits it could change between 1GB and 2GB.
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 5d14bd6..7ad447a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1783,6 +1783,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev,
Error **errp)
/* PCI BAR regions must be powers of 2 */
pow2ceil(proxy->notify.offset + proxy->notify.size));
+#if 0
memory_region_init_alias(&proxy->modern_cfg,
OBJECT(proxy),
"virtio-pci-cfg",
@@ -1791,7 +1792,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev,
Error **errp)
memory_region_size(&proxy->modern_bar));
address_space_init(&proxy->modern_as, &proxy->modern_cfg,
"virtio-pci-cfg-as");
-
+#endif
if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
}
@@ -1860,10 +1861,10 @@ static void virtio_pci_realize(PCIDevice *pci_dev,
Error **errp)
static void virtio_pci_exit(PCIDevice *pci_dev)
{
-VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
+//VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
msix_uninit_exclusive_bar(pci_dev);
-address_space_destroy(&proxy->modern_as);
+//address_space_destroy(&proxy->modern_as);
}
static void virtio_pci_reset(DeviceState *qdev)
> Likely, it is NOT -rc4
> material, and thus can wait for 2.11; but it should be fine for -stable
> as part of 2.10.1 down the road.
>
>> +++ b/block/qcow2-cluster.c
>> @@ -1516,6 +1516,23 @@ int qcow2_decompress_cluster(BlockDriverState *bs,
>> uint64_t cluster_offset)
>> nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask)
>> + 1;
>> sector_offset = coffset & 511;
>> csize = nb_csectors * 512 - sector_offset;
>> +
>> +/* Allocate buffers on first decompress operation, most images are
>> + * uncompressed and the memory overhead can be avoided. The buffers
>> + * are freed in .bdrv_close().
>> + */
>> +if (!s->cluster_data) {
>> +/* one more sector for decompressed data alignment */
>> +s->cluster_data = qemu_try_blockalign(bs->file->bs,
>> +QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
>> +if (!s->cluster_data) {
>> +return -EIO;
>
> Is -ENOMEM any better than -EIO here?
>
--
Alexey
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v7 2/6] qmp: Create IOThrottle structure
On Aug 16, 2017 11:41 PM, "Markus Armbruster" wrote: Eric Blake writes: > On 08/16/2017 11:13 AM, Markus Armbruster wrote: >> Markus Armbruster writes: >> > >>> >>> Conclusion: no consensus, yet. >> >> All right, let's start over and try to resolve the impasse and/or >> misunderstanding. >> >> Type BlockIOThrottle lives in qapi/block-core.json, and is used by QMP >> command block_set_io_throttle. Since 1.1. >> >> Pradeep has a use case for throttling in fsdev. Instead of duplicating >> the relevant parts of BlockIOThrottle, qmp_block_set_io_throttle() and >> hmp_block_set_io_throttle(), he factors them out smartly, into >> >> * [PATCH 2] IOThrottle, base type of BlockIOThrottle >> >> * [PATCH 3] throttle_set_io_limits(), called by >> qmp_block_set_io_throttle() >> >> * [PATCH 4] hmp_initialize_io_throttle(), called by >> hmp_block_set_io_throttle() >> >> throttle_set_io_limits() goes into existing util/throttle.c, and >> hmp_initialize_io_throttle() goes into existing hmp.c. The question is >> where IOThrottle should go. > > Good summary. > >> >> Pradeep proposes to put it in new qapi/throttle.json. Certainly >> defensible, but I really don't like putting every little thing shared >> across subsystem boundaries into its own schema file. > > I agree with the dislike of creating new files, if an existing file is > adequate. > >> >> Let me step back and discuss why we split the QAPI schema into multiple >> files in the first place. For me, the one and only reason is >> MAINTAINERS. > > Indeed, that's a good description of why splits would be appropriate. > So the obvious next question is if this is a case that needs a new > maintainer. > >> >> If the block folks should continue to maintain IOThrottle, then it >> should stay put in block-core.json. > > I think Manos' work on making throttling a filter driver at the block > layer is proof enough that it it is still fine to keep throttling > maintained in block-core.json. > >> >> If somebody else should start maintaining it, it should move. We'd need >> a suitable entry in MAINTAINERS then. >> >> I don't see why maintenance should change, and therefore believe it >> should stay put. >> >> Eric? > > I think we're in violent agreement: don't create a new file, and having > the new factored type live in block-core.json is the best fit because we > haven't come up with any reasons why it needs to be split. Thanks, Eric. Pradeep, please put IOThrottle right before BlocIOThrottle in block-core.json. Use it in fsdev.json without including block-core.json. Sorry for the delay. Thanks for all the clarifications, sure I will move iothrottle code to block-core.json. Thanks, Pradeep
Re: [Qemu-devel] [PATCH] qcow2: allocate cluster_cache/cluster_data on demand
On 18/08/17 23:31, Stefan Hajnoczi wrote:
> Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1)
> * cluster_size + 512 bytes upfront. Allocate s->cluster_cache and
> s->cluster_data when the first read operation is performance on a
> compressed cluster.
>
> The buffers are freed in .bdrv_close(). .bdrv_open() no longer has any
> code paths that can allocate these buffers, so remove the free functions
> in the error code path.
>
> Reported-by: Alexey Kardashevskiy
> Cc: Kevin Wolf
> Signed-off-by: Stefan Hajnoczi
> ---
> Alexey: Does this improve your memory profiling results?
Yes, it does:
was:
12.81% (1,023,193,088B)
now:
05.36% (393,893,888B)
Tested-by: Alexey Kardashevskiy
> block/qcow2-cluster.c | 17 +
> block/qcow2.c | 12
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f06c08f64c..c47600a44e 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1516,6 +1516,23 @@ int qcow2_decompress_cluster(BlockDriverState *bs,
> uint64_t cluster_offset)
> nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) +
> 1;
> sector_offset = coffset & 511;
> csize = nb_csectors * 512 - sector_offset;
> +
> +/* Allocate buffers on first decompress operation, most images are
> + * uncompressed and the memory overhead can be avoided. The buffers
> + * are freed in .bdrv_close().
> + */
> +if (!s->cluster_data) {
> +/* one more sector for decompressed data alignment */
> +s->cluster_data = qemu_try_blockalign(bs->file->bs,
> +QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
> +if (!s->cluster_data) {
> +return -EIO;
> +}
> +}
> +if (!s->cluster_cache) {
> +s->cluster_cache = g_malloc(s->cluster_size);
> +}
> +
> BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
> ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
> nb_csectors);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 40ba26c111..0ac201910a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1360,16 +1360,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict
> *options, int flags,
> goto fail;
> }
>
> -s->cluster_cache = g_malloc(s->cluster_size);
> -/* one more sector for decompressed data alignment */
> -s->cluster_data = qemu_try_blockalign(bs->file->bs,
> QCOW_MAX_CRYPT_CLUSTERS
> -* s->cluster_size + 512);
> -if (s->cluster_data == NULL) {
> -error_setg(errp, "Could not allocate temporary cluster buffer");
> -ret = -ENOMEM;
> -goto fail;
> -}
> -
> s->cluster_cache_offset = -1;
> s->flags = flags;
>
> @@ -1507,8 +1497,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict
> *options, int flags,
> if (s->refcount_block_cache) {
> qcow2_cache_destroy(bs, s->refcount_block_cache);
> }
> -g_free(s->cluster_cache);
> -qemu_vfree(s->cluster_data);
> qcrypto_block_free(s->crypto);
> qapi_free_QCryptoBlockOpenOptions(s->crypto_opts);
> return ret;
>
--
Alexey
Re: [Qemu-devel] [RFC PATCH] osdep.h: Prohibit disabling assert() in supported builds
On 08/18/2017 07:23 PM, Eric Blake wrote: We already have several files that knowingly require assert() to work. While we do NOT want to encourage the use of 'assert(side-effects)' (that is a bad practice that prevents copy-and-paste of code to other projects that CAN disable assertions; plus it costs unnecessary reviewer mental cycles to remember our project policy on crippling asserts), we DO want to send a message that anyone that disables assertions has to tweak code in order to compile, making it obvious that we are not going to support their efforts. Signed-off-by: Eric Blake --- First mentioned as an idea here: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html but I'm titling this RFC as I'm not 100% convinced we want to make it a project-wide, rather than a per-file decision. I think project-wide is the correct way. Reviewed-by: Philippe Mathieu-Daudé include/qemu/osdep.h | 12 hw/scsi/mptsas.c | 4 hw/virtio/virtio.c | 4 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 6855b94bbf..9e745a8af9 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -107,6 +107,18 @@ extern int daemon(int, int); #include "glib-compat.h" #include "qemu/typedefs.h" +/* + * We have a lot of unaudited code that will fail in strange ways if + * you disable assertions at compile-time. You are on your own if + * you cripple these safety-checks. + */ +#ifdef NDEBUG +#error building with NDEBUG is not supported +#endif +#ifdef G_DISABLE_ASSERT +#error building with G_DISABLE_ASSERT is not supported +#endif + #ifndef O_LARGEFILE #define O_LARGEFILE 0 #endif diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 765ab53c34..3b93f12cdb 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1236,11 +1236,7 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq) n = qemu_get_be32(f); /* TODO: add a way for SCSIBusInfo's load_request to fail, * and fail migration instead of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(n >= 0); pci_dma_sglist_init(&req->qsg, pci, n); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 464947f76d..2778adabcc 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1025,11 +1025,7 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz) /* TODO: teach all callers that this can fail, and return failure instead * of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(ARRAY_SIZE(data.in_addr) >= data.in_num); assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
Re: [Qemu-devel] [PATCH] scsi-bus: correct responses for INQUIRY and REQUEST SENSE
Hello Hannes,
On 08/18/17 11:37, Hannes Reinecke wrote:
> According to SPC-3 INQUIRY and REQUEST SENSE should return GOOD
> even on unsupported LUNS.
>
> Signed-off-by: Hannes Reinecke
> ---
> hw/scsi/scsi-bus.c | 29 +
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index e364410a23..ade31c11f5 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -516,8 +516,10 @@ static size_t scsi_sense_len(SCSIRequest *req)
> static int32_t scsi_target_send_command(SCSIRequest *req, uint8_t *buf)
> {
> SCSITargetReq *r = DO_UPCAST(SCSITargetReq, req, req);
> +int fixed_sense = (req->cmd.buf[1] & 1) == 0;
>
> -if (req->lun != 0) {
> +if (req->lun != 0 &&
> +buf[0] != INQUIRY && buf[0] != REQUEST_SENSE) {
> scsi_req_build_sense(req, SENSE_CODE(LUN_NOT_SUPPORTED));
> scsi_req_complete(req, CHECK_CONDITION);
> return 0;
> @@ -535,9 +537,28 @@ static int32_t scsi_target_send_command(SCSIRequest
> *req, uint8_t *buf)
> break;
> case REQUEST_SENSE:
> scsi_target_alloc_buf(&r->req, scsi_sense_len(req));
> -r->len = scsi_device_get_sense(r->req.dev, r->buf,
> - MIN(req->cmd.xfer, r->buf_len),
> - (req->cmd.buf[1] & 1) == 0);
> +if (req->lun != 0) {
> +const struct SCSISense sense = SENSE_CODE(LUN_NOT_SUPPORTED);
> +
> +if (fixed_sense) {
> +r->buf[0] = 0x70;
> +r->buf[2] = sense.key;
> +r->buf[10] = 10;
> +r->buf[12] = sense.asc;
> +r->buf[13] = sense.ascq;
> +r->len = MIN(req->cmd.xfer, SCSI_SENSE_LEN);
> +} else {
> +r->buf[0] = 0x72;
> +r->buf[1] = sense.key;
> +r->buf[2] = sense.asc;
> +r->buf[3] = sense.ascq;
> +r->len = 8;
> +}
> +} else {
> +r->len = scsi_device_get_sense(r->req.dev, r->buf,
> + MIN(req->cmd.xfer, r->buf_len),
> + fixed_sense);
> +}
> if (r->req.dev->sense_is_ua) {
> scsi_device_unit_attention_reported(req->dev);
> r->req.dev->sense_len = 0;
>
thank you for the quick patch.
I've repeated my original testing steps now, in the following scenarios:
edk2:ba40cb31b69d ovmf:ce13d2d8c81f
qemu:v2.10.0-rc3 [1][2]
qemu:v2.10.0-rc3+this patch [3][4]
(Edk2 commit ba40cb31b69d was edk2 master just before I pushed my
respective edk2 patches, and edk2 commit ce13d2d8c81f is current edk2
master, with my respective patches.)
[1] FAIL: as reported originally
[2] PASS: my edk2 patches fix ScsiBusDxe (which is justified in itself),
and then it deals with QEMU post-ded6ddc5a7b9
[3] PASS: this patch of yours makes the reported symptoms vanish even
without applying my patches to edk2
[4] PASS: your patch works fine with the current (fixed) ScsiBusDxe as
well, no regressions found.
Tested-by: Laszlo Ersek
If you agree, I'd suggest two additional tags for the commit message:
Reported-by: Laszlo Ersek
Fixes: ded6ddc5a7b95217557fa360913d1213e12d4a6d
Thanks!
Laszlo
[Qemu-devel] [RFC PATCH] osdep.h: Prohibit disabling assert() in supported builds
We already have several files that knowingly require assert() to work. While we do NOT want to encourage the use of 'assert(side-effects)' (that is a bad practice that prevents copy-and-paste of code to other projects that CAN disable assertions; plus it costs unnecessary reviewer mental cycles to remember our project policy on crippling asserts), we DO want to send a message that anyone that disables assertions has to tweak code in order to compile, making it obvious that we are not going to support their efforts. Signed-off-by: Eric Blake --- First mentioned as an idea here: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html but I'm titling this RFC as I'm not 100% convinced we want to make it a project-wide, rather than a per-file decision. include/qemu/osdep.h | 12 hw/scsi/mptsas.c | 4 hw/virtio/virtio.c | 4 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 6855b94bbf..9e745a8af9 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -107,6 +107,18 @@ extern int daemon(int, int); #include "glib-compat.h" #include "qemu/typedefs.h" +/* + * We have a lot of unaudited code that will fail in strange ways if + * you disable assertions at compile-time. You are on your own if + * you cripple these safety-checks. + */ +#ifdef NDEBUG +#error building with NDEBUG is not supported +#endif +#ifdef G_DISABLE_ASSERT +#error building with G_DISABLE_ASSERT is not supported +#endif + #ifndef O_LARGEFILE #define O_LARGEFILE 0 #endif diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index 765ab53c34..3b93f12cdb 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1236,11 +1236,7 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq) n = qemu_get_be32(f); /* TODO: add a way for SCSIBusInfo's load_request to fail, * and fail migration instead of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(n >= 0); pci_dma_sglist_init(&req->qsg, pci, n); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 464947f76d..2778adabcc 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1025,11 +1025,7 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz) /* TODO: teach all callers that this can fail, and return failure instead * of asserting here. - * When we do, we might be able to re-enable NDEBUG below. */ -#ifdef NDEBUG -#error building with NDEBUG is not supported -#endif assert(ARRAY_SIZE(data.in_addr) >= data.in_num); assert(ARRAY_SIZE(data.out_addr) >= data.out_num); -- 2.13.5
Re: [Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()
On 08/18/2017 04:46 PM, Philippe Mathieu-Daudé wrote: > On 08/18/2017 06:15 PM, Eric Blake wrote: >> There was only one caller; it's easier to inline things. > > What's the benefit of this change? Having a separate function ease code > reading. You might add the 'inline' qualifier but the compiler is smart > enough to inline it. > > Instead of this change I'd inline it and remove the return value, having > a void function initializing s->big_endian. Matter of taste I think :) > >> >> Signed-off-by: Eric Blake >> --- >> tests/libqtest.c | 22 ++ >> 1 file changed, 6 insertions(+), 16 deletions(-) I think the diffstat shows that inlining is a win. My other reason that it is a win: >> @@ -351,8 +338,11 @@ QTestState >> *qtest_init_without_qmp_handshake(const char *extra_args) >> } >> >> /* ask endianness of the target */ >> - >> -s->big_endian = qtest_query_target_endianness(s); >> +qtest_sendf(s, "endianness\n"); >> +args = qtest_rsp(s, 1); The old code has to pass an arbitrary QTestState *s down to a helper function. But I'm trying to get rid of as many functions as possible that depend on arbitrary QTestState *s, particularly when callers end up passing 'global_qtest' for 's'. Inlining it makes it more obvious that the changes in 10/13 are safely operating on global_qtest, which in turn makes it possible for 12/13 to drop QTestState *s parameters from qtest_sendf() and qtest_rsp(). If the function were not inlined, those later cleanups would not be as trivial. (Or put another way - I rebased this patch to be earlier in the series precisely because of what I ran into while writing those patches) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
On 08/18/2017 04:58 PM, Eric Blake wrote: >> NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use >> here) - I have to double-check glib documentation to see whether >> g_assert() can be crippled in a manner similar to how I know assert() >> can be crippled. Ideally, we have a form that always performs side >> effects exactly once, whether or not abort-on-error is enabled (assert() >> does not have that, but I don't know whether glib does). > > Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert, > G_DISABLE_ASSERT has the same compile-time effect on g_assert() as > NDEBUG on assert(). Even worse, g_assert_not_reached() has the same > problem. Oh, and I failed to mention: g_assert() has the same problem as assert() - when disabled at compile-time, 'expr' is not evaluated. If you want legible code (by including side-effects within your macro that has optional abort-on-error behavior), then you need some OTHER macro that always performs the evaluation. But if you create such a macro, don't use 'assert' in its name. And I still fail to see why anyone would actually want to disable abort-on-error behavior - once an assertion has gone wrong, continuing execution is liable to hit follow-on problems that are much harder to diagnose than it would have been just quitting at the assertion failure. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
On 08/18/2017 04:39 PM, Eric Blake wrote: > On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: >> Hi Eric, >> >> On 08/18/2017 06:15 PM, Eric Blake wrote: >>> Assertions should be separate from the side effects, since in >>> theory, g_assert() can be disabled (in practice, we can't really >>> ever do that). >> >> What about the suggestion on your "Hacks for building on gcc 7 / Fedora >> 26" series about avoid building without assertions? > > NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use > here) - I have to double-check glib documentation to see whether > g_assert() can be crippled in a manner similar to how I know assert() > can be crippled. Ideally, we have a form that always performs side > effects exactly once, whether or not abort-on-error is enabled (assert() > does not have that, but I don't know whether glib does). Per https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert, G_DISABLE_ASSERT has the same compile-time effect on g_assert() as NDEBUG on assert(). Even worse, g_assert_not_reached() has the same problem. Then there is the runtime switch g_test_set_nonfatal_assertions() which affects the REST of the g_assert_*() family (such as g_assert_cmpint(), g_assert_true(), g_assert_null()) - but NOT g_assert() proper. And I recall Markus has posted in the past about complaining that g_assert_cmpint() should not be used outside of tests/. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 13/13] numa-test: Use hmp()
On 08/18/2017 06:15 PM, Eric Blake wrote:
Don't open-code something that has a convenient helper available.
Signed-off-by: Eric Blake
Reviewed-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
---
tests/numa-test.c | 21 +++--
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/tests/numa-test.c b/tests/numa-test.c
index 3f636840b1..e1b6152244 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -17,21 +17,6 @@ static char *make_cli(const char *generic_cli, const char
*test_cli)
return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
}
-static char *hmp_info_numa(void)
-{
-QDict *resp;
-char *s;
-
-resp = qmp("{ 'execute': 'human-monitor-command', 'arguments': "
- "{ 'command-line': 'info numa '} }");
-g_assert(resp);
-g_assert(qdict_haskey(resp, "return"));
-s = g_strdup(qdict_get_str(resp, "return"));
-g_assert(s);
-QDECREF(resp);
-return s;
-}
-
static void test_mon_explicit(const void *data)
{
char *s;
@@ -42,7 +27,7 @@ static void test_mon_explicit(const void *data)
"-numa node,nodeid=1,cpus=4-7 ");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
g_assert(strstr(s, "node 1 cpus: 4 5 6 7"));
g_free(s);
@@ -59,7 +44,7 @@ static void test_mon_default(const void *data)
cli = make_cli(data, "-smp 8 -numa node -numa node");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
g_assert(strstr(s, "node 1 cpus: 1 3 5 7"));
g_free(s);
@@ -78,7 +63,7 @@ static void test_mon_partial(const void *data)
"-numa node,nodeid=1,cpus=4-5 ");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 1 2 3 6 7"));
g_assert(strstr(s, "node 1 cpus: 4 5"));
g_free(s);
Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
On 08/18/2017 06:39 PM, Eric Blake wrote: On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: Hi Eric, On 08/18/2017 06:15 PM, Eric Blake wrote: Assertions should be separate from the side effects, since in theory, g_assert() can be disabled (in practice, we can't really ever do that). What about the suggestion on your "Hacks for building on gcc 7 / Fedora 26" series about avoid building without assertions? NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use here) - I have to double-check glib documentation to see whether g_assert() can be crippled in a manner similar to how I know assert() can be crippled. Ideally, we have a form that always performs side effects exactly once, whether or not abort-on-error is enabled (assert() does not have that, but I don't know whether glib does). Yes it does: https://developer.gnome.org/glib/stable/glib-Testing.html#g-assert "The macro can be turned off in final releases of code by defining G_DISABLE_ASSERT when compiling the application, so code must not depend on any side effects from expr ." The obvious win is a more readable code. http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html Indeed, that's a patch proposal that I still haven't written, but it can be done independently of this series. Still, even if we guarantee that assertions will never be crippled for qemu, it is bad practice to code side-effects in an assert(), because it makes the code harder to copy-and-paste into projects that DO work with assertions disabled, and it raises red flags in reviewers' minds of whether it was intentional. This is a good point. [And truth be told, this particular patch is not really about libqtest, so much as something that horrified me when I was grepping for qemu_strtoul() in order to fix the checkpatch warning I got on libqtest's raw use of strtol() in patch 6, and which necessitated me to write patch 5 - even though the name of the file in question is 'qtest.c']
Re: [Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()
On 08/18/2017 06:15 PM, Eric Blake wrote:
There was only one caller; it's easier to inline things.
What's the benefit of this change? Having a separate function ease code
reading. You might add the 'inline' qualifier but the compiler is smart
enough to inline it.
Instead of this change I'd inline it and remove the return value, having
a void function initializing s->big_endian. Matter of taste I think :)
Signed-off-by: Eric Blake
---
tests/libqtest.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5d16351e24..b6dd26e54a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -244,20 +244,6 @@ redo:
return words;
}
-static int qtest_query_target_endianness(QTestState *s)
-{
-gchar **args;
-int big_endian;
-
-qtest_sendf(s, "endianness\n");
-args = qtest_rsp(s, 1);
-g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
-big_endian = strcmp(args[1], "big") == 0;
-g_strfreev(args);
-
-return big_endian;
-}
-
static void cleanup_sigabrt_handler(void)
{
sigaction(SIGABRT, &sigact_old, NULL);
@@ -288,6 +274,7 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
gchar *qmp_socket_path;
gchar *command;
const char *qemu_binary;
+gchar **args;
qemu_binary = getenv("QTEST_QEMU_BINARY");
if (!qemu_binary) {
@@ -351,8 +338,11 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
}
/* ask endianness of the target */
-
-s->big_endian = qtest_query_target_endianness(s);
+qtest_sendf(s, "endianness\n");
+args = qtest_rsp(s, 1);
+g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+s->big_endian = strcmp(args[1], "big") == 0;
+g_strfreev(args);
return s;
}
Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote: > Hi Eric, > > On 08/18/2017 06:15 PM, Eric Blake wrote: >> Assertions should be separate from the side effects, since in >> theory, g_assert() can be disabled (in practice, we can't really >> ever do that). > > What about the suggestion on your "Hacks for building on gcc 7 / Fedora > 26" series about avoid building without assertions? NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use here) - I have to double-check glib documentation to see whether g_assert() can be crippled in a manner similar to how I know assert() can be crippled. Ideally, we have a form that always performs side effects exactly once, whether or not abort-on-error is enabled (assert() does not have that, but I don't know whether glib does). > > The obvious win is a more readable code. > > http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html Indeed, that's a patch proposal that I still haven't written, but it can be done independently of this series. Still, even if we guarantee that assertions will never be crippled for qemu, it is bad practice to code side-effects in an assert(), because it makes the code harder to copy-and-paste into projects that DO work with assertions disabled, and it raises red flags in reviewers' minds of whether it was intentional. [And truth be told, this particular patch is not really about libqtest, so much as something that horrified me when I was grepping for qemu_strtoul() in order to fix the checkpatch warning I got on libqtest's raw use of strtol() in patch 6, and which necessitated me to write patch 5 - even though the name of the file in question is 'qtest.c'] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 05/13] libqtest: Use qemu_strtoul()
On 08/18/2017 06:15 PM, Eric Blake wrote:
This will keep checkpatch happy when the next patch does code motion.
Fix the include order to match HACKING when adding the needed header.
Signed-off-by: Eric Blake
Reviewed-by: Philippe Mathieu-Daudé
---
tests/libqtest.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index a6ce21d7f9..438a22678d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -15,12 +15,13 @@
*
*/
#include "qemu/osdep.h"
-#include "libqtest.h"
#include
#include
#include
+#include "libqtest.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qapi/qmp/json-parser.h"
#include "qapi/qmp/json-streamer.h"
@@ -333,12 +334,14 @@ redo:
g_string_free(line, TRUE);
if (strcmp(words[0], "IRQ") == 0) {
-int irq;
+long irq;
+int ret;
g_assert(words[1] != NULL);
g_assert(words[2] != NULL);
-irq = strtoul(words[2], NULL, 0);
+ret = qemu_strtol(words[2], NULL, 0, &irq);
+g_assert(!ret);
g_assert_cmpint(irq, >=, 0);
g_assert_cmpint(irq, <, MAX_IRQ);
@@ -701,11 +704,13 @@ void qtest_outl(QTestState *s, uint16_t addr, uint32_t
value)
static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
{
gchar **args;
-uint32_t value;
+int ret;
+unsigned long value;
qtest_sendf(s, "%s 0x%x\n", cmd, addr);
args = qtest_rsp(s, 2);
-value = strtoul(args[1], NULL, 0);
+ret = qemu_strtoul(args[1], NULL, 0, &value);
+g_assert(!ret && value <= UINT32_MAX);
g_strfreev(args);
return value;
@@ -756,11 +761,13 @@ void qtest_writeq(QTestState *s, uint64_t addr, uint64_t
value)
static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
{
gchar **args;
+int ret;
uint64_t value;
qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
args = qtest_rsp(s, 2);
-value = strtoull(args[1], NULL, 0);
+ret = qemu_strtou64(args[1], NULL, 0, &value);
+g_assert(!ret);
g_strfreev(args);
return value;
Re: [Qemu-devel] [PATCH v5 08/13] tests: Rely more on global_qtest
On 08/18/2017 05:15 PM, Eric Blake wrote: > libqtest provides two layers of functions: qtest_*() that operate > on an explicit object, and a plain version that operates on the > 'global_qtest' object. However, very few tests care about the > distinction, and even the tests that manipulate multiple qtest > connections at once are just fine reassigning global_qtest around > the blocks of code that will then operate on the updated global, > rather than calling the verbose form. Before the next few patches > get rid of the qtest_* layer, we first need to update the remaining > few spots that were using the long form where we can instead rely > on the short form. > Not a big fan of globals and implicit state, but I do at least agree that we don't need two sets of functions. (You just happen to be killing the set I like.) eh, to-may-to to-mah-to. > Signed-off-by: Eric Blake Acked-by: John Snow
Re: [Qemu-devel] [PATCH v5 03/13] libqtest: Remove dead qtest_instances variable
On 08/18/2017 06:15 PM, Eric Blake wrote:
Prior to commit 063c23d9, we were tracking a list of parallel
qtest objects, in order to safely clean up a SIGABRT handler
only after the last connection quits. But when we switched to
more of glib's infrastructure, the list became dead code that
is never assigned to.
Signed-off-by: Eric Blake
Reviewed-by: Philippe Mathieu-Daudé
---
tests/libqtest.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f180e1..3f956f09fc 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -42,7 +42,6 @@ struct QTestState
};
static GHookList abrt_hooks;
-static GList *qtest_instances;
static struct sigaction sigact_old;
#define g_assert_no_errno(ret) do { \
@@ -240,13 +239,10 @@ QTestState *qtest_init(const char *extra_args)
void qtest_quit(QTestState *s)
{
-qtest_instances = g_list_remove(qtest_instances, s);
g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
/* Uninstall SIGABRT handler on last instance */
-if (!qtest_instances) {
-cleanup_sigabrt_handler();
-}
+cleanup_sigabrt_handler();
kill_qemu(s);
close(s->fd);
Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
Hi Eric,
On 08/18/2017 06:15 PM, Eric Blake wrote:
Assertions should be separate from the side effects, since in
theory, g_assert() can be disabled (in practice, we can't really
ever do that).
What about the suggestion on your "Hacks for building on gcc 7 / Fedora
26" series about avoid building without assertions?
The obvious win is a more readable code.
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html
Signed-off-by: Eric Blake
---
qtest.c | 80 ++---
1 file changed, 57 insertions(+), 23 deletions(-)
diff --git a/qtest.c b/qtest.c
index 88a09e9afc..cbbfb71114 100644
--- a/qtest.c
+++ b/qtest.c
@@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "outl") == 0) {
unsigned long addr;
unsigned long value;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0);
+ret = qemu_strtoul(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtoul(words[2], NULL, 0, &value);
+g_assert(ret == 0);
g_assert(addr <= 0x);
if (words[0][3] == 'b') {
@@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "inl") == 0) {
unsigned long addr;
uint32_t value = -1U;
+int ret;
g_assert(words[1]);
-g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
+ret = qemu_strtoul(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
g_assert(addr <= 0x);
if (words[0][2] == 'b') {
@@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "writeq") == 0) {
uint64_t addr;
uint64_t value;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &value);
+g_assert(ret == 0);
if (words[0][5] == 'b') {
uint8_t data = value;
@@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "readq") == 0) {
uint64_t addr;
uint64_t value = UINT64_C(-1);
+int ret;
g_assert(words[1]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
if (words[0][4] == 'b') {
uint8_t data;
@@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len, i;
uint8_t *data;
char *enc;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
/* We'd send garbage to libqtest if len is 0 */
g_assert(len);
@@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len;
uint8_t *data;
gchar *b64_data;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
data = g_malloc(len);
cpu_physical_memory_read(addr, data, len);
@@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len, i;
uint8_t *data;
size_t data_len;
+int ret;
g_assert(words[1] && words[2] && words[3]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
data_len = strlen(words[3]);
if (data_len < 3) {
@@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len;
uint8_t *data;
unsigned long pattern;
+int ret;
g_assert(words[1] && words[2] && words[3]);
-
[Qemu-devel] [PATCH v5 08/13] tests: Rely more on global_qtest
libqtest provides two layers of functions: qtest_*() that operate
on an explicit object, and a plain version that operates on the
'global_qtest' object. However, very few tests care about the
distinction, and even the tests that manipulate multiple qtest
connections at once are just fine reassigning global_qtest around
the blocks of code that will then operate on the updated global,
rather than calling the verbose form. Before the next few patches
get rid of the qtest_* layer, we first need to update the remaining
few spots that were using the long form where we can instead rely
on the short form.
Signed-off-by: Eric Blake
---
tests/fdc-test.c | 2 +-
tests/ide-test.c | 10 +-
tests/ipmi-bt-test.c | 2 +-
tests/ipmi-kcs-test.c| 2 +-
tests/libqos/libqos-pc.c | 2 +-
tests/postcopy-test.c| 14 +++---
tests/rtc-test.c | 9 +++--
tests/tco-test.c | 5 ++---
tests/wdt_ib700-test.c | 30 +-
9 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 325712e0f2..0b68d9aec4 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -565,7 +565,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
qtest_start("-device floppy,id=floppy0");
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
qtest_add_func("/fdc/cmos", test_cmos);
qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
qtest_add_func("/fdc/read_without_media", test_read_without_media);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index aa9de065fc..505a800b44 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -462,7 +462,7 @@ static void test_bmdma_setup(void)
"-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
"-global ide-hd.ver=%s",
tmp_path, "testdisk", "version");
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
}
static void test_bmdma_teardown(void)
@@ -584,7 +584,7 @@ static void test_flush(void)
dev = get_pci_device(&bmdma_bar, &ide_bar);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
/* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
make_dirty(0);
@@ -635,7 +635,7 @@ static void test_retry_flush(const char *machine)
dev = get_pci_device(&bmdma_bar, &ide_bar);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
/* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
make_dirty(0);
@@ -826,7 +826,7 @@ static void cdrom_pio_impl(int nblocks)
ide_test_start("-drive
if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
"-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
dev = get_pci_device(&bmdma_bar, &ide_bar);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
/* PACKET command on device 0 */
qpci_io_writeb(dev, ide_bar, reg_device, 0);
@@ -909,7 +909,7 @@ static void test_cdrom_dma(void)
ide_test_start("-drive
if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
"-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
guest_buf = guest_alloc(guest_malloc, len);
prdt[0].addr = cpu_to_le32(guest_buf);
diff --git a/tests/ipmi-bt-test.c b/tests/ipmi-bt-test.c
index 7e21a9bbcb..891f5bfb13 100644
--- a/tests/ipmi-bt-test.c
+++ b/tests/ipmi-bt-test.c
@@ -421,7 +421,7 @@ int main(int argc, char **argv)
" -device isa-ipmi-bt,bmc=bmc0", emu_port);
qtest_start(cmdline);
g_free(cmdline);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
qtest_add_func("/ipmi/extern/connect", test_connect);
qtest_add_func("/ipmi/extern/bt_base", test_bt_base);
qtest_add_func("/ipmi/extern/bt_enable_irq", test_enable_irq);
diff --git a/tests/ipmi-kcs-test.c b/tests/ipmi-kcs-test.c
index 178ffc1797..53127d2884 100644
--- a/tests/ipmi-kcs-test.c
+++ b/tests/ipmi-kcs-test.c
@@ -280,7 +280,7 @@ int main(int argc, char **argv)
" -device isa-ipmi-kcs,bmc=bmc0");
qtest_start(cmdline);
g_free(cmdline);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+irq_intercept_in("ioapic");
qtest_add_func("/ipmi/local/kcs_base", test_kcs_base);
qtest_add_func("/ipmi/local/kcs_abort", test_kcs_abort);
qtest_add_func("/ipmi/local/kcs_enable_irq", test_enable_irq);
diff --git a/tests/libqos/libqos-pc.c b/tests/libqos/libqos-pc.c
index b554758802..6a2ff6608b 100644
--- a/tests/libqos/libqos-pc.c
+++ b/tests/libqos/libqos-pc.c
@@ -25,7 +25,7 @@ QOSState *qtest_pc_boot(const char *cmdline_fmt, ...)
qs = qtest_vboot(&qos_ops, cmdline_fmt, ap);
va_end(ap);
-qtest_irq_intercept_in(global_qtest, "ioapic");
+i
[Qemu-devel] [PATCH v5 13/13] numa-test: Use hmp()
Don't open-code something that has a convenient helper available.
Signed-off-by: Eric Blake
Reviewed-by: Markus Armbruster
---
tests/numa-test.c | 21 +++--
1 file changed, 3 insertions(+), 18 deletions(-)
diff --git a/tests/numa-test.c b/tests/numa-test.c
index 3f636840b1..e1b6152244 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -17,21 +17,6 @@ static char *make_cli(const char *generic_cli, const char
*test_cli)
return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
}
-static char *hmp_info_numa(void)
-{
-QDict *resp;
-char *s;
-
-resp = qmp("{ 'execute': 'human-monitor-command', 'arguments': "
- "{ 'command-line': 'info numa '} }");
-g_assert(resp);
-g_assert(qdict_haskey(resp, "return"));
-s = g_strdup(qdict_get_str(resp, "return"));
-g_assert(s);
-QDECREF(resp);
-return s;
-}
-
static void test_mon_explicit(const void *data)
{
char *s;
@@ -42,7 +27,7 @@ static void test_mon_explicit(const void *data)
"-numa node,nodeid=1,cpus=4-7 ");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
g_assert(strstr(s, "node 1 cpus: 4 5 6 7"));
g_free(s);
@@ -59,7 +44,7 @@ static void test_mon_default(const void *data)
cli = make_cli(data, "-smp 8 -numa node -numa node");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
g_assert(strstr(s, "node 1 cpus: 1 3 5 7"));
g_free(s);
@@ -78,7 +63,7 @@ static void test_mon_partial(const void *data)
"-numa node,nodeid=1,cpus=4-5 ");
qtest_start(cli);
-s = hmp_info_numa();
+s = hmp("info numa");
g_assert(strstr(s, "node 0 cpus: 0 1 2 3 6 7"));
g_assert(strstr(s, "node 1 cpus: 4 5"));
g_free(s);
--
2.13.5
[Qemu-devel] [PATCH v5 12/13] libqtest: Use global_qtest in qtest_sendf() and qtest_rsp()
All callers are now passing global_qtest, just remove the parameter
instead. While at it, improve the naming of the va_list variant
of socket_sendf() to be more like the printf/vprintf naming scheme.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 77 +++-
1 file changed, 37 insertions(+), 40 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2998b173f0..0ec8668c88 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -150,7 +150,7 @@ static void socket_send(int fd, const char *buf, ssize_t
size)
}
}
-static void socket_sendf(int fd, const char *fmt, va_list ap)
+static void socket_vsendf(int fd, const char *fmt, va_list ap)
{
gchar *str = g_strdup_vprintf(fmt, ap);
@@ -158,12 +158,12 @@ static void socket_sendf(int fd, const char *fmt, va_list
ap)
g_free(str);
}
-static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
+static void GCC_FMT_ATTR(1, 2) qtest_sendf(const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
-socket_sendf(s->fd, fmt, ap);
+socket_vsendf(global_qtest->fd, fmt, ap);
va_end(ap);
}
@@ -197,14 +197,14 @@ static GString *qtest_recv_line(QTestState *s)
return line;
}
-static gchar **qtest_rsp(QTestState *s, int expected_args)
+static gchar **qtest_rsp(int expected_args)
{
GString *line;
gchar **words;
int i;
redo:
-line = qtest_recv_line(s);
+line = qtest_recv_line(global_qtest);
words = g_strsplit(line->str, " ", 0);
g_string_free(line, TRUE);
@@ -221,9 +221,9 @@ redo:
g_assert_cmpint(irq, <, MAX_IRQ);
if (strcmp(words[1], "raise") == 0) {
-s->irq_level[irq] = true;
+global_qtest->irq_level[irq] = true;
} else {
-s->irq_level[irq] = false;
+global_qtest->irq_level[irq] = false;
}
g_strfreev(words);
@@ -338,8 +338,8 @@ void qtest_start_without_qmp_handshake(const char
*extra_args)
}
/* ask endianness of the target */
-qtest_sendf(global_qtest, "endianness\n");
-args = qtest_rsp(global_qtest, 1);
+qtest_sendf("endianness\n");
+args = qtest_rsp(1);
g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
s->big_endian = strcmp(args[1], "big") == 0;
g_strfreev(args);
@@ -618,7 +618,7 @@ static int64_t qtest_clock_rsp(void)
{
gchar **words;
int64_t clock;
-words = qtest_rsp(global_qtest, 2);
+words = qtest_rsp(2);
clock = g_ascii_strtoll(words[1], NULL, 0);
g_strfreev(words);
return clock;
@@ -626,38 +626,38 @@ static int64_t qtest_clock_rsp(void)
int64_t clock_step_next(void)
{
-qtest_sendf(global_qtest, "clock_step\n");
+qtest_sendf("clock_step\n");
return qtest_clock_rsp();
}
int64_t clock_step(int64_t step)
{
-qtest_sendf(global_qtest, "clock_step %"PRIi64"\n", step);
+qtest_sendf("clock_step %"PRIi64"\n", step);
return qtest_clock_rsp();
}
int64_t clock_set(int64_t val)
{
-qtest_sendf(global_qtest, "clock_set %"PRIi64"\n", val);
+qtest_sendf("clock_set %"PRIi64"\n", val);
return qtest_clock_rsp();
}
void irq_intercept_out(const char *qom_path)
{
-qtest_sendf(global_qtest, "irq_intercept_out %s\n", qom_path);
-qtest_rsp(global_qtest, 0);
+qtest_sendf("irq_intercept_out %s\n", qom_path);
+qtest_rsp(0);
}
void irq_intercept_in(const char *qom_path)
{
-qtest_sendf(global_qtest, "irq_intercept_in %s\n", qom_path);
-qtest_rsp(global_qtest, 0);
+qtest_sendf("irq_intercept_in %s\n", qom_path);
+qtest_rsp(0);
}
static void out(const char *cmd, uint16_t addr, uint32_t value)
{
-qtest_sendf(global_qtest, "%s 0x%x 0x%x\n", cmd, addr, value);
-qtest_rsp(global_qtest, 0);
+qtest_sendf("%s 0x%x 0x%x\n", cmd, addr, value);
+qtest_rsp(0);
}
void outb(uint16_t addr, uint8_t value)
@@ -681,8 +681,8 @@ static uint32_t in(const char *cmd, uint16_t addr)
int ret;
unsigned long value;
-qtest_sendf(global_qtest, "%s 0x%x\n", cmd, addr);
-args = qtest_rsp(global_qtest, 2);
+qtest_sendf("%s 0x%x\n", cmd, addr);
+args = qtest_rsp(2);
ret = qemu_strtoul(args[1], NULL, 0, &value);
g_assert(!ret && value <= UINT32_MAX);
g_strfreev(args);
@@ -708,9 +708,8 @@ uint32_t inl(uint16_t addr)
static void qtest_write(const char *cmd, uint64_t addr,
uint64_t value)
{
-qtest_sendf(global_qtest, "%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr,
-value);
-qtest_rsp(global_qtest, 0);
+qtest_sendf("%s 0x%" PRIx64 " 0x%" PRIx64 "\n", cmd, addr, value);
+qtest_rsp(0);
}
void writeb(uint64_t addr, uint8_t value)
@@ -739,8 +738,8 @@ static uint64_t qtest_read(const char *cmd, uint64_t addr)
int ret;
uint64_t value;
-qtest_sendf(global_qtest, "%s 0x%" PRIx64 "\n", cmd, addr);
-args = qtest_rsp(global_qtest, 2);
+qtest_sendf("%s
[Qemu-devel] [PATCH v5 10/13] libqtest: Drop qtest_init() and qtest_qmp_discard_response()
Most of our tests were using qtest_start() to initialize the
connection and then set global_qtest; the few tests that were
using qtest_init() instead are still setting global_qtest shortly
afterwards. So it makes more sense to just always set
global_qtest, and have all callers go through a single entry
point.
Furthermore, we already have another qtest_init() in
include/sysemu/qtest.h, with a different signature - which makes
it hard to trace which version is being called based on what was
being included.
So, morph qtest_init() into qtest_start(), and hoist the setting
of global_qtest earlier during initialization; which in turn lets
the initialization sequence also benefit from the global variable.
For now, having a separate qtest_end() and qtest_quit() still
makes some tests easier, but hoisting the declaration so the two
are side-by-side makes the overall lifecycle management of
global_qtest easier to see.
While reworking things, prefer 0-initialization, and simplify the
initial handshake work; rendering qtest_qmp_discard_response()
unused. As a bonus: it removes one spot passing an empty string
through varargs, so we are one step closer to using compile-time
format checking on qmp() without tripping over -Wformat-zero-length.
Signed-off-by: Eric Blake
---
tests/libqtest.h| 61 -
tests/libqtest.c| 45 +---
tests/postcopy-test.c | 2 +-
tests/qmp-test.c| 2 +-
tests/vhost-user-test.c | 3 ++-
5 files changed, 35 insertions(+), 78 deletions(-)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 28945b3f7f..dca62fd8da 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -24,20 +24,23 @@ typedef struct QTestState QTestState;
extern QTestState *global_qtest;
/**
- * qtest_init:
+ * qtest_start:
* @extra_args: other arguments to pass to QEMU.
*
+ * Start QEMU, and complete the QMP handshake. Sets #global_qtest, which
+ * is returned for convenience.
+ *
* Returns: #QTestState instance.
*/
-QTestState *qtest_init(const char *extra_args);
+QTestState *qtest_start(const char *extra_args);
/**
- * qtest_init_without_qmp_handshake:
+ * qtest_start_without_qmp_handshake:
* @extra_args: other arguments to pass to QEMU.
*
- * Returns: #QTestState instance.
+ * Starts the connection, but does no handshakes; sets #global_qtest.
*/
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
+void qtest_start_without_qmp_handshake(const char *extra_args);
/**
* qtest_quit:
@@ -48,13 +51,15 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args);
void qtest_quit(QTestState *s);
/**
- * qtest_qmp_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt...: QMP message to send to qemu
+ * qtest_end:
*
- * Sends a QMP message to QEMU and consumes the response.
+ * Shut down the current #global_qtest QEMU process.
*/
-void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...);
+static inline void qtest_end(void)
+{
+qtest_quit(global_qtest);
+global_qtest = NULL;
+}
/**
* qtest_qmp:
@@ -75,16 +80,6 @@ QDict *qtest_qmp(QTestState *s, const char *fmt, ...);
void qtest_async_qmp(QTestState *s, const char *fmt, ...);
/**
- * qtest_qmpv_discard_response:
- * @s: #QTestState instance to operate on.
- * @fmt: QMP message to send to QEMU
- * @ap: QMP message arguments
- *
- * Sends a QMP message to QEMU and consumes the response.
- */
-void qtest_qmpv_discard_response(QTestState *s, const char *fmt, va_list ap);
-
-/**
* qtest_qmpv:
* @s: #QTestState instance to operate on.
* @fmt: QMP message to send to QEMU
@@ -506,32 +501,6 @@ void qtest_add_data_func_full(const char *str, void *data,
void qtest_add_abrt_handler(GHookFunc fn, const void *data);
/**
- * qtest_start:
- * @args: other arguments to pass to QEMU
- *
- * Start QEMU and assign the resulting #QTestState to a global variable.
- * The global variable is used by "shortcut" functions documented below.
- *
- * Returns: #QTestState instance.
- */
-static inline QTestState *qtest_start(const char *args)
-{
-global_qtest = qtest_init(args);
-return global_qtest;
-}
-
-/**
- * qtest_end:
- *
- * Shut down the QEMU process started by qtest_start().
- */
-static inline void qtest_end(void)
-{
-qtest_quit(global_qtest);
-global_qtest = NULL;
-}
-
-/**
* qmp:
* @fmt...: QMP message to send to qemu
*
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 261d86df5a..c4e41261ab 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -266,7 +266,7 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data)
g_hook_prepend(&abrt_hooks, hook);
}
-QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
+void qtest_start_without_qmp_handshake(const char *extra_args)
{
QTestState *s;
int sock, qmpsock, i;
@@ -282,7 +282,7 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
exit(1);
[Qemu-devel] [PATCH v5 06/13] libqtest: Topologically sort functions
Put static functions prior to public headers, in part so that
improvements to qtest_start() can benefit from the static
helpers without needing forward references. Code motion, with
no semantic change.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 263 +++
1 file changed, 131 insertions(+), 132 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 438a22678d..5d16351e24 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -49,7 +49,6 @@ static struct sigaction sigact_old;
g_assert_cmpint(ret, !=, -1); \
} while (0)
-static int qtest_query_target_endianness(QTestState *s);
static int init_socket(const char *socket_path)
{
@@ -128,6 +127,137 @@ static void setup_sigabrt_handler(void)
sigaction(SIGABRT, &sigact, &sigact_old);
}
+static void socket_send(int fd, const char *buf, ssize_t size)
+{
+size_t offset;
+
+if (size < 0) {
+size = strlen(buf);
+}
+offset = 0;
+while (offset < size) {
+ssize_t len;
+
+len = write(fd, buf + offset, size - offset);
+if (len == -1 && errno == EINTR) {
+continue;
+}
+
+g_assert_no_errno(len);
+g_assert_cmpint(len, >, 0);
+
+offset += len;
+}
+}
+
+static void socket_sendf(int fd, const char *fmt, va_list ap)
+{
+gchar *str = g_strdup_vprintf(fmt, ap);
+
+socket_send(fd, str, -1);
+g_free(str);
+}
+
+static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
+{
+va_list ap;
+
+va_start(ap, fmt);
+socket_sendf(s->fd, fmt, ap);
+va_end(ap);
+}
+
+static GString *qtest_recv_line(QTestState *s)
+{
+GString *line;
+size_t offset;
+char *eol;
+
+while ((eol = strchr(s->rx->str, '\n')) == NULL) {
+ssize_t len;
+char buffer[1024];
+
+len = read(s->fd, buffer, sizeof(buffer));
+if (len == -1 && errno == EINTR) {
+continue;
+}
+
+if (len == -1 || len == 0) {
+fprintf(stderr, "Broken pipe\n");
+exit(1);
+}
+
+g_string_append_len(s->rx, buffer, len);
+}
+
+offset = eol - s->rx->str;
+line = g_string_new_len(s->rx->str, offset);
+g_string_erase(s->rx, 0, offset + 1);
+
+return line;
+}
+
+static gchar **qtest_rsp(QTestState *s, int expected_args)
+{
+GString *line;
+gchar **words;
+int i;
+
+redo:
+line = qtest_recv_line(s);
+words = g_strsplit(line->str, " ", 0);
+g_string_free(line, TRUE);
+
+if (strcmp(words[0], "IRQ") == 0) {
+long irq;
+int ret;
+
+g_assert(words[1] != NULL);
+g_assert(words[2] != NULL);
+
+ret = qemu_strtol(words[2], NULL, 0, &irq);
+g_assert(!ret);
+g_assert_cmpint(irq, >=, 0);
+g_assert_cmpint(irq, <, MAX_IRQ);
+
+if (strcmp(words[1], "raise") == 0) {
+s->irq_level[irq] = true;
+} else {
+s->irq_level[irq] = false;
+}
+
+g_strfreev(words);
+goto redo;
+}
+
+g_assert(words[0] != NULL);
+g_assert_cmpstr(words[0], ==, "OK");
+
+if (expected_args) {
+for (i = 0; i < expected_args; i++) {
+g_assert(words[i] != NULL);
+}
+} else {
+g_strfreev(words);
+}
+
+return words;
+}
+
+static int qtest_query_target_endianness(QTestState *s)
+{
+gchar **args;
+int big_endian;
+
+qtest_sendf(s, "endianness\n");
+args = qtest_rsp(s, 1);
+g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+big_endian = strcmp(args[1], "big") == 0;
+g_strfreev(args);
+
+return big_endian;
+}
+
static void cleanup_sigabrt_handler(void)
{
sigaction(SIGABRT, &sigact_old, NULL);
@@ -252,137 +382,6 @@ void qtest_quit(QTestState *s)
g_free(s);
}
-static void socket_send(int fd, const char *buf, ssize_t size)
-{
-size_t offset;
-
-if (size < 0) {
-size = strlen(buf);
-}
-offset = 0;
-while (offset < size) {
-ssize_t len;
-
-len = write(fd, buf + offset, size - offset);
-if (len == -1 && errno == EINTR) {
-continue;
-}
-
-g_assert_no_errno(len);
-g_assert_cmpint(len, >, 0);
-
-offset += len;
-}
-}
-
-static void socket_sendf(int fd, const char *fmt, va_list ap)
-{
-gchar *str = g_strdup_vprintf(fmt, ap);
-
-socket_send(fd, str, -1);
-g_free(str);
-}
-
-static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
-{
-va_list ap;
-
-va_start(ap, fmt);
-socket_sendf(s->fd, fmt, ap);
-va_end(ap);
-}
-
-static GString *qtest_recv_line(QTestState *s)
-{
-GString *line;
-size_t offset;
-char *eol;
-
-while ((eol = strchr(s->rx->str, '\n')) == NULL) {
-ssize_t len;
-char buffer[1024];
-
-len = read(s->fd, buffer, sizeof(buffer));
-if (len == -1 && errno == EINTR) {
-
[Qemu-devel] [PATCH v5 09/13] libqtest: Shorten a couple more qtest_* functions
qtest_rtas_call() and qtest_big_endian() did not have a short
version with an implied global_qtest; but changing these two
functions fits with the theme of the previous patch.
It doesn't hurt that we are now no longer ambiguous with the
qtest_rtas_call() of include/hw/ppc/spapr_rtos.h.
Signed-off-by: Eric Blake
---
tests/libqos/virtio.h | 2 +-
tests/libqtest.h | 18 --
tests/libqtest.c | 13 ++---
tests/libqos/rtas.c | 3 +--
4 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
index 8fbcd1869c..7bca1b418a 100644
--- a/tests/libqos/virtio.h
+++ b/tests/libqos/virtio.h
@@ -93,7 +93,7 @@ struct QVirtioBus {
static inline bool qvirtio_is_big_endian(QVirtioDevice *d)
{
/* FIXME: virtio 1.0 is always little-endian */
-return qtest_big_endian(global_qtest);
+return big_endian();
}
static inline uint32_t qvring_size(uint32_t num, uint32_t align)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 3ae570927a..28945b3f7f 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -338,19 +338,17 @@ uint64_t qtest_readq(QTestState *s, uint64_t addr);
void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
/**
- * qtest_rtas_call:
- * @s: #QTestState instance to operate on.
+ * rtas_call:
* @name: name of the command to call.
* @nargs: Number of args.
* @args: Guest address to read args from.
* @nret: Number of return value.
* @ret: Guest address to write return values to.
*
- * Call an RTAS function
+ * Call an RTAS function, using #global_qtest
*/
-uint64_t qtest_rtas_call(QTestState *s, const char *name,
- uint32_t nargs, uint64_t args,
- uint32_t nret, uint64_t ret);
+uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
+ uint32_t nret, uint64_t ret);
/**
* qtest_bufread:
@@ -430,12 +428,12 @@ int64_t qtest_clock_step(QTestState *s, int64_t step);
int64_t qtest_clock_set(QTestState *s, int64_t val);
/**
- * qtest_big_endian:
- * @s: QTestState instance to operate on.
+ * big_endian:
*
- * Returns: True if the architecture under test has a big endian configuration.
+ * Returns: True if the architecture under test, via #global_qtest,
+ * has a big endian configuration.
*/
-bool qtest_big_endian(QTestState *s);
+bool big_endian(void);
/**
* qtest_get_arch:
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b6dd26e54a..261d86df5a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -816,13 +816,12 @@ void qtest_memread(QTestState *s, uint64_t addr, void
*data, size_t size)
g_strfreev(args);
}
-uint64_t qtest_rtas_call(QTestState *s, const char *name,
- uint32_t nargs, uint64_t args,
- uint32_t nret, uint64_t ret)
+uint64_t rtas_call(const char *name, uint32_t nargs, uint64_t args,
+ uint32_t nret, uint64_t ret)
{
-qtest_sendf(s, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
+qtest_sendf(global_qtest, "rtas %s %u 0x%"PRIx64" %u 0x%"PRIx64"\n",
name, nargs, args, nret, ret);
-qtest_rsp(s, 0);
+qtest_rsp(global_qtest, 0);
return 0;
}
@@ -947,9 +946,9 @@ char *hmp(const char *fmt, ...)
return ret;
}
-bool qtest_big_endian(QTestState *s)
+bool big_endian(void)
{
-return s->big_endian;
+return global_qtest->big_endian;
}
void qtest_cb_for_every_machine(void (*cb)(const char *machine))
diff --git a/tests/libqos/rtas.c b/tests/libqos/rtas.c
index 0269803ce0..e7ba7ab18f 100644
--- a/tests/libqos/rtas.c
+++ b/tests/libqos/rtas.c
@@ -37,8 +37,7 @@ static uint64_t qrtas_call(QGuestAllocator *alloc, const char
*name,
target_ret = guest_alloc(alloc, nret * sizeof(uint32_t));
qrtas_copy_args(target_args, nargs, args);
-res = qtest_rtas_call(global_qtest, name,
- nargs, target_args, nret, target_ret);
+res = rtas_call(name, nargs, target_args, nret, target_ret);
qrtas_copy_ret(target_ret, nret, ret);
guest_free(alloc, target_ret);
--
2.13.5
[Qemu-devel] [PATCH v5 11/13] libqtest: Drop many static inline qtest_ wrappers
None of the tests were calling the long qtest_*() form, except via the static inline short forms. Remove a layer of indirection by only supporting the short form, and using global_qtest directly in the .c file. (Yes, this flies in the face of thread-safety, by relying on a global instead of passing all state through parameters, but ease of writing/maintaining tests trumps ivory-tower design, and our tests aren't really multi-threaded.) The list of affected functions (by their short name): qmp_receive qmp_eventwait qmp_eventwait_ref get_irq irq_intercept_in irq_intercept_out outb outw outl inb inw inl writeb writew writel writeq readb readw readl readq memread bufread memwrite bufwrite qmemset clock_step_next clock_step clock_set Signed-off-by: Eric Blake --- tests/libqtest.h | 563 ++- tests/libqtest.c | 173 - 2 files changed, 190 insertions(+), 546 deletions(-) diff --git a/tests/libqtest.h b/tests/libqtest.h index dca62fd8da..431e546193 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -21,6 +21,15 @@ typedef struct QTestState QTestState; +/** + * global_qtest: + * The current test object. + * + * Many functions in this file implicitly operate on the current + * object; tests that need to alternate between two parallel + * connections can do so by switching which test state is current + * before issuing commands. + */ extern QTestState *global_qtest; /** @@ -46,7 +55,8 @@ void qtest_start_without_qmp_handshake(const char *extra_args); * qtest_quit: * @s: #QTestState instance to operate on. * - * Shut down the QEMU process associated to @s. + * Shut down the QEMU process associated to @s. See also qtest_end() + * for clearing #global_qtest. */ void qtest_quit(QTestState *s); @@ -100,31 +110,31 @@ QDict *qtest_qmpv(QTestState *s, const char *fmt, va_list ap); void qtest_async_qmpv(QTestState *s, const char *fmt, va_list ap); /** - * qtest_receive: - * @s: #QTestState instance to operate on. + * qmp_receive: * - * Reads a QMP message from QEMU and returns the response. + * Reads a QMP message from QEMU, using #global_qtest, and returns the + * response. */ -QDict *qtest_qmp_receive(QTestState *s); +QDict *qmp_receive(void); /** - * qtest_qmp_eventwait: - * @s: #QTestState instance to operate on. + * qmp_eventwait: * @s: #event event to wait for. * - * Continuously polls for QMP responses until it receives the desired event. + * Continuously polls for QMP responses, using #global_qtest, until it + * receives the desired event. */ -void qtest_qmp_eventwait(QTestState *s, const char *event); +void qmp_eventwait(const char *event); /** - * qtest_qmp_eventwait_ref: - * @s: #QTestState instance to operate on. + * qmp_eventwait_ref: * @s: #event event to wait for. * - * Continuously polls for QMP responses until it receives the desired event. - * Returns a copy of the event for further investigation. + * Continuously polls for QMP responses, using #global_qtest, until it + * receives the desired event. Returns a copy of the event for + * further investigation. */ -QDict *qtest_qmp_eventwait_ref(QTestState *s, const char *event); +QDict *qmp_eventwait_ref(const char *event); /** * qtest_hmp: @@ -152,185 +162,167 @@ char *qtest_hmp(QTestState *s, const char *fmt, ...); char *qtest_hmpv(QTestState *s, const char *fmt, va_list ap); /** - * qtest_get_irq: - * @s: #QTestState instance to operate on. + * get_irq: * @num: Interrupt to observe. * - * Returns: The level of the @num interrupt. + * Returns: The level of the @num interrupt, using #global_qtest. */ -bool qtest_get_irq(QTestState *s, int num); +bool get_irq(int num); /** - * qtest_irq_intercept_in: - * @s: #QTestState instance to operate on. + * irq_intercept_in: * @string: QOM path of a device. * * Associate qtest irqs with the GPIO-in pins of the device - * whose path is specified by @string. + * whose path is specified by @string, using #global_qtest. */ -void qtest_irq_intercept_in(QTestState *s, const char *string); +void irq_intercept_in(const char *string); /** - * qtest_irq_intercept_out: - * @s: #QTestState instance to operate on. + * irq_intercept_out: * @string: QOM path of a device. * * Associate qtest irqs with the GPIO-out pins of the device - * whose path is specified by @string. + * whose path is specified by @string, using #global_qtest. */ -void qtest_irq_intercept_out(QTestState *s, const char *string); +void irq_intercept_out(const char *string); /** - * qtest_outb: - * @s: #QTestState instance to operate on. + * outb: * @addr: I/O port to write to. * @value: Value being written. * - * Write an 8-bit value to an I/O port. + * Write an 8-bit value to an I/O port, using #global_qtest. */ -void qtest_outb(QTestState *s, uint16_t addr, uint8_t value); +void outb(uint16_t addr, uint8_t value); /** - * qtest_outw: - * @s: #QTestState instance to operate on. + * outw: * @addr: I/
[Qemu-devel] [PATCH v5 07/13] libqtest: Inline qtest_query_target_endianness()
There was only one caller; it's easier to inline things.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5d16351e24..b6dd26e54a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -244,20 +244,6 @@ redo:
return words;
}
-static int qtest_query_target_endianness(QTestState *s)
-{
-gchar **args;
-int big_endian;
-
-qtest_sendf(s, "endianness\n");
-args = qtest_rsp(s, 1);
-g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
-big_endian = strcmp(args[1], "big") == 0;
-g_strfreev(args);
-
-return big_endian;
-}
-
static void cleanup_sigabrt_handler(void)
{
sigaction(SIGABRT, &sigact_old, NULL);
@@ -288,6 +274,7 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
gchar *qmp_socket_path;
gchar *command;
const char *qemu_binary;
+gchar **args;
qemu_binary = getenv("QTEST_QEMU_BINARY");
if (!qemu_binary) {
@@ -351,8 +338,11 @@ QTestState *qtest_init_without_qmp_handshake(const char
*extra_args)
}
/* ask endianness of the target */
-
-s->big_endian = qtest_query_target_endianness(s);
+qtest_sendf(s, "endianness\n");
+args = qtest_rsp(s, 1);
+g_assert(strcmp(args[1], "big") == 0 || strcmp(args[1], "little") == 0);
+s->big_endian = strcmp(args[1], "big") == 0;
+g_strfreev(args);
return s;
}
--
2.13.5
[Qemu-devel] [PATCH v5 00/13] Preliminary libqtest cleanups
Markus gave some good advice on my 'v4: Clean up around qmp() and hmp()' series [1]. Among other things, we agreed that if I'm going to get rid of the qtest_* layer of function wrappers, I should do it all the way (rather than just on the qmp() functions), and up front. So that's what this series does - focus on the stuff that should be easier to commit up front, while I still play around with the hairier stuff related to improving the qmp() interfaces. I'm naming this v5, even though most of it is new content (patch 1 was posted independently [2], and patch 13 was 7/22 of v4). The overall diffstat is rather fun. [1] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00595.html [2] https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00385.html Eric Blake (13): test-qga: Kill broken and dead QGA_TEST_SIDE_EFFECTING code qtest: Don't perform side effects inside assertion libqtest: Remove dead qtest_instances variable libqtest: Let socket_send() compute length libqtest: Use qemu_strtoul() libqtest: Topologically sort functions libqtest: Inline qtest_query_target_endianness() tests: Rely more on global_qtest libqtest: Shorten a couple more qtest_* functions libqtest: Drop qtest_init() and qtest_qmp_discard_response() libqtest: Drop many static inline qtest_ wrappers libqtest: Use global_qtest in qtest_sendf() and qtest_rsp() numa-test: Use hmp() tests/libqos/virtio.h| 2 +- tests/libqtest.h | 642 +-- tests/libqtest.c | 496 ++-- qtest.c | 80 -- tests/fdc-test.c | 2 +- tests/ide-test.c | 10 +- tests/ipmi-bt-test.c | 2 +- tests/ipmi-kcs-test.c| 2 +- tests/libqos/libqos-pc.c | 2 +- tests/libqos/rtas.c | 3 +- tests/numa-test.c| 21 +- tests/postcopy-test.c| 16 +- tests/qmp-test.c | 2 +- tests/rtc-test.c | 9 +- tests/tco-test.c | 5 +- tests/test-qga.c | 90 --- tests/vhost-user-test.c | 3 +- tests/wdt_ib700-test.c | 30 ++- 18 files changed, 467 insertions(+), 950 deletions(-) -- 2.13.5
[Qemu-devel] [PATCH v5 01/13] test-qga: Kill broken and dead QGA_TEST_SIDE_EFFECTING code
Back when the test was introduced, in commit 62c39b307, the
test was set up to run qemu-ga directly on the host performing
the test, and defaults to limiting itself to safe commands. At
the time, it was envisioned that setting QGA_TEST_SIDE_EFFECTING
in the environment could cover a few more commands, while noting
the potential danger of those side effects running in the host.
But this has NEVER been tested: if you enable the environment
variable, the test WILL fail. One obvious reason: if you are not
running as root, you'll probably get a permission failure when
trying to freeze the file systems, or when changing system time.
Less obvious: if you run the test as root (wow, you're brave), you
could end up hanging if the test tries to log things to a
temporarily frozen filesystem. But the cutest reason of all: if
you get past the above hurdles, the test uses invalid JSON in
test_qga_fstrim() (missing '' around the dictionary key 'minimum'),
and will thus fail an assertion in qmp_fd().
Rather than leave this untested time-bomb in place, rip it out.
Hopefully, as originally envisioned, we can find an opportunity
to test an actual sandboxed guest where the guest-agent has
full permissions and will not unduly affect the host running
the test - if so, 'git revert' can be used if desired, for
salvaging any useful parts of this attempt.
Signed-off-by: Eric Blake
Reviewed-by: Marc-André Lureau
---
tests/test-qga.c | 90
1 file changed, 90 deletions(-)
diff --git a/tests/test-qga.c b/tests/test-qga.c
index 06783e7585..fd6bc7690f 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -642,65 +642,6 @@ static void test_qga_get_time(gconstpointer fix)
QDECREF(ret);
}
-static void test_qga_set_time(gconstpointer fix)
-{
-const TestFixture *fixture = fix;
-QDict *ret;
-int64_t current, time;
-gchar *cmd;
-
-/* get current time */
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-current = qdict_get_int(ret, "return");
-g_assert_cmpint(current, >, 0);
-QDECREF(ret);
-
-/* set some old time */
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-set-time',"
- " 'arguments': { 'time': 1000 } }");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-QDECREF(ret);
-
-/* check old time */
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-time = qdict_get_int(ret, "return");
-g_assert_cmpint(time / 1000, <, G_USEC_PER_SEC * 10);
-QDECREF(ret);
-
-/* set back current time */
-cmd = g_strdup_printf("{'execute': 'guest-set-time',"
- " 'arguments': { 'time': %" PRId64 " } }",
- current + time * 1000);
-ret = qmp_fd(fixture->fd, cmd);
-g_free(cmd);
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-QDECREF(ret);
-}
-
-static void test_qga_fstrim(gconstpointer fix)
-{
-const TestFixture *fixture = fix;
-QDict *ret;
-QList *list;
-const QListEntry *entry;
-
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-fstrim',"
- " arguments: { minimum: 4194304 } }");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-list = qdict_get_qlist(ret, "return");
-entry = qlist_first(list);
-g_assert(qdict_haskey(qobject_to_qdict(entry->value), "paths"));
-
-QDECREF(ret);
-}
-
static void test_qga_blacklist(gconstpointer data)
{
TestFixture fix;
@@ -831,30 +772,6 @@ static void test_qga_fsfreeze_status(gconstpointer fix)
QDECREF(ret);
}
-static void test_qga_fsfreeze_and_thaw(gconstpointer fix)
-{
-const TestFixture *fixture = fix;
-QDict *ret;
-const gchar *status;
-
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-freeze'}");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-QDECREF(ret);
-
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-status'}");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-status = qdict_get_try_str(ret, "return");
-g_assert_cmpstr(status, ==, "frozen");
-QDECREF(ret);
-
-ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-thaw'}");
-g_assert_nonnull(ret);
-qmp_assert_no_error(ret);
-QDECREF(ret);
-}
-
static void test_qga_guest_exec(gconstpointer fix)
{
const TestFixture *fixture = fix;
@@ -1029,13 +946,6 @@ int main(int argc, char **argv)
g_test_add_data_func("/qga/guest-get-osinfo", &fix,
test_qga_guest_get_osinfo);
-if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
-g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
- test_qga_fsfreeze_and_thaw);
-g_test_add_data_func("/qga/set-time", &fix, test_qga_set_time);
-g_test_add_data_func("/qga/fstrim", &fix, test_qga_fstrim);
-}
-
ret = g_test_run(
[Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion
Assertions should be separate from the side effects, since in
theory, g_assert() can be disabled (in practice, we can't really
ever do that).
Signed-off-by: Eric Blake
---
qtest.c | 80 ++---
1 file changed, 57 insertions(+), 23 deletions(-)
diff --git a/qtest.c b/qtest.c
index 88a09e9afc..cbbfb71114 100644
--- a/qtest.c
+++ b/qtest.c
@@ -332,10 +332,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "outl") == 0) {
unsigned long addr;
unsigned long value;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtoul(words[2], NULL, 0, &value) == 0);
+ret = qemu_strtoul(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtoul(words[2], NULL, 0, &value);
+g_assert(ret == 0);
g_assert(addr <= 0x);
if (words[0][3] == 'b') {
@@ -352,9 +355,11 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "inl") == 0) {
unsigned long addr;
uint32_t value = -1U;
+int ret;
g_assert(words[1]);
-g_assert(qemu_strtoul(words[1], NULL, 0, &addr) == 0);
+ret = qemu_strtoul(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
g_assert(addr <= 0x);
if (words[0][2] == 'b') {
@@ -372,10 +377,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "writeq") == 0) {
uint64_t addr;
uint64_t value;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &value) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &value);
+g_assert(ret == 0);
if (words[0][5] == 'b') {
uint8_t data = value;
@@ -401,9 +409,11 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
strcmp(words[0], "readq") == 0) {
uint64_t addr;
uint64_t value = UINT64_C(-1);
+int ret;
g_assert(words[1]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
if (words[0][4] == 'b') {
uint8_t data;
@@ -427,10 +437,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len, i;
uint8_t *data;
char *enc;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
/* We'd send garbage to libqtest if len is 0 */
g_assert(len);
@@ -451,10 +464,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len;
uint8_t *data;
gchar *b64_data;
+int ret;
g_assert(words[1] && words[2]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
data = g_malloc(len);
cpu_physical_memory_read(addr, data, len);
@@ -468,10 +484,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len, i;
uint8_t *data;
size_t data_len;
+int ret;
g_assert(words[1] && words[2] && words[3]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert(ret == 0);
data_len = strlen(words[3]);
if (data_len < 3) {
@@ -497,11 +516,15 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
uint64_t addr, len;
uint8_t *data;
unsigned long pattern;
+int ret;
g_assert(words[1] && words[2] && words[3]);
-g_assert(qemu_strtou64(words[1], NULL, 0, &addr) == 0);
-g_assert(qemu_strtou64(words[2], NULL, 0, &len) == 0);
-g_assert(qemu_strtoul(words[3], NULL, 0, &pattern) == 0);
+ret = qemu_strtou64(words[1], NULL, 0, &addr);
+g_assert(ret == 0);
+ret = qemu_strtou64(words[2], NULL, 0, &len);
+g_assert
[Qemu-devel] [PATCH v5 03/13] libqtest: Remove dead qtest_instances variable
Prior to commit 063c23d9, we were tracking a list of parallel
qtest objects, in order to safely clean up a SIGABRT handler
only after the last connection quits. But when we switched to
more of glib's infrastructure, the list became dead code that
is never assigned to.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f180e1..3f956f09fc 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -42,7 +42,6 @@ struct QTestState
};
static GHookList abrt_hooks;
-static GList *qtest_instances;
static struct sigaction sigact_old;
#define g_assert_no_errno(ret) do { \
@@ -240,13 +239,10 @@ QTestState *qtest_init(const char *extra_args)
void qtest_quit(QTestState *s)
{
-qtest_instances = g_list_remove(qtest_instances, s);
g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
/* Uninstall SIGABRT handler on last instance */
-if (!qtest_instances) {
-cleanup_sigabrt_handler();
-}
+cleanup_sigabrt_handler();
kill_qemu(s);
close(s->fd);
--
2.13.5
[Qemu-devel] [PATCH v5 04/13] libqtest: Let socket_send() compute length
Rather than make multiple callers call strlen(), it's easier if
socket_send() itself can compute a length via strlen() if none
was provided (caller passes -1). Callers that can get at the
length more efficiently are left that way.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 3f956f09fc..a6ce21d7f9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -251,10 +251,13 @@ void qtest_quit(QTestState *s)
g_free(s);
}
-static void socket_send(int fd, const char *buf, size_t size)
+static void socket_send(int fd, const char *buf, ssize_t size)
{
size_t offset;
+if (size < 0) {
+size = strlen(buf);
+}
offset = 0;
while (offset < size) {
ssize_t len;
@@ -274,9 +277,8 @@ static void socket_send(int fd, const char *buf, size_t
size)
static void socket_sendf(int fd, const char *fmt, va_list ap)
{
gchar *str = g_strdup_vprintf(fmt, ap);
-size_t size = strlen(str);
-socket_send(fd, str, size);
+socket_send(fd, str, -1);
g_free(str);
}
@@ -858,7 +860,7 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, const
void *data, size_t size)
bdata = g_base64_encode(data, size);
qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
-socket_send(s->fd, bdata, strlen(bdata));
+socket_send(s->fd, bdata, -1);
socket_send(s->fd, "\n", 1);
qtest_rsp(s, 0);
g_free(bdata);
--
2.13.5
[Qemu-devel] [PATCH v5 05/13] libqtest: Use qemu_strtoul()
This will keep checkpatch happy when the next patch does code motion.
Fix the include order to match HACKING when adding the needed header.
Signed-off-by: Eric Blake
---
tests/libqtest.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index a6ce21d7f9..438a22678d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -15,12 +15,13 @@
*
*/
#include "qemu/osdep.h"
-#include "libqtest.h"
#include
#include
#include
+#include "libqtest.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "qapi/qmp/json-parser.h"
#include "qapi/qmp/json-streamer.h"
@@ -333,12 +334,14 @@ redo:
g_string_free(line, TRUE);
if (strcmp(words[0], "IRQ") == 0) {
-int irq;
+long irq;
+int ret;
g_assert(words[1] != NULL);
g_assert(words[2] != NULL);
-irq = strtoul(words[2], NULL, 0);
+ret = qemu_strtol(words[2], NULL, 0, &irq);
+g_assert(!ret);
g_assert_cmpint(irq, >=, 0);
g_assert_cmpint(irq, <, MAX_IRQ);
@@ -701,11 +704,13 @@ void qtest_outl(QTestState *s, uint16_t addr, uint32_t
value)
static uint32_t qtest_in(QTestState *s, const char *cmd, uint16_t addr)
{
gchar **args;
-uint32_t value;
+int ret;
+unsigned long value;
qtest_sendf(s, "%s 0x%x\n", cmd, addr);
args = qtest_rsp(s, 2);
-value = strtoul(args[1], NULL, 0);
+ret = qemu_strtoul(args[1], NULL, 0, &value);
+g_assert(!ret && value <= UINT32_MAX);
g_strfreev(args);
return value;
@@ -756,11 +761,13 @@ void qtest_writeq(QTestState *s, uint64_t addr, uint64_t
value)
static uint64_t qtest_read(QTestState *s, const char *cmd, uint64_t addr)
{
gchar **args;
+int ret;
uint64_t value;
qtest_sendf(s, "%s 0x%" PRIx64 "\n", cmd, addr);
args = qtest_rsp(s, 2);
-value = strtoull(args[1], NULL, 0);
+ret = qemu_strtou64(args[1], NULL, 0, &value);
+g_assert(!ret);
g_strfreev(args);
return value;
--
2.13.5
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH 09/12] ppc440: Add emulation of plb-pcix controller found in some 440 SoCs
Le 18/08/2017 à 21:43, luigi burdo a écrit : > can i ask you something ? why you dont try to integrate in qemu the > pegasos 2 or the efika machine. i have the feeling that probably it > can be more simple because more old machine and components. > Except they are antique, and the Pegasos at least has an horribly buggy OpenFirmware implementation. I'd rather work on BeBox support ;-) François.
Re: [Qemu-devel] [PATCH v1 1/2] Makefile: Remove libqemustub.a
On Fri, Aug 18, 2017 at 12:29 PM, Philippe Mathieu-Daudé wrote: > Hi Alistair, > > > On 08/18/2017 03:47 PM, Alistair Francis wrote: >> >> Using two libraries (libqemuutil.a and libqemustub.a) would sometimes >> result in circular dependencies. To avoid these issues let's just >> combine both into a single library that functions as both. >> >> Signed-off-by: Alistair Francis >> --- >> >> Makefile| 7 +++ >> Makefile.target | 2 +- >> docs/devel/build-system.txt | 14 +- >> tests/Makefile.include | 6 +++--- >> 4 files changed, 12 insertions(+), 17 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 81447b1f08..f111e93c63 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -344,7 +344,7 @@ subdir-dtc:dtc/libfdt dtc/tests >> dtc/%: >> mkdir -p $@ >> -$(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) >> $(chardev-obj-y) \ >> +$(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \ >> $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) >> ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS)) >> @@ -364,12 +364,11 @@ Makefile: $(version-obj-y) >> ## >> # Build libraries >> -libqemustub.a: $(stub-obj-y) >> -libqemuutil.a: $(util-obj-y) $(trace-obj-y) >> +libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y) >> ## >> -COMMON_LDADDS = libqemuutil.a libqemustub.a >> +COMMON_LDADDS = libqemuutil.a >> qemu-img.o: qemu-img-cmds.h >> diff --git a/Makefile.target b/Makefile.target >> index 7f42c45db8..0a80caf79c 100644 >> --- a/Makefile.target >> +++ b/Makefile.target >> @@ -193,7 +193,7 @@ all-obj-$(CONFIG_SOFTMMU) += $(io-obj-y) >> $(QEMU_PROG_BUILD): config-devices.mak >> -COMMON_LDADDS = ../libqemuutil.a ../libqemustub.a >> +COMMON_LDADDS = ../libqemuutil.a >> # build either PROG or PROGW >> $(QEMU_PROG_BUILD): $(all-obj-y) $(COMMON_LDADDS) >> diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt >> index 2af1e668c5..5f32e53248 100644 >> --- a/docs/devel/build-system.txt >> +++ b/docs/devel/build-system.txt >> @@ -232,15 +232,11 @@ The utility code that is used by all binaries is >> built into a >> static archive called libqemuutil.a, which is then linked to all the >> binaries. In order to provide hooks that are only needed by some of the >> binaries, code in libqemuutil.a may depend on other functions that are >> -not fully implemented by all QEMU binaries. To deal with this there is a >> -second library called libqemustub.a which provides dummy stubs for all >> -these functions. These will get lazy linked into the binary if the real >> -implementation is not present. In this way, the libqemustub.a static >> -library can be thought of as a portable implementation of the weak >> -symbols concept. All binaries should link to both libqemuutil.a and >> -libqemustub.a. e.g. >> - >> - qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a libqemustub.a >> +not fully implemented by all QEMU binaries. To deal with this there are >> +dummy stubs for all these functions in libqemuutil.a. >> +All binaries should link to both libqemuutil.a and libqemustub.a. e.g. > > > still libqemustub.a Ah, I missed this. I'll fix this up. > >> + >> + qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a > > > changed a bit: > > The utility code that is used by all binaries is built into a > static archive called libqemuutil.a, which is then linked to all the > binaries. In order to provide hooks that are only needed by some of the > binaries, code in libqemuutil.a may depend on other functions that are > not fully implemented by all QEMU binaries. Dummy stubs for all these > functions are also provided by this library, and will get lazy linked > into the binary if the real implementation is not present. In this way, > this static library can be thought of as a portable implementation of > the weak symbols concept. All binaries should link to libqemuutil.a. e.g. > > qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a > > This or removing "libqemustub.a": > Reviewed-by: Philippe Mathieu-Daudé Thanks! Alistair > > >> Windows platform portability >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index 37c1bed683..80527a8763 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -553,7 +553,7 @@ QEMU_CFLAGS += -I$(SRC_PATH)/tests >> # Deps that are common to various different sets of tests below >> -test-util-obj-y = libqemuutil.a libqemustub.a >> +test-util-obj-y = libqemuutil.a >> test-qom-obj-y = $(qom-obj-y) $(test-util-obj-y) >> test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \ >> tests/test-qapi-event.o tests/test-qmp-introspect.o \ >> @@ -608,8 +608,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \ >> $(test-io-obj-y) >> tests/test-timed-aver
Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu
On 08/17/2017 12:22 PM, Igor Mammedov wrote: On Thu, 17 Aug 2017 01:30:54 -0300 Philippe Mathieu-Daudé wrote: [...] Also I couldn't test it with KVM. Tested in TCG mode (boots debian mips/mips64 kernel with different cpu types), and new CPU leaf types show up on QOM tree as expected (QOMifycation is done as expected) you mean the "info qom-tree" output? and '-cpu help' also works as expected, so with checkpatch issues fixed you may add to patches my Tested-by: Igor Mammedov ok thanks for the testing! I'll wait to see if there is some KVM feedback from imgtec folks before spaming a v2. Igor Mammedov (2): mips: MIPSCPU model subclasses mips: replace cpu_mips_init() with cpu_generic_init() Philippe Mathieu-Daudé (6): mips: move hw/mips/cputimer.c to target/mips/ mips: introduce internal.h and cleanup cpu.h mips: split cpu_mips_realize_env() out of cpu_mips_init() mips: call cpu_mips_realize_env() from mips_cpu_realizefn() mips: now than MIPSCPU is QOMified, mark it abstract mips: update mips_cpu_list() to use object_class_get_list() target/mips/cpu-qom.h | 1 + target/mips/cpu.h | 357 +- target/mips/internal.h| 422 ++ hw/mips/cps.c | 2 +- hw/mips/mips_fulong2e.c | 2 +- hw/mips/mips_jazz.c | 2 +- hw/mips/mips_malta.c | 2 +- hw/mips/mips_mipssim.c| 2 +- hw/mips/mips_r4k.c| 2 +- hw/mips/cputimer.c => target/mips/cp0_timer.c | 2 +- target/mips/cpu.c | 57 +++- target/mips/gdbstub.c | 1 + target/mips/helper.c | 47 +++ target/mips/kvm.c | 1 + target/mips/machine.c | 1 + target/mips/msa_helper.c | 1 + target/mips/op_helper.c | 1 + target/mips/translate.c | 23 +- target/mips/translate_init.c | 68 + hw/mips/Makefile.objs | 2 +- target/mips/Makefile.objs | 2 +- 21 files changed, 549 insertions(+), 449 deletions(-) create mode 100644 target/mips/internal.h rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)
Re: [Qemu-devel] [PATCH 2/8] mips: introduce internal.h and cleanup cpu.h
Hi Igor, On 08/17/2017 12:06 PM, Philippe Mathieu-Daudé wrote: no logical change, only code movement (and fix a comment typo). while at that fix checkpatch errors or first fix checkpatch noted issues in cpu.h and then move it in next patch to internal.h After asking confirmation on IRC these 3 checkpatch issues are false positives, and I'm not fluent in Perl to fix it, so I'll add a comment about it in the next series cover. Regards, Phil.
Re: [Qemu-devel] [PATCH for-2.11 08/27] sparc: replace cpu_sparc_init() with cpu_generic_init()
I succeed booting a SPARC image once applying 1 to 8 (this commit),
so not being able to boot the image previously is probably due to patch
order, which mean current order is not bisect-able.
On 08/18/2017 07:08 AM, Igor Mammedov wrote:
it's just a wrapper, drop it and use cpu_generic_init() directly
Signed-off-by: Igor Mammedov
---
CC: Fabien Chouteau
CC: Mark Cave-Ayland
CC: Artyom Tarasenko
---
target/sparc/cpu.h | 3 +--
hw/sparc/leon3.c | 2 +-
hw/sparc/sun4m.c | 2 +-
hw/sparc64/sparc64.c | 2 +-
target/sparc/cpu.c | 5 -
5 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 0e41916..b45cfb4 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -579,7 +579,6 @@ void cpu_raise_exception_ra(CPUSPARCState *, int,
uintptr_t) QEMU_NORETURN;
#ifndef NO_CPU_IO_DEFS
/* cpu_init.c */
-SPARCCPU *cpu_sparc_init(const char *cpu_model);
void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu);
void sparc_cpu_list(FILE *f, fprintf_function cpu_fprintf);
/* mmu_helper.c */
@@ -656,7 +655,7 @@ hwaddr cpu_get_phys_page_nofault(CPUSPARCState *env,
target_ulong addr,
int cpu_sparc_signal_handler(int host_signum, void *pinfo, void *puc);
#ifndef NO_CPU_IO_DEFS
-#define cpu_init(cpu_model) CPU(cpu_sparc_init(cpu_model))
+#define cpu_init(cpu_model) cpu_generic_init(TYPE_SPARC_CPU, cpu_model)
#endif
#define cpu_signal_handler cpu_sparc_signal_handler
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index d5ff188..56512ec 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -126,7 +126,7 @@ static void leon3_generic_hw_init(MachineState *machine)
cpu_model = "LEON3";
}
-cpu = cpu_sparc_init(cpu_model);
+cpu = SPARC_CPU(cpu_generic_init(TYPE_SPARC_CPU, cpu_model));
if (cpu == NULL) {
fprintf(stderr, "qemu: Unable to find Sparc CPU definition\n");
exit(1);
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 89dd8a9..cf47dca 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -797,7 +797,7 @@ static void cpu_devinit(const char *cpu_model, unsigned int
id,
SPARCCPU *cpu;
CPUSPARCState *env;
-cpu = cpu_sparc_init(cpu_model);
+cpu = SPARC_CPU(cpu_generic_init(TYPE_SPARC_CPU, cpu_model));
if (cpu == NULL) {
fprintf(stderr, "qemu: Unable to find Sparc CPU definition\n");
exit(1);
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 4e4fdab..ecf38a4 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -353,7 +353,7 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_model,
if (cpu_model == NULL) {
cpu_model = default_cpu_model;
}
-cpu = cpu_sparc_init(cpu_model);
+cpu = SPARC_CPU(cpu_generic_init(TYPE_SPARC_CPU, cpu_model));
if (cpu == NULL) {
fprintf(stderr, "Unable to find Sparc CPU definition\n");
exit(1);
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index fd01cbf..2917021 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -110,11 +110,6 @@ static void sparc_cpu_parse_features(const char *typename,
char *features,
cpu_legacy_parse_featurestr(typename, features, errp);
}
-SPARCCPU *cpu_sparc_init(const char *cpu_model)
-{
-return SPARC_CPU(cpu_generic_init(TYPE_SPARC_CPU, cpu_model));
-}
-
void cpu_sparc_set_id(CPUSPARCState *env, unsigned int cpu)
{
#if !defined(TARGET_SPARC64)
Re: [Qemu-devel] Help with Windows NT 4.0
> On Aug 18, 2017, at 3:23 PM, John Snow wrote:
>
>
>
> On 08/18/2017 09:36 AM, Programmingkid wrote:
>>
>>> On Aug 18, 2017, at 4:46 AM, Artyom Tarasenko wrote:
>>>
>>> On Fri, Aug 18, 2017 at 6:31 AM, Programmingkid
>>> wrote:
> On Aug 15, 2017, at 6:27 PM, Paolo Bonzini wrote:
>
> On 15/08/2017 20:46, Programmingkid wrote:
>>
>>> On Aug 14, 2017, at 2:51 AM, Paolo Bonzini wrote:
>>>
>>> On 13/08/2017 21:13, Programmingkid wrote:
Lately I found out that Windows NT 4.0 seems to work well with the
486 and pentium processors. Using "-cpu 486" made installing it
actually work. Now I am seeing another issue. When I boot Windows NT
4.0 I see this error message:
*** STOP: 0x007B (0x807A8610,0x,0x,0x)
INACESSIBLE_BOOT_DEVICE
Would anyone know a way to solve this issue?
>>>
>>> Hervé is probably the best person to answer this question. Maybe try
>>> installing it with SCSI disks ("-drive if=scsi,id=hd,file=... -drive
>>> if=scsi,id=cd,file=... -device lsi -device scsi-hd,drive=hd -device
>>> scsi-cd,drive=cd").
>>>
>>> Thanks,
>>>
>>> Paolo
>>
>> Thanks for the help. Unfortunately trying to boot from the install CD
>> leads to the INACCESSIBLE_BOOT_DEVICE error when using SCSI.
>
> Try with 0.12.
>
> Paolo
I finally figured out why I was seeing the INACESSIBLE_BOOT_DEVICE error.
It was because of the qcow2 image format. As soon as I switched to the
qcow format, the error disappeared.
>>>
>>> That's weird. The image format is not guest visible. Probably you are
>>> hitting some sort of timing issue.
>>>
>>>
>>> --
>>> Regards,
>>> Artyom Tarasenko
>>>
>>> SPARC and PPC PReP under qemu blog:
>>> http://tyom.blogspot.com/search/label/qemu
>>
>> I'm not sure how timing is involved but I'm pretty sure I've heard about
>> guests having problems with the qcow2 format in the past.
>>
>
> If the qcow2 driver in QEMU is written correctly, it should be 100%
> invisible to guests. There should be no actual way to determine in any
> way that qcow2 is being used.
That is what I thought.
> Either the qcow2 driver is broken (unlikely), or the way in which the
> qcow2 driver is written is exposing race conditions or other timing
> problems in either the qcow2 driver, the guest, or both.
Interesting idea.
>
> The odds of this being a "qcow2 problem" are pretty slim.
>
> --js
To anyone who wishes to recreate this test here is what you need to do.
Download this iso:
https://archive.org/download/Microsoft_Windows_NT_Workstation_4.0_Microsoft_1996/Microsoft%20Windows%20NT%20Workstation%20(4.0)%20(Microsoft)%20(1996).iso
Create the qcow hard drive image file:
qemu-img create -f qcow qcowtest.qcow 4G
Create the qcow2 hard drive image file:
qemu-img create -f qcow2 qcow2test.qcow2 4G
Install Windows NT 4.0 for each hard drive image file type:
qemu-system-i386 -cpu pentium -vga cirrus -hda qcowtest.qcow -boot c
-cdrom
qemu-system-i386 -cpu pentium -vga cirrus -hda qcow2test.qcow2 -boot c
-cdrom
The actual installation for Windows NT 4.0 only takes about 5 minutes to do, so
it should be an easy test to do. Once done installing and setting up Windows,
try quitting and relaunching QEMU for each image format.
Windows NT Workstation 4.0 CD-KEY: 30495-0006276-08164
Please let us know your results.
Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH 09/12] ppc440: Add emulation of plb-pcix controller found in some 440 SoCs
hi Balaton, > I know about this and if you read the cover message (RFC PATCH 00/12) I link > to a fixed version of that U-Boot firmware (rebuilt from source with the > mentioned patches) which works with this emulation. (The original one from > the updater also starts but cannot boot due to some problems which are fixed > by these patches in my image. This is described in the cover message.) With > that fixed firmware image it should be possible to at least try booting > AmigaOS on the emulation and get some logs. I'd be surprised if it also > worked at this point but I could not try it as it needs an appropriate > AmigaOS version that runs on the Sam460. It should produce some logs though > with -serial stdio which may help finding what is missing. i have sam460 amigaos cdrom iso as my backup and can test it inside qemu. sharing the logs if you needed. > I think it would not be possible to use the U-Boot in QEMU now as that is for > e500 CPU but this is not needed either. See above, we are aiming to emulate > enough of the board that it can run the original firmware which should be > able to boot these Amiga like OSes and normal Linux images used on the > Sam460ex. This already works but things fail after or during boot currently. > This is what needs to be debugged. So we'd need someone who has a Sam460 (ex > or cr) board and can test on that to get logs from OSes on real hardware for > comparison. about real hardware no problem i will ask inside amigan community if someone have the opportunity to share the serial debug of sam 460 runinng amigaos. can i ask you something ? why you dont try to integrate in qemu the pegasos 2 or the efika machine. i have the feeling that probably it can be more simple because more old machine and components. bye luigi
Re: [Qemu-devel] [PATCH v1 2/2] Convert remaining single line fprintf() to warn_report()
On 08/18/2017 03:47 PM, Alistair Francis wrote:
Convert any remaining uses of fprintf(stderr, "warning:"...
to use warn_report() instead. This helps standardise on a single
method of printing warnings to the user.
All of the warnings were changed using this command:
find ./* -type f -exec sed -i 's|fprintf(.*".*warning[,:] |warn_report("|Ig'
{} +
The #include lines and chagnes to the test Makefile were manually
line, changes
updated to allow the code to compile.
Signed-off-by: Alistair Francis
Reviewed-by: Philippe Mathieu-Daudé
---
This was split out of my original series fixing QEMU warnings.
tests/Makefile.include | 2 +-
util/cutils.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 80527a8763..91d5a4544f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -584,7 +584,7 @@ tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
$(test-crypto-obj-y)
tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o
migration/page_cache.o $(test-util-obj-y)
-tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
+tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
$(test-util-obj-y)
tests/test-int128$(EXESUF): tests/test-int128.o
tests/rcutorture$(EXESUF): tests/rcutorture.o $(test-util-obj-y)
tests/test-rcu-list$(EXESUF): tests/test-rcu-list.o $(test-util-obj-y)
diff --git a/util/cutils.c b/util/cutils.c
index 1534682083..b33ede83d1 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -30,6 +30,7 @@
#include "qemu/iov.h"
#include "net/net.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
void strpadcpy(char *buf, int buf_size, const char *str, char pad)
{
@@ -601,7 +602,7 @@ int parse_debug_env(const char *name, int max, int initial)
return initial;
}
if (debug < 0 || debug > max || errno != 0) {
-fprintf(stderr, "warning: %s not in [0, %d]", name, max);
+warn_report("%s not in [0, %d]", name, max);
return initial;
}
return debug;
Re: [Qemu-devel] [PATCH v1 1/2] Makefile: Remove libqemustub.a
Hi Alistair, On 08/18/2017 03:47 PM, Alistair Francis wrote: Using two libraries (libqemuutil.a and libqemustub.a) would sometimes result in circular dependencies. To avoid these issues let's just combine both into a single library that functions as both. Signed-off-by: Alistair Francis --- Makefile| 7 +++ Makefile.target | 2 +- docs/devel/build-system.txt | 14 +- tests/Makefile.include | 6 +++--- 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 81447b1f08..f111e93c63 100644 --- a/Makefile +++ b/Makefile @@ -344,7 +344,7 @@ subdir-dtc:dtc/libfdt dtc/tests dtc/%: mkdir -p $@ -$(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) $(chardev-obj-y) \ +$(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \ $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS)) @@ -364,12 +364,11 @@ Makefile: $(version-obj-y) ## # Build libraries -libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) $(trace-obj-y) +libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y) ## -COMMON_LDADDS = libqemuutil.a libqemustub.a +COMMON_LDADDS = libqemuutil.a qemu-img.o: qemu-img-cmds.h diff --git a/Makefile.target b/Makefile.target index 7f42c45db8..0a80caf79c 100644 --- a/Makefile.target +++ b/Makefile.target @@ -193,7 +193,7 @@ all-obj-$(CONFIG_SOFTMMU) += $(io-obj-y) $(QEMU_PROG_BUILD): config-devices.mak -COMMON_LDADDS = ../libqemuutil.a ../libqemustub.a +COMMON_LDADDS = ../libqemuutil.a # build either PROG or PROGW $(QEMU_PROG_BUILD): $(all-obj-y) $(COMMON_LDADDS) diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt index 2af1e668c5..5f32e53248 100644 --- a/docs/devel/build-system.txt +++ b/docs/devel/build-system.txt @@ -232,15 +232,11 @@ The utility code that is used by all binaries is built into a static archive called libqemuutil.a, which is then linked to all the binaries. In order to provide hooks that are only needed by some of the binaries, code in libqemuutil.a may depend on other functions that are -not fully implemented by all QEMU binaries. To deal with this there is a -second library called libqemustub.a which provides dummy stubs for all -these functions. These will get lazy linked into the binary if the real -implementation is not present. In this way, the libqemustub.a static -library can be thought of as a portable implementation of the weak -symbols concept. All binaries should link to both libqemuutil.a and -libqemustub.a. e.g. - - qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a libqemustub.a +not fully implemented by all QEMU binaries. To deal with this there are +dummy stubs for all these functions in libqemuutil.a. +All binaries should link to both libqemuutil.a and libqemustub.a. e.g. still libqemustub.a + + qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a changed a bit: The utility code that is used by all binaries is built into a static archive called libqemuutil.a, which is then linked to all the binaries. In order to provide hooks that are only needed by some of the binaries, code in libqemuutil.a may depend on other functions that are not fully implemented by all QEMU binaries. Dummy stubs for all these functions are also provided by this library, and will get lazy linked into the binary if the real implementation is not present. In this way, this static library can be thought of as a portable implementation of the weak symbols concept. All binaries should link to libqemuutil.a. e.g. qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a This or removing "libqemustub.a": Reviewed-by: Philippe Mathieu-Daudé Windows platform portability diff --git a/tests/Makefile.include b/tests/Makefile.include index 37c1bed683..80527a8763 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -553,7 +553,7 @@ QEMU_CFLAGS += -I$(SRC_PATH)/tests # Deps that are common to various different sets of tests below -test-util-obj-y = libqemuutil.a libqemustub.a +test-util-obj-y = libqemuutil.a test-qom-obj-y = $(qom-obj-y) $(test-util-obj-y) test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \ tests/test-qapi-event.o tests/test-qmp-introspect.o \ @@ -608,8 +608,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \ $(test-io-obj-y) tests/test-timed-average$(EXESUF): tests/test-timed-average.o $(test-util-obj-y) tests/test-base64$(EXESUF): tests/test-base64.o \ - libqemuutil.a libqemustub.a -tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o libqemustub.a + libqemuutil.a +tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o $(test-util-obj-y)
Re: [Qemu-devel] Help with Windows NT 4.0
On 08/18/2017 09:36 AM, Programmingkid wrote:
>
>> On Aug 18, 2017, at 4:46 AM, Artyom Tarasenko wrote:
>>
>> On Fri, Aug 18, 2017 at 6:31 AM, Programmingkid
>> wrote:
>>>
On Aug 15, 2017, at 6:27 PM, Paolo Bonzini wrote:
On 15/08/2017 20:46, Programmingkid wrote:
>
>> On Aug 14, 2017, at 2:51 AM, Paolo Bonzini wrote:
>>
>> On 13/08/2017 21:13, Programmingkid wrote:
>>> Lately I found out that Windows NT 4.0 seems to work well with the
>>> 486 and pentium processors. Using "-cpu 486" made installing it
>>> actually work. Now I am seeing another issue. When I boot Windows NT
>>> 4.0 I see this error message:
>>>
>>> *** STOP: 0x007B (0x807A8610,0x,0x,0x)
>>> INACESSIBLE_BOOT_DEVICE
>>>
>>> Would anyone know a way to solve this issue?
>>
>> Hervé is probably the best person to answer this question. Maybe try
>> installing it with SCSI disks ("-drive if=scsi,id=hd,file=... -drive
>> if=scsi,id=cd,file=... -device lsi -device scsi-hd,drive=hd -device
>> scsi-cd,drive=cd").
>>
>> Thanks,
>>
>> Paolo
>
> Thanks for the help. Unfortunately trying to boot from the install CD
> leads to the INACCESSIBLE_BOOT_DEVICE error when using SCSI.
Try with 0.12.
Paolo
>>>
>>> I finally figured out why I was seeing the INACESSIBLE_BOOT_DEVICE error.
>>> It was because of the qcow2 image format. As soon as I switched to the qcow
>>> format, the error disappeared.
>>
>> That's weird. The image format is not guest visible. Probably you are
>> hitting some sort of timing issue.
>>
>>
>> --
>> Regards,
>> Artyom Tarasenko
>>
>> SPARC and PPC PReP under qemu blog:
>> http://tyom.blogspot.com/search/label/qemu
>
> I'm not sure how timing is involved but I'm pretty sure I've heard about
> guests having problems with the qcow2 format in the past.
>
If the qcow2 driver in QEMU is written correctly, it should be 100%
invisible to guests. There should be no actual way to determine in any
way that qcow2 is being used.
Either the qcow2 driver is broken (unlikely), or the way in which the
qcow2 driver is written is exposing race conditions or other timing
problems in either the qcow2 driver, the guest, or both.
The odds of this being a "qcow2 problem" are pretty slim.
--js
Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
On Fri, Aug 18, 2017 at 09:35:00PM +0300, Michael S. Tsirkin wrote: > On Wed, Aug 16, 2017 at 09:45:35AM +0800, Dou Liyang wrote: > > Currently, Using the fisrt node without memory on the machine makes > > QEMU unhappy. With this example command line: > > ... \ > > -m 1024M,slots=4,maxmem=32G \ > > -numa node,nodeid=0 \ > > -numa node,mem=1024M,nodeid=1 \ > > -numa node,nodeid=2 \ > > -numa node,nodeid=3 \ > > Guest reports "No NUMA configuration found" and the NUMA topology is > > wrong. > > > > This is because when QEMU builds ACPI SRAT, it regards node0 as the > > default node to deal with the memory hole(640K-1M). this means the > > node0 must have some memory(>1M), but, actually it can have no > > memory. > > > > Fix this problem by replace the node0 with the first node which has > > memory on it. Add a new function for each node. Also do some cleanup. > > > > Signed-off-by: Dou Liyang > > This isn't a regression, is it? If it isn't, it's not a 2.10 candidate > IMHO. As noted in another reply to v2, I agree and I'm treating it as a 2.11 candidate. -- Eduardo
Re: [Qemu-devel] [RFC 25/29] vhu: enable = false on get_vring_base
* Michael S. Tsirkin (m...@redhat.com) wrote:
> On Wed, Jun 28, 2017 at 08:00:43PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert"
> >
> > When we receive a GET_VRING_BASE message set enable = false
> > to stop any new received packets modifying the ring.
> >
> > Signed-off-by: Dr. David Alan Gilbert
>
> I think I already reviewed a similar patch.
> Spec says:
> Client must only process each ring when it is started.
>
> IMHO the real fix is to fix client to check the started
> flag before processing the ring.
Done, I added a vu_queue_started to match vu_queue_enabled
and then used it.
Dave
> > ---
> > contrib/libvhost-user/libvhost-user.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/contrib/libvhost-user/libvhost-user.c
> > b/contrib/libvhost-user/libvhost-user.c
> > index ceddeac74f..d37052b7b0 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -652,6 +652,7 @@ vu_get_vring_base_exec(VuDev *dev, VhostUserMsg *vmsg)
> > vmsg->size = sizeof(vmsg->payload.state);
> >
> > dev->vq[index].started = false;
> > +dev->vq[index].enable = false;
> > if (dev->iface->queue_set_started) {
> > dev->iface->queue_set_started(dev, index, false);
> > }
> > --
> > 2.13.0
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2] hw/acpi: Select an node with memory for mapping memory hole to
On Fri, Aug 18, 2017 at 09:28:55PM +0300, Michael S. Tsirkin wrote: > On Wed, Aug 16, 2017 at 09:26:51AM +0800, Dou Liyang wrote: > > Currently, Using the fisrt node without memory on the machine makes > > QEMU unhappy. With this example command line: > > ... \ > > -m 1024M,slots=4,maxmem=32G \ > > -numa node,nodeid=0 \ > > -numa node,mem=1024M,nodeid=1 \ > > -numa node,nodeid=2 \ > > -numa node,nodeid=3 \ > > Guest reports "No NUMA configuration found" and the NUMA topology is > > wrong. > > > > This is because when QEMU builds ACPI SRAT, it regards node0 as the > > default node to deal with the memory hole(640K-1M). this means the > > node0 must have some memory(>1M), but, actually it can have no > > memory. > > > > Fix this problem by replace the node0 with the first node which has > > memory on it. Add a new function for each node. Also do some cleanup. > > > > Signed-off-by: Dou Liyang > > This isn't a regression, is it? > If so I think it's safe to postpone this to 2.11. Agreed. I was already treating it as a candidate for 2.11 only. -- Eduardo
Re: [Qemu-devel] [PATCH for-2.10] numa: Move numa_legacy_auto_assign_ram to pc-i440fx-2.9
On Fri, Aug 18, 2017 at 04:09:43PM -0300, Eduardo Habkost wrote:
> The 'm->numa_auto_assign_ram = numa_legacy_auto_assign_ram;' line
> was supposed to be in pc_i440fx_2_9_machine_options() (see commit
> 3bfe5716 "numa: equally distribute memory on nodes"), but the
> merge commit adb354dd ("Merge remote-tracking branch
> 'mst/tags/for_upstream' into staging") moved it to the
> pc_i440fx_2_10_machine_options().
>
> Move the line back to pc_i440fx_2_9_machine_options().
>
> Signed-off-by: Eduardo Habkost
I just re-read the schedule wiki page and noticed 2017-08-22 is
"release _or_ tag -rc4". Does this mean including this patch
would slip the schedule for 1 week?
In that case, I don't think this patch should block the release
and cause a schedule slip. We can simply change the NUMA RAM
assignment algorithm in pc-2.11 and keep the old one in pc-2.10.
--
Eduardo
[Qemu-devel] [PATCH for-2.10] numa: Move numa_legacy_auto_assign_ram to pc-i440fx-2.9
The 'm->numa_auto_assign_ram = numa_legacy_auto_assign_ram;' line
was supposed to be in pc_i440fx_2_9_machine_options() (see commit
3bfe5716 "numa: equally distribute memory on nodes"), but the
merge commit adb354dd ("Merge remote-tracking branch
'mst/tags/for_upstream' into staging") moved it to the
pc_i440fx_2_10_machine_options().
Move the line back to pc_i440fx_2_9_machine_options().
Signed-off-by: Eduardo Habkost
---
hw/i386/pc_piix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 11b4336..46dfd2c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -441,7 +441,6 @@ static void pc_i440fx_2_10_machine_options(MachineClass *m)
pc_i440fx_machine_options(m);
m->alias = "pc";
m->is_default = 1;
-m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
}
DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
@@ -453,6 +452,7 @@ static void pc_i440fx_2_9_machine_options(MachineClass *m)
m->is_default = 0;
m->alias = NULL;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
+m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
}
DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
--
2.9.4
Re: [Qemu-devel] [PATCH v3 1/2] pc: add 2.11 machine type
On Tue, Aug 15, 2017 at 08:06:18PM -0400, Lan Tianyu wrote:
> Signed-off-by: Lan Tianyu
> ---
> hw/i386/pc_piix.c| 14 +++---
> hw/i386/pc_q35.c | 14 +++---
> include/hw/compat.h | 3 +++
> include/hw/i386/pc.h | 3 +++
> 4 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 11b4336..e99ec8c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -436,11 +436,21 @@ static void pc_i440fx_machine_options(MachineClass *m)
> m->default_display = "std";
> }
>
> -static void pc_i440fx_2_10_machine_options(MachineClass *m)
> +static void pc_i440fx_2_11_machine_options(MachineClass *m)
> {
> pc_i440fx_machine_options(m);
> m->alias = "pc";
> m->is_default = 1;
> +}
> +
> +DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
> + pc_i440fx_2_11_machine_options);
> +
> +static void pc_i440fx_2_10_machine_options(MachineClass *m)
> +{
> +pc_i440fx_machine_options(m);
> +m->is_default = 0;
> +m->alias = NULL;
> m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
Interesting, this was supposed to be in
pc_i440fx_2_9_machine_options() (see commit 3bfe5716 "numa:
equally distribute memory on nodes"), but the merge commit
adb354dd ("Merge remote-tracking branch 'mst/tags/for_upstream'
into staging") moved it to the pc_i440fx_2_10_machine_options().
I will submit a fix for -rc4 ASAP.
--
Eduardo
Re: [Qemu-devel] [PATCH for-2.11 01/27] sparc: convert cpu models to SPARC cpu subclasses
Hi Igor,
On 08/18/2017 07:08 AM, Igor Mammedov wrote:
QOMfy cpu models handling introducing propper cpu types
for each cpu model.
Signed-off-by: Igor Mammedov
---
with this and conversion of features to properties,
it would be possible to replace cpu_sparc_init() with
cpu_generic_init() and reuse common -cpu handling
infrastructure.
CC: Mark Cave-Ayland
CC: Artyom Tarasenko
CC: Philippe Mathieu-Daudé
v2:
* make base class abstract (Philippe Mathieu-Daudé )
---
target/sparc/cpu-qom.h | 2 +
target/sparc/cpu.c | 121 +
2 files changed, 84 insertions(+), 39 deletions(-)
diff --git a/target/sparc/cpu-qom.h b/target/sparc/cpu-qom.h
index f63af72..af6d57a 100644
--- a/target/sparc/cpu-qom.h
+++ b/target/sparc/cpu-qom.h
@@ -35,6 +35,7 @@
#define SPARC_CPU_GET_CLASS(obj) \
OBJECT_GET_CLASS(SPARCCPUClass, (obj), TYPE_SPARC_CPU)
+typedef struct sparc_def_t sparc_def_t;
/**
* SPARCCPUClass:
* @parent_realize: The parent class' realize handler.
@@ -49,6 +50,7 @@ typedef struct SPARCCPUClass {
DeviceRealize parent_realize;
void (*parent_reset)(CPUState *cpu);
+sparc_def_t *cpu_def;
} SPARCCPUClass;
typedef struct SPARCCPU SPARCCPU;
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index d606eb5..2994c09 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -25,8 +25,6 @@
//#define DEBUG_FEATURES
-static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model);
-
/* CPUClass::reset() */
static void sparc_cpu_reset(CPUState *s)
{
@@ -111,17 +109,9 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char
*cpu_model)
{
CPUSPARCState *env = &cpu->env;
char *s = g_strdup(cpu_model);
-char *featurestr, *name = strtok(s, ",");
-sparc_def_t def1, *def = &def1;
+char *featurestr = strtok(s, ",");
Error *err = NULL;
-if (cpu_sparc_find_by_name(def, name) < 0) {
-g_free(s);
-return -1;
-}
-
-env->def = g_memdup(def, sizeof(*def));
-
featurestr = strtok(NULL, ",");
sparc_cpu_parse_features(CPU(cpu), featurestr, &err);
g_free(s);
@@ -130,18 +120,18 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char
*cpu_model)
return -1;
}
-env->version = def->iu_version;
-env->fsr = def->fpu_version;
-env->nwindows = def->nwindows;
+env->version = env->def->iu_version;
+env->fsr = env->def->fpu_version;
+env->nwindows = env->def->nwindows;
#if !defined(TARGET_SPARC64)
-env->mmuregs[0] |= def->mmu_version;
+env->mmuregs[0] |= env->def->mmu_version;
cpu_sparc_set_id(env, 0);
-env->mxccregs[7] |= def->mxcc_version;
+env->mxccregs[7] |= env->def->mxcc_version;
#else
-env->mmu_version = def->mmu_version;
-env->maxtl = def->maxtl;
-env->version |= def->maxtl << 8;
-env->version |= def->nwindows - 1;
+env->mmu_version = env->def->mmu_version;
+env->maxtl = env->def->maxtl;
+env->version |= env->def->maxtl << 8;
+env->version |= env->def->nwindows - 1;
#endif
return 0;
}
@@ -149,8 +139,19 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char
*cpu_model)
SPARCCPU *cpu_sparc_init(const char *cpu_model)
{
SPARCCPU *cpu;
+ObjectClass *oc;
+char *str, *name;
+
+str = g_strdup(cpu_model);
+name = strtok(str, ",");
you can free 'str' once here:
g_free(str);
+oc = cpu_class_by_name(TYPE_SPARC_CPU, name);
+if (oc == NULL) {
+g_free(str);
drop
+return NULL;
+}
+g_free(str);
drop
-cpu = SPARC_CPU(object_new(TYPE_SPARC_CPU));
+cpu = SPARC_CPU(object_new(object_class_get_name(oc)));
if (cpu_sparc_register(cpu, cpu_model) < 0) {
object_unref(OBJECT(cpu));
@@ -553,23 +554,6 @@ static void add_flagname_to_bitmaps(const char *flagname,
uint32_t *features)
error_report("CPU feature %s not found", flagname);
}
-static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *name)
-{
-unsigned int i;
-const sparc_def_t *def = NULL;
-
-for (i = 0; i < ARRAY_SIZE(sparc_defs); i++) {
-if (strcasecmp(name, sparc_defs[i].name) == 0) {
-def = &sparc_defs[i];
-}
-}
-if (!def) {
-return -1;
-}
-memcpy(cpu_def, def, sizeof(*def));
-return 0;
-}
-
static void sparc_cpu_parse_features(CPUState *cs, char *features,
Error **errp)
{
@@ -796,6 +780,36 @@ static bool sparc_cpu_has_work(CPUState *cs)
cpu_interrupts_enabled(env);
}
+static char *sparc_cpu_type_name(const char *cpu_model)
+{
+char *name = g_strdup_printf("%s-" TYPE_SPARC_CPU, cpu_model);
+char *s = name;
+
+/* SPARC cpu model names happen to have whitespaces,
+ * as type names shouldn't have spaces replace them with '-'
+ */
+while ((s = strchr(s, ' '))) {
+*s = '-';
+}
+
+
[Qemu-devel] [PATCH v1 0/2] Remove libqemustub.a in order to improve error
As discussed with Paolo and Markus let's combine libqemustub.a into libqemuutil.a to avoid circular dependencies. Alistair Francis (2): Makefile: Remove libqemustub.a Convert remaining single line fprintf() to warn_report() Makefile| 7 +++ Makefile.target | 2 +- docs/devel/build-system.txt | 14 +- tests/Makefile.include | 8 util/cutils.c | 3 ++- 5 files changed, 15 insertions(+), 19 deletions(-) -- 2.11.0
[Qemu-devel] [PATCH v1 1/2] Makefile: Remove libqemustub.a
Using two libraries (libqemuutil.a and libqemustub.a) would sometimes result in circular dependencies. To avoid these issues let's just combine both into a single library that functions as both. Signed-off-by: Alistair Francis --- Makefile| 7 +++ Makefile.target | 2 +- docs/devel/build-system.txt | 14 +- tests/Makefile.include | 6 +++--- 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/Makefile b/Makefile index 81447b1f08..f111e93c63 100644 --- a/Makefile +++ b/Makefile @@ -344,7 +344,7 @@ subdir-dtc:dtc/libfdt dtc/tests dtc/%: mkdir -p $@ -$(SUBDIR_RULES): libqemuutil.a libqemustub.a $(common-obj-y) $(chardev-obj-y) \ +$(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \ $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) ROMSUBDIR_RULES=$(patsubst %,romsubdir-%, $(ROMS)) @@ -364,12 +364,11 @@ Makefile: $(version-obj-y) ## # Build libraries -libqemustub.a: $(stub-obj-y) -libqemuutil.a: $(util-obj-y) $(trace-obj-y) +libqemuutil.a: $(util-obj-y) $(trace-obj-y) $(stub-obj-y) ## -COMMON_LDADDS = libqemuutil.a libqemustub.a +COMMON_LDADDS = libqemuutil.a qemu-img.o: qemu-img-cmds.h diff --git a/Makefile.target b/Makefile.target index 7f42c45db8..0a80caf79c 100644 --- a/Makefile.target +++ b/Makefile.target @@ -193,7 +193,7 @@ all-obj-$(CONFIG_SOFTMMU) += $(io-obj-y) $(QEMU_PROG_BUILD): config-devices.mak -COMMON_LDADDS = ../libqemuutil.a ../libqemustub.a +COMMON_LDADDS = ../libqemuutil.a # build either PROG or PROGW $(QEMU_PROG_BUILD): $(all-obj-y) $(COMMON_LDADDS) diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt index 2af1e668c5..5f32e53248 100644 --- a/docs/devel/build-system.txt +++ b/docs/devel/build-system.txt @@ -232,15 +232,11 @@ The utility code that is used by all binaries is built into a static archive called libqemuutil.a, which is then linked to all the binaries. In order to provide hooks that are only needed by some of the binaries, code in libqemuutil.a may depend on other functions that are -not fully implemented by all QEMU binaries. To deal with this there is a -second library called libqemustub.a which provides dummy stubs for all -these functions. These will get lazy linked into the binary if the real -implementation is not present. In this way, the libqemustub.a static -library can be thought of as a portable implementation of the weak -symbols concept. All binaries should link to both libqemuutil.a and -libqemustub.a. e.g. - - qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a libqemustub.a +not fully implemented by all QEMU binaries. To deal with this there are +dummy stubs for all these functions in libqemuutil.a. +All binaries should link to both libqemuutil.a and libqemustub.a. e.g. + + qemu-img$(EXESUF): qemu-img.o ..snip.. libqemuutil.a Windows platform portability diff --git a/tests/Makefile.include b/tests/Makefile.include index 37c1bed683..80527a8763 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -553,7 +553,7 @@ QEMU_CFLAGS += -I$(SRC_PATH)/tests # Deps that are common to various different sets of tests below -test-util-obj-y = libqemuutil.a libqemustub.a +test-util-obj-y = libqemuutil.a test-qom-obj-y = $(qom-obj-y) $(test-util-obj-y) test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \ tests/test-qapi-event.o tests/test-qmp-introspect.o \ @@ -608,8 +608,8 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \ $(test-io-obj-y) tests/test-timed-average$(EXESUF): tests/test-timed-average.o $(test-util-obj-y) tests/test-base64$(EXESUF): tests/test-base64.o \ - libqemuutil.a libqemustub.a -tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o libqemustub.a + libqemuutil.a +tests/ptimer-test$(EXESUF): tests/ptimer-test.o tests/ptimer-test-stubs.o hw/core/ptimer.o $(test-util-obj-y) tests/test-logging$(EXESUF): tests/test-logging.o $(test-util-obj-y) -- 2.11.0
[Qemu-devel] [PATCH v1 2/2] Convert remaining single line fprintf() to warn_report()
Convert any remaining uses of fprintf(stderr, "warning:"...
to use warn_report() instead. This helps standardise on a single
method of printing warnings to the user.
All of the warnings were changed using this command:
find ./* -type f -exec sed -i 's|fprintf(.*".*warning[,:] |warn_report("|Ig'
{} +
The #include lines and chagnes to the test Makefile were manually
updated to allow the code to compile.
Signed-off-by: Alistair Francis
---
This was split out of my original series fixing QEMU warnings.
tests/Makefile.include | 2 +-
util/cutils.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 80527a8763..91d5a4544f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -584,7 +584,7 @@ tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
$(test-crypto-obj-y)
tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o
migration/page_cache.o $(test-util-obj-y)
-tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
+tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
$(test-util-obj-y)
tests/test-int128$(EXESUF): tests/test-int128.o
tests/rcutorture$(EXESUF): tests/rcutorture.o $(test-util-obj-y)
tests/test-rcu-list$(EXESUF): tests/test-rcu-list.o $(test-util-obj-y)
diff --git a/util/cutils.c b/util/cutils.c
index 1534682083..b33ede83d1 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -30,6 +30,7 @@
#include "qemu/iov.h"
#include "net/net.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
void strpadcpy(char *buf, int buf_size, const char *str, char pad)
{
@@ -601,7 +602,7 @@ int parse_debug_env(const char *name, int max, int initial)
return initial;
}
if (debug < 0 || debug > max || errno != 0) {
-fprintf(stderr, "warning: %s not in [0, %d]", name, max);
+warn_report("%s not in [0, %d]", name, max);
return initial;
}
return debug;
--
2.11.0
Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
On Wed, Aug 16, 2017 at 09:45:35AM +0800, Dou Liyang wrote:
> Currently, Using the fisrt node without memory on the machine makes
> QEMU unhappy. With this example command line:
> ... \
> -m 1024M,slots=4,maxmem=32G \
> -numa node,nodeid=0 \
> -numa node,mem=1024M,nodeid=1 \
> -numa node,nodeid=2 \
> -numa node,nodeid=3 \
> Guest reports "No NUMA configuration found" and the NUMA topology is
> wrong.
>
> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> default node to deal with the memory hole(640K-1M). this means the
> node0 must have some memory(>1M), but, actually it can have no
> memory.
>
> Fix this problem by replace the node0 with the first node which has
> memory on it. Add a new function for each node. Also do some cleanup.
>
> Signed-off-by: Dou Liyang
This isn't a regression, is it? If it isn't, it's not a 2.10 candidate
IMHO.
> ---
> V3 --> V2
> -Modify the title
> V2 --> V1:
> -Fix a coding style problem
> Replace
> for (node = 0;
> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> node++);
>
> with
> for (node = 0; node < pcms->numa_nodes; node++) {
>if (pcms->node_mem[node] != 0) {
> break;
> }
>
> hw/i386/acpi-build.c | 78
> +---
> 1 file changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..f93d712 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> }
>
> +static uint64_t
> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> +int i, uint64_t mem_base, uint64_t mem_len)
> +{
> +AcpiSratMemoryAffinity *numamem;
> +uint64_t next_base;
> +
> +next_base = mem_base + mem_len;
> +
> +/* Cut out the ACPI_PCI hole */
> +if (mem_base <= pcms->below_4g_mem_size &&
> +next_base > pcms->below_4g_mem_size) {
> +mem_len -= next_base - pcms->below_4g_mem_size;
> +if (mem_len > 0) {
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> +}
> +mem_base = 1ULL << 32;
> +mem_len = next_base - pcms->below_4g_mem_size;
> +next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> +return next_base;
> +}
> +
> static void
> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> {
> AcpiSystemResourceAffinityTable *srat;
> AcpiSratMemoryAffinity *numamem;
>
> -int i;
> +int i, node;
> int srat_start, numa_start, slots;
> -uint64_t mem_len, mem_base, next_base;
> +uint64_t mem_len, mem_base;
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> PCMachineState *pcms = PC_MACHINE(machine);
> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> MachineState *machine)
> /* the memory map is a bit tricky, it contains at least one hole
> * from 640k-1M and possibly another one from 3.5G-4G.
> */
> -next_base = 0;
> +
> numa_start = table_data->len;
>
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> -next_base = 1024 * 1024;
> -for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> -mem_base = next_base;
> -mem_len = pcms->node_mem[i - 1];
> -if (i == 1) {
> -mem_len -= 1024 * 1024;
> +/* get the first node which has memory and map the hole from 640K-1M */
> +for (node = 0; node < pcms->numa_nodes; node++) {
> +if (pcms->node_mem[node] != 0) {
> +break;
> }
> -next_base = mem_base + mem_len;
> -
> -/* Cut out the ACPI_PCI hole */
> -if (mem_base <= pcms->below_4g_mem_size &&
> -next_base > pcms->below_4g_mem_size) {
> -mem_len -= next_base - pcms->below_4g_mem_size;
> -if (mem_len > 0) {
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, mem_base, mem_len, i - 1,
> - MEM_AFFINITY_ENABLED);
> -}
> -mem_base = 1ULL << 32;
> -mem_len = next_base - pcms->below_4g_mem_size;
> -next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, 0, 640 * 1024,
Re: [Qemu-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp
On Fri, Aug 18, 2017 at 05:00:18PM +0100, Anthony PERARD wrote: > > > > > > > > > > Clean it up after 2.10. > > > > > > > > > > > So is the v2 good enough or do I need to resend it? > > Do you really need it in 2.10? > > it's only 2 days left till release so unless it's blocker > > I'd wait till after release and do clean fix. > > It mostly means that someone building QEMU 2.10 would be unable to do > PCI passthrough hotplug. But PCI PT works fine when the device is added > before a guest is started. Maybe hotplug can work as well with extra > steps done in the guest to force to probe for new devices. > > So I would say it is not a blocker, and could be added in the known > issue of the release notes? Regressions aren't nice. But risk to working setups isn't nice either. If you can come up with a patch that obviously limits the effect to xen only, I'll merge it or ack for merge through Xen tree. > -- > Anthony PERARD
Re: [Qemu-devel] [PATCH for-2.10 v3 2/3] hw/acpi: Move acpi_set_pci_info to pcihp
On Fri, Aug 18, 2017 at 04:19:57PM +0200, Igor Mammedov wrote: > > > > > > > > Clean it up after 2.10. > > > > > > > > So is the v2 good enough or do I need to resend it? > Do you really need it in 2.10? > it's only 2 days left till release so unless it's blocker > I'd wait till after release and do clean fix. Well it's a regression isn't it? > > > Thanks, > >
Re: [Qemu-devel] [PATCH v2] hw/acpi: Select an node with memory for mapping memory hole to
On Wed, Aug 16, 2017 at 09:26:51AM +0800, Dou Liyang wrote:
> Currently, Using the fisrt node without memory on the machine makes
> QEMU unhappy. With this example command line:
> ... \
> -m 1024M,slots=4,maxmem=32G \
> -numa node,nodeid=0 \
> -numa node,mem=1024M,nodeid=1 \
> -numa node,nodeid=2 \
> -numa node,nodeid=3 \
> Guest reports "No NUMA configuration found" and the NUMA topology is
> wrong.
>
> This is because when QEMU builds ACPI SRAT, it regards node0 as the
> default node to deal with the memory hole(640K-1M). this means the
> node0 must have some memory(>1M), but, actually it can have no
> memory.
>
> Fix this problem by replace the node0 with the first node which has
> memory on it. Add a new function for each node. Also do some cleanup.
>
> Signed-off-by: Dou Liyang
This isn't a regression, is it?
If so I think it's safe to postpone this to 2.11.
> ---
> V2 --> V1:
> -Fix a coding style problem
> Replace
> for (node = 0;
> node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> node++);
>
> with
> for (node = 0; node < pcms->numa_nodes; node++) {
>if (pcms->node_mem[node] != 0) {
> break;
> }
>
> hw/i386/acpi-build.c | 78
> +---
> 1 file changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 98dd424..f93d712 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
> }
>
> +static uint64_t
> +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> +int i, uint64_t mem_base, uint64_t mem_len)
> +{
> +AcpiSratMemoryAffinity *numamem;
> +uint64_t next_base;
> +
> +next_base = mem_base + mem_len;
> +
> +/* Cut out the ACPI_PCI hole */
> +if (mem_base <= pcms->below_4g_mem_size &&
> +next_base > pcms->below_4g_mem_size) {
> +mem_len -= next_base - pcms->below_4g_mem_size;
> +if (mem_len > 0) {
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> +}
> +mem_base = 1ULL << 32;
> +mem_len = next_base - pcms->below_4g_mem_size;
> +next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, mem_base, mem_len, i,
> + MEM_AFFINITY_ENABLED);
> +return next_base;
> +}
> +
> static void
> build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> {
> AcpiSystemResourceAffinityTable *srat;
> AcpiSratMemoryAffinity *numamem;
>
> -int i;
> +int i, node;
> int srat_start, numa_start, slots;
> -uint64_t mem_len, mem_base, next_base;
> +uint64_t mem_len, mem_base;
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> PCMachineState *pcms = PC_MACHINE(machine);
> @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> MachineState *machine)
> /* the memory map is a bit tricky, it contains at least one hole
> * from 640k-1M and possibly another one from 3.5G-4G.
> */
> -next_base = 0;
> +
> numa_start = table_data->len;
>
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> -next_base = 1024 * 1024;
> -for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> -mem_base = next_base;
> -mem_len = pcms->node_mem[i - 1];
> -if (i == 1) {
> -mem_len -= 1024 * 1024;
> +/* get the first node which has memory and map the hole from 640K-1M */
> +for (node = 0; node < pcms->numa_nodes; node++) {
> +if (pcms->node_mem[node] != 0) {
> +break;
> }
> -next_base = mem_base + mem_len;
> -
> -/* Cut out the ACPI_PCI hole */
> -if (mem_base <= pcms->below_4g_mem_size &&
> -next_base > pcms->below_4g_mem_size) {
> -mem_len -= next_base - pcms->below_4g_mem_size;
> -if (mem_len > 0) {
> -numamem = acpi_data_push(table_data, sizeof *numamem);
> -build_srat_memory(numamem, mem_base, mem_len, i - 1,
> - MEM_AFFINITY_ENABLED);
> -}
> -mem_base = 1ULL << 32;
> -mem_len = next_base - pcms->below_4g_mem_size;
> -next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> +}
> +numamem = acpi_data_push(table_data, sizeof *numamem);
> +build_srat_memory(numamem, 0, 640 * 1024, node, MEM_AFFINITY_ENABLED);
Re: [Qemu-devel] [PATCH v14 5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
On Fri, Aug 18, 2017 at 04:41:41PM +0800, Wei Wang wrote:
> On 08/18/2017 10:13 AM, Michael S. Tsirkin wrote:
> > On Thu, Aug 17, 2017 at 11:26:56AM +0800, Wei Wang wrote:
> > > Add a new vq to report hints of guest free pages to the host.
> > Please add some text here explaining the report_free_page_signal
> > thing.
> >
> >
> > I also really think we need some kind of ID in the
> > buffer to do a handshake. whenever id changes you
> > add another outbuf.
>
> Please let me introduce the current design first:
> 1) device put the signal buf to the vq and notify the driver (we need
> a buffer because currently the device can't notify when the vq is empty);
>
> 2) the driver starts the report of free page blocks via inbuf;
>
> 3) the driver adds an the signal buf via outbuf to tell the device all are
> reported.
>
>
> Could you please elaborate more on the usage of ID?
While driver is free to maintain at most one buffer in flight
the design must work with pipelined requests as that
is important for performance.
So host might be able to request the reporting twice.
How does it know what is the report in response to?
If we put an id in request and in response, then that fixes it.
So there's a vq used for requesting free page reports.
driver does add_inbuf( &device->id).
Then when it starts reporting it does
add_outbuf(&device->id)
followed by pages.
Also if device->id changes it knows it should restart
reporting from beginning.
> > > +retry:
> > > + ret = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > + virtqueue_kick(vq);
> > > + if (unlikely(ret == -ENOSPC)) {
> > what if there's another error?
>
> Another error is -EIO, how about disabling the free page report feature?
> (I also saw it isn't handled in many other virtio devices e.g. virtio-net)
>
> > > + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > + goto retry;
> > > + }
> > what is this trickery doing? needs more comments or
> > a simplification.
>
> Just this:
> if the vq is full, blocking wait till an entry gets released, then retry.
> This is the
> final one, which puts the signal buf to the vq to signify the end of the
> report and
> the mm lock is not held here, so it is fine to block.
>
But why do you kick here on failure? I would understand it if you
did not kick when adding pages, as it is I don't understand.
Also pls rewrite this with a for or while loop for clarity.
> >
> >
> > > +}
> > > +
> > > +static void report_free_page(struct work_struct *work)
> > > +{
> > > + struct virtio_balloon *vb;
> > > +
> > > + vb = container_of(work, struct virtio_balloon, report_free_page_work);
> > > + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> > That's a lot of work here. And system_wq documentation says:
> > *
> > * system_wq is the one used by schedule[_delayed]_work[_on]().
> > * Multi-CPU multi-threaded. There are users which expect relatively
> > * short queue flush time. Don't queue works which can run for too
> > * long.
> >
> > You might want to create your own wq, maybe even with WQ_CPU_INTENSIVE.
>
> Thanks for the reminder. If not creating a new wq, how about
> system_unbound_wq?
I don't think that one's freezeable.
> The first round of live migration needs the free pages, in that way we can
> have the
> pages reported to the hypervisor quicker.
The reason people call it *live* migration is because tasks keep
running. If you pin VCPUs with maintainance tasks it becomes pointless.
Maybe we need to set a special wq which will create idle
class threads. Does not seem to be supported but not hard to do.
> >
> > > + report_free_page_completion(vb);
> > So first you get list of pages, then an outbuf telling you
> > what they are in end of. I think it's backwards.
> > Add an outbuf first followed by inbufs that tell you
> > what they are.
>
>
> If we have the signal filled with those flags like
> VIRTIO_BALLOON_F_FREE_PAGE_REPORT_START,
> Probably not necessary to have an inbuf followed by an outbuf, right?
>
>
> Best,
> Wei
You really should document the messages in the commit log
and in the header.
--
MST
Re: [Qemu-devel] [PATCH for-2.11 03/27] sparc: convert cpu features to qdev properties
On Fri, Aug 18, 2017 at 12:08:35PM +0200, Igor Mammedov wrote:
> SPARC is the last target that uses legacy way of parsing
> and initializing cpu features, drop legacy approach and
> convert features to properties so that SPARC could as minimum
> benefit from generic cpu_generic_init(), common with
> x86 +-feat parser
>
> PS:
> the main purpose is to remove legacy way of cpu creation as
> a blocker for unifying cpu creation code across targets.
>
> Signed-off-by: Igor Mammedov
[...]
> ---
> CC: Mark Cave-Ayland
> CC: Artyom Tarasenko
> CC: Eduardo Habkost
> ---
> target/sparc/cpu.c | 66
> ++
> 1 file changed, 66 insertions(+)
>
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index f4e7343..e735d73 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -22,6 +22,8 @@
> #include "cpu.h"
> #include "qemu/error-report.h"
> #include "exec/exec-all.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/visitor.h"
>
> //#define DEBUG_FEATURES
>
> @@ -853,6 +855,69 @@ static void sparc_cpu_initfn(Object *obj)
> }
> }
>
> +static void sparc_get_nwindows(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +SPARCCPU *cpu = SPARC_CPU(obj);
> +int64_t value = cpu->env.def.nwindows;
> +
> +visit_type_int(v, name, &value, errp);
> +}
> +
> +static void sparc_set_nwindows(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> +const int64_t min = MIN_NWINDOWS;
> +const int64_t max = MAX_NWINDOWS;
> +SPARCCPU *cpu = SPARC_CPU(obj);
> +Error *err = NULL;
> +int64_t value;
> +
> +visit_type_int(v, name, &value, &err);
> +if (err) {
> +error_propagate(errp, err);
> +return;
> +}
> +
> +if (value < min || value > max) {
> +error_setg(errp, "Property %s.%s doesn't take value %" PRId64
> + " (minimum: %" PRId64 ", maximum: %" PRId64 ")",
> + object_get_typename(obj), name ? name : "null",
> + value, min, max);
> +return;
> +}
> +cpu->env.def.nwindows = value;
I think it would be much simpler to just validate nwindows at
realize time and use DEFINE_PROP_UINT32 below, but I won't mind
if you really prefer the custom setter.
But I have another question:
> +}
> +
> +static PropertyInfo qdev_prop_nwindows = {
> +.name = "int",
> +.get = sparc_get_nwindows,
> +.set = sparc_set_nwindows,
> +};
> +
> +static Property sparc_cpu_properties[] = {
> +DEFINE_PROP_BIT("float",SPARCCPU, env.def.features, 0, false),
> +DEFINE_PROP_BIT("float128", SPARCCPU, env.def.features, 1, false),
> +DEFINE_PROP_BIT("swap", SPARCCPU, env.def.features, 2, false),
> +DEFINE_PROP_BIT("mul", SPARCCPU, env.def.features, 3, false),
> +DEFINE_PROP_BIT("div", SPARCCPU, env.def.features, 4, false),
> +DEFINE_PROP_BIT("flush",SPARCCPU, env.def.features, 5, false),
> +DEFINE_PROP_BIT("fsqrt",SPARCCPU, env.def.features, 6, false),
> +DEFINE_PROP_BIT("fmul", SPARCCPU, env.def.features, 7, false),
> +DEFINE_PROP_BIT("vis1", SPARCCPU, env.def.features, 8, false),
> +DEFINE_PROP_BIT("vis2", SPARCCPU, env.def.features, 9, false),
> +DEFINE_PROP_BIT("fsmuld", SPARCCPU, env.def.features, 10, false),
> +DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false),
> +DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false),
> +DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false),
> +DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0,
> + qdev_prop_uint64, target_ulong),
> +DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0),
> +DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0),
> +{ .name = "nwindows", .info = &qdev_prop_nwindows },
What's the advantage of using a custom PropertyInfo struct if you
can just call object_class_property_add() at
sparc_cpu_class_init()?
> +DEFINE_PROP_END_OF_LIST()
> +};
> +
> static void sparc_cpu_class_init(ObjectClass *oc, void *data)
> {
> SPARCCPUClass *scc = SPARC_CPU_CLASS(oc);
> @@ -861,6 +926,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void
> *data)
>
> scc->parent_realize = dc->realize;
> dc->realize = sparc_cpu_realizefn;
> +dc->props = sparc_cpu_properties;
>
> scc->parent_reset = cc->reset;
> cc->reset = sparc_cpu_reset;
> --
> 2.7.4
>
>
--
Eduardo
Re: [Qemu-devel] [PATCH 1/4] net: move CRC32 calculation from compute_mcast_idx() into its own net_crc32() function
On 18/08/17 18:06, Philippe Mathieu-Daudé wrote:
> On 08/18/2017 01:51 PM, Philippe Mathieu-Daudé wrote:
>> On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
>>> Separate out the standard ethernet CRC32 calculation into a new
>>> net_crc32()
>>> function, renaming the constant POLYNOMIAL to POLYNOMIAL_BE to make
>>> it clear
>>> that this is a big-endian CRC32 calculation.
>>>
>>> Then remove the existing implementation from compute_mcast_idx() and
>>> call the
>>> new function in its place.
>>>
>>> Signed-off-by: Mark Cave-Ayland
>>> ---
>>> include/net/net.h |3 ++-
>>> net/net.c | 16 +++-
>>> 2 files changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/net.h b/include/net/net.h
>>> index 1c55a93..586098c 100644
>>> --- a/include/net/net.h
>>> +++ b/include/net/net.h
>>> @@ -227,7 +227,8 @@ NetClientState *net_hub_port_find(int hub_id);
>>> void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
>>> -#define POLYNOMIAL 0x04c11db6
>>> +#define POLYNOMIAL_BE 0x04c11db6
>>> +uint32_t net_crc32(const uint8_t *p, int len);
>>> unsigned compute_mcast_idx(const uint8_t *ep);
>>> #define vmstate_offset_macaddr(_state, _field) \
>>> diff --git a/net/net.c b/net/net.c
>>> index 0e28099..a856907 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -1568,25 +1568,31 @@ int net_client_parse(QemuOptsList *opts_list,
>>> const char *optarg)
>>> /* From FreeBSD */
>>> /* XXX: optimize */
>>> -unsigned compute_mcast_idx(const uint8_t *ep)
>>> +uint32_t net_crc32(const uint8_t *p, int len)
>>
>> imho there is no point in using signed length.
>>
>>> {
>>> uint32_t crc;
>>> int carry, i, j;
>>> uint8_t b;
>>> crc = 0x;
>>> -for (i = 0; i < 6; i++) {
>>> -b = *ep++;
>>> +for (i = 0; i < len; i++) {
>>> +b = *p++;
>>> for (j = 0; j < 8; j++) {
>>> carry = ((crc & 0x8000L) ? 1 : 0) ^ (b & 0x01);
>>> crc <<= 1;
>>> b >>= 1;
>>> if (carry) {
>>> -crc = ((crc ^ POLYNOMIAL) | carry);
>>> +crc = ((crc ^ POLYNOMIAL_BE) | carry);
>>> }
>>> }
>>> }
>>> -return crc >> 26;
>>> +
>>> +return crc;
>>> +}
>>> +
>>> +unsigned compute_mcast_idx(const uint8_t *ep)
>>> +{
>>> +return net_crc32(ep, 6) >> 26;
>>
>> while here let's use ETH_ALEN
>>
>
> reading the next patches, what do you think about:
>
> static inline uint32_t mac_crc32_be(const uint8_t *p)
> {
> return net_crc32_be(ep, ETH_ALEN);
> }
>
> unsigned compute_mcast_idx(const uint8_t *ep)
> {
> return mac_crc32_be(p) >> 26;
> }
Yes, using ETH_ALEN looks like a good idea - I can do that for the next
revision.
As for the naming of the functions, I've followed the kernel convention
that by default the CRC32 functions are big-endian unless they have a
_le suffix. I don't really feel strongly either way, so I'm happy to go
with whatever naming scheme Jason prefers.
ATB,
Mark.
Re: [Qemu-devel] [PATCH v14 5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
On Fri, Aug 18, 2017 at 04:36:06PM +0800, Wei Wang wrote:
> On 08/18/2017 10:28 AM, Michael S. Tsirkin wrote:
> > On Thu, Aug 17, 2017 at 11:26:56AM +0800, Wei Wang wrote:
> > > Add a new vq to report hints of guest free pages to the host.
> > >
> > > Signed-off-by: Wei Wang
> > > Signed-off-by: Liang Li
> > > ---
> > > drivers/virtio/virtio_balloon.c | 167
> > > +++-
> > > include/uapi/linux/virtio_balloon.h | 1 +
> > > 2 files changed, 147 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c
> > > b/drivers/virtio/virtio_balloon.c
> > > index 72041b4..e6755bc 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> > > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > > /* The balloon servicing is delegated to a freezable workqueue.
> > > */
> > > struct work_struct update_balloon_stats_work;
> > > struct work_struct update_balloon_size_work;
> > > + struct work_struct report_free_page_work;
> > > /* Prevent updating balloon when it is being canceled. */
> > > spinlock_t stop_update_lock;
> > > @@ -90,6 +91,13 @@ struct virtio_balloon {
> > > /* Memory statistics */
> > > struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> > > + /*
> > > + * Used by the device and driver to signal each other.
> > > + * device->driver: start the free page report.
> > > + * driver->device: end the free page report.
> > > + */
> > > + __virtio32 report_free_page_signal;
> > > +
> > > /* To register callback in oom notifier call chain */
> > > struct notifier_block nb;
> > > };
> > > @@ -174,6 +182,17 @@ static void send_balloon_page_sg(struct
> > > virtio_balloon *vb,
> > > } while (unlikely(ret == -ENOSPC));
> > > }
> > > +static void send_free_page_sg(struct virtqueue *vq, void *addr, uint32_t
> > > size)
> > > +{
> > > + unsigned int len;
> > > +
> > > + add_one_sg(vq, addr, size);
> > > + virtqueue_kick(vq);
> > > + /* Release entries if there are */
> > > + while (virtqueue_get_buf(vq, &len))
> > > + ;
> > > +}
> > > +
> > > /*
> > >* Send balloon pages in sgs to host. The balloon pages are recorded in
> > > the
> > >* page xbitmap. Each bit in the bitmap corresponds to a page of
> > > PAGE_SIZE.
> > > @@ -511,42 +530,143 @@ static void update_balloon_size_func(struct
> > > work_struct *work)
> > > queue_work(system_freezable_wq, work);
> > > }
> > > +static void virtio_balloon_send_free_pages(void *opaque, unsigned long
> > > pfn,
> > > +unsigned long nr_pages)
> > > +{
> > > + struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> > > + void *addr = (void *)pfn_to_kaddr(pfn);
> > > + uint32_t len = nr_pages << PAGE_SHIFT;
> > > +
> > > + send_free_page_sg(vb->free_page_vq, addr, len);
> > > +}
> > > +
> > > +static void report_free_page_completion(struct virtio_balloon *vb)
> > > +{
> > > + struct virtqueue *vq = vb->free_page_vq;
> > > + struct scatterlist sg;
> > > + unsigned int len;
> > > + int ret;
> > > +
> > > + sg_init_one(&sg, &vb->report_free_page_signal, sizeof(__virtio32));
> > > +retry:
> > > + ret = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > > + virtqueue_kick(vq);
> > > + if (unlikely(ret == -ENOSPC)) {
> > > + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> > > + goto retry;
> > > + }
> > > +}
> > So the annoying thing here is that once this starts going,
> > it will keep sending free pages from the list even if
> > host is no longer interested. There should be a way
> > for host to tell guest "stop" or "start from the beginning".
>
> This can be achieved via two output signal buf here:
> signal_buf_start: filled with VIRTIO_BALLOON_F_FREE_PAGE_REPORT_START
> signal_buf_end: filled with VIRTIO_BALLOON_F_FREE_PAGE_REPORT_END
>
> The device holds both, and can put one of them to the vq and notify.
Do you mean device writes start and end in the buf? then it's an inbuf
not an outbuf.
>
>
> >
> > It's the result of using same vq for guest to host and
> > host to guest communication, and I think it's not a great idea.
> > I'd reuse stats vq for host to guest requests maybe.
> >
>
>
> As we discussed before, we can't have a vq interleave the report of stats
> and free pages.
> The vq will be locked when one command is in use. So, when live migration
> starts, the
> periodically reported stats will be delayed.
> Would this be OK? Or would you
> like to have
> one host to guest vq, and multiple host to guest vqs? That is,
>
> - host to guest:
> CMD_VQ
>
> - guest to host:
> STATS_REPORT_VQ
> FREE_PAGE_VQ
>
>
> Best,
[Qemu-devel] [Bug 1711602] Re: --copy-storage-all failing with qemu 2.10
The 'host doesn't support requested feature' is probably a red-herring in this case The fact it's failing with an IO error but nothing else suggests either: a) it's something closing the socket between the two qemu's b) The I/O error is from storage/NBD Assuming it fails on precopy, I'd look at the qemu_loadvm_state_section_startfull to watch all the device states load. You could also add some debug/tracing in qemu_loadvm_state to see at what point it fails. Dave -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1711602 Title: --copy-storage-all failing with qemu 2.10 Status in QEMU: New Status in libvirt package in Ubuntu: Confirmed Status in qemu package in Ubuntu: Confirmed Bug description: We fixed an issue around disk locking already in regard to qemu-nbd [1], but there still seem to be issues. $ virsh migrate --live --copy-storage-all kvmguest-artful-normal qemu+ssh://10.22.69.196/system error: internal error: qemu unexpectedly closed the monitor: 2017-08-18T12:10:29.800397Z qemu-system-x86_64: -chardev pty,id=charserial0: char device redirected to /dev/pts/0 (label charserial0) 2017-08-18T12:10:48.545776Z qemu-system-x86_64: load of migration failed: Input/output error Source libvirt log for the guest: 2017-08-18 12:09:08.251+: initiating migration 2017-08-18T12:09:08.809023Z qemu-system-x86_64: Unable to read from socket: Connection reset by peer 2017-08-18T12:09:08.809481Z qemu-system-x86_64: Unable to read from socket: Connection reset by peer Target libvirt log for the guest: 2017-08-18T12:09:08.730911Z qemu-system-x86_64: load of migration failed: Input/output error 2017-08-18 12:09:09.010+: shutting down, reason=crashed Given the timing it seems that the actual copy now works (it is busy ~10 seconds on my environment which would be the copy). Also we don't see the old errors we saw before, but afterwards on the actual take-over it fails. Dmesg has no related denials as often apparmor is in the mix. Need to check libvirt logs of source [2] and target [3] in Detail. [1]: https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02200.html [2]: http://paste.ubuntu.com/25339356/ [3]: http://paste.ubuntu.com/25339358/ To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1711602/+subscriptions
Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser
On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote: > Move cpu_model +-feat parsing into a separate file so that it > could be reused later for parsing similar format of sparc target > > Signed-off-by: Igor Mammedov [...] > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 25eefea..30247dc 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -1038,4 +1038,6 @@ extern const struct VMStateDescription > vmstate_cpu_common; > > #define UNASSIGNED_CPU_INDEX -1 > > +void cpu_legacy_parse_featurestr(const char *typename, char *features, > + Error **errp); > #endif > diff --git a/default-configs/i386-bsd-user.mak > b/default-configs/i386-bsd-user.mak > index af1b31a..b28a05f 100644 > --- a/default-configs/i386-bsd-user.mak > +++ b/default-configs/i386-bsd-user.mak > @@ -1 +1,2 @@ > # Default configuration for i386-bsd-user > +CONFIG_LEGACY_CPU_FEATURES=y > diff --git a/default-configs/i386-linux-user.mak > b/default-configs/i386-linux-user.mak > index 8657e68..c136967 100644 > --- a/default-configs/i386-linux-user.mak > +++ b/default-configs/i386-linux-user.mak > @@ -1 +1,2 @@ > # Default configuration for i386-linux-user > +CONFIG_LEGACY_CPU_FEATURES=y > diff --git a/default-configs/i386-softmmu.mak > b/default-configs/i386-softmmu.mak > index d2ab2f6..e3e7c0e 100644 > --- a/default-configs/i386-softmmu.mak > +++ b/default-configs/i386-softmmu.mak > @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y > CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM) > CONFIG_PXB=y > CONFIG_ACPI_VMGENID=y > +CONFIG_LEGACY_CPU_FEATURES=y > diff --git a/default-configs/x86_64-bsd-user.mak > b/default-configs/x86_64-bsd-user.mak > index 73e5d34..952323c 100644 > --- a/default-configs/x86_64-bsd-user.mak > +++ b/default-configs/x86_64-bsd-user.mak > @@ -1 +1,2 @@ > # Default configuration for x86_64-bsd-user > +CONFIG_LEGACY_CPU_FEATURES=y > diff --git a/default-configs/x86_64-linux-user.mak > b/default-configs/x86_64-linux-user.mak > index bec1d9e..b513ef2 100644 > --- a/default-configs/x86_64-linux-user.mak > +++ b/default-configs/x86_64-linux-user.mak > @@ -1 +1,2 @@ > # Default configuration for x86_64-linux-user > +CONFIG_LEGACY_CPU_FEATURES=y > diff --git a/default-configs/x86_64-softmmu.mak > b/default-configs/x86_64-softmmu.mak > index 9bde2f1..6594ddf 100644 > --- a/default-configs/x86_64-softmmu.mak > +++ b/default-configs/x86_64-softmmu.mak > @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y > CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM) > CONFIG_PXB=y > CONFIG_ACPI_VMGENID=y > +CONFIG_LEGACY_CPU_FEATURES=y [...] > diff --git a/util/Makefile.objs b/util/Makefile.objs > index 50a55ec..14e28f7 100644 > --- a/util/Makefile.objs > +++ b/util/Makefile.objs > @@ -45,3 +45,4 @@ util-obj-y += qht.o > util-obj-y += range.o > util-obj-y += stats64.o > util-obj-y += systemd.o > +util-obj-$(CONFIG_LEGACY_CPU_FEATURES) += legacy_cpu_features_parser.o Do we really need this? Isn't it easier to just tell the linker to drop the object file if it's unused? -- Eduardo
[Qemu-devel] [PATCH] .dir-locals.el: Explicitly set indentation level
At least in some configurations, setting c-file-style is not enough to conform to the QEMU coding style, so explicitly set c-basic-offset as well. Signed-off-by: Thiago Jung Bauermann --- My emacs was using indentation level of 8 spaces and this patch convinced it to use the correct value. I set c-basic-offset set to tab-width in my ~/.spacemacs which perhaps isn't the wisest thing to do, but since there's a .dir-locals.el we can make the editor always do the right thing. .dir-locals.el | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.dir-locals.el b/.dir-locals.el index 3ac0cfc6f0d6..8e47418c5996 100644 --- a/.dir-locals.el +++ b/.dir-locals.el @@ -1,2 +1,3 @@ ((c-mode . ((c-file-style . "stroustrup") - (indent-tabs-mode . nil + (indent-tabs-mode . nil) + (c-basic-offset . 4
Re: [Qemu-devel] [PATCH 1/2] target-i386: cpu: convert plus/minus properties to global properties
On Thu, Aug 17, 2017 at 04:07:56PM +0200, Igor Mammedov wrote:
> Since
> (commit d4a606b3 i386: Don't override -cpu options on -cpu host/max)
> it became possible to delete hack where it was necessary to
> postpone applying plus/minus features to realize time
> after max_features were applied to keep legacy +-feat
> override semantics.
>
> With above commit it's possible to convert +-feat to a set
> of GlobalProperty items and keep +-feat override semantics,
> these properties should be added to global list at the end
> to override properties that were set with feat=on|off syntax.
>
> Signed-off-by: Igor Mammedov
[...]
> +/* Parse "+feature,-feature,feature=foo" CPU feature string */
> static void x86_cpu_parse_featurestr(const char *typename, char *features,
> Error **errp)
> {
> +/* Compatibily hack to maintain legacy +-feat semantic,
> + * where +-feat overwrites any feature set by
> + * feat=on|feat even if the later is parsed after +-feat
> + * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> + */
> +GList *l, *plus_features = NULL, *minus_features = NULL;
The warning about ambiguous CPU options exists since 2.8, I think
this is a good opportunity to get rid of the "[+-]feat overrides
feat=on|off" rule and simplify the parsing code. Do you want to
do this in the same patch?
> char *featurestr; /* Single 'key=value" string being parsed */
> static bool cpu_globals_initialized;
> bool ambiguous = false;
> @@ -2095,7 +2104,6 @@ static void x86_cpu_parse_featurestr(const char
> *typename, char *features,
> const char *val = NULL;
> char *eq = NULL;
> char num[32];
> -GlobalProperty *prop;
>
> /* Compatibility syntax: */
> if (featurestr[0] == '+') {
> @@ -2147,21 +2155,32 @@ static void x86_cpu_parse_featurestr(const char
> *typename, char *features,
> name = "tsc-frequency";
> }
>
> -prop = g_new0(typeof(*prop), 1);
> -prop->driver = typename;
> -prop->property = g_strdup(name);
> -prop->value = g_strdup(val);
> -prop->errp = &error_fatal;
> -qdev_prop_register_global(prop);
> +cpu_add_feat_as_prop(typename, name, val);
> }
>
> if (ambiguous) {
> warn_report("Compatibility of ambiguous CPU model "
> "strings won't be kept on future QEMU versions");
> }
> +
> +for (l = plus_features; l; l = l->next) {
> +const char *name = l->data;
> +cpu_add_feat_as_prop(typename, name, "on");
> +}
> +if (plus_features) {
> +g_list_free_full(plus_features, g_free);
> +}
> +
> +for (l = minus_features; l; l = l->next) {
> +const char *name = l->data;
> +cpu_add_feat_as_prop(typename, name, "off");
> +}
> +if (minus_features) {
> +g_list_free_full(minus_features, g_free);
> +}
> }
>
> -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
> +static void x86_cpu_expand_features(X86CPU *cpu);
> static int x86_cpu_filter_features(X86CPU *cpu);
>
> /* Check for missing features that may prevent the CPU class from
> @@ -2172,7 +2191,6 @@ static void
> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> {
> X86CPU *xc;
> FeatureWord w;
> -Error *err = NULL;
> strList **next = missing_feats;
>
> if (xcc->kvm_required && !kvm_enabled()) {
> @@ -2184,18 +2202,7 @@ static void
> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>
> xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc;
>
> -x86_cpu_expand_features(xc, &err);
> -if (err) {
> -/* Errors at x86_cpu_expand_features should never happen,
> - * but in case it does, just report the model as not
> - * runnable at all using the "type" property.
> - */
> -strList *new = g_new0(strList, 1);
> -new->value = g_strdup("type");
> -*next = new;
> -next = &new->next;
> -}
> -
> +x86_cpu_expand_features(xc);
> x86_cpu_filter_features(xc);
>
> for (w = 0; w < FEATURE_WORDS; w++) {
> @@ -2559,11 +2566,7 @@ static X86CPU *x86_cpu_from_model(const char *model,
> QDict *props, Error **errp)
> }
> }
>
> -x86_cpu_expand_features(xc, &err);
> -if (err) {
> -goto out;
> -}
> -
> +x86_cpu_expand_features(xc);
> out:
> if (err) {
> error_propagate(errp, err);
> @@ -3453,18 +3456,11 @@ static void x86_cpu_enable_xsave_components(X86CPU
> *cpu)
> /* Expand CPU configuration data, based on configured features
> * and host/accelerator capabilities when appropriate.
> */
> -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> +static void x86_cpu_expand_features(X86CPU *cpu)
> {
> CPUX86State *env = &cpu->env;
> FeatureWord w;
> -GList *l;
> -Error *local_err = NULL;
>
> -/*TODO: N
Re: [Qemu-devel] [PATCH v2 5/5] Convert single line fprintf() to warn_report()
On 08/18/2017 02:32 AM, Markus Armbruster wrote:
Philippe Mathieu-Daudé writes:
On 08/17/2017 02:55 PM, Alistair Francis wrote:
On 15/08/2017 09:30, Markus Armbruster wrote:
The stupid fix is to repeat libraries until the link succeeds:
test-util-obj-y = libqemuutil.a libqemustub.a libqemuutil.a
[...]
Sticking '-Wp,-(' and '-Wp,-)' into the command line I get from make V=1
doesn't work for me, though.
The smart solution is not to have .a reference each other.
Nah, I think we should teach those new kids on the block about -lX11
instead. :)
This sounds scary...
Paolo, what do you think?
Another possibility is to just merge the two static libraries into one.
Sounds good to me!
I feel like I have opened a can of worms.
you are good at it! IIRC it all started with a 1-line change in
tcp_chr_wait_connected() more than 2 months ago :)
I can try and combine libqemustub.a into libqemuutil.a is that the
solution? I just want to make sure before I start this.
IMHO your series is OK like this, add a "TODO remove once
libqemuutil.a circular dep is resolved" comment in the Makefile is
enough, and let this issue for another time.
I disagree.
If merging the two .a is beyond your reach (I hope it isn't), then the
spot to mess up is this one:
# TODO bla bla explain bla
test-util-obj-y = libqemuutil.a libqemustub.a libqemuutil.a
I was talking about the first patch:
+tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
$(test-qom-obj-y)
not merging circularly :)
Re: [Qemu-devel] [PATCH v3 for-2.11 07/18] s390x: drop inclusion of sysemu/kvm.h from some files
On 18.08.2017 18:08, Thomas Huth wrote: > On 18.08.2017 13:43, David Hildenbrand wrote: >> s390-stattrib.c needs definition of TARGET_PAGE_SIZE, solve it via cpu.h. > > Why not simply #include "exec/cpu-all.h" ? If cpu.h is not really needed > here... > > Thomas > > Git grep showed me that this is a very rare thing to do, so I went for cpu.h. If you prefer cpu-all.h, I can use that. -- Thanks, David
Re: [Qemu-devel] [PATCH v3 for-2.11 08/18] target/s390x: move gtod_*() declarations to s390-virtio.h
On 18.08.2017 18:11, Thomas Huth wrote: > Suggesting to add a patch description like: "The functions are not used > in target/s390x/ so a header in hw/s390x/ is a better place" ? Sure, I will include that, thanks! > > On 18.08.2017 13:43, David Hildenbrand wrote: >> Signed-off-by: David Hildenbrand >> --- >> hw/s390x/s390-virtio.h | 2 ++ >> target/s390x/cpu.h | 3 --- >> 2 files changed, 2 insertions(+), 3 deletions(-) > > Anyway: > > Reviewed-by: Thomas Huth > -- Thanks, David
Re: [Qemu-devel] [PATCH v14 4/5] mm: support reporting free page blocks
On Thu, Aug 17, 2017 at 11:26:55AM +0800, Wei Wang wrote:
> This patch adds support to walk through the free page blocks in the
> system and report them via a callback function. Some page blocks may
> leave the free list after zone->lock is released, so it is the caller's
> responsibility to either detect or prevent the use of such pages.
>
> Signed-off-by: Wei Wang
> Signed-off-by: Liang Li
> Cc: Michal Hocko
> Cc: Michael S. Tsirkin
> ---
> include/linux/mm.h | 6 ++
> mm/page_alloc.c| 44
> 2 files changed, 50 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 46b9ac5..cd29b9f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1835,6 +1835,12 @@ extern void free_area_init_node(int nid, unsigned long
> * zones_size,
> unsigned long zone_start_pfn, unsigned long *zholes_size);
> extern void free_initmem(void);
>
> +extern void walk_free_mem_block(void *opaque1,
> + unsigned int min_order,
> + void (*visit)(void *opaque2,
> + unsigned long pfn,
> + unsigned long nr_pages));
> +
> /*
> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> * into the buddy system. The freed pages will be poisoned with pattern
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6d00f74..a721a35 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4762,6 +4762,50 @@ void show_free_areas(unsigned int filter, nodemask_t
> *nodemask)
> show_swap_cache_info();
> }
>
> +/**
> + * walk_free_mem_block - Walk through the free page blocks in the system
> + * @opaque1: the context passed from the caller
> + * @min_order: the minimum order of free lists to check
> + * @visit: the callback function given by the caller
> + *
> + * The function is used to walk through the free page blocks in the system,
> + * and each free page block is reported to the caller via the @visit
> callback.
> + * Please note:
> + * 1) The function is used to report hints of free pages, so the caller
> should
> + * not use those reported pages after the callback returns.
> + * 2) The callback is invoked with the zone->lock being held, so it should
> not
> + * block and should finish as soon as possible.
> + */
> +void walk_free_mem_block(void *opaque1,
> + unsigned int min_order,
> + void (*visit)(void *opaque2,
You can just avoid opaque2 completely I think, then opaque1 can
be renamed opaque.
> +unsigned long pfn,
> +unsigned long nr_pages))
> +{
> + struct zone *zone;
> + struct page *page;
> + struct list_head *list;
> + unsigned int order;
> + enum migratetype mt;
> + unsigned long pfn, flags;
> +
> + for_each_populated_zone(zone) {
> + for (order = MAX_ORDER - 1;
> + order < MAX_ORDER && order >= min_order; order--) {
> + for (mt = 0; mt < MIGRATE_TYPES; mt++) {
> + spin_lock_irqsave(&zone->lock, flags);
> + list = &zone->free_area[order].free_list[mt];
> + list_for_each_entry(page, list, lru) {
> + pfn = page_to_pfn(page);
> + visit(opaque1, pfn, 1 << order);
My only concern here is inability of callback to
1. break out of list
2. remove page from the list
So I would make the callback bool, and I would use
list_for_each_entry_safe.
> + }
> + spin_unlock_irqrestore(&zone->lock, flags);
> + }
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(walk_free_mem_block);
> +
> static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> {
> zoneref->zone = zone;
> --
> 2.7.4
[Qemu-devel] [PATCH v7 03/11] qemu.py: use os.path.null instead of /dev/null
For increased portability, let's use os.path.devnull.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index ef531bb23b..51545f7f97 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -78,7 +78,7 @@ class QEMUMachine(object):
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
-devnull = open('/dev/null', 'rb')
+devnull = open(os.path.devnull, 'rb')
p = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT)
output = p.communicate()[0]
@@ -139,7 +139,7 @@ class QEMUMachine(object):
def launch(self):
'''Launch the VM and establish a QMP connection'''
-devnull = open('/dev/null', 'rb')
+devnull = open(os.path.devnull, 'rb')
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
--
2.13.5
[Qemu-devel] [PATCH v7 01/11] qemu.py: fix is_running() return before first launch()
is_running() returns None when called before the first time we
call launch():
>>> import qemu
>>> vm = qemu.QEMUMachine('qemu-system-x86_64')
>>> vm.is_running()
>>>
It should return False instead. This patch fixes that.
For consistence, this patch removes the parenthesis from the
second clause as it's not really needed.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8219..0ae5d39414 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -86,7 +86,7 @@ class QEMUMachine(object):
raise
def is_running(self):
-return self._popen and (self._popen.returncode is None)
+return self._popen is not None and self._popen.returncode is None
def exitcode(self):
if self._popen is None:
--
2.13.5
[Qemu-devel] [PATCH v7 09/11] qemu.py: launch vm only if it's not running
A new call to launch() with a running VM will fall in exception and
consequently call shutdown().
This patch makes launch() to raise an exception when it's called with VM
already running.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 9ae65e454e..46e8ff3232 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -170,6 +170,9 @@ class QEMUMachine(object):
self._iolog = None
self._qemu_full_args = None
devnull = open(os.path.devnull, 'rb')
+if self.is_running():
+raise QEMULaunchError('VM already running')
+
try:
self._pre_launch()
self._qemu_full_args = (self._wrapper + [self._binary] +
--
2.13.5
[Qemu-devel] [PATCH v7 04/11] qemu.py: use poll() instead of 'returncode'
The 'returncode' Popen attribute is not guaranteed to be updated. It
actually depends on a call to either poll(), wait() or communicate().
On the other hand, poll() will: "Check if child process has terminated.
Set and return returncode attribute."
Let's use the poll() to check whether the process is running and also
to get the updated process exit code, when the process is finished.
As consequence, the shutdown() calls to self._load_io_log()
self._post_shutdown() are now always executed to make sure we cleanup
even if VM is not running when the shutdown() is called.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 51545f7f97..774f971f69 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -98,12 +98,12 @@ class QEMUMachine(object):
raise
def is_running(self):
-return self._popen is not None and self._popen.returncode is None
+return self._popen is not None and self._popen.poll() is None
def exitcode(self):
if self._popen is None:
return None
-return self._popen.returncode
+return self._popen.poll()
def get_pid(self):
if not self.is_running():
@@ -111,8 +111,15 @@ class QEMUMachine(object):
return self._popen.pid
def _load_io_log(self):
-with open(self._qemu_log_path, "r") as fh:
-self._iolog = fh.read()
+'''
+Load the Qemu log file content (if available) into the proper
+instance variable
+'''
+try:
+with open(self._qemu_log_path, "r") as fh:
+self._iolog = fh.read()
+except IOError:
+pass
def _base_args(self):
if isinstance(self._monitor_address, tuple):
@@ -168,8 +175,8 @@ class QEMUMachine(object):
if exitcode < 0:
LOG.debug('qemu received signal %i: %s', -exitcode,
' '.join(self._args))
-self._load_io_log()
-self._post_shutdown()
+self._load_io_log()
+self._post_shutdown()
underscore_to_dash = string.maketrans('_', '-')
def qmp(self, cmd, conv_keys=True, **args):
--
2.13.5
[Qemu-devel] [PATCH v7 02/11] qemu.py: avoid writing to stdout/stderr
This module should not write directly to stdout/stderr. Instead, it
should either raise exceptions or just log the messages and let the
callers handle them and decide what to do. For example, scripts could
choose to send the log messages stderr or/and write them to a file if
verbose or debugging mode is enabled.
This patch replaces the writes to stderr by an exception in the
send_fd_scm() when _socket_scm_helper is not set or not present. In the
same method, the subprocess Popen will now redirect the stdout/stderr to
logging.debug instead of writing to system stderr. As consequence, since
the Popen.communicate() is now used (in order to get the stdout), the
further call to wait() became redundant and was replaced by
Popen.returncode.
The shutdown() message on negative exit code will now be logged
to logging.debug instead of written to system stderr.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 0ae5d39414..ef531bb23b 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,6 +13,7 @@
#
import errno
+import logging
import string
import os
import sys
@@ -20,6 +21,15 @@ import subprocess
import qmp.qmp
+LOG = logging.getLogger(__name__)
+
+
+class QEMUMachineError(Exception):
+"""
+Exception called when an error in QEMUMachine happens.
+"""
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -62,18 +72,20 @@ class QEMUMachine(object):
# In iotest.py, the qmp should always use unix socket.
assert self._qmp.is_scm_available()
if self._socket_scm_helper is None:
-print >>sys.stderr, "No path to socket_scm_helper set"
-return -1
+raise QEMUMachineError("No path to socket_scm_helper set")
if os.path.exists(self._socket_scm_helper) == False:
-print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
-return -1
+raise QEMUMachineError("%s does not exist" %
self._socket_scm_helper)
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
-p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
-return p.wait()
+p = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT)
+output = p.communicate()[0]
+if output:
+LOG.debug(output)
+
+return p.returncode
@staticmethod
def _remove_if_exists(path):
@@ -154,7 +166,8 @@ class QEMUMachine(object):
exitcode = self._popen.wait()
if exitcode < 0:
-sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode,
' '.join(self._args)))
+LOG.debug('qemu received signal %i: %s', -exitcode,
+ ' '.join(self._args))
self._load_io_log()
self._post_shutdown()
--
2.13.5
[Qemu-devel] [PATCH v7 08/11] qemu.py: make sure we only remove files we create
To launch a VM, we need to create basically two files: the monitor socket (if it's a UNIX socket) and the qemu log file. For the qemu log file, we currently just open the path, which will create the file if it does not exist or overwrite the file if it does exist. For the monitor socket, if it already exists, we are currently removing it, even if it's not created by us. This patch moves to pre_launch() the responsibility to make sure we only create files that are not pre-existent and to populate a list of controlled files. This list will then be used as the reference of files to remove during the cleanup (post_shutdown()). Signed-off-by: Amador Pahim --- scripts/qemu.py | 33 ++--- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 29fd2469f9..9ae65e454e 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -41,6 +41,7 @@ class QEMUMachine(object): monitor_address = os.path.join(test_dir, name + "-monitor.sock") self._monitor_address = monitor_address self._qemu_log_path = os.path.join(test_dir, name + ".log") +self._qemu_log_fd = None self._popen = None self._binary = binary self._args = list(args) # Force copy args in case we modify them @@ -50,6 +51,7 @@ class QEMUMachine(object): self._socket_scm_helper = socket_scm_helper self._debug = debug self._qemu_full_args = None +self._created_files = [] # This can be used to add an unused monitor instance. def add_monitor_telnet(self, ip, port): @@ -134,30 +136,47 @@ class QEMUMachine(object): '-display', 'none', '-vga', 'none'] def _pre_launch(self): -self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True, -debug=self._debug) +try: +self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, +server=True, +debug=self._debug) +except: +raise +else: +if not isinstance(self._monitor_address, tuple): +self._created_files.append(self._monitor_address) + +try: +flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY +os.open(self._qemu_log_path, flags) +except: +raise +else: +self._created_files.append(self._qemu_log_path) +self._qemu_log_fd = open(self._qemu_log_path, 'wb') def _post_launch(self): self._qmp.accept() def _post_shutdown(self): -if not isinstance(self._monitor_address, tuple): -self._remove_if_exists(self._monitor_address) -self._remove_if_exists(self._qemu_log_path) +if self._qemu_log_fd is not None: +self._qemu_log_fd.close() + +while self._created_files: +self._remove_if_exists(self._created_files.pop()) def launch(self): '''Launch the VM and establish a QMP connection''' self._iolog = None self._qemu_full_args = None devnull = open(os.path.devnull, 'rb') -qemulog = open(self._qemu_log_path, 'wb') try: self._pre_launch() self._qemu_full_args = (self._wrapper + [self._binary] + self._base_args() + self._args) self._popen = subprocess.Popen(self._qemu_full_args, stdin=devnull, - stdout=qemulog, + stdout=self._qemu_log_fd, stderr=subprocess.STDOUT, shell=False) self._post_launch() -- 2.13.5
Re: [Qemu-devel] [PATCH] hw/arm/allwinner: Fix crash with -nodefaults -M cubieboard
On 18 August 2017 at 18:08, Thomas Huth wrote:
> The allwinner-a10 device uses serial_hds[0] without checking whether
> it is available or not. So using the cubieboard with -nodefaults
> currently results in a segmentation fault. Fix it by adding a
> proper check here.
> And while we're at it, mark the device as "user_creatable = false"
> since this apparently can not directly be used by the users but has
> to be wired up in code instead.
>
> Signed-off-by: Thomas Huth
> ---
> hw/arm/allwinner-a10.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index f62a9a3..e152566 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -109,8 +109,10 @@ static void aw_a10_realize(DeviceState *dev, Error
> **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, s->irq[56]);
>
> /* FIXME use a qdev chardev prop instead of serial_hds[] */
> -serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
> - 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
> +if (serial_hds[0]) {
> +serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2,
> s->irq[1],
> + 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
> +}
This doesn't look like the right fix, because it means that
there won't be a UART device at that point in system memory
at all. What you want is for there to be a UART device there
but not connected to anything, ie serial_mm_init() should cope
with being passed a NULL Chardev*.
thanks
-- PMM
[Qemu-devel] [PATCH v7 00/11] scripts/qemu.py fixes and cleanups
Changes v1->v2:
- Style fixes to make checkpatch.pl happy.
- Rebased.
Changes v2->v3:
- Fix typo in patch 3 ("qemu.py: make 'args' public") commit message.
Changes v3->v4:
- Squash the 2 first commits since they are co-dependant.
- Cleanup launch() and shutdown().
- Reorder the commits, putting the rename of self._args first.
- Rebased.
Changes v4->v5:
- Break the cleanup commit into logical changes and include in the
commit messages the rationale for making them.
Changes v5->v6:
- Remove the commit to rename self._args.
- Fix is_running() return before first call to maunch().
- Use python logging system.
- Include the full command line on negative exit code error message.
- Use os.path.null instead of /dev/null.
- Improve the control over the created/deleted files.
Changes v6->v7:
- Split commits in self-contained/atomic changes.
- Addressed the comments from previous version, basically improving the
logging messages and the control over created files. See individual
commit messages for details.
Amador Pahim (11):
qemu.py: fix is_running() return before first launch()
qemu.py: avoid writing to stdout/stderr
qemu.py: use os.path.null instead of /dev/null
qemu.py: use poll() instead of 'returncode'
qemu.py: cleanup redundant calls in launch()
qemu.py: improve message on negative exit code
qemu.py: include debug information on launch error
qemu.py: make sure we only remove files we create
qemu.py: launch vm only if it's not running
qemu.py: don't launch again before shutdown()
qemu.py: refactor launch()
scripts/qemu.py | 141 ++--
1 file changed, 106 insertions(+), 35 deletions(-)
--
2.13.5
[Qemu-devel] [PATCH v7 06/11] qemu.py: improve message on negative exit code
The current message shows 'self._args', which contains only part of the
options used in the Qemu command line.
This patch makes the qemu full args list an instance variable and then
uses it in the negative exit code message.
Message was moved outside the 'if is_running' block to make sure it will
be logged if the VM finishes before the call to shutdown().
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 26 +++---
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 902d4c6c78..0bcec4b3b1 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -49,6 +49,7 @@ class QEMUMachine(object):
self._iolog = None
self._socket_scm_helper = socket_scm_helper
self._debug = debug
+self._qemu_full_args = None
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -146,13 +147,18 @@ class QEMUMachine(object):
def launch(self):
'''Launch the VM and establish a QMP connection'''
+self._qemu_full_args = None
devnull = open(os.path.devnull, 'rb')
qemulog = open(self._qemu_log_path, 'wb')
try:
self._pre_launch()
-args = self._wrapper + [self._binary] + self._base_args() +
self._args
-self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
- stderr=subprocess.STDOUT,
shell=False)
+self._qemu_full_args = (self._wrapper + [self._binary] +
+self._base_args() + self._args)
+self._popen = subprocess.Popen(self._qemu_full_args,
+ stdin=devnull,
+ stdout=qemulog,
+ stderr=subprocess.STDOUT,
+ shell=False)
self._post_launch()
except:
self.shutdown()
@@ -166,11 +172,17 @@ class QEMUMachine(object):
self._qmp.close()
except:
self._popen.kill()
+self._popen.wait()
+
+exitcode = self.exitcode()
+if exitcode is not None and exitcode < 0:
+msg = 'qemu received signal -%i: %s'
+if self._qemu_full_args:
+command = ' '.join(self._qemu_full_args)
+else:
+command = ''
+LOG.debug(msg, exitcode, command)
-exitcode = self._popen.wait()
-if exitcode < 0:
-LOG.debug('qemu received signal %i: %s', -exitcode,
- ' '.join(self._args))
self._load_io_log()
self._post_shutdown()
--
2.13.5
[Qemu-devel] [PATCH v7 11/11] qemu.py: refactor launch()
This is just an refactor to separate the exception handler from the
actual launch procedure, improving the readability and making future
maintenances in this piece of code easier.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 33 -
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 2bd81afcf2..7f890ff8e5 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -167,10 +167,10 @@ class QEMUMachine(object):
self._remove_if_exists(self._created_files.pop())
def launch(self):
-'''Launch the VM and establish a QMP connection'''
-self._iolog = None
-self._qemu_full_args = None
-devnull = open(os.path.devnull, 'rb')
+'''
+Try to launch the VM and make sure we cleanup and expose the
+command line/output in case of exception.
+'''
if self.is_running():
raise QEMULaunchError('VM already running')
@@ -178,15 +178,7 @@ class QEMUMachine(object):
raise QEMULaunchError('Shutdown pending after previous launch')
try:
-self._pre_launch()
-self._qemu_full_args = (self._wrapper + [self._binary] +
-self._base_args() + self._args)
-self._popen = subprocess.Popen(self._qemu_full_args,
- stdin=devnull,
- stdout=self._qemu_log_fd,
- stderr=subprocess.STDOUT,
- shell=False)
-self._post_launch()
+self._launch()
self._pending_shutdown = True
except:
self.shutdown()
@@ -199,6 +191,21 @@ class QEMUMachine(object):
raise
+def _launch(self):
+'''Launch the VM and establish a QMP connection'''
+self._iolog = None
+self._qemu_full_args = None
+devnull = open(os.path.devnull, 'rb')
+self._pre_launch()
+self._qemu_full_args = (self._wrapper + [self._binary] +
+self._base_args() + self._args)
+self._popen = subprocess.Popen(self._qemu_full_args,
+ stdin=devnull,
+ stdout=self._qemu_log_fd,
+ stderr=subprocess.STDOUT,
+ shell=False)
+self._post_launch()
+
def shutdown(self):
'''Terminate the VM and clean up'''
if self.is_running():
--
2.13.5
[Qemu-devel] [PATCH v7 10/11] qemu.py: don't launch again before shutdown()
If a VM is launched, files are created and a cleanup is required before
a new launch. This cleanup is executed by shutdown(), so shutdown() must
be called even if the VM is manually terminated (i.e. using kill).
This patch creates a control to make sure launch() will not be executed
again if shutdown() is not called after the previous launch().
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 46e8ff3232..2bd81afcf2 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -52,6 +52,7 @@ class QEMUMachine(object):
self._debug = debug
self._qemu_full_args = None
self._created_files = []
+self._pending_shutdown = False
# This can be used to add an unused monitor instance.
def add_monitor_telnet(self, ip, port):
@@ -173,6 +174,9 @@ class QEMUMachine(object):
if self.is_running():
raise QEMULaunchError('VM already running')
+if self._pending_shutdown:
+raise QEMULaunchError('Shutdown pending after previous launch')
+
try:
self._pre_launch()
self._qemu_full_args = (self._wrapper + [self._binary] +
@@ -183,6 +187,7 @@ class QEMUMachine(object):
stderr=subprocess.STDOUT,
shell=False)
self._post_launch()
+self._pending_shutdown = True
except:
self.shutdown()
@@ -215,6 +220,7 @@ class QEMUMachine(object):
self._load_io_log()
self._post_shutdown()
+self._pending_shutdown = False
underscore_to_dash = string.maketrans('_', '-')
def qmp(self, cmd, conv_keys=True, **args):
--
2.13.5
Re: [Qemu-devel] [PATCH 4/4] eepro100: switch e100_compute_mcast_idx() over to use net_crc32()
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
Signed-off-by: Mark Cave-Ayland
---
hw/net/eepro100.c | 19 +--
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 5a4774a..4226572 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -327,26 +327,9 @@ static const uint16_t eepro100_mdi_mask[] = {
static E100PCIDeviceInfo *eepro100_get_class(EEPRO100State *s);
-/* From FreeBSD (locally modified). */
static unsigned e100_compute_mcast_idx(const uint8_t *ep)
{
-uint32_t crc;
-int carry, i, j;
-uint8_t b;
-
-crc = 0x;
-for (i = 0; i < 6; i++) {
-b = *ep++;
-for (j = 0; j < 8; j++) {
-carry = ((crc & 0x8000L) ? 1 : 0) ^ (b & 0x01);
-crc <<= 1;
-b >>= 1;
-if (carry) {
-crc = ((crc ^ POLYNOMIAL) | carry);
-}
-}
-}
-return (crc & BITS(7, 2)) >> 2;
+return (net_crc32(ep, 6) & BITS(7, 2)) >> 2;
with previous suggestion from patch 1/4:
return (mac_crc32_be(ep) & BITS(7, 2)) >> 2;
then
Reviewed-by: Philippe Mathieu-Daudé
}
/* Read a 16 bit control/status (CSR) register. */
[Qemu-devel] [PATCH v7 07/11] qemu.py: include debug information on launch error
When launching a VM, if an exception happens and the VM is not
initiated, it might be useful to see the qemu command line and
the qemu command output.
This patch creates that message. Notice that self._iolog needs to be
cleaned up in the beginning of the launch() to make sure we will not
expose the qemu log from a previous launch if the current one fails.
Signed-off-by: Amador Pahim
---
scripts/qemu.py | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/qemu.py b/scripts/qemu.py
index 0bcec4b3b1..29fd2469f9 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -147,6 +147,7 @@ class QEMUMachine(object):
def launch(self):
'''Launch the VM and establish a QMP connection'''
+self._iolog = None
self._qemu_full_args = None
devnull = open(os.path.devnull, 'rb')
qemulog = open(self._qemu_log_path, 'wb')
@@ -162,6 +163,13 @@ class QEMUMachine(object):
self._post_launch()
except:
self.shutdown()
+
+LOG.debug('Error launching VM')
+if self._qemu_full_args:
+LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
+if self._iolog:
+LOG.debug('Output: %r', self._iolog)
+
raise
def shutdown(self):
--
2.13.5
[Qemu-devel] [PATCH v7 05/11] qemu.py: cleanup redundant calls in launch()
Now that shutdown() is guaranteed to always execute self._load_io_log() and self._post_shutdown(), their calls in 'except' became redundant and we can safely replace it by a call to shutdown(). Signed-off-by: Amador Pahim --- scripts/qemu.py | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 774f971f69..902d4c6c78 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -155,11 +155,7 @@ class QEMUMachine(object): stderr=subprocess.STDOUT, shell=False) self._post_launch() except: -if self.is_running(): -self._popen.kill() -self._popen.wait() -self._load_io_log() -self._post_shutdown() +self.shutdown() raise def shutdown(self): -- 2.13.5
Re: [Qemu-devel] [PATCH v2 5/5] Convert single line fprintf() to warn_report()
On Thu, Aug 17, 2017 at 10:32 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
>> On 08/17/2017 02:55 PM, Alistair Francis wrote:
> On 15/08/2017 09:30, Markus Armbruster wrote:
>> The stupid fix is to repeat libraries until the link succeeds:
>>
>> test-util-obj-y = libqemuutil.a libqemustub.a libqemuutil.a
>>
>> [...]
>>
>> Sticking '-Wp,-(' and '-Wp,-)' into the command line I get from make V=1
>> doesn't work for me, though.
>>
>> The smart solution is not to have .a reference each other.
>
> Nah, I think we should teach those new kids on the block about -lX11
> instead. :)
>>>
>>> This sounds scary...
>>>
>
>> Paolo, what do you think?
>
> Another possibility is to just merge the two static libraries into one.
Sounds good to me!
>>>
>>> I feel like I have opened a can of worms.
>>
>> you are good at it! IIRC it all started with a 1-line change in
>> tcp_chr_wait_connected() more than 2 months ago :)
>>
>>>
>>> I can try and combine libqemustub.a into libqemuutil.a is that the
>>> solution? I just want to make sure before I start this.
>>
>> IMHO your series is OK like this, add a "TODO remove once
>> libqemuutil.a circular dep is resolved" comment in the Makefile is
>> enough, and let this issue for another time.
>
> I disagree.
>
> If merging the two .a is beyond your reach (I hope it isn't), then the
> spot to mess up is this one:
This actually seems pretty easy to do.
I'm going to split this patch and the Makefile changes into a separate
series and send those so I don't end up spamming everyone with the
earlier patches in the series. Then after 2.10 I can combine them all
and send the full series.
Thanks,
Alistair
>
> # TODO bla bla explain bla
> test-util-obj-y = libqemuutil.a libqemustub.a libqemuutil.a
>
Re: [Qemu-devel] [PATCH 3/4] pcnet: switch lnc_mchash() over to use net_crc32_le()
On 08/18/2017 01:59 PM, Philippe Mathieu-Daudé wrote:
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
Signed-off-by: Mark Cave-Ayland
---
hw/net/pcnet.c | 16 +---
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 6544553..c050993 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -522,23 +522,9 @@ static inline void pcnet_rmd_store(PCNetState *s,
struct pcnet_RMD *rmd,
be16_to_cpu(hdr->ether_type)); \
} while (0)
-#define MULTICAST_FILTER_LEN 8
-
static inline uint32_t lnc_mchash(const uint8_t *ether_addr)
{
-#define LNC_POLYNOMIAL 0xEDB88320UL
-uint32_t crc = 0x;
-int idx, bit;
-uint8_t data;
-
-for (idx = 0; idx < 6; idx++) {
-for (data = *ether_addr++, bit = 0; bit <
MULTICAST_FILTER_LEN; bit++) {
-crc = (crc >> 1) ^ (((crc ^ data) & 1) ? LNC_POLYNOMIAL :
0);
-data >>= 1;
-}
-}
-return crc;
-#undef LNC_POLYNOMIAL
+return net_crc32_le(ether_addr, 6);
with previous suggestion from patch 1/4:
return mac_crc32_le(ether_addr);
using ETH_ALEN:
Reviewed-by: Philippe Mathieu-Daudé
}
#define CRC(crc, ch) (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) &
0xff])
[Qemu-devel] [PATCH] hw/arm/allwinner: Fix crash with -nodefaults -M cubieboard
The allwinner-a10 device uses serial_hds[0] without checking whether
it is available or not. So using the cubieboard with -nodefaults
currently results in a segmentation fault. Fix it by adding a
proper check here.
And while we're at it, mark the device as "user_creatable = false"
since this apparently can not directly be used by the users but has
to be wired up in code instead.
Signed-off-by: Thomas Huth
---
hw/arm/allwinner-a10.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index f62a9a3..e152566 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -109,8 +109,10 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->sata), 0, s->irq[56]);
/* FIXME use a qdev chardev prop instead of serial_hds[] */
-serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
- 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
+if (serial_hds[0]) {
+serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2,
s->irq[1],
+ 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
+}
}
static void aw_a10_class_init(ObjectClass *oc, void *data)
@@ -118,6 +120,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = aw_a10_realize;
+/* Reason: Needs to be wired up in code, see cubieboard_init() */
+dc->user_creatable = false;
}
static const TypeInfo aw_a10_type_info = {
--
1.8.3.1
Re: [Qemu-devel] [PATCH 1/4] net: move CRC32 calculation from compute_mcast_idx() into its own net_crc32() function
On 08/18/2017 01:51 PM, Philippe Mathieu-Daudé wrote:
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
Separate out the standard ethernet CRC32 calculation into a new
net_crc32()
function, renaming the constant POLYNOMIAL to POLYNOMIAL_BE to make it
clear
that this is a big-endian CRC32 calculation.
Then remove the existing implementation from compute_mcast_idx() and
call the
new function in its place.
Signed-off-by: Mark Cave-Ayland
---
include/net/net.h |3 ++-
net/net.c | 16 +++-
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 1c55a93..586098c 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -227,7 +227,8 @@ NetClientState *net_hub_port_find(int hub_id);
void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
-#define POLYNOMIAL 0x04c11db6
+#define POLYNOMIAL_BE 0x04c11db6
+uint32_t net_crc32(const uint8_t *p, int len);
unsigned compute_mcast_idx(const uint8_t *ep);
#define vmstate_offset_macaddr(_state, _field) \
diff --git a/net/net.c b/net/net.c
index 0e28099..a856907 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1568,25 +1568,31 @@ int net_client_parse(QemuOptsList *opts_list,
const char *optarg)
/* From FreeBSD */
/* XXX: optimize */
-unsigned compute_mcast_idx(const uint8_t *ep)
+uint32_t net_crc32(const uint8_t *p, int len)
imho there is no point in using signed length.
{
uint32_t crc;
int carry, i, j;
uint8_t b;
crc = 0x;
-for (i = 0; i < 6; i++) {
-b = *ep++;
+for (i = 0; i < len; i++) {
+b = *p++;
for (j = 0; j < 8; j++) {
carry = ((crc & 0x8000L) ? 1 : 0) ^ (b & 0x01);
crc <<= 1;
b >>= 1;
if (carry) {
-crc = ((crc ^ POLYNOMIAL) | carry);
+crc = ((crc ^ POLYNOMIAL_BE) | carry);
}
}
}
-return crc >> 26;
+
+return crc;
+}
+
+unsigned compute_mcast_idx(const uint8_t *ep)
+{
+return net_crc32(ep, 6) >> 26;
while here let's use ETH_ALEN
reading the next patches, what do you think about:
static inline uint32_t mac_crc32_be(const uint8_t *p)
{
return net_crc32_be(ep, ETH_ALEN);
}
unsigned compute_mcast_idx(const uint8_t *ep)
{
return mac_crc32_be(p) >> 26;
}
}
QemuOptsList qemu_netdev_opts = {
with "uint32_t net_crc32(const uint8_t *p, size_t len);":
Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH for-2.11 v2 4/5] qmp-shell: Accept QMP command as argument
On Wed, Aug 16, 2017 at 08:25:41AM +0200, Markus Armbruster wrote:
> Eduardo Habkost writes:
>
> > On Tue, Aug 15, 2017 at 12:03:53PM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost writes:
> >>
> >> Suggest to insert here:
> >>
> >> If additional arguments QMP-COMMAND ARG=VAL... are given, run just
> >> that QMP command instead of the REPL.
> >>
> >> Question: is this limited to simple arguments? If no, how would I write
> >> an object argument? For instance, how would I do
> >>
> >> { "execute": "blockdev-add", "arguments": { "node-name": "foo",
> >> "driver": "nbd", "server": { "type": "inet", "host": "localhost", "port":
> >> "12345" } } }
> >>
> >> ?
> >
> > Exactly the same way you would write it when running qmp-shell in
> > interactive mode. e.g.:
> >
> > $ ./scripts/qmp/qmp-shell /tmp/qmp blockdev-add driver=qcow2
> > node-name=node-E 'file={"driver":"file","filename":"/path/to/file.qcow2"}'
>
> I see.
>
> The QEMU command line uses dotted key syntax instead.
You mean -blockdev, or is there another way to send QMP commands
to QEMU that I'm not aware of?
>
> To be honest, the less qmp-shell is used, the happier I am.
What people would use instead of it?
> Would you
> like to serve as its sub-maintainer?
I'd be glad to.
--
Eduardo
Re: [Qemu-devel] [PATCH 3/4] pcnet: switch lnc_mchash() over to use net_crc32_le()
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
Signed-off-by: Mark Cave-Ayland
---
hw/net/pcnet.c | 16 +---
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index 6544553..c050993 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -522,23 +522,9 @@ static inline void pcnet_rmd_store(PCNetState *s, struct
pcnet_RMD *rmd,
be16_to_cpu(hdr->ether_type)); \
} while (0)
-#define MULTICAST_FILTER_LEN 8
-
static inline uint32_t lnc_mchash(const uint8_t *ether_addr)
{
-#define LNC_POLYNOMIAL 0xEDB88320UL
-uint32_t crc = 0x;
-int idx, bit;
-uint8_t data;
-
-for (idx = 0; idx < 6; idx++) {
-for (data = *ether_addr++, bit = 0; bit < MULTICAST_FILTER_LEN; bit++)
{
-crc = (crc >> 1) ^ (((crc ^ data) & 1) ? LNC_POLYNOMIAL : 0);
-data >>= 1;
-}
-}
-return crc;
-#undef LNC_POLYNOMIAL
+return net_crc32_le(ether_addr, 6);
using ETH_ALEN:
Reviewed-by: Philippe Mathieu-Daudé
}
#define CRC(crc, ch) (crc = (crc >> 8) ^ crctab[(crc ^ (ch)) & 0xff])
Re: [Qemu-devel] [RFC v4 08/13] ide: enumerate_slots implementation
On Wed, Aug 16, 2017 at 05:46:18PM -0400, John Snow wrote:
>
>
> On 08/14/2017 05:57 PM, Eduardo Habkost wrote:
> > Example output when using "-machine q35":
> >
> > {
> > "available": true,
> > "count": 1,
> > "device-types": [
> > "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> > { "option": "unit", "values": 0 },
> > { "option": "bus", "values": "ide.2" }
> > ],
> > "opts-complete": true
> > }
> > {
> > "available": false,
> > "count": 1,
> > "device": "/machine/unattached/device[19]",
> > "device-types": [
> > "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> > { "option": "unit", "values": 1 },
> > { "option": "bus", "values": "ide.2" } ],
> > "opts-complete": true
> > }
> > {
> > "available": true,
> > "count": 10,
> > "device-types": [
> > "ide-device"
> > ],
> > "hotpluggable": false,
> > "opts": [
> > { "option": "unit", "values": [ [ 0, 1 ] ] },
>
> Hm, these unit values aren't really correct -- we do not support
> primary/secondary semantics for IDE buses on the AHCI device. I guess
> they technically exist, but you cannot use them for anything.
>
> Should I do something to "disable" or otherwise hide the unusable
> secondary unit slots for AHCI devices?
If the device is already rejecting -device ...,unit=1, then the
bug is in my implementation of enumerate_devices. Maybe it
should just look at IDEBus::max_units to find that out?
--
Eduardo
Re: [Qemu-devel] [PATCH 2/4] net: introduce net_crc32_le() function
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
This provides a standard ethernet CRC32 little-endian implementation.
Signed-off-by: Mark Cave-Ayland
---
include/net/net.h |2 ++
net/net.c | 22 ++
2 files changed, 24 insertions(+)
diff --git a/include/net/net.h b/include/net/net.h
index 586098c..4afac1a 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -228,7 +228,9 @@ NetClientState *net_hub_port_find(int hub_id);
void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
#define POLYNOMIAL_BE 0x04c11db6
+#define POLYNOMIAL_LE 0xedb88320
uint32_t net_crc32(const uint8_t *p, int len);
+uint32_t net_crc32_le(const uint8_t *p, int len);
same here, with size_t len:
Reviewed-by: Philippe Mathieu-Daudé
unsigned compute_mcast_idx(const uint8_t *ep);
#define vmstate_offset_macaddr(_state, _field) \
diff --git a/net/net.c b/net/net.c
index a856907..c99e705 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1590,6 +1590,28 @@ uint32_t net_crc32(const uint8_t *p, int len)
return crc;
}
+uint32_t net_crc32_le(const uint8_t *p, int len)
+{
+uint32_t crc;
+int carry, i, j;
+uint8_t b;
+
+crc = 0x;
+for (i = 0; i < len; i++) {
+b = *p++;
+for (j = 0; j < 8; j++) {
+carry = (crc & 0x1) ^ (b & 0x01);
+crc >>= 1;
+b >>= 1;
+if (carry) {
+crc = crc ^ POLYNOMIAL_LE;
+}
+}
+}
+
+return crc;
+}
+
unsigned compute_mcast_idx(const uint8_t *ep)
{
return net_crc32(ep, 6) >> 26;
Re: [Qemu-devel] [PATCH] x86: Skip check apic_id_limit for Xen
On 18/08/2017 18:38, Eduardo Habkost wrote: >>> I think you still need to do a check for vIOMMU being enabled. >> >> Yes, this will be done in the Xen tool stack and Qemu doesn't have such >> knowledge. Operations of create, destroy Xen vIOMMU will be done in the >> Xen tool stack. > > Shouldn't we make QEMU have knowledge of the vIOMMU device, then? > Won't QEMU need to know about it eventually? No, it actually makes sense since Xen hosts all system-wide interrupt sources outside QEMU, unlike KVM which only hosts the (per-CPU) local APIC. Paolo
Re: [Qemu-devel] [PATCH 1/4] net: move CRC32 calculation from compute_mcast_idx() into its own net_crc32() function
On 08/18/2017 11:15 AM, Mark Cave-Ayland wrote:
Separate out the standard ethernet CRC32 calculation into a new net_crc32()
function, renaming the constant POLYNOMIAL to POLYNOMIAL_BE to make it clear
that this is a big-endian CRC32 calculation.
Then remove the existing implementation from compute_mcast_idx() and call the
new function in its place.
Signed-off-by: Mark Cave-Ayland
---
include/net/net.h |3 ++-
net/net.c | 16 +++-
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 1c55a93..586098c 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -227,7 +227,8 @@ NetClientState *net_hub_port_find(int hub_id);
void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd);
-#define POLYNOMIAL 0x04c11db6
+#define POLYNOMIAL_BE 0x04c11db6
+uint32_t net_crc32(const uint8_t *p, int len);
unsigned compute_mcast_idx(const uint8_t *ep);
#define vmstate_offset_macaddr(_state, _field) \
diff --git a/net/net.c b/net/net.c
index 0e28099..a856907 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1568,25 +1568,31 @@ int net_client_parse(QemuOptsList *opts_list, const
char *optarg)
/* From FreeBSD */
/* XXX: optimize */
-unsigned compute_mcast_idx(const uint8_t *ep)
+uint32_t net_crc32(const uint8_t *p, int len)
imho there is no point in using signed length.
{
uint32_t crc;
int carry, i, j;
uint8_t b;
crc = 0x;
-for (i = 0; i < 6; i++) {
-b = *ep++;
+for (i = 0; i < len; i++) {
+b = *p++;
for (j = 0; j < 8; j++) {
carry = ((crc & 0x8000L) ? 1 : 0) ^ (b & 0x01);
crc <<= 1;
b >>= 1;
if (carry) {
-crc = ((crc ^ POLYNOMIAL) | carry);
+crc = ((crc ^ POLYNOMIAL_BE) | carry);
}
}
}
-return crc >> 26;
+
+return crc;
+}
+
+unsigned compute_mcast_idx(const uint8_t *ep)
+{
+return net_crc32(ep, 6) >> 26;
while here let's use ETH_ALEN
}
QemuOptsList qemu_netdev_opts = {
with "uint32_t net_crc32(const uint8_t *p, size_t len);":
Reviewed-by: Philippe Mathieu-Daudé
Re: [Qemu-devel] [PATCH 18/28] xtensa: replace cpu_xtensa_init() with cpu_generic_init()
On Thu, Aug 17, 2017 at 04:32:23PM +0200, Igor Mammedov wrote: > On Wed, 16 Aug 2017 16:56:40 -0300 > Eduardo Habkost wrote: > > > On Fri, Jul 14, 2017 at 03:52:09PM +0200, Igor Mammedov wrote: > > > call xtensa_irq_init() at realize time which makes > > > cpu_xtensa_init() like generic cpu creation function. > > > As result we can replace it with cpu_generic_init() > > > which does the same job, reducing code duplication a bit. > > > > > > Signed-off-by: Igor Mammedov > > > > Looks good to me. Were you able to test it? > images are from > http://jcmvbkbc.spb.ru/~dumb/ws/osll/qemu-xtensa/20110829/xtensa-dc232b_kernel_rootfs.tgz > http://jcmvbkbc.spb.ru/~dumb/tmp/201508231001/default-sim-fsf/Image.elf > > instructions to test is from > http://qemu-buch.de/de/index.php?title=QEMU-KVM-Buch/_Gast-Systeme/_Xtensa-Architektur > > it boots till some point, so CPU is still there > which should be sufficient for this patch Thanks! Reviewed-by: Eduardo Habkost -- Eduardo
Re: [Qemu-devel] [PATCH v3] hw/acpi-build: Fix SRAT memory building when there is no memory in node0
On Thu, Aug 17, 2017 at 01:58:40PM +0800, Dou Liyang wrote:
> Hi Igor,
>
> I tested this patch with following guests:
>
> 1. RHEL 6.5 with Linux 2.6.32
> 2. RHEL 7.0 with Linux 3.10.0
> 3. Fedora 23 with Linux 4.13.0-rc5
> 4. window 2003 service
> 5. window 7
> 6. window 10
What's the command-line(s) you have tested with each OS? Have
you tested both the node0-with-RAM and node0-without-RAM cases?
I would be interested in a demonstration that no bytes in the
ACPI table are changed by this patch when there is some RAM
configured in node 0. (Is that already covered by our existing
ACPI test cases?)
A new test case in bios-tables-test.c for the bug you are fixing
would be nice to have.
>
> Thanks,
> dou.
>
> At 08/16/2017 09:45 AM, Dou Liyang wrote:
> > Currently, Using the fisrt node without memory on the machine makes
> > QEMU unhappy. With this example command line:
> > ... \
> > -m 1024M,slots=4,maxmem=32G \
> > -numa node,nodeid=0 \
> > -numa node,mem=1024M,nodeid=1 \
> > -numa node,nodeid=2 \
> > -numa node,nodeid=3 \
> > Guest reports "No NUMA configuration found" and the NUMA topology is
> > wrong.
> >
> > This is because when QEMU builds ACPI SRAT, it regards node0 as the
> > default node to deal with the memory hole(640K-1M). this means the
> > node0 must have some memory(>1M), but, actually it can have no
> > memory.
> >
> > Fix this problem by replace the node0 with the first node which has
> > memory on it. Add a new function for each node. Also do some cleanup.
> >
> > Signed-off-by: Dou Liyang
> > ---
> > V3 --> V2
> > -Modify the title
> > V2 --> V1:
> > -Fix a coding style problem
> > Replace
> > for (node = 0;
> > node < pcms->numa_nodes && pcms->node_mem[node] == 0;
> > node++);
> >
> > with
> > for (node = 0; node < pcms->numa_nodes; node++) {
> >if (pcms->node_mem[node] != 0) {
> > break;
> > }
> >
> > hw/i386/acpi-build.c | 78
> > +---
> > 1 file changed, 50 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 98dd424..f93d712 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2318,15 +2318,43 @@ build_tpm2(GArray *table_data, BIOSLinker *linker)
> > (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL,
> > NULL);
> > }
> >
> > +static uint64_t
> > +build_srat_node_entry(GArray *table_data, PCMachineState *pcms,
> > +int i, uint64_t mem_base, uint64_t mem_len)
> > +{
> > +AcpiSratMemoryAffinity *numamem;
> > +uint64_t next_base;
> > +
> > +next_base = mem_base + mem_len;
> > +
> > +/* Cut out the ACPI_PCI hole */
> > +if (mem_base <= pcms->below_4g_mem_size &&
> > +next_base > pcms->below_4g_mem_size) {
> > +mem_len -= next_base - pcms->below_4g_mem_size;
> > +if (mem_len > 0) {
> > +numamem = acpi_data_push(table_data, sizeof *numamem);
> > +build_srat_memory(numamem, mem_base, mem_len, i,
> > + MEM_AFFINITY_ENABLED);
> > +}
> > +mem_base = 1ULL << 32;
> > +mem_len = next_base - pcms->below_4g_mem_size;
> > +next_base += (1ULL << 32) - pcms->below_4g_mem_size;
> > +}
> > +numamem = acpi_data_push(table_data, sizeof *numamem);
> > +build_srat_memory(numamem, mem_base, mem_len, i,
> > + MEM_AFFINITY_ENABLED);
> > +return next_base;
> > +}
> > +
> > static void
> > build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> > {
> > AcpiSystemResourceAffinityTable *srat;
> > AcpiSratMemoryAffinity *numamem;
> >
> > -int i;
> > +int i, node;
> > int srat_start, numa_start, slots;
> > -uint64_t mem_len, mem_base, next_base;
> > +uint64_t mem_len, mem_base;
> > MachineClass *mc = MACHINE_GET_CLASS(machine);
> > const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> > PCMachineState *pcms = PC_MACHINE(machine);
> > @@ -2370,36 +2398,30 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> > MachineState *machine)
> > /* the memory map is a bit tricky, it contains at least one hole
> > * from 640k-1M and possibly another one from 3.5G-4G.
> > */
> > -next_base = 0;
> > +
> > numa_start = table_data->len;
> >
> > -numamem = acpi_data_push(table_data, sizeof *numamem);
> > -build_srat_memory(numamem, 0, 640 * 1024, 0, MEM_AFFINITY_ENABLED);
> > -next_base = 1024 * 1024;
> > -for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> > -mem_base = next_base;
> > -mem_len = pcms->node_mem[i - 1];
> > -if (i == 1) {
> > -mem_len -= 1024 * 1024;
> > +/* get the first node which has memory and map the hole from 640K-1M */
> > +for (node = 0; node < pcms->numa_nodes; node++) {
> > +if (pcms->node_me
Re: [Qemu-devel] [PATCH] x86: Skip check apic_id_limit for Xen
On Thu, Aug 17, 2017 at 09:37:10AM +0800, Lan Tianyu wrote: > On 2017年08月16日 19:21, Paolo Bonzini wrote: > > On 16/08/2017 02:22, Lan Tianyu wrote: > >> Xen vIOMMU device model will be in Xen hypervisor. Skip vIOMMU > >> check for Xen here when vcpu number is more than 255. > > > > I think you still need to do a check for vIOMMU being enabled. > > Yes, this will be done in the Xen tool stack and Qemu doesn't have such > knowledge. Operations of create, destroy Xen vIOMMU will be done in the > Xen tool stack. Shouldn't we make QEMU have knowledge of the vIOMMU device, then? Won't QEMU need to know about it eventually? -- Eduardo
[Qemu-devel] [Bug 1089005] Re: Qemu does not shutdown with vnc enabled on OS X
Thanks for checking the latest version, Leander - I changed the status accordingly. ** Changed in: qemu Status: Expired => 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/1089005 Title: Qemu does not shutdown with vnc enabled on OS X Status in QEMU: Fix Released Bug description: I am running OS X 10.8.2 and Qemu 1.3.50 from your git repository. Running qemu-system-i386 works fine. I can quit the process using ctrl-c. When I try to use qemu-system-i386 -vnc : ctrl-c does nothing and I have to kill the process trough the activity monitor. Furthermore terminating the process from my java program does not work either. I have also posted a question on Stackoverflow: http://stackoverflow.com/questions/13798367/qemu-does-not-shutdown-with-vnc-enabled-on-os-x Thanks Leander To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1089005/+subscriptions
Re: [Qemu-devel] [PATCH v2] hw/acpi: Select an node with memory for mapping memory hole to
On Wed, Aug 16, 2017 at 11:18:07AM +0200, Igor Mammedov wrote: > On Wed, 16 Aug 2017 09:26:51 +0800 > Dou Liyang wrote: > > > Currently, Using the fisrt node without memory on the machine makes > > QEMU unhappy. With this example command line: > > ... \ > > -m 1024M,slots=4,maxmem=32G \ > > -numa node,nodeid=0 \ > > -numa node,mem=1024M,nodeid=1 \ > > -numa node,nodeid=2 \ > > -numa node,nodeid=3 \ > > Guest reports "No NUMA configuration found" and the NUMA topology is > > wrong. > > > > This is because when QEMU builds ACPI SRAT, it regards node0 as the > > default node to deal with the memory hole(640K-1M). this means the > > node0 must have some memory(>1M), but, actually it can have no > > memory. > > > > Fix this problem by replace the node0 with the first node which has > > memory on it. Add a new function for each node. Also do some cleanup. > It seems harmless but one never knows for sure, > could you test it with different guests including old windows (up to XP)/ > linux (2.6 stable kernel) versions? This patch is supposed to affect only the cases where there's no RAM configured on node 0. I won't be surprised if some guest OSes don't like it, but in this case the solution is to not configure the VM that way. That means I don't think we really need to test ancient OSes if we ensure there are no ACPI table changes on the existing known-to-work configurations. -- Eduardo
Re: [Qemu-devel] [PATCH v3 for-2.11 08/18] target/s390x: move gtod_*() declarations to s390-virtio.h
Suggesting to add a patch description like: "The functions are not used in target/s390x/ so a header in hw/s390x/ is a better place" ? On 18.08.2017 13:43, David Hildenbrand wrote: > Signed-off-by: David Hildenbrand > --- > hw/s390x/s390-virtio.h | 2 ++ > target/s390x/cpu.h | 3 --- > 2 files changed, 2 insertions(+), 3 deletions(-) Anyway: Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v3 for-2.11 07/18] s390x: drop inclusion of sysemu/kvm.h from some files
On 18.08.2017 13:43, David Hildenbrand wrote: > s390-stattrib.c needs definition of TARGET_PAGE_SIZE, solve it via cpu.h. Why not simply #include "exec/cpu-all.h" ? If cpu.h is not really needed here... Thomas
Re: [Qemu-devel] [PATCH v3 for-2.11 06/18] s390x/cpumodel: factor out determination of default model name
On 18.08.2017 13:43, David Hildenbrand wrote: > Now we can drop inclusion of "sysemu/kvm.h" from "s390-virtio.c". > > Signed-off-by: David Hildenbrand > --- > hw/s390x/s390-virtio.c| 7 +-- > target/s390x/cpu.h| 1 + > target/s390x/cpu_models.c | 8 > 3 files changed, 10 insertions(+), 6 deletions(-) > Reviewed-by: Thomas Huth
