Re: [PATCH v4 11/15] hw/nvme: Calculate BAR attributes in a function
On Jan 26 18:11, Lukasz Maniak wrote:
> From: Łukasz Gieryk
>
> An NVMe device with SR-IOV capability calculates the BAR size
> differently for PF and VF, so it makes sense to extract the common code
> to a separate function.
>
> Signed-off-by: Łukasz Gieryk
> ---
> hw/nvme/ctrl.c | 45 +++--
> 1 file changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 40eb6bd1a8..e101cb7d7c 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6431,6 +6431,34 @@ static void nvme_init_pmr(NvmeCtrl *n, PCIDevice
> *pci_dev)
> memory_region_set_enabled(>pmr.dev->mr, false);
> }
>
> +static uint64_t nvme_bar_size(unsigned total_queues, unsigned total_irqs,
> + unsigned *msix_table_offset,
> + unsigned *msix_pba_offset)
> +{
> +uint64_t bar_size, msix_table_size, msix_pba_size;
> +
> +bar_size = sizeof(NvmeBar) + 2 * total_queues * NVME_DB_SIZE;
> +bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
> +
> +if (msix_table_offset) {
> +*msix_table_offset = bar_size;
> +}
> +
> +msix_table_size = PCI_MSIX_ENTRY_SIZE * total_irqs;
> +bar_size += msix_table_size;
> +bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
> +
> +if (msix_pba_offset) {
> +*msix_pba_offset = bar_size;
> +}
> +
> +msix_pba_size = QEMU_ALIGN_UP(total_irqs, 64) / 8;
> +bar_size += msix_pba_size;
> +
> +bar_size = pow2ceil(bar_size);
> +return bar_size;
> +}
> +
> static void nvme_init_sriov(NvmeCtrl *n, PCIDevice *pci_dev, uint16_t offset,
> uint64_t bar_size)
> {
> @@ -6470,7 +6498,7 @@ static int nvme_add_pm_capability(PCIDevice *pci_dev,
> uint8_t offset)
> static int nvme_init_pci(NvmeCtrl *n, PCIDevice *pci_dev, Error **errp)
> {
> uint8_t *pci_conf = pci_dev->config;
> -uint64_t bar_size, msix_table_size, msix_pba_size;
> +uint64_t bar_size;
> unsigned msix_table_offset, msix_pba_offset;
> int ret;
>
> @@ -6496,19 +6524,8 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice
> *pci_dev, Error **errp)
> }
>
> /* add one to max_ioqpairs to account for the admin queue pair */
> -bar_size = sizeof(NvmeBar) +
> - 2 * (n->params.max_ioqpairs + 1) * NVME_DB_SIZE;
> -bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
> -msix_table_offset = bar_size;
> -msix_table_size = PCI_MSIX_ENTRY_SIZE * n->params.msix_qsize;
> -
> -bar_size += msix_table_size;
> -bar_size = QEMU_ALIGN_UP(bar_size, 4 * KiB);
> -msix_pba_offset = bar_size;
> -msix_pba_size = QEMU_ALIGN_UP(n->params.msix_qsize, 64) / 8;
> -
> -bar_size += msix_pba_size;
> -bar_size = pow2ceil(bar_size);
> +bar_size = nvme_bar_size(n->params.max_ioqpairs + 1,
> n->params.msix_qsize,
> + _table_offset, _pba_offset);
>
> memory_region_init(>bar0, OBJECT(n), "nvme-bar0", bar_size);
> memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n, "nvme",
> --
> 2.25.1
>
Looks good,
Reviewed-by: Klaus Jensen
signature.asc
Description: PGP signature
Re: [PATCH v4 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers
On Jan 26 18:11, Lukasz Maniak wrote:
> From: Łukasz Gieryk
>
> With four new properties:
> - sriov_v{i,q}_flexible,
> - sriov_max_v{i,q}_per_vf,
> one can configure the number of available flexible resources, as well as
> the limits. The primary and secondary controller capability structures
> are initialized accordingly.
>
> Since the number of available queues (interrupts) now varies between
> VF/PF, BAR size calculation is also adjusted.
>
> Signed-off-by: Łukasz Gieryk
> ---
> hw/nvme/ctrl.c | 142 ---
> hw/nvme/nvme.h | 4 ++
> include/block/nvme.h | 5 ++
> 3 files changed, 144 insertions(+), 7 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index e101cb7d7c..551c8795f2 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6379,14 +6464,41 @@ static void nvme_init_state(NvmeCtrl *n)
> n->starttime_ms = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
>
> -list->numcntl = cpu_to_le16(n->params.sriov_max_vfs);
> -for (i = 0; i < n->params.sriov_max_vfs; i++) {
> +list->numcntl = cpu_to_le16(max_vfs);
> +for (i = 0; i < max_vfs; i++) {
> sctrl = >sec[i];
> sctrl->pcid = cpu_to_le16(n->cntlid);
> sctrl->vfn = cpu_to_le16(i + 1);
> }
>
> cap->cntlid = cpu_to_le16(n->cntlid);
> +cap->crt = NVME_CRT_VQ | NVME_CRT_VI;
> +
> +if (pci_is_vf(>parent_obj)) {
> +cap->vqprt = cpu_to_le16(1 + n->conf_ioqpairs);
> +} else {
> +cap->vqprt = cpu_to_le16(1 + n->params.max_ioqpairs -
> + n->params.sriov_vq_flexible);
> +cap->vqfrt = cpu_to_le32(n->params.sriov_vq_flexible);
> +cap->vqrfap = cap->vqfrt;
> +cap->vqgran = cpu_to_le16(NVME_VF_RES_GRANULARITY);
> +cap->vqfrsm = n->params.sriov_max_vq_per_vf ?
> +cpu_to_le16(n->params.sriov_max_vq_per_vf) :
> +cap->vqfrt / n->params.sriov_max_vfs;
Getting a division by zero on non-sriov enabled controllers here.
> +}
> +
> +if (pci_is_vf(>parent_obj)) {
> +cap->viprt = cpu_to_le16(n->conf_msix_qsize);
> +} else {
> +cap->viprt = cpu_to_le16(n->params.msix_qsize -
> + n->params.sriov_vi_flexible);
> +cap->vifrt = cpu_to_le32(n->params.sriov_vi_flexible);
> +cap->virfap = cap->vifrt;
> +cap->vigran = cpu_to_le16(NVME_VF_RES_GRANULARITY);
> +cap->vifrsm = n->params.sriov_max_vi_per_vf ?
> +cpu_to_le16(n->params.sriov_max_vi_per_vf) :
> +cap->vifrt / n->params.sriov_max_vfs;
Same here.
signature.asc
Description: PGP signature
Re: [PATCH v4 15/15] hw/nvme: Update the initalization place for the AER queue
On Jan 26 18:11, Lukasz Maniak wrote: > From: Łukasz Gieryk > > This patch updates the initialization place for the AER queue, so it’s > initialized once, at controller initialization, and not every time > controller is enabled. > > While the original version works for a non-SR-IOV device, as it’s hard > to interact with the controller if it’s not enabled, the multiple > reinitialization is not necessarily correct. > > With the SR/IOV feature enabled a segfault can happen: a VF can have its > controller disabled, while a namespace can still be attached to the > controller through the parent PF. An event generated in such case ends > up on an uninitialized queue. > > While it’s an interesting question whether a VF should support AER in > the first place, I don’t think it must be answered today. > > Signed-off-by: Łukasz Gieryk > --- > hw/nvme/ctrl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 624db2f9c6..b2228e960f 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -6029,8 +6029,6 @@ static int nvme_start_ctrl(NvmeCtrl *n) > > nvme_set_timestamp(n, 0ULL); > > -QTAILQ_INIT(>aer_queue); > - > nvme_select_iocs(n); > > return 0; > @@ -7007,6 +7005,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice > *pci_dev) > id->cmic |= NVME_CMIC_MULTI_CTRL; > } > > +QTAILQ_INIT(>aer_queue); > + > NVME_CAP_SET_MQES(cap, 0x7ff); > NVME_CAP_SET_CQR(cap, 1); > NVME_CAP_SET_TO(cap, 0xf); > -- > 2.25.1 > Fix is good, but I think this belongs in nvme_init_state(). Otherwise, Reviewed-by: Klaus Jensen signature.asc Description: PGP signature
Re: [PATCH v4 00/15] hw/nvme: SR-IOV with Virtualization Enhancements
On Jan 26 18:11, Lukasz Maniak wrote: > Changes since v3: > - Addressed comments to review on pcie: Add support for Single Root I/O > Virtualization (SR/IOV) > - Fixed issues reported by checkpatch.pl > > Knut Omang (2): > pcie: Add support for Single Root I/O Virtualization (SR/IOV) > pcie: Add some SR/IOV API documentation in docs/pcie_sriov.txt > > Lukasz Maniak (4): > hw/nvme: Add support for SR-IOV > hw/nvme: Add support for Primary Controller Capabilities > hw/nvme: Add support for Secondary Controller List > docs: Add documentation for SR-IOV and Virtualization Enhancements > > Łukasz Gieryk (9): > pcie: Add a helper to the SR/IOV API > pcie: Add 1.2 version token for the Power Management Capability > hw/nvme: Implement the Function Level Reset > hw/nvme: Make max_ioqpairs and msix_qsize configurable in runtime > hw/nvme: Remove reg_size variable and update BAR0 size calculation > hw/nvme: Calculate BAR attributes in a function > hw/nvme: Initialize capability structures for primary/secondary > controllers > hw/nvme: Add support for the Virtualization Management command > hw/nvme: Update the initalization place for the AER queue > > docs/pcie_sriov.txt | 115 ++ > docs/system/devices/nvme.rst | 36 ++ > hw/nvme/ctrl.c | 675 --- > hw/nvme/ns.c | 2 +- > hw/nvme/nvme.h | 55 ++- > hw/nvme/subsys.c | 75 +++- > hw/nvme/trace-events | 6 + > hw/pci/meson.build | 1 + > hw/pci/pci.c | 100 -- > hw/pci/pcie.c| 5 + > hw/pci/pcie_sriov.c | 302 > hw/pci/trace-events | 5 + > include/block/nvme.h | 65 > include/hw/pci/pci.h | 12 +- > include/hw/pci/pci_ids.h | 1 + > include/hw/pci/pci_regs.h| 1 + > include/hw/pci/pcie.h| 6 + > include/hw/pci/pcie_sriov.h | 77 > include/qemu/typedefs.h | 2 + > 19 files changed, 1460 insertions(+), 81 deletions(-) > create mode 100644 docs/pcie_sriov.txt > create mode 100644 hw/pci/pcie_sriov.c > create mode 100644 include/hw/pci/pcie_sriov.h > > -- > 2.25.1 > > Hi Lukasz, Back in v3 you changed this: - Secondary controller cannot be set online unless the corresponding VF is enabled (sriov_numvfs set to at least the secondary controller's VF number) I'm having issues getting this to work now. As I understand it, this now requires that sriov_numvfs is set prior to onlining the devices, i.e.: echo 1 > /sys/bus/pci/devices/\:01\:00.0/sriov_numvfs However, this causes the kernel to reject it: nvme nvme1: Device not ready; aborting initialisation, CSTS=0x2 nvme nvme1: Removing after probe failure status: -19 Is this the expected behavior? Must I manually bind the device again to the nvme driver? Prior to v3 this worked just fine since the VF was onlined at this point. It would be useful if you added a small "onlining for dummies" section to the docs ;) signature.asc Description: PGP signature
Re: qemu iotest 161 and make check
On 10/02/2022 15.51, Christian Borntraeger wrote:
Am 10.02.22 um 15:47 schrieb Vladimir Sementsov-Ogievskiy:
10.02.2022 10:57, Christian Borntraeger wrote:
Hello,
I do see spurious failures of 161 in our CI, but only when I use
make check with parallelism (-j).
I have not yet figured out which other testcase could interfere
@@ -34,6 +34,8 @@
*** Commit and then change an option on the backing file
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+qemu-img: TEST_DIR/t.IMGFMT.base: Failed to get "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT.base]?
Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
{ 'execute': 'qmp_capabilities' }
any ideas?
Hmm, interesting.. Is it always 161 and always exactly this diff?
Its always 161 and only 161. I would need to check if its always the same
error.
First, this place in 161 is usual: we just create and image, like in many
other tests.
Second, why _make_test_img trigger "Failed to get write lock"? It should
just create an image. Hmm. And probably starts QSD if protocol is fuse.
So, that start of QSD may probably fail.. Is that the case? What is image
format and protocol used in test run?
But anyway, tests running in parallel should not break each other as each
test has own TEST_DIR and SOCK_DIR..
Unless you run into the issue that Hanna described here:
https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01735.html
Thomas
Re: [PATCH v5 03/20] job.c: API functions not used outside should be static
On Tue, Feb 08, 2022 at 09:34:56AM -0500, Emanuele Giuseppe Esposito wrote: > job_event_* functions can all be static, as they are not used > outside job.c. > > Same applies for job_txn_add_job(). > > Signed-off-by: Emanuele Giuseppe Esposito > --- > include/qemu/job.h | 18 -- > job.c | 12 +--- > 2 files changed, 9 insertions(+), 21 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 86ec46c09e..6000463126 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -356,18 +356,6 @@ JobTxn *job_txn_new(void); > */ > void job_txn_unref(JobTxn *txn); > > -/** > - * @txn: The transaction (may be NULL) > - * @job: Job to add to the transaction > - * > - * Add @job to the transaction. The @job must not already be in a > transaction. > - * The caller must call either job_txn_unref() or job_completed() to release > - * the reference that is automatically grabbed here. > - * > - * If @txn is NULL, the function does nothing. > - */ > -void job_txn_add_job(JobTxn *txn, Job *job); ... > diff --git a/job.c b/job.c > index d603f9ad1e..f13939d3c6 100644 > --- a/job.c > +++ b/job.c > @@ -125,7 +125,7 @@ void job_txn_unref(JobTxn *txn) > } > } > > -void job_txn_add_job(JobTxn *txn, Job *job) > +static void job_txn_add_job(JobTxn *txn, Job *job) Please move the doc comments into the .c file too. signature.asc Description: PGP signature
Re: [PATCH v12 4/8] qmp: add QMP command x-query-virtio-status
> This new command shows the status of a VirtIODevice, including
> its corresponding vhost device's status (if active).
>
> Next patch will improve output by decoding feature bits, including
> vhost device's feature bits (backend, protocol, acked, and features).
> Also will decode status bits of a VirtIODevice.
>
> [Jonah: Similar to previous patch, added a check to @virtio_device_find
> to ensure synchronicity between @virtio_list and the devices in the QOM
> composition tree.]
>
> Signed-off-by: Jonah Palmer
> ---
> hw/virtio/virtio-stub.c | 5 ++
> hw/virtio/virtio.c | 104 +++
> qapi/virtio.json| 222
>
> 3 files changed, 331 insertions(+)
>
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> index 05a81ed..0b432e8 100644
> --- a/hw/virtio/virtio-stub.c
> +++ b/hw/virtio/virtio-stub.c
> @@ -12,3 +12,8 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
> {
> return qmp_virtio_unsupported(errp);
> }
> +
> +VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> +{
> +return qmp_virtio_unsupported(errp);
> +}
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e59f0d7..30ccd7b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3928,6 +3928,110 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
> return list;
> }
>
> +static VirtIODevice *virtio_device_find(const char *path)
> +{
> +VirtIODevice *vdev;
> +
> +QTAILQ_FOREACH(vdev, _list, next) {
> +DeviceState *dev = DEVICE(vdev);
> +
> +if (strcmp(dev->canonical_path, path) != 0) {
> +continue;
> +}
> +
> +Error *err = NULL;
> +QObject *obj = qmp_qom_get(dev->canonical_path, "realized", );
> +if (err == NULL) {
> +GString *is_realized = qobject_to_json_pretty(obj, true);
> +/* virtio device is NOT realized, remove it from list */
> +if (!strncmp(is_realized->str, "false", 4)) {
> +g_string_free(is_realized, true);
> +qobject_unref(obj);
> +QTAILQ_REMOVE(_list, vdev, next);
> +return NULL;
> +}
> +g_string_free(is_realized, true);
> +} else {
> +/* virtio device doesn't exist in QOM tree */
> +QTAILQ_REMOVE(_list, vdev, next);
> +qobject_unref(obj);
> +return NULL;
> +}
> +/* device exists in QOM tree & is realized */
> +qobject_unref(obj);
> +return vdev;
> +}
> +return NULL;
> +}
> +
> +VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> +{
> +VirtIODevice *vdev;
> +VirtioStatus *status;
> +
> +vdev = virtio_device_find(path);
> +if (vdev == NULL) {
> +error_setg(errp, "Path %s is not a VirtIODevice", path);
> +return NULL;
> +}
> +
> +status = g_new0(VirtioStatus, 1);
> +status->name = g_strdup(vdev->name);
> +status->device_id = vdev->device_id;
> +status->vhost_started = vdev->vhost_started;
> +status->guest_features = vdev->guest_features;
> +status->host_features = vdev->host_features;
> +status->backend_features = vdev->backend_features;
> +
> +switch (vdev->device_endian) {
> +case VIRTIO_DEVICE_ENDIAN_LITTLE:
> +status->device_endian = g_strdup("little");
> +break;
> +case VIRTIO_DEVICE_ENDIAN_BIG:
> +status->device_endian = g_strdup("big");
> +break;
> +default:
> +status->device_endian = g_strdup("unknown");
> +break;
> +}
> +
> +status->num_vqs = virtio_get_num_queues(vdev);
> +status->status = vdev->status;
> +status->isr = vdev->isr;
> +status->queue_sel = vdev->queue_sel;
> +status->vm_running = vdev->vm_running;
> +status->broken = vdev->broken;
> +status->disabled = vdev->disabled;
> +status->use_started = vdev->use_started;
> +status->started = vdev->started;
> +status->start_on_kick = vdev->start_on_kick;
> +status->disable_legacy_check = vdev->disable_legacy_check;
> +status->bus_name = g_strdup(vdev->bus_name);
> +status->use_guest_notifier_mask = vdev->use_guest_notifier_mask;
> +status->has_vhost_dev = vdev->vhost_started;
> +
> +if (vdev->vhost_started) {
> +VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +struct vhost_dev *hdev = vdc->get_vhost(vdev);
> +
> +status->vhost_dev = g_new0(VhostStatus, 1);
> +status->vhost_dev->n_mem_sections = hdev->n_mem_sections;
> +status->vhost_dev->n_tmp_sections = hdev->n_tmp_sections;
> +status->vhost_dev->nvqs = hdev->nvqs;
> +status->vhost_dev->vq_index = hdev->vq_index;
> +status->vhost_dev->features = hdev->features;
> +status->vhost_dev->acked_features = hdev->acked_features;
> +status->vhost_dev->backend_features = hdev->backend_features;
> +
Re: [PATCH 6/6] jobs: ensure sleep in job_sleep_ns is fully performed
08.02.2022 18:36, Emanuele Giuseppe Esposito wrote:
If a drain happens while a job is sleeping, the timeout
gets cancelled and the job continues once the drain ends.
This is especially bad for the sleep performed in commit and stream
jobs, since that is dictated by ratelimit to maintain a certain speed.
Basically the execution path is the following:
1. job calls job_sleep_ns, and yield with a timer in @ns ns.
2. meanwhile, a drain is executed, and
child_job_drained_{begin/end} could be executed as ->drained_begin()
and ->drained_end() callbacks.
Therefore child_job_drained_begin() enters the job, that continues
execution in job_sleep_ns() and calls job_pause_point_locked().
3. job_pause_point_locked() detects that we are in the middle of a
drain, and firstly deletes any existing timer and then yields again,
waiting for ->drained_end().
4. Once draining is finished, child_job_drained_end() runs and resumes
the job. At this point, the timer has been lost and we just resume
without checking if enough time has passed.
This fix implies that from now onwards, job_sleep_ns will force the job
to sleep @ns, even if it is wake up (purposefully or not) in the middle
of the sleep. Therefore qemu-iotests test might run a little bit slower,
depending on the speed of the job. Setting a job speed to values like "1"
is not allowed anymore (unless you want to wait forever).
Because of this fix, test_stream_parallel() in tests/qemu-iotests/030
takes too long, since speed of stream job is just 1024 and before
it was skipping all the wait thanks to the drains. Increase the
speed to 256 * 1024. Exactly the same happens for test 151.
Instead we need to sleep less in test_cancel_ready() test-blockjob.c,
so that the job will be able to exit the sleep and transition to ready
before the main loop asserts.
Signed-off-by: Emanuele Giuseppe Esposito
Acked-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [PATCH v12 3/8] qmp: add QMP command x-query-virtio
> This new command lists all the instances of VirtIODevices with
> their canonical QOM path and name.
>
> [Jonah: @virtio_list duplicates information that already exists in
> the QOM composition tree. However, extracting necessary information
> from this tree seems to be a bit convoluted.
>
> Instead, we still create our own list of realized virtio devices
> but use @qmp_qom_get with the device's canonical QOM path to confirm
> that the device exists and is realized. If the device exists but
> is actually not realized, then we remove it from our list (for
> synchronicity to the QOM composition tree).
>
> Also, the QMP command @x-query-virtio is redundant as @qom-list
> and @qom-get are sufficient to search '/machine/' for realized
> virtio devices. However, @x-query-virtio is much more convenient
> in listing realized virtio devices.]
>
> Signed-off-by: Jonah Palmer
> ---
> hw/virtio/meson.build | 2 ++
> hw/virtio/virtio-stub.c| 14 ++
> hw/virtio/virtio.c | 44 ++
> include/hw/virtio/virtio.h | 1 +
> qapi/meson.build | 1 +
> qapi/qapi-schema.json | 1 +
> qapi/virtio.json | 68
> ++
> tests/qtest/qmp-cmd-test.c | 1 +
> 8 files changed, 132 insertions(+)
> create mode 100644 hw/virtio/virtio-stub.c
> create mode 100644 qapi/virtio.json
>
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index 521f7d6..d893f5f 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -6,8 +6,10 @@ softmmu_virtio_ss.add(when: 'CONFIG_VHOST', if_false:
> files('vhost-stub.c'))
>
> softmmu_ss.add_all(when: 'CONFIG_VIRTIO', if_true: softmmu_virtio_ss)
> softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('vhost-stub.c'))
> +softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('virtio-stub.c'))
>
> softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
> +softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
>
> virtio_ss = ss.source_set()
> virtio_ss.add(files('virtio.c'))
> diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
> new file mode 100644
> index 000..05a81ed
> --- /dev/null
> +++ b/hw/virtio/virtio-stub.c
> @@ -0,0 +1,14 @@
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-virtio.h"
> +
> +static void *qmp_virtio_unsupported(Error **errp)
> +{
> +error_setg(errp, "Virtio is disabled");
> +return NULL;
> +}
> +
> +VirtioInfoList *qmp_x_query_virtio(Error **errp)
> +{
> +return qmp_virtio_unsupported(errp);
> +}
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7c1b1dd..e59f0d7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -13,12 +13,18 @@
>
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qapi-commands-virtio.h"
> +#include "qapi/qapi-commands-qom.h"
> +#include "qapi/qapi-visit-virtio.h"
> +#include "qapi/qmp/qjson.h"
> #include "cpu.h"
> #include "trace.h"
> #include "qemu/error-report.h"
> #include "qemu/log.h"
> #include "qemu/main-loop.h"
> #include "qemu/module.h"
> +#include "qom/object_interfaces.h"
> #include "hw/virtio/virtio.h"
> #include "migration/qemu-file-types.h"
> #include "qemu/atomic.h"
> @@ -29,6 +35,9 @@
> #include "sysemu/runstate.h"
> #include "standard-headers/linux/virtio_ids.h"
>
> +/* QAPI list of realized VirtIODevices */
> +static QTAILQ_HEAD(, VirtIODevice) virtio_list;
> +
> /*
> * The alignment to use between consumer and producer parts of vring.
> * x86 pagesize again. This is the default, used by transports like PCI
> @@ -3687,6 +3696,7 @@ static void virtio_device_realize(DeviceState *dev,
> Error **errp)
> vdev->listener.commit = virtio_memory_listener_commit;
> vdev->listener.name = "virtio";
> memory_listener_register(>listener, vdev->dma_as);
> +QTAILQ_INSERT_TAIL(_list, vdev, next);
> }
>
> static void virtio_device_unrealize(DeviceState *dev)
> @@ -3701,6 +3711,7 @@ static void virtio_device_unrealize(DeviceState *dev)
> vdc->unrealize(dev);
> }
>
> +QTAILQ_REMOVE(_list, vdev, next);
> g_free(vdev->bus_name);
> vdev->bus_name = NULL;
> }
> @@ -3874,6 +3885,8 @@ static void virtio_device_class_init(ObjectClass
> *klass, void *data)
> vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
>
> vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
> +
> +QTAILQ_INIT(_list);
> }
>
> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
> @@ -3884,6 +3897,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice
> *vdev)
> return virtio_bus_ioeventfd_enabled(vbus);
> }
>
> +VirtioInfoList *qmp_x_query_virtio(Error **errp)
> +{
> +VirtioInfoList *list = NULL;
> +VirtioInfoList *node;
> +VirtIODevice *vdev;
> +
> +QTAILQ_FOREACH(vdev, _list, next) {
> +DeviceState *dev = DEVICE(vdev);
> +Error *err = NULL;
> +
Re: [PATCH v12 1/8] virtio: drop name parameter for virtio_init()
> This patch drops the name parameter for the virtio_init function.
>
> The pair between the numeric device ID and the string device ID
> (name) of a virtio device already exists, but not in a way that
> lets us map between them.
>
> This patch lets us do this and removes the need for the name
> parameter in the virtio_init function.
>
> [Jonah: added new virtio IDs to virtio device list from rebase].
>
> Signed-off-by: Jonah Palmer
> ---
> hw/9pfs/virtio-9p-device.c | 2 +-
> hw/block/vhost-user-blk.c | 2 +-
> hw/block/virtio-blk.c | 2 +-
> hw/char/virtio-serial-bus.c| 3 +-
> hw/display/virtio-gpu-base.c | 2 +-
> hw/input/virtio-input.c| 3 +-
> hw/net/virtio-net.c| 2 +-
> hw/scsi/virtio-scsi.c | 3 +-
> hw/virtio/vhost-user-fs.c | 3 +-
> hw/virtio/vhost-user-i2c.c | 7 +
> hw/virtio/vhost-user-rng.c | 2 +-
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c | 5 ++--
> hw/virtio/vhost-vsock.c| 2 +-
> hw/virtio/virtio-balloon.c | 3 +-
> hw/virtio/virtio-crypto.c | 2 +-
> hw/virtio/virtio-iommu.c | 3 +-
> hw/virtio/virtio-mem.c | 3 +-
> hw/virtio/virtio-pmem.c| 3 +-
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 55
> --
> include/hw/virtio/vhost-vsock-common.h | 2 +-
> include/hw/virtio/virtio-gpu.h | 3 +-
> include/hw/virtio/virtio.h | 4 +--
> 24 files changed, 77 insertions(+), 43 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 54ee93b..5f522e6 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -216,7 +216,7 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp)
> }
>
> v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
> -virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
> +virtio_init(vdev, VIRTIO_ID_9P, v->config_size);
> v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> }
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 1a42ae9..e8cb170 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp)
> return;
> }
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> +virtio_init(vdev, VIRTIO_ID_BLOCK,
> sizeof(struct virtio_blk_config));
>
> s->virtqs = g_new(VirtQueue *, s->num_queues);
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 82676cd..2e3809d 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1205,7 +1205,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
> Error **errp)
>
> virtio_blk_set_config_size(s, s->host_features);
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> +virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
>
> s->blk = conf->conf.blk;
> s->rq = NULL;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index f01ec21..9f19fd0 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1044,8 +1044,7 @@ static void virtio_serial_device_realize(DeviceState
> *dev, Error **errp)
> VIRTIO_CONSOLE_F_EMERG_WRITE)) {
> config_size = offsetof(struct virtio_console_config, emerg_wr);
> }
> -virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
> -config_size);
> +virtio_init(vdev, VIRTIO_ID_CONSOLE, config_size);
>
> /* Spawn a new virtio-serial bus on which the ports will ride as devices
> */
> qbus_init(>bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index fff0fb4..8ba5da4 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -173,7 +173,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
> }
>
> g->virtio_config.num_scanouts = cpu_to_le32(g->conf.max_outputs);
> -virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
> +virtio_init(VIRTIO_DEVICE(g), VIRTIO_ID_GPU,
> sizeof(struct virtio_gpu_config));
>
> if (virtio_gpu_virgl_enabled(g->conf)) {
> diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
> index 54bcb46..5b5398b 100644
> --- a/hw/input/virtio-input.c
> +++ b/hw/input/virtio-input.c
> @@ -257,8 +257,7 @@ static void virtio_input_device_realize(DeviceState *dev,
> Error **errp)
> vinput->cfg_size += 8;
> assert(vinput->cfg_size <= sizeof(virtio_input_config));
>
> -virtio_init(vdev, "virtio-input",
Re: [PATCH v6 00/33] block layer: split block APIs in global state and I/O
On 08/02/2022 14:08, Kevin Wolf wrote: > Am 08.02.2022 um 12:42 hat Emanuele Giuseppe Esposito geschrieben: >> >> >> On 07/02/2022 19:30, Kevin Wolf wrote: >>> Am 21.01.2022 um 18:05 hat Emanuele Giuseppe Esposito geschrieben: Each function in the GS API will have an assertion, checking that it is always running under BQL. I/O functions are instead thread safe (or so should be), meaning that they *can* run under BQL, but also in an iothread in another AioContext. Therefore they do not provide any assertion, and need to be audited manually to verify the correctness. >>> >>> I wonder if we could actually do something to catch at least some kinds >>> of bugs. The first conclusion from thinking about it is that we probably >>> shouldn't open-code assert(qemu_in_main_thread()) everywhere, but have a >>> macro or inline function for each category to be called in each function. >>> >>> So an IO_CODE() macro could increase a counter in the coroutine object >>> (that is decreased again at the end of the function with g_auto), and >>> then GLOBAL_STATE_CODE() could not only assert that we're holding the >>> BQL, but also that the counter is still 0, i.e. it is not (indirectly) >>> called by an I/O function. >>> >>> We may want to enable this only in debug builds, but maybe still worth a >>> thought anyway? >> >> I don't understand what is the point of the counter, do you want to use >> it as a boolean flag? > > It would only be checked as a boolean flag, but it needs to be a counter > because of nesting where e.g. one I/O function calls another I/O > function. > >> Would a single counter work in a multi-threaded context? Shouldn't we >> have it per-thread? And why you increase it only in coroutines? > > I don't mean increasing it only in coroutine context, but having a > per-coroutine counter, including the leader coroutine which exists for > non-coroutine context in every thread. > As agreed also on IRC, while we wait also additional feedback from others on the counter logic, I am going to add GLOBAL_STATE_CODE, IO_CODE and IO_OR_GS_CODE macros in the respective functions. GLOBAL_STATE_CODE will just replace the assert(qemu_in_main_thread()), while IO_CODE and IO_OR_GS_CODE will be nop. This will also visually help understanding in which category each function is, without looking at the header. In the future we can extend the macro to support also additional logic, like counters. Emanuele
Re: [PATCH v5 02/20] job.h: categorize fields in struct Job
On Thu, Feb 10, 2022 at 05:26:52PM +0100, Emanuele Giuseppe Esposito wrote:
>
>
> On 10/02/2022 16:40, Stefan Hajnoczi wrote:
> > On Tue, Feb 08, 2022 at 09:34:55AM -0500, Emanuele Giuseppe Esposito wrote:
> >> Categorize the fields in struct Job to understand which ones
> >> need to be protected by the job mutex and which don't.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito
> >> ---
> >> include/qemu/job.h | 59 ++
> >> 1 file changed, 34 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/include/qemu/job.h b/include/qemu/job.h
> >> index d1192ffd61..86ec46c09e 100644
> >> --- a/include/qemu/job.h
> >> +++ b/include/qemu/job.h
> >> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
> >> * Long-running operation.
> >> */
> >> typedef struct Job {
> >> +
> >> +/* Fields set at initialization (job_create), and never modified */
> >
> > Is there a corresponding "Field protected by job_mutex" comment that
> > separates fields that need locking?
> >
>
> That would be the comment
>
> /** Protected by job_mutex */
>
> situated right after the field "ProgressMeter progress;".
>
> Do you want me to change it in "Fields protected by job_mutex"?
I don't see it:
+/** The opaque value that is passed to the completion function. */
+void *opaque;
+
+/* ProgressMeter API is thread-safe */
+ProgressMeter progress;
+
+
+/** AioContext to run the job coroutine in */
+AioContext *aio_context;
+
+/** Reference count of the block job */
+int refcnt;
Is it added by a later patch or did I miss it?
Stefan
signature.asc
Description: PGP signature
Re: qemu iotest 161 and make check
10.02.2022 20:13, Thomas Huth wrote:
On 10/02/2022 15.51, Christian Borntraeger wrote:
Am 10.02.22 um 15:47 schrieb Vladimir Sementsov-Ogievskiy:
10.02.2022 10:57, Christian Borntraeger wrote:
Hello,
I do see spurious failures of 161 in our CI, but only when I use
make check with parallelism (-j).
I have not yet figured out which other testcase could interfere
@@ -34,6 +34,8 @@
*** Commit and then change an option on the backing file
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+qemu-img: TEST_DIR/t.IMGFMT.base: Failed to get "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT.base]?
Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
{ 'execute': 'qmp_capabilities' }
any ideas?
Hmm, interesting.. Is it always 161 and always exactly this diff?
Its always 161 and only 161. I would need to check if its always the same error.
First, this place in 161 is usual: we just create and image, like in many other
tests.
Second, why _make_test_img trigger "Failed to get write lock"? It should just
create an image. Hmm. And probably starts QSD if protocol is fuse. So, that start of QSD
may probably fail.. Is that the case? What is image format and protocol used in test run?
But anyway, tests running in parallel should not break each other as each test
has own TEST_DIR and SOCK_DIR..
Unless you run into the issue that Hanna described here:
https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01735.html
Yes, we can't execute same test several times (for different formats) in
parallel.. But that's about any test, not only 161.
And I don't think that it's currently possible that we run same test in
parallel several times somewhere, do we? In tests/check-block.sh we have a
sequential loop through $format_list ..
--
Best regards,
Vladimir
Re: [PATCH v5 02/20] job.h: categorize fields in struct Job
On 10/02/2022 16:40, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 09:34:55AM -0500, Emanuele Giuseppe Esposito wrote:
>> Categorize the fields in struct Job to understand which ones
>> need to be protected by the job mutex and which don't.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> include/qemu/job.h | 59 ++
>> 1 file changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index d1192ffd61..86ec46c09e 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
>> * Long-running operation.
>> */
>> typedef struct Job {
>> +
>> +/* Fields set at initialization (job_create), and never modified */
>
> Is there a corresponding "Field protected by job_mutex" comment that
> separates fields that need locking?
>
That would be the comment
/** Protected by job_mutex */
situated right after the field "ProgressMeter progress;".
Do you want me to change it in "Fields protected by job_mutex"?
Thank you,
Emanuele
Re: qemu iotest 161 and make check
Am 10.02.22 um 15:47 schrieb Vladimir Sementsov-Ogievskiy:
10.02.2022 10:57, Christian Borntraeger wrote:
Hello,
I do see spurious failures of 161 in our CI, but only when I use
make check with parallelism (-j).
I have not yet figured out which other testcase could interfere
@@ -34,6 +34,8 @@
*** Commit and then change an option on the backing file
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+qemu-img: TEST_DIR/t.IMGFMT.base: Failed to get "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT.base]?
Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
{ 'execute': 'qmp_capabilities' }
any ideas?
Hmm, interesting.. Is it always 161 and always exactly this diff?
Its always 161 and only 161. I would need to check if its always the same error.
First, this place in 161 is usual: we just create and image, like in many other
tests.
Second, why _make_test_img trigger "Failed to get write lock"? It should just
create an image. Hmm. And probably starts QSD if protocol is fuse. So, that start of QSD
may probably fail.. Is that the case? What is image format and protocol used in test run?
But anyway, tests running in parallel should not break each other as each test
has own TEST_DIR and SOCK_DIR..
Re: qemu iotest 161 and make check
10.02.2022 10:57, Christian Borntraeger wrote:
Hello,
I do see spurious failures of 161 in our CI, but only when I use
make check with parallelism (-j).
I have not yet figured out which other testcase could interfere
@@ -34,6 +34,8 @@
*** Commit and then change an option on the backing file
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+qemu-img: TEST_DIR/t.IMGFMT.base: Failed to get "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT.base]?
Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
{ 'execute': 'qmp_capabilities' }
any ideas?
Hmm, interesting.. Is it always 161 and always exactly this diff?
First, this place in 161 is usual: we just create and image, like in many other
tests.
Second, why _make_test_img trigger "Failed to get write lock"? It should just
create an image. Hmm. And probably starts QSD if protocol is fuse. So, that start of QSD
may probably fail.. Is that the case? What is image format and protocol used in test run?
But anyway, tests running in parallel should not break each other as each test
has own TEST_DIR and SOCK_DIR..
--
Best regards,
Vladimir
Re: [PATCH v11 4/8] qmp: add QMP command x-query-virtio-status
Jonah Palmer writes: > From: Laurent Vivier > > This new command shows the status of a VirtIODevice, including > its corresponding vhost device's status (if active). > > Next patch will improve output by decoding feature bits, including > vhost device's feature bits (backend, protocol, acked, and features). > Also will decode status bits of a VirtIODevice. > > [Jonah: Similar to previous patch, added a check to @virtio_device_find > to ensure synchronicity between @virtio_list and the devices in the QOM > composition tree.] > > Signed-off-by: Jonah Palmer QAPI schema Acked-by: Markus Armbruster
Re: [PATCH v11 3/8] qmp: add QMP command x-query-virtio
Jonah Palmer writes: > From: Laurent Vivier > > This new command lists all the instances of VirtIODevices with > their canonical QOM path and name. > > [Jonah: @virtio_list duplicates information that already exists in > the QOM composition tree. However, extracting necessary information > from this tree seems to be a bit convoluted. > > Instead, we still create our own list of realized virtio devices > but use @qmp_qom_get with the device's canonical QOM path to confirm > that the device exists and is realized. If the device exists but > is actually not realized, then we remove it from our list (for > synchronicity to the QOM composition tree). > > Also, the QMP command @x-query-virtio is redundant as @qom-list > and @qom-get are sufficient to search '/machine/' for realized > virtio devices. However, @x-query-virtio is much more convenient > in listing realized virtio devices.] Thanks for explaining this. Whether the convenience is worth the extra code is for the virtio maintainer to decide. > > Signed-off-by: Jonah Palmer QAPI schema Acked-by: Markus Armbruster
Re: [PATCH 4/6] test-bdrv-drain.c: adapt test to the coming subtree drains
On 10/02/2022 15:32, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 10:36:53AM -0500, Emanuele Giuseppe Esposito wrote:
>> There will be 2 problems in this test when we will add
>> subtree drains in bdrv_replace_child_noperm:
>>
>> - First, the test is inconsistent about taking the AioContext lock when
>> calling bdrv_replace_child_noperm. bdrv_replace_child_noperm is reached
>> in two places: from blk_insert_bs directly, and via block_job_create.
>> Only the second does it with the AioContext lock taken, and there seems
>> to be no reason why the lock is needed.
>> Move aio_context_acquire further down, to just protect block_job_add_bdrv()
>>
>> - Second, test_detach_indirect is only interested in observing the first
>> call to .drained_begin. In the original test, there was only a single
>> subtree drain; however, with additional drains introduced in
>> bdrv_replace_child_noperm(), the test callback would be called too early
>> and/or multiple times.
>> Override the callback only when we actually want to use it, and put back
>> the original after it's been invoked.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> tests/unit/test-bdrv-drain.c | 17 -
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
>> index 4924ceb562..c52ba2db4e 100644
>> --- a/tests/unit/test-bdrv-drain.c
>> +++ b/tests/unit/test-bdrv-drain.c
>> @@ -912,12 +912,12 @@ static void test_blockjob_common_drain_node(enum
>> drain_type drain_type,
>> blk_insert_bs(blk_target, target, _abort);
>> blk_set_allow_aio_context_change(blk_target, true);
>>
>> -aio_context_acquire(ctx);
>> tjob = block_job_create("job0", _job_driver, NULL, src,
>> 0, BLK_PERM_ALL,
>> 0, 0, NULL, NULL, _abort);
>> tjob->bs = src;
>> job = >common;
>> +aio_context_acquire(ctx);
>
> block_job_create() uses src's AioContext. In the IOThread case the
> AioContext is not qemu_aio_context. My expectation is that src's
> AioContext must be acquired before block_job_create() starts using src.
>
> blockdev.c QMP commands acquire the BDS's AioContext before calling the
> function that creates the job. It seems strange to do it differently in
> this test case.
>
> You mentioned that blk_insert_bs() is called without acquiring an
> AioContext. That may be because we know blk_target is in
> qemu_aio_context and we assume our thread holds it (even if we don't
> explicitly hold it).
This is an assumption done in all unit tests, so it's not worth changing
only this case.
If you want to fix an inconsistency then maybe fix
> that instead of removing the acquire around block_job_create()?
Your explanation above makes sense, I think I will drop this change here.
Thank you,
Emanuele
Re: [PATCH v5 02/20] job.h: categorize fields in struct Job
On Tue, Feb 08, 2022 at 09:34:55AM -0500, Emanuele Giuseppe Esposito wrote:
> Categorize the fields in struct Job to understand which ones
> need to be protected by the job mutex and which don't.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> include/qemu/job.h | 59 ++
> 1 file changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index d1192ffd61..86ec46c09e 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
> * Long-running operation.
> */
> typedef struct Job {
> +
> +/* Fields set at initialization (job_create), and never modified */
Is there a corresponding "Field protected by job_mutex" comment that
separates fields that need locking?
signature.asc
Description: PGP signature
Re: [PATCH v5 01/20] job.c: make job_mutex and job_lock/unlock() public
On Tue, Feb 08, 2022 at 09:34:54AM -0500, Emanuele Giuseppe Esposito wrote:
> job mutex will be used to protect the job struct elements and list,
> replacing AioContext locks.
>
> Right now use a shared lock for all jobs, in order to keep things
> simple. Once the AioContext lock is gone, we can introduce per-job
> locks.
>
> To simplify the switch from aiocontext to job lock, introduce
> *nop* lock/unlock functions and macros.
> We want to always call job_lock/unlock outside the AioContext locks,
> and not vice-versa, otherwise we might get a deadlock. This is not
> straightforward to do, and that's why we start with nop functions.
> Once everything is protected by job_lock/unlock, we can change the nop into
> an actual mutex and remove the aiocontext lock.
>
> Since job_mutex is already being used, add static
> real_job_{lock/unlock} for the existing usage.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> include/qemu/job.h | 24
> job.c | 35 +++
> 2 files changed, 47 insertions(+), 12 deletions(-)
Reviewed-by: Stefan Hajnoczi
signature.asc
Description: PGP signature
Re: [PATCH v12 1/8] virtio: drop name parameter for virtio_init()
On Donnerstag, 10. Februar 2022 11:21:53 CET Jonah Palmer wrote:
> This patch drops the name parameter for the virtio_init function.
>
> The pair between the numeric device ID and the string device ID
> (name) of a virtio device already exists, but not in a way that
> lets us map between them.
>
> This patch lets us do this and removes the need for the name
> parameter in the virtio_init function.
>
> [Jonah: added new virtio IDs to virtio device list from rebase].
>
> Signed-off-by: Jonah Palmer
Acked-by: Christian Schoenebeck
> ---
> hw/9pfs/virtio-9p-device.c | 2 +-
> hw/block/vhost-user-blk.c | 2 +-
> hw/block/virtio-blk.c | 2 +-
> hw/char/virtio-serial-bus.c| 3 +-
> hw/display/virtio-gpu-base.c | 2 +-
> hw/input/virtio-input.c| 3 +-
> hw/net/virtio-net.c| 2 +-
> hw/scsi/virtio-scsi.c | 3 +-
> hw/virtio/vhost-user-fs.c | 3 +-
> hw/virtio/vhost-user-i2c.c | 7 +
> hw/virtio/vhost-user-rng.c | 2 +-
> hw/virtio/vhost-user-vsock.c | 2 +-
> hw/virtio/vhost-vsock-common.c | 5 ++--
> hw/virtio/vhost-vsock.c| 2 +-
> hw/virtio/virtio-balloon.c | 3 +-
> hw/virtio/virtio-crypto.c | 2 +-
> hw/virtio/virtio-iommu.c | 3 +-
> hw/virtio/virtio-mem.c | 3 +-
> hw/virtio/virtio-pmem.c| 3 +-
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 55
> -- include/hw/virtio/vhost-vsock-common.h |
> 2 +-
> include/hw/virtio/virtio-gpu.h | 3 +-
> include/hw/virtio/virtio.h | 4 +--
> 24 files changed, 77 insertions(+), 43 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index 54ee93b..5f522e6 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -216,7 +216,7 @@ static void virtio_9p_device_realize(DeviceState *dev,
> Error **errp) }
>
> v->config_size = sizeof(struct virtio_9p_config) +
> strlen(s->fsconf.tag); -virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P,
> v->config_size); +virtio_init(vdev, VIRTIO_ID_9P, v->config_size);
> v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
> }
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 1a42ae9..e8cb170 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState
> *dev, Error **errp) return;
> }
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
> +virtio_init(vdev, VIRTIO_ID_BLOCK,
> sizeof(struct virtio_blk_config));
>
> s->virtqs = g_new(VirtQueue *, s->num_queues);
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 82676cd..2e3809d 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1205,7 +1205,7 @@ static void virtio_blk_device_realize(DeviceState
> *dev, Error **errp)
>
> virtio_blk_set_config_size(s, s->host_features);
>
> -virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
> +virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
>
> s->blk = conf->conf.blk;
> s->rq = NULL;
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index f01ec21..9f19fd0 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1044,8 +1044,7 @@ static void virtio_serial_device_realize(DeviceState
> *dev, Error **errp) VIRTIO_CONSOLE_F_EMERG_WRITE)) {
> config_size = offsetof(struct virtio_console_config, emerg_wr);
> }
> -virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
> -config_size);
> +virtio_init(vdev, VIRTIO_ID_CONSOLE, config_size);
>
> /* Spawn a new virtio-serial bus on which the ports will ride as
> devices */ qbus_init(>bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> index fff0fb4..8ba5da4 100644
> --- a/hw/display/virtio-gpu-base.c
> +++ b/hw/display/virtio-gpu-base.c
> @@ -173,7 +173,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
> }
>
> g->virtio_config.num_scanouts = cpu_to_le32(g->conf.max_outputs);
> -virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
> +virtio_init(VIRTIO_DEVICE(g), VIRTIO_ID_GPU,
> sizeof(struct virtio_gpu_config));
>
> if (virtio_gpu_virgl_enabled(g->conf)) {
> diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
> index 54bcb46..5b5398b 100644
> --- a/hw/input/virtio-input.c
> +++ b/hw/input/virtio-input.c
> @@ -257,8 +257,7 @@ static void virtio_input_device_realize(DeviceState
> *dev, Error **errp) vinput->cfg_size += 8;
> assert(vinput->cfg_size <= sizeof(virtio_input_config));
>
> -
Re: [PATCH 1/6] block/io.c: fix bdrv_child_cb_drained_begin invocations from a coroutine
On Tue, Feb 08, 2022 at 10:36:50AM -0500, Emanuele Giuseppe Esposito wrote: > Using bdrv_do_drained_begin_quiesce() in bdrv_child_cb_drained_begin() > is not a good idea: the callback might be called when running > a drain in a coroutine, and bdrv_drained_begin_poll() does not > handle that case, resulting in assertion failure. > > Instead, bdrv_do_drained_begin with no recursion and poll > will accomplish the same thing (invoking bdrv_do_drained_begin_quiesce) > but will firstly check if we are already in a coroutine, and exit > from that via bdrv_co_yield_to_drain(). > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 2 +- > block/io.c | 7 ++- > include/block/block-io.h | 8 +--- > 3 files changed, 12 insertions(+), 5 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH 6/6] jobs: ensure sleep in job_sleep_ns is fully performed
On Tue, Feb 08, 2022 at 10:36:55AM -0500, Emanuele Giuseppe Esposito wrote:
> If a drain happens while a job is sleeping, the timeout
> gets cancelled and the job continues once the drain ends.
> This is especially bad for the sleep performed in commit and stream
> jobs, since that is dictated by ratelimit to maintain a certain speed.
>
> Basically the execution path is the following:
> 1. job calls job_sleep_ns, and yield with a timer in @ns ns.
> 2. meanwhile, a drain is executed, and
>child_job_drained_{begin/end} could be executed as ->drained_begin()
>and ->drained_end() callbacks.
>Therefore child_job_drained_begin() enters the job, that continues
>execution in job_sleep_ns() and calls job_pause_point_locked().
> 3. job_pause_point_locked() detects that we are in the middle of a
>drain, and firstly deletes any existing timer and then yields again,
>waiting for ->drained_end().
> 4. Once draining is finished, child_job_drained_end() runs and resumes
>the job. At this point, the timer has been lost and we just resume
>without checking if enough time has passed.
>
> This fix implies that from now onwards, job_sleep_ns will force the job
> to sleep @ns, even if it is wake up (purposefully or not) in the middle
> of the sleep. Therefore qemu-iotests test might run a little bit slower,
> depending on the speed of the job. Setting a job speed to values like "1"
> is not allowed anymore (unless you want to wait forever).
>
> Because of this fix, test_stream_parallel() in tests/qemu-iotests/030
> takes too long, since speed of stream job is just 1024 and before
> it was skipping all the wait thanks to the drains. Increase the
> speed to 256 * 1024. Exactly the same happens for test 151.
>
> Instead we need to sleep less in test_cancel_ready() test-blockjob.c,
> so that the job will be able to exit the sleep and transition to ready
> before the main loop asserts.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> job.c | 19 +++
> tests/qemu-iotests/030 | 2 +-
> tests/qemu-iotests/151 | 4 ++--
> tests/unit/test-blockjob.c | 2 +-
> 4 files changed, 15 insertions(+), 12 deletions(-)
Acked-by: Stefan Hajnoczi
signature.asc
Description: PGP signature
[PATCH 3/4] qemu-img: dd: add -n option (skip target volume creation)
From: Alexandre Derumier
Same rationale as in
b2e10493c7 ("add qemu-img convert -n option (skip target volume creation)")
Originally-by: Alexandre Derumier
Signed-off-by: Thomas Lamprecht
[FE: avoid wrong colon in getopt's optstring
add documentation + commit message]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 6 +-
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 23 ++-
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 43328fe108..9b022d9363 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -210,6 +210,10 @@ Parameters to dd subcommand:
.. program:: qemu-img-dd
+.. option:: -n
+
+ Skip the creation of the target volume
+
.. option:: bs=BLOCK_SIZE
Defines the block size
@@ -496,7 +500,7 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 50993e6c47..97e750623f 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 630928773d..89bf6fd087 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4944,7 +4944,7 @@ static int img_dd(int argc, char **argv)
const char *fmt = NULL;
int64_t size = 0, readsize = 0;
int64_t block_count = 0, out_pos, in_pos;
-bool force_share = false;
+bool force_share = false, skip_create = false;
struct DdInfo dd = {
.flags = 0,
.count = 0,
@@ -4982,7 +4982,7 @@ static int img_dd(int argc, char **argv)
{ 0, 0, 0, 0 }
};
-while ((c = getopt_long(argc, argv, ":hf:O:U", long_options, NULL))) {
+while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
if (c == EOF) {
break;
}
@@ -5002,6 +5002,9 @@ static int img_dd(int argc, char **argv)
case 'h':
help();
break;
+case 'n':
+skip_create = true;
+break;
case 'U':
force_share = true;
break;
@@ -5144,13 +5147,15 @@ static int img_dd(int argc, char **argv)
size - in.bsz * in.offset, _abort);
}
-ret = bdrv_create(drv, out.filename, opts, _err);
-if (ret < 0) {
-error_reportf_err(local_err,
- "%s: error while creating output image: ",
- out.filename);
-ret = -1;
-goto out;
+if (!skip_create) {
+ret = bdrv_create(drv, out.filename, opts, _err);
+if (ret < 0) {
+error_reportf_err(local_err,
+ "%s: error while creating output image: ",
+ out.filename);
+ret = -1;
+goto out;
+}
}
/* TODO, we can't honour --image-opts for the target,
--
2.30.2
[PATCH 1/4] qemu-img: dd: add osize and read from/to stdin/stdout
From: Wolfgang Bumiller
Neither convert nor dd were previously able to write to or
read from a pipe. Particularly serializing an image file
into a raw stream or vice versa can be useful, but using
`qemu-img convert -f qcow2 -O raw foo.qcow2 /dev/stdout` in
a pipe will fail trying to seek.
While dd and convert have overlapping use cases, `dd` is a
simple read/write loop while convert is much more
sophisticated and has ways to dealing with holes and blocks
of zeroes.
Since these typically can't be detected in pipes via
SEEK_DATA/HOLE or skipped while writing, dd seems to be the
better choice for implementing stdin/stdout streams.
This patch causes "if" and "of" to default to stdin and
stdout respectively, allowing only the "raw" format to be
used in these cases.
Since the input can now be a pipe we have no way of
detecting the size of the output image to create. Since we
also want to support images with a size not matching the
dd command's "bs" parameter (which, together with "count"
could be used to calculate the desired size, and is already
used to limit it), the "osize" option is added to explicitly
override the output file's size.
Signed-off-by: Wolfgang Bumiller
Signed-off-by: Thomas Lamprecht
[FE: add documentation
avoid error when osize is larger than input image's size
fail if both count and osize are specified
fail if skip is specified when reading from stdin]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 17 +++-
qemu-img-cmds.hx| 4 +-
qemu-img.c | 201 ++--
3 files changed, 146 insertions(+), 76 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 8885ea11cf..775eaf3097 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -220,16 +220,20 @@ Parameters to dd subcommand:
.. option:: if=INPUT
- Sets the input file
+ Sets the input file (defaults to STDIN)
.. option:: of=OUTPUT
- Sets the output file
+ Sets the output file (defaults to STDOUT)
.. option:: skip=BLOCKS
Sets the number of input blocks to skip
+.. option:: osize=OUTPUT_SIZE
+
+ Sets the output image's size
+
Parameters to snapshot subcommand:
.. program:: qemu-img-snapshot
@@ -488,10 +492,10 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] if=INPUT of=OUTPUT
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
- dd copies from *INPUT* file to *OUTPUT* file converting it from
- *FMT* format to *OUTPUT_FMT* format.
+ dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
+ STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
The data is by default read and written using blocks of 512 bytes but can be
modified by specifying *BLOCK_SIZE*. If count=\ *BLOCKS* is specified
@@ -499,6 +503,9 @@ Command description:
The size syntax is similar to :manpage:`dd(1)`'s size syntax.
+ The output image will be created with size *OUTPUT_SIZE* and at most this
many
+ bytes will be copied.
+
.. option:: info [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT]
[--backing-chain] [-U] FILENAME
Give information about the disk image *FILENAME*. Use it in
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1b1dab5b17..e4935365c9 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] if=input of=output")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [osize=output_size] [if=input] [of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] if=INPUT of=OUTPUT
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 6fe2466032..ea488fd190 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4819,10 +4819,12 @@ static int img_bitmap(int argc, char **argv)
#define C_IF 04
#define C_OF 010
#define C_SKIP020
+#define C_OSIZE 040
struct DdInfo {
unsigned int flags;
int64_t count;
+int64_t osize;
};
struct DdIo {
@@ -4898,6 +4900,19 @@ static int img_dd_skip(const char *arg,
return 0;
}
+static int img_dd_osize(const char *arg,
+struct DdIo *in, struct DdIo *out,
+struct DdInfo *dd)
+{
+dd->osize = cvtnum("osize", arg);
+
+if (dd->osize < 0) {
+return 1;
+}
+
+return 0;
+}
+
static int img_dd(int argc, char **argv)
{
int ret = 0;
@@ -4912,12 +4927,13
Re: [PATCH 4/6] test-bdrv-drain.c: adapt test to the coming subtree drains
On Tue, Feb 08, 2022 at 10:36:53AM -0500, Emanuele Giuseppe Esposito wrote:
> There will be 2 problems in this test when we will add
> subtree drains in bdrv_replace_child_noperm:
>
> - First, the test is inconsistent about taking the AioContext lock when
> calling bdrv_replace_child_noperm. bdrv_replace_child_noperm is reached
> in two places: from blk_insert_bs directly, and via block_job_create.
> Only the second does it with the AioContext lock taken, and there seems
> to be no reason why the lock is needed.
> Move aio_context_acquire further down, to just protect block_job_add_bdrv()
>
> - Second, test_detach_indirect is only interested in observing the first
> call to .drained_begin. In the original test, there was only a single
> subtree drain; however, with additional drains introduced in
> bdrv_replace_child_noperm(), the test callback would be called too early
> and/or multiple times.
> Override the callback only when we actually want to use it, and put back
> the original after it's been invoked.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> tests/unit/test-bdrv-drain.c | 17 -
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index 4924ceb562..c52ba2db4e 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -912,12 +912,12 @@ static void test_blockjob_common_drain_node(enum
> drain_type drain_type,
> blk_insert_bs(blk_target, target, _abort);
> blk_set_allow_aio_context_change(blk_target, true);
>
> -aio_context_acquire(ctx);
> tjob = block_job_create("job0", _job_driver, NULL, src,
> 0, BLK_PERM_ALL,
> 0, 0, NULL, NULL, _abort);
> tjob->bs = src;
> job = >common;
> +aio_context_acquire(ctx);
block_job_create() uses src's AioContext. In the IOThread case the
AioContext is not qemu_aio_context. My expectation is that src's
AioContext must be acquired before block_job_create() starts using src.
blockdev.c QMP commands acquire the BDS's AioContext before calling the
function that creates the job. It seems strange to do it differently in
this test case.
You mentioned that blk_insert_bs() is called without acquiring an
AioContext. That may be because we know blk_target is in
qemu_aio_context and we assume our thread holds it (even if we don't
explicitly hold it). If you want to fix an inconsistency then maybe fix
that instead of removing the acquire around block_job_create()?
Stefan
signature.asc
Description: PGP signature
Re: [PATCH 3/6] block.c: bdrv_replace_child_noperm: first call ->attach(), and then add child
On Tue, Feb 08, 2022 at 10:36:52AM -0500, Emanuele Giuseppe Esposito wrote: > Doing the opposite can make adding the child node to a non-drained node, > as apply_subtree_drain is only done in ->attach() and thus make > assert_bdrv_graph_writable fail. > > This can happen for example during a transaction rollback (test 245, > test_io_with_graph_changes): > 1. a node is removed from the graph, thus it is undrained > 2. then something happens, and we need to roll back the transactions >through tran_abort() > 3. at this point, the current code would first attach the undrained node >to the graph via QLIST_INSERT_HEAD, and then call ->attach() that >will take care of restoring the drain with apply_subtree_drain(), >leaving the node undrained between the two operations. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH 5/6] test-bdrv-drain.c: remove test_detach_by_parent_cb()
On Tue, Feb 08, 2022 at 10:36:54AM -0500, Emanuele Giuseppe Esposito wrote: > This test uses a callback of an I/O function (blk_aio_preadv) > to modify the graph, using bdrv_attach_child. > This is simply not allowed anymore. I/O cannot change the graph. > > Before "block/io.c: make bdrv_do_drained_begin_quiesce static > and introduce bdrv_drained_begin_no_poll", the test would simply > be at risk of failure, because if bdrv_replace_child_noperm() > (called to modify the graph) would call a drain, > then one callback of .drained_begin() is bdrv_do_drained_begin_quiesce, > that specifically asserts that we are not in a coroutine. > > Now that we fixed the behavior, the drain will invoke a bh in the > main loop, so we don't have such problem. However, this test is still > illegal and fails because we forbid graph changes from I/O paths. > > Once we add the required subtree_drains to protect > bdrv_replace_child_noperm(), the key problem in this test is in: > > acb = blk_aio_preadv(blk, 0, , 0, detach_by_parent_aio_cb, NULL); > /* Drain and check the expected result */ > bdrv_subtree_drained_begin(parent_b); > > because the detach_by_parent_aio_cb calls detach_indirect_bh(), that > modifies the graph and is invoked during bdrv_subtree_drained_begin(). > The call stack is the following: > 1. blk_aio_preadv() creates a coroutine, increments in_flight counter > and enters the coroutine running blk_aio_read_entry() > 2. blk_aio_read_entry() performs the read and then schedules a bh to >complete (blk_aio_complete) > 3. at this point, subtree_drained_begin() kicks in and waits for all >in_flight requests, polling > 4. polling allows the bh to be scheduled, so blk_aio_complete runs > 5. blk_aio_complete *first* invokes the callback >(detach_by_parent_aio_cb) and then decrements the in_flight counter > 6. Here we have the problem: detach_by_parent_aio_cb modifies the graph, >so both bdrv_unref_child() and bdrv_attach_child() will have >subtree_drains inside. And this causes a deadlock, because the >nested drain will wait for in_flight counter to go to zero, which >is only happening once the drain itself finishes. > > Different story is test_detach_by_driver_cb(): in this case, > detach_by_parent_aio_cb() does not call detach_indirect_bh(), > but it is instead called as a bh running in the main loop by > detach_by_driver_cb_drained_begin(), the callback for > .drained_begin(). > > This test was added in 231281ab42 and part of the series > "Drain fixes and cleanups, part 3" > https://lists.nongnu.org/archive/html/qemu-block/2018-05/msg01132.html > but as explained above I believe that it is not valid anymore, and > can be discarded. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > tests/unit/test-bdrv-drain.c | 46 +--- > 1 file changed, 11 insertions(+), 35 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH 2/6] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()
On Tue, Feb 08, 2022 at 10:36:51AM -0500, Emanuele Giuseppe Esposito wrote: > Doing the opposite can make ->detach() (more precisely > bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain > just performed to protect the removal of the child from the graph, > thus making the fully-enabled assert_bdrv_graph_writable fail. > > Note that assert_bdrv_graph_writable is not yet fully enabled. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
[PATCH 0/4] Make qemu-img dd more flexible
Adds support for reading from stdin and writing to stdout (when raw format is used), as well as overriding the size of the output and input image/stream. Additionally, the options -n for skipping output image creation and -l for loading a snapshot are made available like for convert. Alexandre Derumier (1): qemu-img: dd: add -n option (skip target volume creation) Fabian Ebner (1): qemu-img: dd: add -l option for loading a snapshot Wolfgang Bumiller (2): qemu-img: dd: add osize and read from/to stdin/stdout qemu-img: dd: add isize parameter docs/tools/qemu-img.rst | 28 - qemu-img-cmds.hx| 4 +- qemu-img.c | 261 +--- 3 files changed, 215 insertions(+), 78 deletions(-) -- 2.30.2
[PATCH 2/4] qemu-img: dd: add isize parameter
From: Wolfgang Bumiller
for writing small images from stdin to bigger ones.
In order to distinguish between an actually unexpected and
an expected end of input.
Signed-off-by: Wolfgang Bumiller
Signed-off-by: Thomas Lamprecht
[FE: override size earlier
use flag to detect parameter
add documenation]
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 10 --
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 24 +++-
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 775eaf3097..43328fe108 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -230,6 +230,10 @@ Parameters to dd subcommand:
Sets the number of input blocks to skip
+.. option:: isize=INPUT_SIZE
+
+ Treat the input image or stream as if it had this size
+
.. option:: osize=OUTPUT_SIZE
Sets the output image's size
@@ -492,7 +496,7 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
@@ -504,7 +508,9 @@ Command description:
The size syntax is similar to :manpage:`dd(1)`'s size syntax.
The output image will be created with size *OUTPUT_SIZE* and at most this
many
- bytes will be copied.
+ bytes will be copied. When *INPUT_SIZE* is positive, it overrides the input
+ image's size for the copy operation. When *INPUT_SIZE* is zero and reading
+ from STDIN, do not treat premature end of the input stream as an error.
.. option:: info [--object OBJECTDEF] [--image-opts] [-f FMT] [--output=OFMT]
[--backing-chain] [-U] FILENAME
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e4935365c9..50993e6c47 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [osize=output_size] [if=input] [of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE]
[count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE] [osize=OUTPUT_SIZE] [if=INPUT]
[of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index ea488fd190..630928773d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4820,11 +4820,13 @@ static int img_bitmap(int argc, char **argv)
#define C_OF 010
#define C_SKIP020
#define C_OSIZE 040
+#define C_ISIZE 0100
struct DdInfo {
unsigned int flags;
int64_t count;
int64_t osize;
+int64_t isize;
};
struct DdIo {
@@ -4913,6 +4915,19 @@ static int img_dd_osize(const char *arg,
return 0;
}
+static int img_dd_isize(const char *arg,
+struct DdIo *in, struct DdIo *out,
+struct DdInfo *dd)
+{
+dd->isize = cvtnum("isize", arg);
+
+if (dd->isize < 0) {
+return 1;
+}
+
+return 0;
+}
+
static int img_dd(int argc, char **argv)
{
int ret = 0;
@@ -4934,6 +4949,7 @@ static int img_dd(int argc, char **argv)
.flags = 0,
.count = 0,
.osize = 0,
+.isize = 0,
};
struct DdIo in = {
.bsz = 512, /* Block size is by default 512 bytes */
@@ -4955,6 +4971,7 @@ static int img_dd(int argc, char **argv)
{ "of", img_dd_of, C_OF },
{ "skip", img_dd_skip, C_SKIP },
{ "osize", img_dd_osize, C_OSIZE },
+{ "isize", img_dd_isize, C_ISIZE },
{ NULL, NULL, 0 }
};
const struct option long_options[] = {
@@ -5061,7 +5078,9 @@ static int img_dd(int argc, char **argv)
}
}
-if (dd.flags & C_IF) {
+if (dd.flags & C_ISIZE && dd.isize > 0) {
+size = dd.isize;
+} else if (dd.flags & C_IF) {
size = blk_getlength(blk1);
if (size < 0) {
error_report("Failed to get size for '%s'", in.filename);
@@ -5174,6 +5193,9 @@ static int img_dd(int argc, char **argv)
} else {
in_ret = read(STDIN_FILENO, in.buf, in_bsz);
if (in_ret == 0) {
+if (dd.flags & C_ISIZE && dd.isize == 0) {
+goto out;
+}
/* early EOF is considered an error */
[PATCH 4/4] qemu-img: dd: add -l option for loading a snapshot
Signed-off-by: Fabian Ebner
---
docs/tools/qemu-img.rst | 7 ---
qemu-img-cmds.hx| 4 ++--
qemu-img.c | 33 +++--
3 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 9b022d9363..b2333d7b04 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -500,10 +500,11 @@ Command description:
it doesn't need to be specified separately in this case.
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n] [-l
SNAPSHOT_PARAM] [bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
- dd copies from *INPUT* file (default: STDIN) to *OUTPUT* file (default:
- STDOUT) converting it from *FMT* format to *OUTPUT_FMT* format.
+ dd copies from *INPUT* file (default: STDIN) or snapshot *SNAPSHOT_PARAM* to
+ *OUTPUT* file (default: STDOUT) converting it from *FMT* format to
+ *OUTPUT_FMT* format.
The data is by default read and written using blocks of 512 bytes but can be
modified by specifying *BLOCK_SIZE*. If count=\ *BLOCKS* is specified
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 97e750623f..2f527306b0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -58,9 +58,9 @@ SRST
ERST
DEF("dd", img_dd,
-"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [bs=block_size]
[count=blocks] [skip=blocks] [isize=input_size] [osize=output_size] [if=input]
[of=output]")
+"dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [-n] [-l snapshot_param]
[bs=block_size] [count=blocks] [skip=blocks] [isize=input_size]
[osize=output_size] [if=input] [of=output]")
SRST
-.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n]
[bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [-n] [-l
SNAPSHOT_PARAM] [bs=BLOCK_SIZE] [count=BLOCKS] [skip=BLOCKS] [isize=INPUT_SIZE]
[osize=OUTPUT_SIZE] [if=INPUT] [of=OUTPUT]
ERST
DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 89bf6fd087..28b6430800 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4936,6 +4936,7 @@ static int img_dd(int argc, char **argv)
BlockDriver *drv = NULL, *proto_drv = NULL;
BlockBackend *blk1 = NULL, *blk2 = NULL;
QemuOpts *opts = NULL;
+QemuOpts *sn_opts = NULL;
QemuOptsList *create_opts = NULL;
Error *local_err = NULL;
bool image_opts = false;
@@ -4945,6 +4946,7 @@ static int img_dd(int argc, char **argv)
int64_t size = 0, readsize = 0;
int64_t block_count = 0, out_pos, in_pos;
bool force_share = false, skip_create = false;
+const char *snapshot_name = NULL;
struct DdInfo dd = {
.flags = 0,
.count = 0,
@@ -4982,7 +4984,7 @@ static int img_dd(int argc, char **argv)
{ 0, 0, 0, 0 }
};
-while ((c = getopt_long(argc, argv, ":hf:O:Un", long_options, NULL))) {
+while ((c = getopt_long(argc, argv, ":hf:O:l:Un", long_options, NULL))) {
if (c == EOF) {
break;
}
@@ -5005,6 +5007,19 @@ static int img_dd(int argc, char **argv)
case 'n':
skip_create = true;
break;
+case 'l':
+if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
+sn_opts = qemu_opts_parse_noisily(_snapshot_opts,
+ optarg, false);
+if (!sn_opts) {
+error_report("Failed in parsing snapshot param '%s'",
+ optarg);
+goto out;
+}
+} else {
+snapshot_name = optarg;
+}
+break;
case 'U':
force_share = true;
break;
@@ -5074,11 +5089,24 @@ static int img_dd(int argc, char **argv)
if (dd.flags & C_IF) {
blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
force_share);
-
if (!blk1) {
ret = -1;
goto out;
}
+if (sn_opts) {
+bdrv_snapshot_load_tmp(blk_bs(blk1),
+ qemu_opt_get(sn_opts, SNAPSHOT_OPT_ID),
+ qemu_opt_get(sn_opts, SNAPSHOT_OPT_NAME),
+ _err);
+} else if (snapshot_name != NULL) {
+bdrv_snapshot_load_tmp_by_id_or_name(blk_bs(blk1), snapshot_name,
+ _err);
+}
+if (local_err) {
+error_reportf_err(local_err, "Failed to load snapshot: ");
+ret = -1;
+goto out;
+}
}
if (dd.flags & C_ISIZE && dd.isize > 0) {
qemu iotest 161 and make check
Hello,
I do see spurious failures of 161 in our CI, but only when I use
make check with parallelism (-j).
I have not yet figured out which other testcase could interfere
@@ -34,6 +34,8 @@
*** Commit and then change an option on the backing file
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+qemu-img: TEST_DIR/t.IMGFMT.base: Failed to get "write" lock
+Is another process using the image [TEST_DIR/t.IMGFMT.base]?
Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
{ 'execute': 'qmp_capabilities' }
any ideas?
Christian
[PATCH v12 2/8] virtio: add vhost support for virtio devices
This patch adds a get_vhost() callback function for VirtIODevices that
returns the device's corresponding vhost_dev structure, if the vhost
device is running. This patch also adds a vhost_started flag for
VirtIODevices.
Previously, a VirtIODevice wouldn't be able to tell if its corresponding
vhost device was active or not.
Signed-off-by: Jonah Palmer
---
hw/block/vhost-user-blk.c | 7 +++
hw/display/vhost-user-gpu.c| 7 +++
hw/input/vhost-user-input.c| 7 +++
hw/net/virtio-net.c| 9 +
hw/scsi/vhost-scsi.c | 8
hw/virtio/vhost-user-fs.c | 7 +++
hw/virtio/vhost-user-rng.c | 7 +++
hw/virtio/vhost-vsock-common.c | 7 +++
hw/virtio/vhost.c | 4 +++-
hw/virtio/virtio-crypto.c | 10 ++
hw/virtio/virtio.c | 1 +
include/hw/virtio/virtio.h | 3 +++
12 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index e8cb170..5dca4ea 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -569,6 +569,12 @@ static void vhost_user_blk_instance_init(Object *obj)
"/disk@0,0", DEVICE(obj));
}
+static struct vhost_dev *vhost_user_blk_get_vhost(VirtIODevice *vdev)
+{
+VHostUserBlk *s = VHOST_USER_BLK(vdev);
+return >dev;
+}
+
static const VMStateDescription vmstate_vhost_user_blk = {
.name = "vhost-user-blk",
.minimum_version_id = 1,
@@ -603,6 +609,7 @@ static void vhost_user_blk_class_init(ObjectClass *klass,
void *data)
vdc->get_features = vhost_user_blk_get_features;
vdc->set_status = vhost_user_blk_set_status;
vdc->reset = vhost_user_blk_reset;
+vdc->get_vhost = vhost_user_blk_get_vhost;
}
static const TypeInfo vhost_user_blk_info = {
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 0981823..96e56c4 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -565,6 +565,12 @@ vhost_user_gpu_device_realize(DeviceState *qdev, Error
**errp)
g->vhost_gpu_fd = -1;
}
+static struct vhost_dev *vhost_user_gpu_get_vhost(VirtIODevice *vdev)
+{
+VhostUserGPU *g = VHOST_USER_GPU(vdev);
+return >vhost->dev;
+}
+
static Property vhost_user_gpu_properties[] = {
VIRTIO_GPU_BASE_PROPERTIES(VhostUserGPU, parent_obj.conf),
DEFINE_PROP_END_OF_LIST(),
@@ -586,6 +592,7 @@ vhost_user_gpu_class_init(ObjectClass *klass, void *data)
vdc->guest_notifier_pending = vhost_user_gpu_guest_notifier_pending;
vdc->get_config = vhost_user_gpu_get_config;
vdc->set_config = vhost_user_gpu_set_config;
+vdc->get_vhost = vhost_user_gpu_get_vhost;
device_class_set_props(dc, vhost_user_gpu_properties);
}
diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
index 273e96a..43d2ff3 100644
--- a/hw/input/vhost-user-input.c
+++ b/hw/input/vhost-user-input.c
@@ -79,6 +79,12 @@ static void vhost_input_set_config(VirtIODevice *vdev,
virtio_notify_config(vdev);
}
+static struct vhost_dev *vhost_input_get_vhost(VirtIODevice *vdev)
+{
+VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
+return >vhost->dev;
+}
+
static const VMStateDescription vmstate_vhost_input = {
.name = "vhost-user-input",
.unmigratable = 1,
@@ -93,6 +99,7 @@ static void vhost_input_class_init(ObjectClass *klass, void
*data)
dc->vmsd = _vhost_input;
vdc->get_config = vhost_input_get_config;
vdc->set_config = vhost_input_set_config;
+vdc->get_vhost = vhost_input_get_vhost;
vic->realize = vhost_input_realize;
vic->change_active = vhost_input_change_active;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 25f494c..21328dc 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3615,6 +3615,14 @@ static bool dev_unplug_pending(void *opaque)
return vdc->primary_unplug_pending(dev);
}
+static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+NetClientState *nc = qemu_get_queue(n->nic);
+struct vhost_net *net = get_vhost_net(nc->peer);
+return >dev;
+}
+
static const VMStateDescription vmstate_virtio_net = {
.name = "virtio-net",
.minimum_version_id = VIRTIO_NET_VM_VERSION,
@@ -3717,6 +3725,7 @@ static void virtio_net_class_init(ObjectClass *klass,
void *data)
vdc->post_load = virtio_net_post_load_virtio;
vdc->vmsd = _virtio_net_device;
vdc->primary_unplug_pending = primary_unplug_pending;
+vdc->get_vhost = virtio_net_get_vhost;
}
static const TypeInfo virtio_net_info = {
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 778f43e..3059068 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -273,6 +273,13 @@ static void vhost_scsi_unrealize(DeviceState *dev)
virtio_scsi_common_unrealize(dev);
}
+static struct vhost_dev *vhost_scsi_get_vhost(VirtIODevice *vdev)
+{
+VHostSCSI *s =
[PATCH v12 8/8] hmp: add virtio commands
From: Laurent Vivier
This patch implements the HMP versions of the virtio QMP commands.
[Jonah: Fixed virtio hmp command output format (e.g. use PRI types).]
Signed-off-by: Jonah Palmer
---
hmp-commands-info.hx | 70
include/monitor/hmp.h | 5 +
monitor/hmp-cmds.c| 311 ++
3 files changed, 386 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 407a1da..e49d852 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -877,3 +877,73 @@ SRST
``info sgx``
Show intel SGX information.
ERST
+
+{
+.name = "virtio",
+.args_type = "",
+.params= "",
+.help = "List all available virtio devices",
+.cmd = hmp_virtio_query,
+.flags = "p",
+},
+
+SRST
+ ``info virtio``
+List all available virtio devices
+ERST
+
+{
+.name = "virtio-status",
+.args_type = "path:s",
+.params= "path",
+.help = "Display status of a given virtio device",
+.cmd = hmp_virtio_status,
+.flags = "p",
+},
+
+SRST
+ ``info virtio-status`` *path*
+Display status of a given virtio device
+ERST
+
+{
+.name = "virtio-queue-status",
+.args_type = "path:s,queue:i",
+.params= "path queue",
+.help = "Display status of a given virtio queue",
+.cmd = hmp_virtio_queue_status,
+.flags = "p",
+},
+
+SRST
+ ``info virtio-queue-status`` *path* *queue*
+Display status of a given virtio queue
+ERST
+
+{
+.name = "virtio-vhost-queue-status",
+.args_type = "path:s,queue:i",
+.params= "path queue",
+.help = "Display status of a given vhost queue",
+.cmd = hmp_vhost_queue_status,
+.flags = "p",
+},
+
+SRST
+ ``info virtio-vhost-queue-status`` *path* *queue*
+Display status of a given vhost queue
+ERST
+
+{
+.name = "virtio-queue-element",
+.args_type = "path:s,queue:i,index:i?",
+.params = "path queue [index]",
+.help = "Display element of a given virtio queue",
+.cmd= hmp_virtio_queue_element,
+.flags = "p",
+},
+
+SRST
+ ``info virtio-queue-element`` *path* *queue* [*index*]
+Display element of a given virtio queue
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d0148..47446d8 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -95,6 +95,11 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict);
void hmp_qom_get(Monitor *mon, const QDict *qdict);
void hmp_qom_set(Monitor *mon, const QDict *qdict);
void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
+void hmp_virtio_query(Monitor *mon, const QDict *qdict);
+void hmp_virtio_status(Monitor *mon, const QDict *qdict);
+void hmp_virtio_queue_status(Monitor *mon, const QDict *qdict);
+void hmp_vhost_queue_status(Monitor *mon, const QDict *qdict);
+void hmp_virtio_queue_element(Monitor *mon, const QDict *qdict);
void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2669156..d001238 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -42,6 +42,8 @@
#include "qapi/qapi-commands-run-state.h"
#include "qapi/qapi-commands-tpm.h"
#include "qapi/qapi-commands-ui.h"
+#include "qapi/qapi-commands-virtio.h"
+#include "qapi/qapi-visit-virtio.h"
#include "qapi/qapi-visit-net.h"
#include "qapi/qapi-visit-migration.h"
#include "qapi/qmp/qdict.h"
@@ -2166,3 +2168,312 @@ void hmp_info_memory_size_summary(Monitor *mon, const
QDict *qdict)
}
hmp_handle_error(mon, err);
}
+
+static void hmp_virtio_dump_protocols(Monitor *mon,
+ VhostDeviceProtocols *pcol)
+{
+strList *pcol_list = pcol->protocols;
+while (pcol_list) {
+monitor_printf(mon, "%s", pcol_list->value);
+pcol_list = pcol_list->next;
+if (pcol_list != NULL) {
+monitor_printf(mon, ", ");
+}
+}
+monitor_printf(mon, "\n");
+if (pcol->has_unknown_protocols) {
+monitor_printf(mon, " unknown-protocols(0x%016"PRIx64")\n",
+ pcol->unknown_protocols);
+}
+}
+
+static void hmp_virtio_dump_status(Monitor *mon,
+ VirtioDeviceStatus *status)
+{
+strList *status_list = status->statuses;
+while (status_list) {
+monitor_printf(mon, "%s", status_list->value);
+status_list = status_list->next;
+if (status_list != NULL) {
+monitor_printf(mon, ", ");
+}
+}
+monitor_printf(mon, "\n");
+if (status->has_unknown_statuses) {
+
[PATCH v12 7/8] qmp: add QMP command x-query-virtio-queue-element
From: Laurent Vivier
This new command shows the information of a VirtQueue element.
[Note: Up until v10 of this patch series, virtio.json had many (15+)
enums defined (e.g. decoded device features, statuses, etc.). In v10
most of these enums were removed and replaced with string literals.
By doing this we get (1) simpler schema, (2) smaller generated code,
and (3) less maintenance burden for when new things are added (e.g.
devices, device features, etc.).]
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio-stub.c | 9 +++
hw/virtio/virtio.c | 154
qapi/virtio.json| 183
3 files changed, 346 insertions(+)
diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index 13e5f93..7ddb22c 100644
--- a/hw/virtio/virtio-stub.c
+++ b/hw/virtio/virtio-stub.c
@@ -31,3 +31,12 @@ VirtQueueStatus *qmp_x_query_virtio_queue_status(const char
*path,
{
return qmp_virtio_unsupported(errp);
}
+
+VirtioQueueElement *qmp_x_query_virtio_queue_element(const char *path,
+ uint16_t queue,
+ bool has_index,
+ uint16_t index,
+ Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index c81210b..ec37235 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -489,6 +489,19 @@ static inline void vring_used_write(VirtQueue *vq,
VRingUsedElem *uelem,
address_space_cache_invalidate(>used, pa, sizeof(VRingUsedElem));
}
+/* Called within rcu_read_lock(). */
+static inline uint16_t vring_used_flags(VirtQueue *vq)
+{
+VRingMemoryRegionCaches *caches = vring_get_region_caches(vq);
+hwaddr pa = offsetof(VRingUsed, flags);
+
+if (!caches) {
+return 0;
+}
+
+return virtio_lduw_phys_cached(vq->vdev, >used, pa);
+}
+
/* Called within rcu_read_lock(). */
static uint16_t vring_used_idx(VirtQueue *vq)
{
@@ -4416,6 +4429,147 @@ VirtQueueStatus *qmp_x_query_virtio_queue_status(const
char *path,
return status;
}
+static strList *qmp_decode_vring_desc_flags(uint16_t flags)
+{
+strList *list = NULL;
+strList *node;
+int i;
+
+struct {
+uint16_t flag;
+const char *value;
+} map[] = {
+{ VRING_DESC_F_NEXT, "next" },
+{ VRING_DESC_F_WRITE, "write" },
+{ VRING_DESC_F_INDIRECT, "indirect" },
+{ 1 << VRING_PACKED_DESC_F_AVAIL, "avail" },
+{ 1 << VRING_PACKED_DESC_F_USED, "used" },
+{ 0, "" }
+};
+
+for (i = 0; map[i].flag; i++) {
+if ((map[i].flag & flags) == 0) {
+continue;
+}
+node = g_malloc0(sizeof(strList));
+node->value = g_strdup(map[i].value);
+node->next = list;
+list = node;
+}
+
+return list;
+}
+
+VirtioQueueElement *qmp_x_query_virtio_queue_element(const char *path,
+ uint16_t queue,
+ bool has_index,
+ uint16_t index,
+ Error **errp)
+{
+VirtIODevice *vdev;
+VirtQueue *vq;
+VirtioQueueElement *element = NULL;
+
+vdev = virtio_device_find(path);
+if (vdev == NULL) {
+error_setg(errp, "Path %s is not a VirtIO device", path);
+return NULL;
+}
+
+if (queue >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, queue)) {
+error_setg(errp, "Invalid virtqueue number %d", queue);
+return NULL;
+}
+vq = >vq[queue];
+
+if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+error_setg(errp, "Packed ring not supported");
+return NULL;
+} else {
+unsigned int head, i, max;
+VRingMemoryRegionCaches *caches;
+MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+MemoryRegionCache *desc_cache;
+VRingDesc desc;
+VirtioRingDescList *list = NULL;
+VirtioRingDescList *node;
+int rc; int ndescs;
+
+RCU_READ_LOCK_GUARD();
+
+max = vq->vring.num;
+
+if (!has_index) {
+head = vring_avail_ring(vq, vq->last_avail_idx % vq->vring.num);
+} else {
+head = vring_avail_ring(vq, index % vq->vring.num);
+}
+i = head;
+
+caches = vring_get_region_caches(vq);
+if (!caches) {
+error_setg(errp, "Region caches not initialized");
+return NULL;
+}
+if (caches->desc.len < max * sizeof(VRingDesc)) {
+error_setg(errp, "Cannot map descriptor ring");
+return NULL;
+}
+
+desc_cache = >desc;
+vring_split_desc_read(vdev, , desc_cache, i);
+
[PATCH v12 5/8] qmp: decode feature & status bits in virtio-status
From: Laurent Vivier
Display feature names instead of bitmaps for host, guest, and
backend for VirtIODevices.
Display status names instead of bitmaps for VirtIODevices.
Display feature names instead of bitmaps for backend, protocol,
acked, and features (hdev->features) for vhost devices.
Decode features according to device ID. Decode statuses
according to configuration status bitmap (config_status_map).
Decode vhost user protocol features according to vhost user
protocol bitmap (vhost_user_protocol_map).
Transport features are on the first line. Undecoded bits (if
any) are stored in a separate field.
Signed-off-by: Jonah Palmer
---
hw/block/virtio-blk.c | 29
hw/char/virtio-serial-bus.c| 11 ++
hw/display/virtio-gpu-base.c | 18 ++-
hw/input/virtio-input.c| 10 ++
hw/net/virtio-net.c| 47 +++
hw/scsi/virtio-scsi.c | 17 +++
hw/virtio/vhost-user-fs.c | 10 ++
hw/virtio/vhost-vsock-common.c | 10 ++
hw/virtio/virtio-balloon.c | 14 ++
hw/virtio/virtio-crypto.c | 10 ++
hw/virtio/virtio-iommu.c | 14 ++
hw/virtio/virtio-mem.c | 11 ++
hw/virtio/virtio.c | 297 +++--
include/hw/virtio/vhost.h | 3 +
include/hw/virtio/virtio.h | 18 +++
qapi/virtio.json | 156 +++---
16 files changed, 646 insertions(+), 29 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 2e3809d..55d291e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
#include "qemu/iov.h"
#include "qemu/module.h"
#include "qemu/error-report.h"
@@ -32,10 +33,38 @@
#include "hw/virtio/virtio-bus.h"
#include "migration/qemu-file-types.h"
#include "hw/virtio/virtio-access.h"
+#include "standard-headers/linux/vhost_types.h"
/* Config size before the discard support (hide associated config fields) */
#define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
max_discard_sectors)
+
+qmp_virtio_feature_map_t blk_map[] = {
+#define FEATURE_ENTRY(name) \
+{ VIRTIO_BLK_F_##name, #name }
+FEATURE_ENTRY(SIZE_MAX),
+FEATURE_ENTRY(SEG_MAX),
+FEATURE_ENTRY(GEOMETRY),
+FEATURE_ENTRY(RO),
+FEATURE_ENTRY(BLK_SIZE),
+FEATURE_ENTRY(TOPOLOGY),
+FEATURE_ENTRY(MQ),
+FEATURE_ENTRY(DISCARD),
+FEATURE_ENTRY(WRITE_ZEROES),
+#ifndef VIRTIO_BLK_NO_LEGACY
+FEATURE_ENTRY(BARRIER),
+FEATURE_ENTRY(SCSI),
+FEATURE_ENTRY(FLUSH),
+FEATURE_ENTRY(CONFIG_WCE),
+#endif /* !VIRTIO_BLK_NO_LEGACY */
+#undef FEATURE_ENTRY
+#define FEATURE_ENTRY(name) \
+{ VHOST_F_##name, #name }
+FEATURE_ENTRY(LOG_ALL),
+#undef FEATURE_ENTRY
+{ -1, "" }
+};
+
/*
* Starting from the discard feature, we can use this array to properly
* set the config size depending on the features enabled.
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 9f19fd0..9de2575 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -20,6 +20,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
#include "qemu/iov.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
@@ -32,6 +33,16 @@
#include "hw/virtio/virtio-serial.h"
#include "hw/virtio/virtio-access.h"
+qmp_virtio_feature_map_t serial_map[] = {
+#define FEATURE_ENTRY(name) \
+{ VIRTIO_CONSOLE_F_##name, #name }
+FEATURE_ENTRY(SIZE),
+FEATURE_ENTRY(MULTIPORT),
+FEATURE_ENTRY(EMERG_WRITE),
+#undef FEATURE_ENTRY
+{ -1, "" }
+};
+
static struct VirtIOSerialDevices {
QLIST_HEAD(, VirtIOSerial) devices;
} vserdevices;
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 8ba5da4..796786a 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -12,13 +12,29 @@
*/
#include "qemu/osdep.h"
-
+#include "standard-headers/linux/vhost_types.h"
#include "hw/virtio/virtio-gpu.h"
#include "migration/blocker.h"
#include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
#include "qemu/error-report.h"
#include "trace.h"
+qmp_virtio_feature_map_t gpu_map[] = {
+#define FEATURE_ENTRY(name) \
+{ VIRTIO_GPU_F_##name, #name }
+FEATURE_ENTRY(VIRGL),
+FEATURE_ENTRY(EDID),
+FEATURE_ENTRY(RESOURCE_UUID),
+FEATURE_ENTRY(RESOURCE_BLOB),
+#undef FEATURE_ENTRY
+#define FEATURE_ENTRY(name) \
+{ VHOST_F_##name, #name }
+FEATURE_ENTRY(LOG_ALL),
+#undef FEATURE_ENTRY
+{ -1, "" }
+};
+
void
virtio_gpu_base_reset(VirtIOGPUBase *g)
{
diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index 5b5398b..fe0ed6d 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -6,6 +6,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qapi-visit-virtio.h"
#include "qemu/iov.h"
#include "qemu/module.h"
#include
[PATCH v12 3/8] qmp: add QMP command x-query-virtio
From: Laurent Vivier
This new command lists all the instances of VirtIODevices with
their canonical QOM path and name.
[Jonah: @virtio_list duplicates information that already exists in
the QOM composition tree. However, extracting necessary information
from this tree seems to be a bit convoluted.
Instead, we still create our own list of realized virtio devices
but use @qmp_qom_get with the device's canonical QOM path to confirm
that the device exists and is realized. If the device exists but
is actually not realized, then we remove it from our list (for
synchronicity to the QOM composition tree).
Also, the QMP command @x-query-virtio is redundant as @qom-list
and @qom-get are sufficient to search '/machine/' for realized
virtio devices. However, @x-query-virtio is much more convenient
in listing realized virtio devices.]
Signed-off-by: Jonah Palmer
---
hw/virtio/meson.build | 2 ++
hw/virtio/virtio-stub.c| 14 ++
hw/virtio/virtio.c | 44 ++
include/hw/virtio/virtio.h | 1 +
qapi/meson.build | 1 +
qapi/qapi-schema.json | 1 +
qapi/virtio.json | 68 ++
tests/qtest/qmp-cmd-test.c | 1 +
8 files changed, 132 insertions(+)
create mode 100644 hw/virtio/virtio-stub.c
create mode 100644 qapi/virtio.json
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 521f7d6..d893f5f 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -6,8 +6,10 @@ softmmu_virtio_ss.add(when: 'CONFIG_VHOST', if_false:
files('vhost-stub.c'))
softmmu_ss.add_all(when: 'CONFIG_VIRTIO', if_true: softmmu_virtio_ss)
softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('vhost-stub.c'))
+softmmu_ss.add(when: 'CONFIG_VIRTIO', if_false: files('virtio-stub.c'))
softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('vhost-stub.c'))
+softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('virtio-stub.c'))
virtio_ss = ss.source_set()
virtio_ss.add(files('virtio.c'))
diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
new file mode 100644
index 000..05a81ed
--- /dev/null
+++ b/hw/virtio/virtio-stub.c
@@ -0,0 +1,14 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-virtio.h"
+
+static void *qmp_virtio_unsupported(Error **errp)
+{
+error_setg(errp, "Virtio is disabled");
+return NULL;
+}
+
+VirtioInfoList *qmp_x_query_virtio(Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7c1b1dd..e59f0d7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -13,12 +13,18 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qapi-commands-virtio.h"
+#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-virtio.h"
+#include "qapi/qmp/qjson.h"
#include "cpu.h"
#include "trace.h"
#include "qemu/error-report.h"
#include "qemu/log.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
+#include "qom/object_interfaces.h"
#include "hw/virtio/virtio.h"
#include "migration/qemu-file-types.h"
#include "qemu/atomic.h"
@@ -29,6 +35,9 @@
#include "sysemu/runstate.h"
#include "standard-headers/linux/virtio_ids.h"
+/* QAPI list of realized VirtIODevices */
+static QTAILQ_HEAD(, VirtIODevice) virtio_list;
+
/*
* The alignment to use between consumer and producer parts of vring.
* x86 pagesize again. This is the default, used by transports like PCI
@@ -3687,6 +3696,7 @@ static void virtio_device_realize(DeviceState *dev, Error
**errp)
vdev->listener.commit = virtio_memory_listener_commit;
vdev->listener.name = "virtio";
memory_listener_register(>listener, vdev->dma_as);
+QTAILQ_INSERT_TAIL(_list, vdev, next);
}
static void virtio_device_unrealize(DeviceState *dev)
@@ -3701,6 +3711,7 @@ static void virtio_device_unrealize(DeviceState *dev)
vdc->unrealize(dev);
}
+QTAILQ_REMOVE(_list, vdev, next);
g_free(vdev->bus_name);
vdev->bus_name = NULL;
}
@@ -3874,6 +3885,8 @@ static void virtio_device_class_init(ObjectClass *klass,
void *data)
vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
+
+QTAILQ_INIT(_list);
}
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
@@ -3884,6 +3897,37 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
return virtio_bus_ioeventfd_enabled(vbus);
}
+VirtioInfoList *qmp_x_query_virtio(Error **errp)
+{
+VirtioInfoList *list = NULL;
+VirtioInfoList *node;
+VirtIODevice *vdev;
+
+QTAILQ_FOREACH(vdev, _list, next) {
+DeviceState *dev = DEVICE(vdev);
+Error *err = NULL;
+QObject *obj = qmp_qom_get(dev->canonical_path, "realized", );
+
+if (err == NULL) {
+GString *is_realized = qobject_to_json_pretty(obj, true);
+/* virtio device is NOT realized, remove it from list */
+
[PATCH v12 0/8] hmp,qmp: Add commands to introspect virtio devices
This series introduces new QMP/HMP commands to dump the status of a
virtio device at different levels.
[Jonah: Rebasing from previous patchset from Jan. 20 (v11). Original patches
are by Laurent Vivier from May 2020.
Rebase from v11 to v12 is just fixing some virtio hmp commands to use PRI
when printing out long int types (for 32-bit compatibility).]
1. List available virtio devices in the machine
HMP Form:
info virtio
Example:
(qemu) info virtio
/machine/peripheral/vsock0/virtio-backend [vhost-vsock]
/machine/peripheral/crypto0/virtio-backend [virtio-crypto]
/machine/peripheral-anon/device[2]/virtio-backend [virtio-scsi]
/machine/peripheral-anon/device[1]/virtio-backend [virtio-net]
/machine/peripheral-anon/device[0]/virtio-backend [virtio-serial]
QMP Form:
{ 'command': 'x-query-virtio',
'returns': ['VirtioInfo'],
'features': [ 'unstable' ] }
Example:
-> { "execute": "x-query-virtio" }
<- { "return": [
{
"path": "/machine/peripheral/vsock0/virtio-backend",
"name": "vhost-vsock"
},
{
"path": "/machine/peripheral/crypto0/virtio-backend",
"name": "virtio-crypto"
},
{
"path": "/machine/peripheral-anon/device[2]/virtio-backend",
"name": "virtio-scsi"
},
{
"path": "/machine/peripheral-anon/device[1]/virtio-backend",
"name": "virtio-net"
},
{
"path": "/machine/peripheral-anon/device[0]/virtio-backend",
"name": "virtio-serial"
}
]
}
2. Display status of a given virtio device
HMP Form:
info virtio-status
Example:
(qemu) info virtio-status /machine/peripheral/vsock0/virtio-backend
/machine/peripheral/vsock0/virtio-backend:
device_name: vhost-vsock (vhost)
device_id: 19
vhost_started: true
bus_name:(null)
broken: false
disabled:false
disable_legacy_check:false
started: true
use_started: true
start_on_kick: false
use_guest_notifier_mask: true
vm_running: true
num_vqs: 3
queue_sel: 2
isr: 0
endianness: little
status: ACKNOWLEDGE, DRIVER, FEATURES_OK, DRIVER_OK
Guest features: EVENT_IDX, INDIRECT_DESC, VERSION_1
Host features:PROTOCOL_FEATURES, EVENT_IDX, INDIRECT_DESC,
VERSION_1, ANY_LAYOUT,
NOTIFY_ON_EMPTY
Backend features:
VHost:
nvqs: 2
vq_index: 0
max_queues: 0
n_mem_sections: 4
n_tmp_sections: 4
backend_cap:0
log_enabled:false
log_size: 0
Features: EVENT_IDX, INDIRECT_DESC, VERSION_1,
ANY_LAYOUT, NOTIFY_ON_EMPTY
LOG_ALL
Acked features:EVENT_IDX, INDIRECT_DESC, VERSION_1
Backend features:
Protocol features:
QMP Form:
{ 'command': 'x-query-virtio-status',
'data': { 'path': 'str' },
'returns': 'VirtioStatus',
'features': [ 'unstable' ] }
Example:
-> { "execute": "x-query-virtio-status",
"arguments": { "path": "/machine/peripheral/vsock0/virtio-backend"
}
}
<- { "return": {
"device-endian": "little",
"bus-name": "",
"disable-legacy-check": false,
"name": "vhost-vsock",
"started": true,
"device-id": 19,
"vhost-dev": {
"n-tmp-sections": 4,
"n-mem-sections": 4,
"max-queues": 0,
"backend-cap": 0,
"log-size": 0,
"backend-features": {
"transports": [],
"dev-features": []
},
"nvqs": 2,
"protocol-features": {
"protocols": []
},
"vq-index": 0,
"log-enabled": false,
"acked-features": {
"transports": ["EVENT_IDX", "INDIRECT_DESC",
"VERSION_1"],
"dev-features": []
[PATCH v12 6/8] qmp: add QMP commands for virtio/vhost queue-status
From: Laurent Vivier
These new commands show the internal status of a VirtIODevice's
VirtQueue and a vhost device's vhost_virtqueue (if active).
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio-stub.c | 14 +++
hw/virtio/virtio.c | 103
qapi/virtio.json| 252
3 files changed, 369 insertions(+)
diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index 0b432e8..13e5f93 100644
--- a/hw/virtio/virtio-stub.c
+++ b/hw/virtio/virtio-stub.c
@@ -17,3 +17,17 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path,
Error **errp)
{
return qmp_virtio_unsupported(errp);
}
+
+VirtVhostQueueStatus *qmp_x_query_virtio_vhost_queue_status(const char *path,
+uint16_t queue,
+Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
+
+VirtQueueStatus *qmp_x_query_virtio_queue_status(const char *path,
+ uint16_t queue,
+ Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 41823cd..c81210b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -4313,6 +4313,109 @@ VirtioStatus *qmp_x_query_virtio_status(const char
*path, Error **errp)
return status;
}
+VirtVhostQueueStatus *qmp_x_query_virtio_vhost_queue_status(const char *path,
+uint16_t queue,
+Error **errp)
+{
+VirtIODevice *vdev;
+VirtVhostQueueStatus *status;
+
+vdev = virtio_device_find(path);
+if (vdev == NULL) {
+error_setg(errp, "Path %s is not a VirtIODevice", path);
+return NULL;
+}
+
+if (!vdev->vhost_started) {
+error_setg(errp, "Error: vhost device has not started yet");
+return NULL;
+}
+
+VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+struct vhost_dev *hdev = vdc->get_vhost(vdev);
+
+if (queue < hdev->vq_index || queue >= hdev->vq_index + hdev->nvqs) {
+error_setg(errp, "Invalid vhost virtqueue number %d", queue);
+return NULL;
+}
+
+status = g_new0(VirtVhostQueueStatus, 1);
+status->name = g_strdup(vdev->name);
+status->kick = hdev->vqs[queue].kick;
+status->call = hdev->vqs[queue].call;
+status->desc = (uint64_t)(unsigned long)hdev->vqs[queue].desc;
+status->avail = (uint64_t)(unsigned long)hdev->vqs[queue].avail;
+status->used = (uint64_t)(unsigned long)hdev->vqs[queue].used;
+status->num = hdev->vqs[queue].num;
+status->desc_phys = hdev->vqs[queue].desc_phys;
+status->desc_size = hdev->vqs[queue].desc_size;
+status->avail_phys = hdev->vqs[queue].avail_phys;
+status->avail_size = hdev->vqs[queue].avail_size;
+status->used_phys = hdev->vqs[queue].used_phys;
+status->used_size = hdev->vqs[queue].used_size;
+
+return status;
+}
+
+VirtQueueStatus *qmp_x_query_virtio_queue_status(const char *path,
+ uint16_t queue,
+ Error **errp)
+{
+VirtIODevice *vdev;
+VirtQueueStatus *status;
+
+vdev = virtio_device_find(path);
+if (vdev == NULL) {
+error_setg(errp, "Path %s is not a VirtIODevice", path);
+return NULL;
+}
+
+if (queue >= VIRTIO_QUEUE_MAX || !virtio_queue_get_num(vdev, queue)) {
+error_setg(errp, "Invalid virtqueue number %d", queue);
+return NULL;
+}
+
+status = g_new0(VirtQueueStatus, 1);
+status->name = g_strdup(vdev->name);
+status->queue_index = vdev->vq[queue].queue_index;
+status->inuse = vdev->vq[queue].inuse;
+status->vring_num = vdev->vq[queue].vring.num;
+status->vring_num_default = vdev->vq[queue].vring.num_default;
+status->vring_align = vdev->vq[queue].vring.align;
+status->vring_desc = vdev->vq[queue].vring.desc;
+status->vring_avail = vdev->vq[queue].vring.avail;
+status->vring_used = vdev->vq[queue].vring.used;
+status->used_idx = vdev->vq[queue].used_idx;
+status->signalled_used = vdev->vq[queue].signalled_used;
+status->signalled_used_valid = vdev->vq[queue].signalled_used_valid;
+
+if (vdev->vhost_started) {
+VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+struct vhost_dev *hdev = vdc->get_vhost(vdev);
+
+/* check if vq index exists for vhost as well */
+if (queue >= hdev->vq_index && queue < hdev->vq_index + hdev->nvqs) {
+status->has_last_avail_idx = true;
+
+int vhost_vq_index =
+hdev->vhost_ops->vhost_get_vq_index(hdev, queue);
+struct vhost_vring_state state = {
+.index = vhost_vq_index,
+};
+
+
[PATCH v12 4/8] qmp: add QMP command x-query-virtio-status
From: Laurent Vivier
This new command shows the status of a VirtIODevice, including
its corresponding vhost device's status (if active).
Next patch will improve output by decoding feature bits, including
vhost device's feature bits (backend, protocol, acked, and features).
Also will decode status bits of a VirtIODevice.
[Jonah: Similar to previous patch, added a check to @virtio_device_find
to ensure synchronicity between @virtio_list and the devices in the QOM
composition tree.]
Signed-off-by: Jonah Palmer
---
hw/virtio/virtio-stub.c | 5 ++
hw/virtio/virtio.c | 104 +++
qapi/virtio.json| 222
3 files changed, 331 insertions(+)
diff --git a/hw/virtio/virtio-stub.c b/hw/virtio/virtio-stub.c
index 05a81ed..0b432e8 100644
--- a/hw/virtio/virtio-stub.c
+++ b/hw/virtio/virtio-stub.c
@@ -12,3 +12,8 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
{
return qmp_virtio_unsupported(errp);
}
+
+VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e59f0d7..30ccd7b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3928,6 +3928,110 @@ VirtioInfoList *qmp_x_query_virtio(Error **errp)
return list;
}
+static VirtIODevice *virtio_device_find(const char *path)
+{
+VirtIODevice *vdev;
+
+QTAILQ_FOREACH(vdev, _list, next) {
+DeviceState *dev = DEVICE(vdev);
+
+if (strcmp(dev->canonical_path, path) != 0) {
+continue;
+}
+
+Error *err = NULL;
+QObject *obj = qmp_qom_get(dev->canonical_path, "realized", );
+if (err == NULL) {
+GString *is_realized = qobject_to_json_pretty(obj, true);
+/* virtio device is NOT realized, remove it from list */
+if (!strncmp(is_realized->str, "false", 4)) {
+g_string_free(is_realized, true);
+qobject_unref(obj);
+QTAILQ_REMOVE(_list, vdev, next);
+return NULL;
+}
+g_string_free(is_realized, true);
+} else {
+/* virtio device doesn't exist in QOM tree */
+QTAILQ_REMOVE(_list, vdev, next);
+qobject_unref(obj);
+return NULL;
+}
+/* device exists in QOM tree & is realized */
+qobject_unref(obj);
+return vdev;
+}
+return NULL;
+}
+
+VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
+{
+VirtIODevice *vdev;
+VirtioStatus *status;
+
+vdev = virtio_device_find(path);
+if (vdev == NULL) {
+error_setg(errp, "Path %s is not a VirtIODevice", path);
+return NULL;
+}
+
+status = g_new0(VirtioStatus, 1);
+status->name = g_strdup(vdev->name);
+status->device_id = vdev->device_id;
+status->vhost_started = vdev->vhost_started;
+status->guest_features = vdev->guest_features;
+status->host_features = vdev->host_features;
+status->backend_features = vdev->backend_features;
+
+switch (vdev->device_endian) {
+case VIRTIO_DEVICE_ENDIAN_LITTLE:
+status->device_endian = g_strdup("little");
+break;
+case VIRTIO_DEVICE_ENDIAN_BIG:
+status->device_endian = g_strdup("big");
+break;
+default:
+status->device_endian = g_strdup("unknown");
+break;
+}
+
+status->num_vqs = virtio_get_num_queues(vdev);
+status->status = vdev->status;
+status->isr = vdev->isr;
+status->queue_sel = vdev->queue_sel;
+status->vm_running = vdev->vm_running;
+status->broken = vdev->broken;
+status->disabled = vdev->disabled;
+status->use_started = vdev->use_started;
+status->started = vdev->started;
+status->start_on_kick = vdev->start_on_kick;
+status->disable_legacy_check = vdev->disable_legacy_check;
+status->bus_name = g_strdup(vdev->bus_name);
+status->use_guest_notifier_mask = vdev->use_guest_notifier_mask;
+status->has_vhost_dev = vdev->vhost_started;
+
+if (vdev->vhost_started) {
+VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
+struct vhost_dev *hdev = vdc->get_vhost(vdev);
+
+status->vhost_dev = g_new0(VhostStatus, 1);
+status->vhost_dev->n_mem_sections = hdev->n_mem_sections;
+status->vhost_dev->n_tmp_sections = hdev->n_tmp_sections;
+status->vhost_dev->nvqs = hdev->nvqs;
+status->vhost_dev->vq_index = hdev->vq_index;
+status->vhost_dev->features = hdev->features;
+status->vhost_dev->acked_features = hdev->acked_features;
+status->vhost_dev->backend_features = hdev->backend_features;
+status->vhost_dev->protocol_features = hdev->protocol_features;
+status->vhost_dev->max_queues = hdev->max_queues;
+status->vhost_dev->backend_cap = hdev->backend_cap;
+status->vhost_dev->log_enabled =
[PATCH v12 1/8] virtio: drop name parameter for virtio_init()
This patch drops the name parameter for the virtio_init function.
The pair between the numeric device ID and the string device ID
(name) of a virtio device already exists, but not in a way that
lets us map between them.
This patch lets us do this and removes the need for the name
parameter in the virtio_init function.
[Jonah: added new virtio IDs to virtio device list from rebase].
Signed-off-by: Jonah Palmer
---
hw/9pfs/virtio-9p-device.c | 2 +-
hw/block/vhost-user-blk.c | 2 +-
hw/block/virtio-blk.c | 2 +-
hw/char/virtio-serial-bus.c| 3 +-
hw/display/virtio-gpu-base.c | 2 +-
hw/input/virtio-input.c| 3 +-
hw/net/virtio-net.c| 2 +-
hw/scsi/virtio-scsi.c | 3 +-
hw/virtio/vhost-user-fs.c | 3 +-
hw/virtio/vhost-user-i2c.c | 7 +
hw/virtio/vhost-user-rng.c | 2 +-
hw/virtio/vhost-user-vsock.c | 2 +-
hw/virtio/vhost-vsock-common.c | 5 ++--
hw/virtio/vhost-vsock.c| 2 +-
hw/virtio/virtio-balloon.c | 3 +-
hw/virtio/virtio-crypto.c | 2 +-
hw/virtio/virtio-iommu.c | 3 +-
hw/virtio/virtio-mem.c | 3 +-
hw/virtio/virtio-pmem.c| 3 +-
hw/virtio/virtio-rng.c | 2 +-
hw/virtio/virtio.c | 55 --
include/hw/virtio/vhost-vsock-common.h | 2 +-
include/hw/virtio/virtio-gpu.h | 3 +-
include/hw/virtio/virtio.h | 4 +--
24 files changed, 77 insertions(+), 43 deletions(-)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 54ee93b..5f522e6 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -216,7 +216,7 @@ static void virtio_9p_device_realize(DeviceState *dev,
Error **errp)
}
v->config_size = sizeof(struct virtio_9p_config) + strlen(s->fsconf.tag);
-virtio_init(vdev, "virtio-9p", VIRTIO_ID_9P, v->config_size);
+virtio_init(vdev, VIRTIO_ID_9P, v->config_size);
v->vq = virtio_add_queue(vdev, MAX_REQ, handle_9p_output);
}
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1a42ae9..e8cb170 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -491,7 +491,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
return;
}
-virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
+virtio_init(vdev, VIRTIO_ID_BLOCK,
sizeof(struct virtio_blk_config));
s->virtqs = g_new(VirtQueue *, s->num_queues);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 82676cd..2e3809d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1205,7 +1205,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_set_config_size(s, s->host_features);
-virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
+virtio_init(vdev, VIRTIO_ID_BLOCK, s->config_size);
s->blk = conf->conf.blk;
s->rq = NULL;
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index f01ec21..9f19fd0 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -1044,8 +1044,7 @@ static void virtio_serial_device_realize(DeviceState
*dev, Error **errp)
VIRTIO_CONSOLE_F_EMERG_WRITE)) {
config_size = offsetof(struct virtio_console_config, emerg_wr);
}
-virtio_init(vdev, "virtio-serial", VIRTIO_ID_CONSOLE,
-config_size);
+virtio_init(vdev, VIRTIO_ID_CONSOLE, config_size);
/* Spawn a new virtio-serial bus on which the ports will ride as devices */
qbus_init(>bus, sizeof(vser->bus), TYPE_VIRTIO_SERIAL_BUS,
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index fff0fb4..8ba5da4 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -173,7 +173,7 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
}
g->virtio_config.num_scanouts = cpu_to_le32(g->conf.max_outputs);
-virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU,
+virtio_init(VIRTIO_DEVICE(g), VIRTIO_ID_GPU,
sizeof(struct virtio_gpu_config));
if (virtio_gpu_virgl_enabled(g->conf)) {
diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index 54bcb46..5b5398b 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -257,8 +257,7 @@ static void virtio_input_device_realize(DeviceState *dev,
Error **errp)
vinput->cfg_size += 8;
assert(vinput->cfg_size <= sizeof(virtio_input_config));
-virtio_init(vdev, "virtio-input", VIRTIO_ID_INPUT,
-vinput->cfg_size);
+virtio_init(vdev, VIRTIO_ID_INPUT, vinput->cfg_size);
vinput->evt = virtio_add_queue(vdev, 64, virtio_input_handle_evt);
vinput->sts = virtio_add_queue(vdev,
