Re: [PATCH v8 8/8] hmp: add virtio commands
Jonah Palmer writes:
> From: Laurent Vivier
>
> This patch implements the HMP versions of the virtio QMP commands.
>
> Signed-off-by: Jonah Palmer
> ---
> docs/system/monitor.rst | 2 +
> hmp-commands-virtio.hx | 250 ++
> hmp-commands.hx | 10 ++
> hw/virtio/virtio.c | 355
>
> include/monitor/hmp.h | 5 +
> meson.build | 1 +
> monitor/misc.c | 17 +++
> 7 files changed, 640 insertions(+)
> create mode 100644 hmp-commands-virtio.hx
>
> diff --git a/docs/system/monitor.rst b/docs/system/monitor.rst
> index ff5c434..10418fc 100644
> --- a/docs/system/monitor.rst
> +++ b/docs/system/monitor.rst
> @@ -21,6 +21,8 @@ The following commands are available:
>
> .. hxtool-doc:: hmp-commands.hx
>
> +.. hxtool-doc:: hmp-commands-virtio.hx
> +
> .. hxtool-doc:: hmp-commands-info.hx
>
> Integer expressions
> diff --git a/hmp-commands-virtio.hx b/hmp-commands-virtio.hx
> new file mode 100644
> index 000..36aab94
> --- /dev/null
> +++ b/hmp-commands-virtio.hx
> @@ -0,0 +1,250 @@
> +HXCOMM Use DEFHEADING() to define headings in both help text and rST.
> +HXCOMM Text between SRST and ERST is copied to the rST version and
> +HXCOMM discarded from C version.
> +HXCOMM
> +HXCOMM DEF(command, args, callback, arg_string, help) is used to construct
> +HXCOMM monitor info commands.
> +HXCOMM
> +HXCOMM HXCOMM can be used for comments, discarded from both rST and C.
> +HXCOMM
> +HXCOMM In this file, generally SRST fragments should have two extra
> +HXCOMM spaces of indent, so that the documentation list item for "virtio cmd"
> +HXCOMM appears inside the documentation list item for the top level
> +HXCOMM "virtio" documentation entry. The exception is the first SRST
> +HXCOMM fragment that defines that top level entry.
> +
> +SRST
> + ``virtio`` *subcommand*
> + Show various information about virtio
> +
> + Example:
> +
> + List all sub-commands::
> +
> + (qemu) virtio
> + virtio query -- List all available virtio devices
I get:
qemu/docs/../hmp-commands-virtio.hx:25:Inconsistent literal block quoting.
> + virtio status path -- Display status of a given virtio device
> + virtio queue-status path queue -- Display status of a given virtio queue
> + virtio vhost-queue-status path queue -- Display status of a given vhost
> queue
> + virtio queue-element path queue [index] -- Display element of a given
> virtio queue
> +
> +ERST
[...]
> diff --git a/monitor/misc.c b/monitor/misc.c
> index ffe7966..5e4cd88 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -23,6 +23,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include CONFIG_DEVICES
> #include "monitor-internal.h"
> #include "monitor/qdev.h"
> #include "hw/usb.h"
> @@ -219,6 +220,15 @@ static void hmp_info_help(Monitor *mon, const QDict
> *qdict)
> help_cmd(mon, "info");
> }
>
> +static void hmp_virtio_help(Monitor *mon, const QDict *qdict)
> +{
> +#if defined(CONFIG_VIRTIO)
> +help_cmd(mon, "virtio");
Probably not your patch's fault: extra space before '--' in the line
virtio query -- List all available virtio devices
> +#else
> +monitor_printf(mon, "Virtio is disabled\n");
> +#endif
> +}
> +
> static void monitor_init_qmp_commands(void)
> {
> /*
> @@ -1433,6 +1443,13 @@ static HMPCommand hmp_info_cmds[] = {
> { NULL, NULL, },
> };
>
> +static HMPCommand hmp_virtio_cmds[] = {
> +#if defined(CONFIG_VIRTIO)
> +#include "hmp-commands-virtio.h"
> +#endif
> +{ NULL, NULL, },
> +};
> +
> /* hmp_cmds and hmp_info_cmds would be sorted at runtime */
> HMPCommand hmp_cmds[] = {
> #include "hmp-commands.h"
Re: [PATCH v8 0/8] hmp, qmp: Add commands to introspect virtio devices
Daniel P. Berrangé writes: > On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: >> This series introduces new QMP/HMP commands to dump the status of a >> virtio device at different levels. >> >> [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches >> are from Laurent Vivier from May 2020. >> >> Rebase from v7 to v8 includes an additional assert to make sure >> we're not returning NULL in virtio_id_to_name(). Rebase also >> includes minor additions/edits to qapi/virtio.json.] >> >> 1. Main command >> >> HMP Only: >> >> virtio [subcommand] >> >> Example: >> >> List all sub-commands: >> >> (qemu) virtio >> virtio query -- List all available virtio devices >> virtio status path -- Display status of a given virtio device >> virtio queue-status path queue -- Display status of a given virtio >> queue >> virtio vhost-queue-status path queue -- Display status of a given >> vhost queue >> virtio queue-element path queue [index] -- Display element of a >> given virtio queue > > I don't see a compelling reason why these are setup as sub-commands > under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' > naming just feels needlessly different from the current QEMU practices. > > IMHO they should just be "info" subcommands for HMP. ie > > info virtio -- List all available virtio devices > info virtio-status path -- Display status of a given virtio device > info virtio-queue-status path queue -- Display status of a given > virtio queue > info virtio-vhost-queue-status path queue -- Display status of a > given vhost queue > info virtio-queue-element path queue [index] -- Display element of a > given virtio queue I agree with Dan (but I'm not the maintainer). > While the corresponding QMP commands ought to be > > x-query-virtio > x-query-virtio-status > x-query-virtio-queue-status > x-query-virtio-vhost-queue-status > x-query-virtio-queue-element I agree with Dan (and I am the maintainer). The x- is not strictly required anymore (see commit a3c45b3e62 'qapi: New special feature flag "unstable"'). I lean towards keeping it here, because we don't plan to stabilize these commands.
Re: [PATCH v8 3/8] qmp: add QMP command x-debug-query-virtio
On 11/4/21 11:15, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This new command lists all the instances of VirtIODevice with
their QOM paths and virtio type/name.
Signed-off-by: Jonah Palmer
---
hw/virtio/meson.build | 2 ++
hw/virtio/virtio-stub.c| 14 ++
hw/virtio/virtio.c | 27 +++
include/hw/virtio/virtio.h | 1 +
qapi/meson.build | 1 +
qapi/qapi-schema.json | 1 +
qapi/virtio.json | 67 ++
tests/qtest/qmp-cmd-test.c | 1 +
8 files changed, 114 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..d4a88f5
--- /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_debug_query_virtio(Error **errp)
+{
+return qmp_virtio_unsupported(errp);
+}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7050bd5..ad17be7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -13,6 +13,8 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-virtio.h"
+#include "qapi/qapi-visit-virtio.h"
#include "cpu.h"
#include "trace.h"
#include "qemu/error-report.h"
@@ -29,6 +31,9 @@
#include "sysemu/runstate.h"
#include "standard-headers/linux/virtio_ids.h"
+/* QAPI list of 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
@@ -3709,6 +3714,7 @@ static void virtio_device_realize(DeviceState *dev, Error
**errp)
vdev->listener.commit = virtio_memory_listener_commit;
vdev->listener.name = "virtio";
memory_listener_register(&vdev->listener, vdev->dma_as);
+QTAILQ_INSERT_TAIL(&virtio_list, vdev, next);
}
static void virtio_device_unrealize(DeviceState *dev)
@@ -3723,6 +3729,7 @@ static void virtio_device_unrealize(DeviceState *dev)
vdc->unrealize(dev);
}
+QTAILQ_REMOVE(&virtio_list, vdev, next);
g_free(vdev->bus_name);
vdev->bus_name = NULL;
}
@@ -3896,6 +3903,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(&virtio_list);
}
bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
@@ -3906,6 +3915,24 @@ bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
return virtio_bus_ioeventfd_enabled(vbus);
}
+VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
+{
+VirtioInfoList *list = NULL;
+VirtioInfoList *node;
+VirtIODevice *vdev;
+
+QTAILQ_FOREACH(vdev, &virtio_list, next) {
+DeviceState *dev = DEVICE(vdev);
+node = g_new0(VirtioInfoList, 1);
+node->value = g_new(VirtioInfo, 1);
+node->value->path = g_strdup(dev->canonical_path);
+node->value->type = g_strdup(vdev->name);
+QAPI_LIST_PREPEND(list, node->value);
+}
+
+return list;
+}
+
static const TypeInfo virtio_device_info = {
.name = TYPE_VIRTIO_DEVICE,
.parent = TYPE_DEVICE,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 105b98c..eceaafc 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -110,6 +110,7 @@ struct VirtIODevice
bool use_guest_notifier_mask;
AddressSpace *dma_as;
QLIST_HEAD(, VirtQueue) *vector_queues;
+QTAILQ_ENTRY(VirtIODevice) next;
};
struct VirtioDeviceClass {
diff --git a/qapi/meson.build b/qapi/meson.build
index c356a38..df5662e 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -45,6 +45,7 @@ qapi_all_modules = [
'sockets',
'trace',
'transaction',
+ 'virtio',
'yank',
]
if have_system
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 4912b97..1512ada 100644
--- a/qapi/qapi-schema.json
+++
Re: [PATCH v8 4/8] qmp: add QMP command x-debug-virtio-status
On 11/4/21 11:37, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This new command shows the status of a VirtIODevice, including
its corresponding vhost device 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.
Next patch will also suppress the vhost device field from displaying
if no vhost device is active for a given VirtIODevice.
Signed-off-by: Jonah Palmer
---
[...]
diff --git a/qapi/virtio.json b/qapi/virtio.json
index 4490c2c..656a26f 100644
--- a/qapi/virtio.json
+++ b/qapi/virtio.json
@@ -65,3 +65,258 @@
##
{ 'command': 'x-debug-query-virtio', 'returns': ['VirtioInfo'] }
+
+##
+# @VirtioStatusEndianness:
+#
+# Enumeration of endianness for VirtioDevice
+#
+# Since: 6.2
+##
+
+{ 'enum': 'VirtioStatusEndianness',
+ 'data': [ 'unknown', 'little', 'big' ]
+}
+
+##
+# @VhostStatus:
+#
+# Information about a vhost device. This information will only be
+# displayed if the vhost device is active.
+#
+# @n-mem-sections: vhost_dev n_mem_sections
+#
+# @n-tmp-sections: vhost_dev n_tmp_sections
+#
+# @nvqs: vhost_dev nvqs. This is the number of virtqueues being used
+#by the vhost device.
+#
+# @vq-index: vhost_dev vq_index
+#
+# @features: vhost_dev features
+#
+# @acked-features: vhost_dev acked_features
+#
+# @backend-features: vhost_dev backend_features
+#
+# @protocol-features: vhost_dev protocol_features
+#
+# @max-queues: vhost_dev max_queues
+#
+# @backend-cap: vhost_dev backend_cap
+#
+# @log-enabled: vhost_dev log_enabled flag
+#
+# @log-size: vhost_dev log_size
+#
+# Since: 6.2
+#
+##
+
+{ 'struct': 'VhostStatus',
+'data': {
+'n-mem-sections': 'int',
+'n-tmp-sections': 'int',
Odd indentation. Better
{ 'struct': 'VhostStatus',
'data': {
'n-mem-sections': 'int',
'n-tmp-sections': 'int',
or
{ 'struct': 'VhostStatus',
'data': { 'n-mem-sections': 'int',
'n-tmp-sections': 'int',
More of the same below, and possibly in other patches. I'm not going to
point it out again.
Gotcha, I'll use this format in the .json file. Will change it to this for
all relevant patches.
+'nvqs': 'uint32',
+'vq-index': 'int',
+'features': 'uint64',
+'acked-features': 'uint64',
+'backend-features': 'uint64',
+'protocol-features': 'uint64',
+'max-queues': 'uint64',
+'backend-cap': 'uint64',
+'log-enabled': 'bool',
+'log-size': 'uint64'
+}
+}
I can't tell whether these are all needed. Got to trust virtio experts
there.
Not sure either, I just included whatever I could include.
I'm not checking the schema types match the data sources' C types. I
hope you did :)
Yes c:
More of the same below, and possibly in other patches. I'm not going to
point it out again.
+
+##
+# @VirtioStatus:
+#
+# Full status of the virtio device with most VirtIODevice members.
+# Also includes the full status of the corresponding vhost device
+# if the vhost device is active.
+#
+# @name: VirtIODevice name
+#
+# @device-id: VirtIODevice ID
+#
+# @vhost-started: VirtIODevice vhost_started flag
+#
+# @guest-features: VirtIODevice guest_features
+#
+# @host-features: VirtIODevice host_features
+#
+# @backend-features: VirtIODevice backend_features
+#
+# @device-endian: VirtIODevice device_endian
+#
+# @num-vqs: VirtIODevice virtqueue count. This is the number of active
+# virtqueues being used by the VirtIODevice.
+#
+# @status: VirtIODevice configuration status (e.g. DRIVER_OK,
+# FEATURES_OK, DRIVER, etc.)
+#
+# @isr: VirtIODevice ISR
+#
+# @queue-sel: VirtIODevice queue_sel
+#
+# @vm-running: VirtIODevice vm_running flag
+#
+# @broken: VirtIODevice broken flag
+#
+# @disabled: VirtIODevice disabled flag
+#
+# @use-started: VirtIODevice use_started flag
+#
+# @started: VirtIODevice started flag
+#
+# @start-on-kick: VirtIODevice start_on_kick flag
+#
+# @disable-legacy-check: VirtIODevice disabled_legacy_check flag
+#
+# @bus-name: VirtIODevice bus_name
+#
+# @use-guest-notifier-mask: VirtIODevice use_guest_notifier_mask flag
+#
+# @vhost-dev: corresponding vhost device info for a given VirtIODevice
+#
+# Since: 6.2
+#
+##
+
+{ 'struct': 'VirtioStatus',
+'data': {
+'name': 'str',
+'device-id': 'uint16',
+'vhost-started': 'bool',
+'guest-features': 'uint64',
+'host-features': 'uint64',
+'backend-features': 'uint64',
+'device-endian': 'VirtioStatusEndianness',
+'num-vqs': 'int',
+'status': 'uint8',
+'isr': 'uint8',
+'queue-sel': 'uint16',
+'vm-running': 'bool',
+'broken': 'bool',
+'disabled': 'bool',
+'use-started': 'bool',
+'started': 'bool',
+'start-on-kick': 'bool',
+'disable-legacy-check': 'bool',
+'b
Re: [PATCH v8 8/8] hmp: add virtio commands
On 11/5/21 03:23, Markus Armbruster wrote:
Jonah Palmer writes:
From: Laurent Vivier
This patch implements the HMP versions of the virtio QMP commands.
Signed-off-by: Jonah Palmer
---
docs/system/monitor.rst | 2 +
hmp-commands-virtio.hx | 250 ++
hmp-commands.hx | 10 ++
hw/virtio/virtio.c | 355
include/monitor/hmp.h | 5 +
meson.build | 1 +
monitor/misc.c | 17 +++
7 files changed, 640 insertions(+)
create mode 100644 hmp-commands-virtio.hx
diff --git a/docs/system/monitor.rst b/docs/system/monitor.rst
index ff5c434..10418fc 100644
--- a/docs/system/monitor.rst
+++ b/docs/system/monitor.rst
@@ -21,6 +21,8 @@ The following commands are available:
.. hxtool-doc:: hmp-commands.hx
+.. hxtool-doc:: hmp-commands-virtio.hx
+
.. hxtool-doc:: hmp-commands-info.hx
Integer expressions
diff --git a/hmp-commands-virtio.hx b/hmp-commands-virtio.hx
new file mode 100644
index 000..36aab94
--- /dev/null
+++ b/hmp-commands-virtio.hx
@@ -0,0 +1,250 @@
+HXCOMM Use DEFHEADING() to define headings in both help text and rST.
+HXCOMM Text between SRST and ERST is copied to the rST version and
+HXCOMM discarded from C version.
+HXCOMM
+HXCOMM DEF(command, args, callback, arg_string, help) is used to construct
+HXCOMM monitor info commands.
+HXCOMM
+HXCOMM HXCOMM can be used for comments, discarded from both rST and C.
+HXCOMM
+HXCOMM In this file, generally SRST fragments should have two extra
+HXCOMM spaces of indent, so that the documentation list item for "virtio cmd"
+HXCOMM appears inside the documentation list item for the top level
+HXCOMM "virtio" documentation entry. The exception is the first SRST
+HXCOMM fragment that defines that top level entry.
+
+SRST
+ ``virtio`` *subcommand*
+ Show various information about virtio
+
+ Example:
+
+ List all sub-commands::
+
+ (qemu) virtio
+ virtio query -- List all available virtio devices
I get:
qemu/docs/../hmp-commands-virtio.hx:25:Inconsistent literal block quoting.
+ virtio status path -- Display status of a given virtio device
+ virtio queue-status path queue -- Display status of a given virtio queue
+ virtio vhost-queue-status path queue -- Display status of a given vhost queue
+ virtio queue-element path queue [index] -- Display element of a given virtio
queue
+
+ERST
[...]
diff --git a/monitor/misc.c b/monitor/misc.c
index ffe7966..5e4cd88 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -23,6 +23,7 @@
*/
#include "qemu/osdep.h"
+#include CONFIG_DEVICES
#include "monitor-internal.h"
#include "monitor/qdev.h"
#include "hw/usb.h"
@@ -219,6 +220,15 @@ static void hmp_info_help(Monitor *mon, const QDict *qdict)
help_cmd(mon, "info");
}
+static void hmp_virtio_help(Monitor *mon, const QDict *qdict)
+{
+#if defined(CONFIG_VIRTIO)
+help_cmd(mon, "virtio");
Probably not your patch's fault: extra space before '--' in the line
virtio query -- List all available virtio devices
Huh interesting... I'll get this patched up!
+#else
+monitor_printf(mon, "Virtio is disabled\n");
+#endif
+}
+
static void monitor_init_qmp_commands(void)
{
/*
@@ -1433,6 +1443,13 @@ static HMPCommand hmp_info_cmds[] = {
{ NULL, NULL, },
};
+static HMPCommand hmp_virtio_cmds[] = {
+#if defined(CONFIG_VIRTIO)
+#include "hmp-commands-virtio.h"
+#endif
+{ NULL, NULL, },
+};
+
/* hmp_cmds and hmp_info_cmds would be sorted at runtime */
HMPCommand hmp_cmds[] = {
#include "hmp-commands.h"
Jonah
Re: [PATCH 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers
On Thu, Nov 04, 2021 at 04:48:43PM +0100, Łukasz Gieryk wrote:
> On Wed, Nov 03, 2021 at 01:07:31PM +0100, Klaus Jensen wrote:
> > On Oct 7 18:24, Lukasz Maniak wrote:
> > > From: Łukasz Gieryk
> > >
> > > With two new properties (sriov_max_vi_per_vf, sriov_max_vq_per_vf) one
> > > can configure the maximum number of virtual queues and interrupts
> > > assignable to a single virtual device. 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.
> > >
> >
> > While this patch allows configuring the VQFRSM and VIFRSM fields, it
> > implicitly sets VQFRT and VIFRT (i.e. by setting them to the product of
> > sriov_max_vi_pervf and max_vfs). Which is just setting it to an upper
> > bound and this removes a testable case for host software (e.g.
> > requesting more flexible resources than what is currently available).
> >
> > This patch also requires that these parameters are set if sriov_max_vfs
> > is. I think we can provide better defaults.
> >
>
> Originally I considered more params, but ended up coding the simplest,
> user-friendly solution, because I did not like the mess with so many
> parameters, and the flexibility wasn't needed for my use cases. But I do
> agree: others may need the flexibility. Case (FRT < max_vfs * FRSM) is
> valid and resembles an actual device.
>
> > How about,
> >
> > 1. if only sriov_max_vfs is set, then all VFs get private resources
> >equal to max_ioqpairs. Like before this patch. This limits the number
> >of parameters required to get a basic setup going.
> >
> > 2. if sriov_v{q,i}_private is set (I suggested this parameter in patch
> >10), the difference between that and max_ioqpairs become flexible
> >resources. Also, I'd be just fine with having sriov_v{q,i}_flexible
> >instead and just make the difference become private resources.
> >Potato/potato.
> >
> >a. in the absence of sriov_max_v{q,i}_per_vf, set them to the number
> > of calculated flexible resources.
> >
> > This probably smells a bit like bikeshedding, but I think this gives
> > more flexibility and better defaults, which helps with verifying host
> > software.
> >
> > If we can't agree on this now, I suggest we could go ahead and merge the
> > base functionality (i.e. private resources only) and ruminate some more
> > about these parameters.
>
> The problem is that the spec allows VFs to support either only private,
> or only flexible resources.
>
> At this point I have to admit, that since my use cases for
> QEMU/Nvme/SRIOV require flexible resources, I haven’t paid much
> attention to the case with VFs having private resources. So this SR/IOV
> implementation doesn’t even support such case (max_vX_per_vf != 0).
>
> Let me summarize the possible config space, and how the current
> parameters (could) map to these (interrupt-related ones omitted):
>
> Flexible resources not supported (not implemented):
> - Private resources for PF = max_ioqpairs
> - Private resources per VF = ?
> - (error if flexible resources are configured)
>
> With flexible resources:
> - VQPRT, private resources for PF = max_ioqpairs
> - VQFRT, total flexible resources = max_vq_per_vf * num_vfs
> - VQFRSM, maximum assignable per VF= max_vq_per_vf
> - VQGRAN, granularity = #define constant
> - (error if private resources per VF are configured)
>
> Since I don’t want to misunderstand your suggestion: could you provide a
> similar map with your parameters, formulas, and explain how to determine
> if flexible resources are active? I want to be sure we are on the
> same page.
>
I’ve just re-read through my email and decided that some bits need
clarification.
This implementation supports the “Flexible”-resources-only flavor of
SR/IOV, while the “Private” also could be supported. Some effort is
required to support both, and I cannot afford that (at least I cannot
commit today, neither the other Lukasz).
While I’m ready to rework the Flexible config and prepare it to be
extended later to handle the Private variant, the 2nd version of these
patches will still support the Flexible flavor only.
I will include appropriate TODO/open in the next cover letter.
Re: [PATCH v8 0/8] hmp, qmp: Add commands to introspect virtio devices
This series increases total size (text + data + bss) by 134KiB for me. QAPI clearly was not designed for space efficiency. Still, it's a drop in the bucket. If debugging commands ever become a burden for certain use cases, we can think about making them compile-time optional.
Re: [PATCH v8 0/8] hmp, qmp: Add commands to introspect virtio devices
On 11/5/21 03:26, Markus Armbruster wrote: Daniel P. Berrangé writes: On Wed, Oct 27, 2021 at 07:41:41AM -0400, Jonah Palmer wrote: This series introduces new QMP/HMP commands to dump the status of a virtio device at different levels. [Jonah: Rebasing previous patchset from Oct. 5 (v7). Original patches are from Laurent Vivier from May 2020. Rebase from v7 to v8 includes an additional assert to make sure we're not returning NULL in virtio_id_to_name(). Rebase also includes minor additions/edits to qapi/virtio.json.] 1. Main command HMP Only: virtio [subcommand] Example: List all sub-commands: (qemu) virtio virtio query -- List all available virtio devices virtio status path -- Display status of a given virtio device virtio queue-status path queue -- Display status of a given virtio queue virtio vhost-queue-status path queue -- Display status of a given vhost queue virtio queue-element path queue [index] -- Display element of a given virtio queue I don't see a compelling reason why these are setup as sub-commands under a new "virtio" top level. This HMP approach and the QMP 'x-debug-query' naming just feels needlessly different from the current QEMU practices. IMHO they should just be "info" subcommands for HMP. ie info virtio -- List all available virtio devices info virtio-status path -- Display status of a given virtio device info virtio-queue-status path queue -- Display status of a given virtio queue info virtio-vhost-queue-status path queue -- Display status of a given vhost queue info virtio-queue-element path queue [index] -- Display element of a given virtio queue I agree with Dan (but I'm not the maintainer). I do like this format a bit better than Dave's recommendation. Feels a bit more intuitive to understand what the commands should be doing, but I'm not sure if this is just because I'm new to these things. I'd like to format it like above if that's okay. While the corresponding QMP commands ought to be x-query-virtio x-query-virtio-status x-query-virtio-queue-status x-query-virtio-vhost-queue-status x-query-virtio-queue-element I agree with Dan (and I am the maintainer). The x- is not strictly required anymore (see commit a3c45b3e62 'qapi: New special feature flag "unstable"'). I lean towards keeping it here, because we don't plan to stabilize these commands. Ok! I'll keep the 'x-' in and change them to the above. Thank you for the comments!! Jonah
Re: [PATCH 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers
On Fri, Nov 05, 2021 at 09:46:28AM +0100, Łukasz Gieryk wrote:
> On Thu, Nov 04, 2021 at 04:48:43PM +0100, Łukasz Gieryk wrote:
> > On Wed, Nov 03, 2021 at 01:07:31PM +0100, Klaus Jensen wrote:
> > > On Oct 7 18:24, Lukasz Maniak wrote:
> > > > From: Łukasz Gieryk
> > > >
> > > > With two new properties (sriov_max_vi_per_vf, sriov_max_vq_per_vf) one
> > > > can configure the maximum number of virtual queues and interrupts
> > > > assignable to a single virtual device. 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.
> > > >
> > >
> > > While this patch allows configuring the VQFRSM and VIFRSM fields, it
> > > implicitly sets VQFRT and VIFRT (i.e. by setting them to the product of
> > > sriov_max_vi_pervf and max_vfs). Which is just setting it to an upper
> > > bound and this removes a testable case for host software (e.g.
> > > requesting more flexible resources than what is currently available).
> > >
> > > This patch also requires that these parameters are set if sriov_max_vfs
> > > is. I think we can provide better defaults.
> > >
> >
> > Originally I considered more params, but ended up coding the simplest,
> > user-friendly solution, because I did not like the mess with so many
> > parameters, and the flexibility wasn't needed for my use cases. But I do
> > agree: others may need the flexibility. Case (FRT < max_vfs * FRSM) is
> > valid and resembles an actual device.
> >
> > > How about,
> > >
> > > 1. if only sriov_max_vfs is set, then all VFs get private resources
> > >equal to max_ioqpairs. Like before this patch. This limits the number
> > >of parameters required to get a basic setup going.
> > >
> > > 2. if sriov_v{q,i}_private is set (I suggested this parameter in patch
> > >10), the difference between that and max_ioqpairs become flexible
> > >resources. Also, I'd be just fine with having sriov_v{q,i}_flexible
> > >instead and just make the difference become private resources.
> > >Potato/potato.
> > >
> > >a. in the absence of sriov_max_v{q,i}_per_vf, set them to the number
> > > of calculated flexible resources.
> > >
> > > This probably smells a bit like bikeshedding, but I think this gives
> > > more flexibility and better defaults, which helps with verifying host
> > > software.
> > >
> > > If we can't agree on this now, I suggest we could go ahead and merge the
> > > base functionality (i.e. private resources only) and ruminate some more
> > > about these parameters.
> >
> > The problem is that the spec allows VFs to support either only private,
> > or only flexible resources.
> >
> > At this point I have to admit, that since my use cases for
> > QEMU/Nvme/SRIOV require flexible resources, I haven’t paid much
> > attention to the case with VFs having private resources. So this SR/IOV
> > implementation doesn’t even support such case (max_vX_per_vf != 0).
> >
> > Let me summarize the possible config space, and how the current
> > parameters (could) map to these (interrupt-related ones omitted):
> >
> > Flexible resources not supported (not implemented):
> > - Private resources for PF = max_ioqpairs
> > - Private resources per VF = ?
> > - (error if flexible resources are configured)
> >
> > With flexible resources:
> > - VQPRT, private resources for PF = max_ioqpairs
> > - VQFRT, total flexible resources = max_vq_per_vf * num_vfs
> > - VQFRSM, maximum assignable per VF= max_vq_per_vf
> > - VQGRAN, granularity = #define constant
> > - (error if private resources per VF are configured)
> >
> > Since I don’t want to misunderstand your suggestion: could you provide a
> > similar map with your parameters, formulas, and explain how to determine
> > if flexible resources are active? I want to be sure we are on the
> > same page.
> >
>
> I’ve just re-read through my email and decided that some bits need
> clarification.
>
> This implementation supports the “Flexible”-resources-only flavor of
> SR/IOV, while the “Private” also could be supported. Some effort is
> required to support both, and I cannot afford that (at least I cannot
> commit today, neither the other Lukasz).
>
> While I’m ready to rework the Flexible config and prepare it to be
> extended later to handle the Private variant, the 2nd version of these
> patches will still support the Flexible flavor only.
>
> I will include appropriate TODO/open in the next cover letter.
>
The summary of my thoughts, so far:
- I'm going to introduce sriov_v{q,i}_flexible and better defaults,
according to your suggestion (as far as I understand your intentions,
please correct me if I've missed something).
- The Private SR/IOV flavor, if it's ever implemented, could introduce
sriov_vq_private_per_vf.
- The updated formulas are listed below.
Flexible resour
Re: [PATCH 5/7] block: Pass BdrvChild ** to replace_child_noperm
Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben:
> bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially
> set it to NULL. That is dangerous, because BDS parents generally assume
> that their children's .bs pointer is never NULL. We therefore want to
> let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer
> to NULL, too.
>
> This patch lays the foundation for it by passing a BdrvChild ** pointer
> to bdrv_replace_child_noperm() so that it can later use it to NULL the
> BdrvChild pointer immediately after setting BdrvChild.bs to NULL.
>
> Signed-off-by: Hanna Reitz
> ---
> block.c | 59 -
> 1 file changed, 33 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index 6d230ad3d1..ff45447686 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char
> *filename,
> static bool bdrv_recurse_has_child(BlockDriverState *bs,
> BlockDriverState *child);
>
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **child,
>BlockDriverState *new_bs);
> static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>BdrvChild *child,
> @@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs,
> uint64_t perm,
>
> typedef struct BdrvReplaceChildState {
> BdrvChild *child;
> +BdrvChild **childp;
Would it be clearer to rename child to old_child now that it can be
changed?
I would also consider having childp first because this is really the
object that we're modifying and potentially rolling back now.
> BlockDriverState *old_bs;
> } BdrvReplaceChildState;
>
> @@ -2269,8 +2270,8 @@ static void bdrv_replace_child_abort(void *opaque)
> BdrvReplaceChildState *s = opaque;
> BlockDriverState *new_bs = s->child->bs;
>
> -/* old_bs reference is transparently moved from @s to @s->child */
> -bdrv_replace_child_noperm(s->child, s->old_bs);
> +/* old_bs reference is transparently moved from @s to *s->childp */
> +bdrv_replace_child_noperm(s->childp, s->old_bs);
> bdrv_unref(new_bs);
> }
>
> @@ -2287,21 +2288,23 @@ static TransactionActionDrv bdrv_replace_child_drv = {
> *
> * The function doesn't update permissions, caller is responsible for this.
> */
> -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState
> *new_bs,
> +static void bdrv_replace_child_tran(BdrvChild **childp,
> +BlockDriverState *new_bs,
> Transaction *tran)
> {
> BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
> *s = (BdrvReplaceChildState) {
> -.child = child,
> -.old_bs = child->bs,
> +.child = *childp,
> +.childp = childp,
> +.old_bs = (*childp)->bs,
> };
> tran_add(tran, &bdrv_replace_child_drv, s);
>
> if (new_bs) {
> bdrv_ref(new_bs);
> }
> -bdrv_replace_child_noperm(child, new_bs);
> -/* old_bs reference is transparently moved from @child to @s */
> +bdrv_replace_child_noperm(childp, new_bs);
> +/* old_bs reference is transparently moved from *childp to @s */
> }
>
> /*
> @@ -2672,9 +2675,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission
> qapi_perm)
> return permissions[qapi_perm];
> }
>
> -static void bdrv_replace_child_noperm(BdrvChild *child,
> +static void bdrv_replace_child_noperm(BdrvChild **childp,
>BlockDriverState *new_bs)
> {
> +BdrvChild *child = *childp;
> BlockDriverState *old_bs = child->bs;
> int new_bs_quiesce_counter;
> int drain_saldo;
> @@ -2767,7 +2771,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
> BdrvChild *child = *s->child;
> BlockDriverState *bs = child->bs;
>
> -bdrv_replace_child_noperm(child, NULL);
> +bdrv_replace_child_noperm(s->child, NULL);
>
> if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
> bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
> @@ -2867,7 +2871,7 @@ static int bdrv_attach_child_common(BlockDriverState
> *child_bs,
> }
>
> bdrv_ref(child_bs);
> -bdrv_replace_child_noperm(new_child, child_bs);
> +bdrv_replace_child_noperm(&new_child, child_bs);
>
> *child = new_child;
>
> @@ -2927,12 +2931,12 @@ static int bdrv_attach_child_noperm(BlockDriverState
> *parent_bs,
> return 0;
> }
>
> -static void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild **childp)
> {
> -BlockDriverState *old_bs = child->bs;
> +BlockDriverState *old_bs = (*childp)->bs;
>
> -bdrv_replace_child_noperm(child, NULL);
> -bdrv_child_free(child);
> +bdrv_replace_child_noperm(childp,
Re: [PATCH 0/7] block: Attempt on fixing 030-reported errors
Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben: > Hanna Reitz (7): > stream: Traverse graph after modification > block: Manipulate children list in .attach/.detach > block: Unite remove_empty_child and child_free > block: Drop detached child from ignore list > block: Pass BdrvChild ** to replace_child_noperm > block: Let replace_child_noperm free children > iotests/030: Unthrottle parallel jobs in reverse Now I know that I don't aspire to a new career as a full time borrow checker. :-) Patches 1-4: Reviewed-by: Kevin Wolf
Re: [PATCH 6/7] block: Let replace_child_noperm free children
Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben:
> In most of the block layer, especially when traversing down from other
> BlockDriverStates, we assume that BdrvChild.bs can never be NULL. When
> it becomes NULL, it is expected that the corresponding BdrvChild pointer
> also becomes NULL and the BdrvChild object is freed.
>
> Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
> pointer to NULL, it should also immediately set the corresponding
> BdrvChild pointer (like bs->file or bs->backing) to NULL.
>
> In that context, it also makes sense for this function to free the
> child. Sometimes we cannot do so, though, because it is called in a
> transactional context where the caller might still want to reinstate the
> child in the abort branch (and free it only on commit), so this behavior
> has to remain optional.
>
> In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
> that the BdrvChild passed to bdrv_replace_child_tran() must have had a
> non-NULL .bs pointer initially. Make a note of that and assert it.
>
> Signed-off-by: Hanna Reitz
> ---
> block.c | 102 +++-
> 1 file changed, 86 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index ff45447686..0a85ede8dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char
> *filename,
> static bool bdrv_recurse_has_child(BlockDriverState *bs,
> BlockDriverState *child);
>
> +static void bdrv_child_free(BdrvChild *child);
> static void bdrv_replace_child_noperm(BdrvChild **child,
> - BlockDriverState *new_bs);
> + BlockDriverState *new_bs,
> + bool free_empty_child);
> static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>BdrvChild *child,
>Transaction *tran);
> @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
> BdrvChild *child;
> BdrvChild **childp;
> BlockDriverState *old_bs;
> +bool free_empty_child;
> } BdrvReplaceChildState;
>
> static void bdrv_replace_child_commit(void *opaque)
> {
> BdrvReplaceChildState *s = opaque;
>
> +if (s->free_empty_child && !s->child->bs) {
> +bdrv_child_free(s->child);
> +}
> bdrv_unref(s->old_bs);
> }
>
> @@ -2270,8 +2276,23 @@ static void bdrv_replace_child_abort(void *opaque)
> BdrvReplaceChildState *s = opaque;
> BlockDriverState *new_bs = s->child->bs;
>
> -/* old_bs reference is transparently moved from @s to *s->childp */
> -bdrv_replace_child_noperm(s->childp, s->old_bs);
> +/*
> + * old_bs reference is transparently moved from @s to s->child;
> + * pass &s->child here instead of s->childp, because *s->childp
> + * will have been cleared by bdrv_replace_child_tran()'s
> + * bdrv_replace_child_noperm() call if new_bs is NULL, and we must
> + * not pass a NULL *s->childp here.
> + */
> +bdrv_replace_child_noperm(&s->child, s->old_bs, true);
Passing free_empty_child=true with a non-NULL new_bs looks a bit
confusing because the child isn't supposed to become empty anyway.
> +/*
> + * The child was pre-existing, so s->old_bs must be non-NULL, and
> + * s->child thus must not have been freed
> + */
> +assert(s->child != NULL);
> +if (!new_bs) {
> +/* As described above, *s->childp was cleared, so restore it */
> +*s->childp = s->child;
> +}
If it wasn't cleared, doesn't it still contain s->child, so this could
just be an unconditional rollback?
> bdrv_unref(new_bs);
> }
Kevin
Re: [PATCH v4 3/3] qapi: deprecate drive-backup
On Thu, Nov 04, 2021 at 09:58:11AM +0100, Markus Armbruster wrote: > From: Vladimir Sementsov-Ogievskiy > > Modern way is using blockdev-add + blockdev-backup, which provides a > lot more control on how target is opened. > > As example of drive-backup problems consider the following: > > User of drive-backup expects that target will be opened in the same > cache and aio mode as source. Corresponding logic is in > drive_backup_prepare(), where we take bs->open_flags of source. > > It works rather bad if source was added by blockdev-add. Assume source > is qcow2 image. On blockdev-add we should specify aio and cache options > for file child of qcow2 node. What happens next: > > drive_backup_prepare() looks at bs->open_flags of qcow2 source node. > But there no BDRV_O_NOCAHE neither BDRV_O_NATIVE_AIO: BDRV_O_NOCAHE is > places in bs->file->bs->open_flags, and BDRV_O_NATIVE_AIO is nowhere, > as file-posix parse options and simply set s->use_linux_aio. > > The documentation is updated in a minimal way, so that drive-backup is > noted only as a deprecated command, and blockdev-backup used in most of > places. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Kashyap Chamarthy > Signed-off-by: Markus Armbruster > --- Reviewed-by: Eric Blake I think this is appropriate for inclusion in 6.2, even if it did not quite make soft freeze, so that we aren't delaying the deprecation for an entire cycle. > docs/about/deprecated.rst | 11 ++ > docs/interop/live-block-operations.rst | 47 +- > qapi/block-core.json | 5 ++- > qapi/transaction.json | 6 +++- > 4 files changed, 51 insertions(+), 18 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v4 5/7] blockdev: Add a new IF type IF_OTHER
Hao Wu writes:
> This type is used to represent block devs that are not suitable to
> be represented by other existing types.
Hinting at intended use wouldn't hurt: ", such as "
>
> Signed-of-by: Hao Wu
> ---
> blockdev.c| 3 ++-
> include/sysemu/blockdev.h | 1 +
> meson | 2 +-
> 3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index b35072644e..c26cbcc422 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -80,6 +80,7 @@ static const char *const if_name[IF_COUNT] = {
> [IF_MTD] = "mtd",
> [IF_SD] = "sd",
> [IF_VIRTIO] = "virtio",
> +[IF_OTHER] = "other",
> [IF_XEN] = "xen",
> };
>
> @@ -739,7 +740,7 @@ QemuOptsList qemu_legacy_drive_opts = {
> },{
> .name = "if",
> .type = QEMU_OPT_STRING,
> -.help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> +.help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,
> other)",
> },{
> .name = "file",
> .type = QEMU_OPT_STRING,
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 32c2d6023c..bce6aab573 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -24,6 +24,7 @@ typedef enum {
> */
> IF_NONE = 0,
> IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +IF_OTHER,
> IF_COUNT
> } BlockInterfaceType;
>
> diff --git a/meson b/meson
> index b25d94e7c7..776acd2a80 16
> --- a/meson
> +++ b/meson
> @@ -1 +1 @@
> -Subproject commit b25d94e7c77fda05a7fdfe8afe562cf9760d69da
> +Subproject commit 776acd2a805c9b42b4f0375150977df42130317f
Oopsie :)
Re: [PATCH v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards
Hao Wu writes:
> We made 3 changes to the at24c_eeprom_init function in
> npcm7xx_boards.c:
>
> 1. We allow the function to take a I2CBus* as parameter. This allows
>us to attach an EEPROM device behind an I2C mux which is not
>possible with the old method.
>
> 2. We make at24c EEPROMs are backed by drives so that we can
>specify the content of the EEPROMs.
>
> 3. Instead of using i2c address as unit number, This patch assigns
>unique unit numbers for each eeproms in each board. This avoids
>conflict in providing multiple eeprom contents with the same address.
>In the old method if we specify two drives with the same unit number,
>the following error will occur: `Device with id 'none85' exists`.
When a commit does three things, chances are splitting it into three
commits would be an improvement. This is *not* a demand.
>
> Signed-off-by: Hao Wu
> ---
> hw/arm/npcm7xx_boards.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index dec7d16ae5..9121e081fa 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc,
> uint32_t num)
> return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus"));
> }
>
> -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr,
> - uint32_t rsize)
> +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr,
> + uint32_t rsize, int unit_number)
I'd keep bus and unit number together.
I'd name the new parameter @unit, to match drive_get().
> {
> -I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus);
> I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
> DeviceState *dev = DEVICE(i2c_dev);
> +DriveInfo *dinfo;
>
> +dinfo = drive_get(IF_OTHER, bus, unit_number);
> +if (dinfo) {
> +qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
> +}
> qdev_prop_set_uint32(dev, "rom-size", rsize);
> i2c_slave_realize_and_unref(i2c_dev, i2c_bus, &error_abort);
> }
> @@ -239,8 +243,8 @@ static void quanta_gsj_i2c_init(NPCM7xxState *soc)
> i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x5c);
> i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x5c);
>
> -at24c_eeprom_init(soc, 9, 0x55, 8192);
> -at24c_eeprom_init(soc, 10, 0x55, 8192);
> +at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192, 0);
> +at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192, 1);
>
> /*
> * i2c-11:
