Re: [PATCH v5 5/6] migration: Add migrate_use_tls() helper
Leonardo Bras wrote: > A lot of places check parameters.tls_creds in order to evaluate if TLS is > in use, and sometimes call migrate_get_current() just for that test. > > Add new helper function migrate_use_tls() in order to simplify testing > for TLS usage. > > Signed-off-by: Leonardo Bras Reviewed-by: Juan Quintela
Re: [PATCH v4 05/25] block/block-backend.c: assertions for block-backend
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
All the global state (GS) API functions will check that
qemu_in_main_thread() returns true. If not, it means
that the safety of BQL cannot be guaranteed, and
they need to be moved to I/O.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block/block-backend.c | 90 +-
softmmu/qdev-monitor.c | 2 +
2 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 0afc03fd66..ed45576007 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
[...]
@@ -1550,6 +1596,7 @@ BlockAIOCB *blk_aio_pwritev(BlockBackend *blk, int64_t
offset,
void blk_aio_cancel(BlockAIOCB *acb)
{
+assert(qemu_in_main_thread());
bdrv_aio_cancel(acb);
}
This function is in block-backend-io.h, though.
[...]
@@ -1879,7 +1936,6 @@ void blk_invalidate_cache(BlockBackend *blk, Error **errp)
bool blk_is_inserted(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
-
return bs && bdrv_is_inserted(bs);
}
Seems like an unrelated hunk.
[...]
@@ -2443,11 +2529,13 @@ int coroutine_fn blk_co_copy_range(BlockBackend
*blk_in, int64_t off_in,
[…]
int blk_make_empty(BlockBackend *blk, Error **errp)
{
+assert(qemu_in_main_thread());
if (!blk_is_available(blk)) {
error_setg(errp, "No medium inserted");
return -ENOMEDIUM;
This function too is in block-backend-io.h.
Hanna
Re: [PATCH v5 1/6] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
On Fri, Nov 12, 2021 at 02:10:36AM -0300, Leonardo Bras wrote: > Adds io_writev_zerocopy and io_flush_zerocopy as optional callback to > QIOChannelClass, > allowing the implementation of zerocopy writes by subclasses. > > How to use them: > - Write data using qio_channel_writev_zerocopy(), > - Wait write completion with qio_channel_flush_zerocopy(). > > Notes: > As some zerocopy implementations work asynchronously, it's > recommended to keep the write buffer untouched until the return of > qio_channel_flush_zerocopy(), to avoid the risk of sending an updated > buffer instead of the one at the write. > > As the new callbacks are optional, if a subclass does not implement them, > then: > - io_writev_zerocopy will return -1, > - io_flush_zerocopy will return 0 without changing anything. > > Also, some functions like qio_channel_writev_full_all() were adapted to > receive a flag parameter. That allows shared code between zerocopy and > non-zerocopy writev. > > Signed-off-by: Leonardo Bras > --- > include/io/channel.h | 93 ++-- > io/channel.c | 65 +-- > 2 files changed, 135 insertions(+), 23 deletions(-) > > +/** > + * qio_channel_flush_zerocopy: > + * @ioc: the channel object > + * @errp: pointer to a NULL-initialized error object > + * > + * Will block until every packet queued with > + * qio_channel_writev_zerocopy() is sent, or return > + * in case of any error. > + * > + * Returns -1 if any error is found, 0 otherwise. Returns -1 if any error is found, 0 if all data was sent, or 1 if all data was sent but at least some was copied. > + * If not implemented, acts as a no-op, and returns 0. > + */ > + > +int qio_channel_flush_zerocopy(QIOChannel *ioc, > + Error **errp); Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v5 3/6] QIOChannelSocket: Implement io_writev_zerocopy & io_flush_zerocopy for CONFIG_LINUX
On Fri, Nov 12, 2021 at 02:10:38AM -0300, Leonardo Bras wrote:
> For CONFIG_LINUX, implement the new optional callbacks io_write_zerocopy and
> io_flush_zerocopy on QIOChannelSocket, but enables it only when MSG_ZEROCOPY
> feature is available in the host kernel, which is checked on
> qio_channel_socket_connect_sync()
>
> qio_channel_socket_flush_zerocopy() was implemented by counting how many times
> sendmsg(...,MSG_ZEROCOPY) was successfully called, and then reading the
> socket's error queue, in order to find how many of them finished sending.
> Flush will loop until those counters are the same, or until some error occurs.
>
> A new function qio_channel_socket_poll() was also created in order to avoid
> busy-looping recvmsg() in qio_channel_socket_flush_zerocopy() while waiting
> for
> updates in socket's error queue.
>
> Notes on using writev_zerocopy():
> 1: Buffer
> - As MSG_ZEROCOPY tells the kernel to use the same user buffer to avoid
> copying,
> some caution is necessary to avoid overwriting any buffer before it's sent.
> If something like this happen, a newer version of the buffer may be sent
> instead.
> - If this is a problem, it's recommended to call flush_zerocopy() before
> freeing
> or re-using the buffer.
>
> 2: Locked memory
> - When using MSG_ZERCOCOPY, the buffer memory will be locked after queued, and
> unlocked after it's sent.
> - Depending on the size of each buffer, and how often it's sent, it may
> require
> a larger amount of locked memory than usually available to non-root user.
> - If the required amount of locked memory is not available, writev_zerocopy
> will return an error, which can abort an operation like migration,
> - Because of this, when an user code wants to add zerocopy as a feature, it
> requires a mechanism to disable it, so it can still be accessible to less
> privileged users.
>
> Signed-off-by: Leonardo Bras
> ---
> include/io/channel-socket.h | 2 +
> include/io/channel.h| 1 +
> io/channel-socket.c | 150 +++-
> 3 files changed, 150 insertions(+), 3 deletions(-)
>
> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index e747e63514..81d04baa4c 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -47,6 +47,8 @@ struct QIOChannelSocket {
> socklen_t localAddrLen;
> struct sockaddr_storage remoteAddr;
> socklen_t remoteAddrLen;
> +ssize_t zerocopy_queued;
> +ssize_t zerocopy_sent;
> };
>
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index a19c09bb84..051fff4197 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -31,6 +31,7 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
>
>
> #define QIO_CHANNEL_ERR_BLOCK -2
> +#define QIO_CHANNEL_ERR_NOBUFS -3
>
> #define QIO_CHANNEL_WRITE_FLAG_ZEROCOPY 0x1
>
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index b57a27bf91..c724b849ad 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -26,6 +26,10 @@
> #include "io/channel-watch.h"
> #include "trace.h"
> #include "qapi/clone-visitor.h"
> +#ifdef CONFIG_LINUX
> +#include
> +#include
> +#endif
>
> #define SOCKET_MAX_FDS 16
>
> @@ -55,6 +59,8 @@ qio_channel_socket_new(void)
>
> sioc = QIO_CHANNEL_SOCKET(object_new(TYPE_QIO_CHANNEL_SOCKET));
> sioc->fd = -1;
> +sioc->zerocopy_queued = 0;
> +sioc->zerocopy_sent = 0;
>
> ioc = QIO_CHANNEL(sioc);
> qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
> @@ -140,6 +146,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
> Error **errp)
> {
> int fd;
> +int ret, v = 1;
>
> trace_qio_channel_socket_connect_sync(ioc, addr);
> fd = socket_connect(addr, errp);
> @@ -154,6 +161,15 @@ int qio_channel_socket_connect_sync(QIOChannelSocket
> *ioc,
> return -1;
> }
>
> +#ifdef CONFIG_LINUX
> +ret = qemu_setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &v, sizeof(v));
> +if (ret == 0) {
> +/* Zerocopy available on host */
> +qio_channel_set_feature(QIO_CHANNEL(ioc),
> +QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY);
> +}
> +#endif
> +
> return 0;
> }
>
> @@ -561,12 +577,15 @@ static ssize_t
> qio_channel_socket_writev_flags(QIOChannel *ioc,
> retry:
> ret = sendmsg(sioc->fd, &msg, flags);
> if (ret <= 0) {
> -if (errno == EAGAIN) {
> +switch (errno) {
> +case EAGAIN:
> return QIO_CHANNEL_ERR_BLOCK;
> -}
> -if (errno == EINTR) {
> +case EINTR:
> goto retry;
> +case ENOBUFS:
> +return QIO_CHANNEL_ERR_NOBUFS;
Why does ENOBUFS need handling separately instead of letting
the error_setg_errno below handle it ?
The caller immediately invokes error_setg_errno() again,
just with different error message.
No code in this series ever looks at QIO_CH
Re: [PATCH for 6.2 v2 4/5] hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC
On Thu, 11 Nov 2021 11:19:30 +0530 (IST)
Ani Sinha wrote:
> On Wed, 10 Nov 2021, Igor Mammedov wrote:
>
> > From: Julia Suvorova
> >
> > There are two ways to enable ACPI PCI Hot-plug:
> >
> > * Disable the Hot-plug Capable bit on PCIe slots.
> >
> > This was the first approach which led to regression [1-2], as
> > I/O space for a port is allocated only when it is hot-pluggable,
> > which is determined by HPC bit.
> >
> > * Leave the HPC bit on and disable PCIe Native Hot-plug in _OSC
> > method.
> >
> > This removes the (future) ability of hot-plugging switches with PCIe
> > Native hotplug since ACPI PCI Hot-plug only works with cold-plugged
> > bridges. If the user wants to explicitely use this feature, they can
> > disable ACPI PCI Hot-plug with:
> > --global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> >
> > Change the bit in _OSC method so that the OS selects ACPI PCI Hot-plug
> > instead of PCIe Native.
> >
> > [1] https://gitlab.com/qemu-project/qemu/-/issues/641
> > [2] https://bugzilla.redhat.com/show_bug.cgi?id=2006409
> >
> > Signed-off-by: Julia Suvorova
> > Signed-off-by: Igor Mammedov
> > ---
> > v2:
> > - (mst)
> > * drop local hotplug var and opencode it
> > * rename acpi_pcihp parameter to enable_native_pcie_hotplug
> > to reflect what it actually does
> >
> > tested:
> > with hotplugging nic into 1 root port with seabios/ovmf/Fedora34
> > Windows tested only with seabios (using exiting images)
> > (installer fails to install regardless on bios)
> > ---
> > hw/i386/acpi-build.c | 12
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index a3ad6abd33..a99c6e4fe3 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1337,7 +1337,7 @@ static void build_x86_acpi_pci_hotplug(Aml *table,
> > uint64_t pcihp_addr)
> > aml_append(table, scope);
> > }
> >
> > -static Aml *build_q35_osc_method(void)
> > +static Aml *build_q35_osc_method(bool enable_native_pcie_hotplug)
> > {
> > Aml *if_ctx;
> > Aml *if_ctx2;
> > @@ -1359,8 +1359,10 @@ static Aml *build_q35_osc_method(void)
> > /*
> > * Always allow native PME, AER (no dependencies)
> > * Allow SHPC (PCI bridges can have SHPC controller)
> > + * Disable PCIe Native Hot-plug if ACPI PCI Hot-plug is enabled.
> > */
>
> Based on v2, I think its more useful to have this comment where the
> function is called.
I'd leave it as is, which is consistent with other bits described here
>
> > -aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > +aml_append(if_ctx, aml_and(a_ctrl,
> > +aml_int(0x1E | (enable_native_pcie_hotplug ? 0x1 : 0x0)), a_ctrl));
> >
> > if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1;
> > /* Unknown revision */
> > @@ -1449,7 +1451,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > aml_append(dev, aml_name_decl("_UID",
> > aml_int(pcmc->pci_root_uid)));
> > -aml_append(dev, build_q35_osc_method());
> > +aml_append(dev, build_q35_osc_method(!pm->pcihp_bridge_en));
>
> See above. I think it helps to add a comment here saying native hotplug is
> enabled when acpi hotplug is disabled for cold plugged bridges.
>
>
> > aml_append(sb_scope, dev);
> > if (mcfg_valid) {
> > aml_append(sb_scope, build_q35_dram_controller(&mcfg));
> > @@ -1565,7 +1567,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > if (pci_bus_is_express(bus)) {
> > aml_append(dev, aml_name_decl("_HID",
> > aml_eisaid("PNP0A08")));
> > aml_append(dev, aml_name_decl("_CID",
> > aml_eisaid("PNP0A03")));
> > -aml_append(dev, build_q35_osc_method());
> > +
> > +/* Expander bridges do not have ACPI PCI Hot-plug enabled
> > */
> > +aml_append(dev, build_q35_osc_method(true));
> > } else {
> > aml_append(dev, aml_name_decl("_HID",
> > aml_eisaid("PNP0A03")));
> > }
> > --
> > 2.27.0
> >
> >
>
Re: [PATCH for 6.2 v2 1/5] pcie: rename 'native-hotplug' to 'x-native-hotplug'
On Thu, 11 Nov 2021 08:55:24 +0530 (IST)
Ani Sinha wrote:
> On Wed, 10 Nov 2021, Igor Mammedov wrote:
>
> > Mark property as experimental/internal adding 'x-' prefix.
> >
> > Property was introduced in 6.1 and it should have provided
> > ability to turn on native PCIE hotplug on port even when
> > ACPI PCI hotplug is in use is user explicitly sets property
> > on CLI. However that never worked since slot is wired to
> > ACPI hotplug controller.
> > Another non-intended usecase: disable native hotplug on slot
> > when APCI based hotplug is disabled, which works but slot has
> > 'hotplug' property for this taks.
> >
> > It should be relatively safe to rename it to experimental
> > as no users should exist for it and given that the property
> > is broken we don't really want to leave it around for much
> > longer lest users start using it.
> >
> > Signed-off-by: Igor Mammedov
>
> Barring the comment below,
>
> Reviewed-by: Ani Sinha
Thanks!
>
> > ---
> > CC: qemu-sta...@nongnu.org
> > ---
> > hw/i386/pc_q35.c | 2 +-
> > hw/pci/pcie_port.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 797e09500b..fc34b905ee 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -243,7 +243,7 @@ static void pc_q35_init(MachineState *machine)
> >NULL);
> >
> > if (acpi_pcihp) {
> > -object_register_sugar_prop(TYPE_PCIE_SLOT, "native-hotplug",
> > +object_register_sugar_prop(TYPE_PCIE_SLOT, "x-native-hotplug",
> > "false", true);
>
> Let us document the fact that this property is experimental. It was not at
> once obvious to me that an "x-" prefix meant to indicate experimental
> status.
it's common knowledge, but quick grep shows we only documented
x- prefix for qmp commands but not for properties even though
properties were the first to use it. So we probably should
document it somewhere.
I thought we have acceptable property name format documented
but I couldn't find it quickly (that would be a good place
to document it).
Care to post a patch?
>
>
> > }
> >
> > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> > index da850e8dde..e95c1e5519 100644
> > --- a/hw/pci/pcie_port.c
> > +++ b/hw/pci/pcie_port.c
> > @@ -148,7 +148,7 @@ static Property pcie_slot_props[] = {
> > DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0),
> > DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0),
> > DEFINE_PROP_BOOL("hotplug", PCIESlot, hotplug, true),
> > -DEFINE_PROP_BOOL("native-hotplug", PCIESlot, native_hotplug, true),
> > +DEFINE_PROP_BOOL("x-native-hotplug", PCIESlot, native_hotplug, true),
> > DEFINE_PROP_END_OF_LIST()
> > };
> >
> > --
> > 2.27.0
> >
> >
>
Re: [PATCH v2 0/1] Jobs based on custom runners: add CentOS Stream 8
Cleber Rosa writes: > This adds a new custom runner, showing an example of how other > entities can add their own custom jobs to the GitLab CI pipeline. > > > One example of a job introduced here, running on the host reserved for > this purpose can be seen at: > > - https://gitlab.com/cleber.gnu/qemu/-/jobs/1773761640 As I'm not going to be able to test myself I'll just queue this and we can see it spring into live once merged ;-) Queued to for-6.2/misc-fixes, thanks. -- Alex Bennée
Re: [PATCH v5 1/6] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
On Fri, Nov 12, 2021 at 10:13:01AM +, Daniel P. Berrangé wrote: > On Fri, Nov 12, 2021 at 02:10:36AM -0300, Leonardo Bras wrote: > > Adds io_writev_zerocopy and io_flush_zerocopy as optional callback to > > QIOChannelClass, > > allowing the implementation of zerocopy writes by subclasses. > > > > How to use them: > > - Write data using qio_channel_writev_zerocopy(), > > - Wait write completion with qio_channel_flush_zerocopy(). > > > > Notes: > > As some zerocopy implementations work asynchronously, it's > > recommended to keep the write buffer untouched until the return of > > qio_channel_flush_zerocopy(), to avoid the risk of sending an updated > > buffer instead of the one at the write. > > > > As the new callbacks are optional, if a subclass does not implement them, > > then: > > - io_writev_zerocopy will return -1, > > - io_flush_zerocopy will return 0 without changing anything. > > > > Also, some functions like qio_channel_writev_full_all() were adapted to > > receive a flag parameter. That allows shared code between zerocopy and > > non-zerocopy writev. > > > > Signed-off-by: Leonardo Bras > > --- > > include/io/channel.h | 93 ++-- > > io/channel.c | 65 +-- > > 2 files changed, 135 insertions(+), 23 deletions(-) > > > > diff --git a/include/io/channel.h b/include/io/channel.h > > index 88988979f8..a19c09bb84 100644 > > --- a/include/io/channel.h > > +++ b/include/io/channel.h > > +/** > > + * qio_channel_writev_zerocopy: > > + * @ioc: the channel object > > + * @iov: the array of memory regions to write data from > > + * @niov: the length of the @iov array > > + * @errp: pointer to a NULL-initialized error object > > + * > > + * Behaves like qio_channel_writev_full_all_flags, but may write > > qio_channel_writev > > > + * data asynchronously while avoiding unnecessary data copy. > > + * This function may return before any data is actually written, > > + * but should queue every buffer for writing. > > Callers mustn't rely on "should" docs - they must rely on the > return value indicating how many bytes were accepted. Also mention that this requires locked memory and can/will fail if insufficient locked memory is available. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v4 04/25] include/sysemu/block-backend: split header into I/O and global state (GS) API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
Similarly to the previous patches, split block-backend.h
in block-backend-io.h and block-backend-global-state.h
In addition, remove "block/block.h" include as it seems
it is not necessary anymore, together with "qemu/iov.h"
block-backend-common.h contains the structures shared between
the two headers, and the functions that can't be categorized as
I/O or global state.
Assertions are added in the next patch.
Signed-off-by: Emanuele Giuseppe Esposito
Reviewed-by: Stefan Hajnoczi
---
block/block-backend.c | 9 +-
include/sysemu/block-backend-common.h | 74 ++
include/sysemu/block-backend-global-state.h | 122 +
include/sysemu/block-backend-io.h | 139 ++
include/sysemu/block-backend.h | 269 +---
5 files changed, 344 insertions(+), 269 deletions(-)
create mode 100644 include/sysemu/block-backend-common.h
create mode 100644 include/sysemu/block-backend-global-state.h
create mode 100644 include/sysemu/block-backend-io.h
diff --git a/block/block-backend.c b/block/block-backend.c
index 39cd99df2b..0afc03fd66 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -79,6 +79,7 @@ struct BlockBackend {
bool allow_aio_context_change;
bool allow_write_beyond_eof;
+/* Protected by BQL lock */
NotifierList remove_bs_notifiers, insert_bs_notifiers;
QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
@@ -111,12 +112,14 @@ static const AIOCBInfo block_backend_aiocb_info = {
static void drive_info_del(DriveInfo *dinfo);
static BlockBackend *bdrv_first_blk(BlockDriverState *bs);
-/* All BlockBackends */
+/* All BlockBackends. Protected by BQL lock. */
static QTAILQ_HEAD(, BlockBackend) block_backends =
QTAILQ_HEAD_INITIALIZER(block_backends);
-/* All BlockBackends referenced by the monitor and which are iterated through by
- * blk_next() */
+/*
+ * All BlockBackends referenced by the monitor and which are iterated through
by
+ * blk_next(). Protected by BQL lock.
+ */
static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
diff --git a/include/sysemu/block-backend-common.h b/include/sysemu/block-backend-common.h
new file mode 100644
index 00..52ff6a4d26
--- /dev/null
+++ b/include/sysemu/block-backend-common.h
@@ -0,0 +1,74 @@
+/*
+ * QEMU Block backends
+ *
+ * Copyright (C) 2014-2016 Red Hat, Inc.
+ *
+ * Authors:
+ * Markus Armbruster ,
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1
+ * or later. See the COPYING.LIB file in the top-level directory.
+ */
+
+#ifndef BLOCK_BACKEND_COMMON_H
+#define BLOCK_BACKEND_COMMON_H
+
+#include "block/throttle-groups.h"
+
+/* Callbacks for block device models */
+typedef struct BlockDevOps {
+/*
+ * Runs when virtual media changed (monitor commands eject, change)
+ * Argument load is true on load and false on eject.
+ * Beware: doesn't run when a host device's physical media
+ * changes. Sure would be useful if it did.
+ * Device models with removable media must implement this callback.
+ */
+void (*change_media_cb)(void *opaque, bool load, Error **errp);
+/*
+ * Runs when an eject request is issued from the monitor, the tray
+ * is closed, and the medium is locked.
+ * Device models that do not implement is_medium_locked will not need
+ * this callback. Device models that can lock the medium or tray might
+ * want to implement the callback and unlock the tray when "force" is
+ * true, even if they do not support eject requests.
+ */
+void (*eject_request_cb)(void *opaque, bool force);
+/*
+ * Is the virtual tray open?
+ * Device models implement this only when the device has a tray.
+ */
+bool (*is_tray_open)(void *opaque);
+/*
+ * Is the virtual medium locked into the device?
+ * Device models implement this only when device has such a lock.
+ */
+bool (*is_medium_locked)(void *opaque);
+/*
+ * Runs when the size changed (e.g. monitor command block_resize)
+ */
+void (*resize_cb)(void *opaque);
+/*
+ * Runs when the backend receives a drain request.
+ */
+void (*drained_begin)(void *opaque);
+/*
+ * Runs when the backend's last drain request ends.
+ */
+void (*drained_end)(void *opaque);
+/*
+ * Is the device still busy?
+ */
+bool (*drained_poll)(void *opaque);
+} BlockDevOps;
+
+/*
+ * This struct is embedded in (the private) BlockBackend struct and contains
+ * fields that must be public. This is in particular for QLIST_ENTRY() and
+ * friends so that BlockBackends can be kept in lists outside block-backend.c
+ */
+typedef struct BlockBackendPublic {
+ThrottleGroupMember throttle_group_member;
+} BlockBackendPublic;
+
+#endif /* BLOCK_BACKEND_COMMON_H */
diff --git a/include/sy
Re: [PATCH for 6.2 v2 0/5] Fix Q35 ACPI PCI Hot-plug I/O issues
On Fri, 12 Nov 2021 04:47:05 -0500 "Michael S. Tsirkin" wrote: > On Wed, Nov 10, 2021 at 04:11:35PM -0500, Igor Mammedov wrote: > > Changelog: > > v2: > > * simplify [1/5] and rename property to x-native-hotplug (CC stable) > > * [4/5] > >- rename function parameter to reflect actual action > >- drop local 'hotplug' variable and opencode statement > > * test with SeaBIOS/OVMF and Linux guest, > > Windows also works with SeaBIOS, can't install it in EFI > > mode on current master (it's stuck when formatting disk/or > > copying files to hdd). > > > > Attempt [1] to fix I/O allocation with the 'reserve-io' hint on each > > pcie-root-port resulted in regression [2-3]. This patchset aims to fix > > it by addressing the root cause of the problem - the disabled PCIe > > Slot HPC bit. > > > > [1] 'hw/pcie-root-port: Fix hotplug for PCI devices requiring IO' > > [2] https://gitlab.com/qemu-project/qemu/-/issues/641 > > [3] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 > > > > CC: kra...@redhat.com > > Igor are you going to post v3 with a fixup of the expected tables? > Thanks! I'll post in a short while, it seems going ahead with this is less risky as it resolves remaining issues (hotplug with edk2 and seabios boot loop) than just reverting to native and bringing back old issues. > > Igor Mammedov (1): > > pcie: rename 'native-hotplug' to 'x-native-hotplug' > > > > Julia Suvorova (4): > > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type > > bios-tables-test: Allow changes in DSDT ACPI tables > > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC > > bios-tables-test: Update golden binaries > > > > include/hw/acpi/ich9.h| 1 + > > hw/acpi/ich9.c| 18 ++ > > hw/i386/acpi-build.c | 12 > > hw/i386/pc.c | 2 ++ > > hw/i386/pc_q35.c | 9 +++-- > > hw/pci/pcie_port.c| 2 +- > > tests/data/acpi/q35/DSDT | Bin 8289 -> 8289 bytes > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes > > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes > > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes > > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9943 -> 9943 bytes > > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes > > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes > > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes > > tests/data/acpi/q35/DSDT.memhp| Bin 9648 -> 9648 bytes > > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes > > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes > > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes > > tests/data/acpi/q35/DSDT.nosmm| Bin 0 -> 8289 bytes > > tests/data/acpi/q35/DSDT.numamem | Bin 8295 -> 8295 bytes > > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes > > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes > > tests/data/acpi/q35/DSDT.tis.tpm12| Bin 8894 -> 8894 bytes > > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes > > tests/data/acpi/q35/DSDT.xapic| Bin 35652 -> 35652 bytes > > 25 files changed, 37 insertions(+), 7 deletions(-) > > create mode 100644 tests/data/acpi/q35/DSDT.dmar > > create mode 100644 tests/data/acpi/q35/DSDT.nosmm > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm > > > > -- > > 2.27.0 >
Re: [PATCH 0/6] RfC: try improve native hotplug for pcie root ports
Hi, > involved, but I think it's more a question of luck. This until these > specific patches, but they are only in v2 out of rfc status just today. Never posted a formal non-rfc version because I had no pending changes. Maybe I should have done that ... v2 has no functional changes, it only resolves a conflict, so effectively the same thing as the rfc series. take care, Gerd
Re: [PATCH v5 2/6] QIOChannelSocket: Add flags parameter for writing
On Fri, Nov 12, 2021 at 02:10:37AM -0300, Leonardo Bras wrote: > Change qio_channel_socket_writev() in order to accept flags, so its possible > to selectively make use of sendmsg() flags. > > qio_channel_socket_writev() contents were moved to a helper function > qio_channel_socket_writev_flags() which accepts an extra argument for flags. > (This argument is passed directly to sendmsg(). > > Signed-off-by: Leonardo Bras > --- > io/channel-socket.c | 26 +++--- > 1 file changed, 19 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v5 1/6] QIOChannel: Add io_writev_zerocopy & io_flush_zerocopy callbacks
On Fri, Nov 12, 2021 at 02:10:36AM -0300, Leonardo Bras wrote:
> Adds io_writev_zerocopy and io_flush_zerocopy as optional callback to
> QIOChannelClass,
> allowing the implementation of zerocopy writes by subclasses.
>
> How to use them:
> - Write data using qio_channel_writev_zerocopy(),
> - Wait write completion with qio_channel_flush_zerocopy().
>
> Notes:
> As some zerocopy implementations work asynchronously, it's
> recommended to keep the write buffer untouched until the return of
> qio_channel_flush_zerocopy(), to avoid the risk of sending an updated
> buffer instead of the one at the write.
>
> As the new callbacks are optional, if a subclass does not implement them,
> then:
> - io_writev_zerocopy will return -1,
> - io_flush_zerocopy will return 0 without changing anything.
>
> Also, some functions like qio_channel_writev_full_all() were adapted to
> receive a flag parameter. That allows shared code between zerocopy and
> non-zerocopy writev.
>
> Signed-off-by: Leonardo Bras
> ---
> include/io/channel.h | 93 ++--
> io/channel.c | 65 +--
> 2 files changed, 135 insertions(+), 23 deletions(-)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 88988979f8..a19c09bb84 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -32,12 +32,15 @@ OBJECT_DECLARE_TYPE(QIOChannel, QIOChannelClass,
>
> #define QIO_CHANNEL_ERR_BLOCK -2
>
> +#define QIO_CHANNEL_WRITE_FLAG_ZEROCOPY 0x1
> +
> typedef enum QIOChannelFeature QIOChannelFeature;
>
> enum QIOChannelFeature {
> QIO_CHANNEL_FEATURE_FD_PASS,
> QIO_CHANNEL_FEATURE_SHUTDOWN,
> QIO_CHANNEL_FEATURE_LISTEN,
> +QIO_CHANNEL_FEATURE_WRITE_ZEROCOPY,
> };
>
>
> @@ -136,6 +139,12 @@ struct QIOChannelClass {
>IOHandler *io_read,
>IOHandler *io_write,
>void *opaque);
> +ssize_t (*io_writev_zerocopy)(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + Error **errp);
> +int (*io_flush_zerocopy)(QIOChannel *ioc,
> + Error **errp);
> };
>
> /* General I/O handling functions */
> @@ -321,10 +330,11 @@ int qio_channel_readv_all(QIOChannel *ioc,
>
>
> /**
> - * qio_channel_writev_all:
> + * qio_channel_writev_all_flags:
> * @ioc: the channel object
> * @iov: the array of memory regions to write data from
> * @niov: the length of the @iov array
> + * @flags: write flags (QIO_CHANNEL_WRITE_FLAG_*)
> * @errp: pointer to a NULL-initialized error object
> *
> * Write data to the IO channel, reading it from the
> @@ -337,12 +347,23 @@ int qio_channel_readv_all(QIOChannel *ioc,
> * to be written, yielding from the current coroutine
> * if required.
> *
> + * If QIO_CHANNEL_WRITE_FLAG_ZEROCOPY is passed in flags,
> + * instead of waiting for all requested data to be written,
> + * this function will wait until it's all queued for writing.
> + * In this case, if the buffer gets changed between queueing and
> + * sending, the updated buffer will be sent. If this is not a
> + * desired behavior, it's suggested to call qio_channel_flush_zerocopy()
> + * before reusing the buffer.
> + *
> * Returns: 0 if all bytes were written, or -1 on error
> */
> -int qio_channel_writev_all(QIOChannel *ioc,
> - const struct iovec *iov,
> - size_t niov,
> - Error **erp);
> +int qio_channel_writev_all_flags(QIOChannel *ioc,
> + const struct iovec *iov,
> + size_t niov,
> + int flags,
> + Error **errp);
> +#define qio_channel_writev_all(ioc, iov, niov, errp) \
> +qio_channel_writev_all_flags(ioc, iov, niov, 0, errp)
We already have separate methods for zerocopy, instead of adding
flags, so we shouldn't add flags to this either.
Add a qio_channel_writev_zerocopy_all method instead.
Internally, we can still make both qio_channel_writev_zerocopy_all
and qio_channel_writev_all use the same helper method, just don't
expose flags in the public API. Even internally we don't really
need flags, just a bool
>
> /**
> * qio_channel_readv:
> @@ -831,12 +852,13 @@ int qio_channel_readv_full_all(QIOChannel *ioc,
> Error **errp);
>
> /**
> - * qio_channel_writev_full_all:
> + * qio_channel_writev_full_all_flags:
> * @ioc: the channel object
> * @iov: the array of memory regions to write data from
> * @niov: the length of the @iov array
> * @fds: an array of file handles to send
> * @nfds: number of file handles in @fds
> + * @flags: write flags (QIO_CHANNEL_WRITE_FLAG_*)
> * @errp: pointer to a NULL-initialized error object
>
Re: [PATCH v2 2/3] softmmu: fix watchpoint-interrupt races
On 11.11.21 10:55, Pavel Dovgalyuk wrote:
> Watchpoint may be processed in two phases. First one is detecting
> the instruction with target memory access. And the second one is
> executing only one instruction and setting the debug interrupt flag.
> Hardware interrupts can break this sequence when they happen after
> the first watchpoint phase.
> This patch postpones the interrupt request until watchpoint is
> processed.
>
> Signed-off-by: Pavel Dovgalyuk
> ---
> accel/tcg/cpu-exec.c |5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index df12452b8f..e4526c2f5e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -742,6 +742,11 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
> qemu_mutex_unlock_iothread();
> return true;
> }
> +/* Process watchpoints first, or interrupts will ruin everything */
> +if (cpu->watchpoint_hit) {
> +qemu_mutex_unlock_iothread();
> +return false;
> +}
> #if !defined(CONFIG_USER_ONLY)
> if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
> /* Do nothing */
>
Reviewed-by: David Hildenbrand
--
Thanks,
David / dhildenb
Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags
On Fri, Nov 12, 2021 at 10:23:12AM +0800, Jason Wang wrote:
> On Thu, Nov 11, 2021 at 4:27 PM Michael S. Tsirkin wrote:
> >
> > On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> > > We used to access packed descriptor flags via
> > > address_space_{write|read}_cached(). When we hit the cache, memcpy()
> > > is used which is not an atomic operation which may lead a wrong value
> > > is read or wrote.
> >
> > Could you clarify where's the memcpy that you see?
> > Thanks!
>
> In the address_space_{write|read}_cached it self:
>
> static inline MemTxResult
> =>dress_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>const void *buf, hwaddr len)
> {
> assert(addr < cache->len && len <= cache->len - addr);
> if (likely(cache->ptr)) {
> memcpy(cache->ptr + addr, buf, len);
> return MEMTX_OK;
> } else {
> return address_space_write_cached_slow(cache, addr, buf, len);
> }
> }
>
> Thanks
But that's a copy from the cache, not from guest memory.
I don't see how it can change so I don't see why it needs
to be atomic. Was there an actual issue you observed or
is this theoretical?
> >
> > > So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> > > sure the aceess is atomic.
> > >
> > > Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> > > Cc: qemu-sta...@nongnu.org
> > > Signed-off-by: Jason Wang
> > > ---
> > > hw/virtio/virtio.c | 11 ---
> > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index cc69a9b881..939bcbfeb9 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -507,11 +507,9 @@ static void
> > > vring_packed_desc_read_flags(VirtIODevice *vdev,
> > > MemoryRegionCache *cache,
> > > int i)
> > > {
> > > -address_space_read_cached(cache,
> > > - i * sizeof(VRingPackedDesc) +
> > > - offsetof(VRingPackedDesc, flags),
> > > - flags, sizeof(*flags));
> > > -virtio_tswap16s(vdev, flags);
> > > +hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc,
> > > flags);
> > > +
> > > +*flags = virtio_lduw_phys_cached(vdev, cache, off);
> > > }
> > >
> > > static void vring_packed_desc_read(VirtIODevice *vdev,
> > > @@ -564,8 +562,7 @@ static void
> > > vring_packed_desc_write_flags(VirtIODevice *vdev,
> > > {
> > > hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc,
> > > flags);
> > >
> > > -virtio_tswap16s(vdev, &desc->flags);
> > > -address_space_write_cached(cache, off, &desc->flags,
> > > sizeof(desc->flags));
> > > +virtio_stw_phys_cached(vdev, cache, off, desc->flags);
> > > address_space_cache_invalidate(cache, off, sizeof(desc->flags));
> > > }
> > >
> > > --
> > > 2.25.1
> >
Re: [PATCH v10 16/26] target/loongarch: Add disassembler
Hi Richard,
On 2021/11/12 下午3:39, Richard Henderson wrote:
On 11/12/21 7:53 AM, Song Gao wrote:
+const char * const fccregnames[8] = {
+ "$fcc0", "$fcc1", "$fcc2", "$fcc3", "$fcc4", "$fcc5", "$fcc6",
"$fcc7",
+};
static.
OK.
+static void output_fcsrdrj(DisasContext *ctx, arg_fmt_fcsrdrj *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s", regnames[a->fcsrd],
regnames[a->rj]);
+}
+
+static void output_rdfcsrs(DisasContext *ctx, arg_fmt_rdfcsrs *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s", regnames[a->rd],
regnames[a->fcsrs]);
+}
Wrong names for fcsr[ds]. Probably easiest to just use "fcsr%d"
rather than having an array of strings.
OK.
+static void output_rdrjsi12(DisasContext *ctx, arg_fmt_rdrjsi12 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s, 0x%x",
+ regnames[a->rd], regnames[a->rj], (a->si12) & 0xfff);
+}
Surely printing the signed value is more useful.
+static void output_rdrjsi16(DisasContext *ctx, arg_fmt_rdrjsi16 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s, 0x%x",
+ regnames[a->rd], regnames[a->rj], (a->si16) & 0x);
+}
+
+static void output_rdsi20(DisasContext *ctx, arg_fmt_rdsi20 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, 0x%x", regnames[a->rd], (a->si20) &
0xf);
+}
+
+static void output_rdrjsi14(DisasContext *ctx, arg_fmt_rdrjsi14 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s, 0x%x",
+ regnames[a->rd], regnames[a->rj], (a->si14) & 0x3fff);
+}
+
+static void output_hintrjsi12(DisasContext *ctx, arg_fmt_hintrjsi12 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "0x%x, %s, 0x%x",
+ a->hint, regnames[a->rj], (a->si12) & 0xfff);
+}
+
+static void output_fdrjsi12(DisasContext *ctx, arg_fmt_fdrjsi12 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s, 0x%x",
+ fregnames[a->fd], regnames[a->rj], (a->si12) & 0xfff);
+}
Likewise.
+static void output_rjoffs21(DisasContext *ctx, arg_fmt_rjoffs21 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, 0x%x", regnames[a->rj], (a->offs21) &
0x1f);
+}
+
+static void output_cjoffs21(DisasContext *ctx, arg_fmt_cjoffs21 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, 0x%x",
+ fccregnames[a->cj], (a->offs21) & 0x1f);
+}
+
+static void output_rdrjoffs16(DisasContext *ctx, arg_fmt_rdrjoffs16 *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s, 0x%x",
+ regnames[a->rd], regnames[a->rj], (a->offs16) & 0x);
+}
+
+static void output_offs(DisasContext *ctx, arg_fmt_offs *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "0x%x", (a->offs) & 0x3ff);
+}
These are signed, but they're also pc-relative. It's probably most
helpful to have stored the address into ctx and compute the final
address.
This's a good idea.
+static void output_rdfj(DisasContext *ctx, arg_fmt_rdfj *a,
+ const char *mnemonic)
+{
+ output(ctx, mnemonic, "%s, %s", regnames[a->rd], regnames[a->fj]);
+}
Wrong name for fj.
+#define output_fcmp(C, PREFIX,
SUBFFIX) \
SUFFIX
+ output_fcmp(ctx, "fcmp_slt_", suffix);
+ break;
+ case 0x4:
+ output_fcmp(ctx, "fcmp_ceq_", suffix);
+ break;
+ case 0x5:
+ output_fcmp(ctx, "fcmp_seq_", suffix);
+ break;
+ case 0x6:
+ break;
Here you're going to print nothing at all, which is wrong.
OK.
+ }
+}
+
+#define FCMP_INSN(insn, suffix, type) \
+static bool trans_##insn(DisasContext *ctx, arg_fmt_##type * a) \
+{ \
+ output_##type(ctx, a, #suffix); \
+ return true; \
+}
I think you should drop "insn" and "type" from this define and use
output_cdfjfkfcond directly.
I think that FCMP_INSN should return output_cdfjfkfcond, which should
return false for the default case, so that decodetree returns false,
so that we print "invalid".
Got it, I'll correct it on V11.
r~
Re: [PATCH 05/10] vhost-backend: avoid overflow on memslots_limit
On Fri, Nov 12, 2021 at 10:46:46AM +0300, Roman Kagan wrote:
> On Thu, Nov 11, 2021 at 06:59:43PM +0100, Philippe Mathieu-Daudé wrote:
> > On 11/11/21 16:33, Roman Kagan wrote:
> > > Fix the (hypothetical) potential problem when the value parsed out of
> > > the vhost module parameter in sysfs overflows the return value from
> > > vhost_kernel_memslots_limit.
> > >
> > > Signed-off-by: Roman Kagan
> > > ---
> > > hw/virtio/vhost-backend.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > > index b65f8f7e97..44f7dbb243 100644
> > > --- a/hw/virtio/vhost-backend.c
> > > +++ b/hw/virtio/vhost-backend.c
> > > @@ -58,7 +58,7 @@ static int vhost_kernel_memslots_limit(struct vhost_dev
> > > *dev)
> > > if
> > > (g_file_get_contents("/sys/module/vhost/parameters/max_mem_regions",
> > > &s, NULL, NULL)) {
> > > uint64_t val = g_ascii_strtoull(s, NULL, 10);
> >
> > Would using qemu_strtou64() simplify this?
>
> I'm afraid not. None of the existing strtoXX converting functions has
> the desired output range (0 < retval < INT_MAX), so the following
> condition will remain necessary anyway; then it doesn't seem to matter
> which particular parser is used to extract the value which is in the
> range, so I left the one that was already there to reduce churn.
If qemu_strtou64() can't handle all values in (0 < retval < INT_MAX)
isn't that a bug in qemu_strtou64 ?
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for 6.2 v2 0/5] Fix Q35 ACPI PCI Hot-plug I/O issues
On Wed, Nov 10, 2021 at 04:11:35PM -0500, Igor Mammedov wrote: > Changelog: > v2: > * simplify [1/5] and rename property to x-native-hotplug (CC stable) > * [4/5] >- rename function parameter to reflect actual action >- drop local 'hotplug' variable and opencode statement > * test with SeaBIOS/OVMF and Linux guest, > Windows also works with SeaBIOS, can't install it in EFI > mode on current master (it's stuck when formatting disk/or > copying files to hdd). > > Attempt [1] to fix I/O allocation with the 'reserve-io' hint on each > pcie-root-port resulted in regression [2-3]. This patchset aims to fix > it by addressing the root cause of the problem - the disabled PCIe > Slot HPC bit. > > [1] 'hw/pcie-root-port: Fix hotplug for PCI devices requiring IO' > [2] https://gitlab.com/qemu-project/qemu/-/issues/641 > [3] https://bugzilla.redhat.com/show_bug.cgi?id=2006409 > > CC: kra...@redhat.com Igor are you going to post v3 with a fixup of the expected tables? Thanks! > Igor Mammedov (1): > pcie: rename 'native-hotplug' to 'x-native-hotplug' > > Julia Suvorova (4): > hw/acpi/ich9: Add compat prop to keep HPC bit set for 6.1 machine type > bios-tables-test: Allow changes in DSDT ACPI tables > hw/i386/acpi-build: Deny control on PCIe Native Hot-plug in _OSC > bios-tables-test: Update golden binaries > > include/hw/acpi/ich9.h| 1 + > hw/acpi/ich9.c| 18 ++ > hw/i386/acpi-build.c | 12 > hw/i386/pc.c | 2 ++ > hw/i386/pc_q35.c | 9 +++-- > hw/pci/pcie_port.c| 2 +- > tests/data/acpi/q35/DSDT | Bin 8289 -> 8289 bytes > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes > tests/data/acpi/q35/DSDT.dimmpxm | Bin 9943 -> 9943 bytes > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes > tests/data/acpi/q35/DSDT.memhp| Bin 9648 -> 9648 bytes > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes > tests/data/acpi/q35/DSDT.nosmm| Bin 0 -> 8289 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 8295 -> 8295 bytes > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes > tests/data/acpi/q35/DSDT.tis.tpm12| Bin 8894 -> 8894 bytes > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes > tests/data/acpi/q35/DSDT.xapic| Bin 35652 -> 35652 bytes > 25 files changed, 37 insertions(+), 7 deletions(-) > create mode 100644 tests/data/acpi/q35/DSDT.dmar > create mode 100644 tests/data/acpi/q35/DSDT.nosmm > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm > > -- > 2.27.0
Re: [PATCH v5 4/6] kvm: irqchip: extract kvm_irqchip_add_deferred_msi_route
On 11/3/21 09:16, Longpeng(Mike) wrote:
Extract a common helper that add MSI route for specific vector
but does not commit immediately.
Signed-off-by: Longpeng(Mike)
I think adding the new function is not necessary; I have no problem
moving the call to kvm_irqchip_commit_routes to the callers. Perhaps
you can have an API like this:
typedef struct KVMRouteChange {
KVMState *s;
int changes;
} KVMRouteChange;
KVMRouteChange kvm_irqchip_begin_route_changes(KVMState *s)
{
return (KVMRouteChange) { .s = s, .changes = 0 };
}
void kvm_irqchip_commit_route_changes(KVMRouteChange *c)
{
if (c->changes) {
kvm_irqchip_commit_routes(c->s);
c->changes = 0;
}
}
int kvm_irqchip_add_msi_route(KVMRouteChange *c, int vector, PCIDevice *dev)
{
KVMState *s = c->s;
...
kvm_add_routing_entry(s, &kroute);
kvm_arch_add_msi_route_post(&kroute, vector, dev);
c->changes++;
return virq;
}
so it's harder for the callers to "forget" kvm_irqchip_commit_route_changes.
Paolo
---
accel/kvm/kvm-all.c | 15 +--
include/sysemu/kvm.h | 6 ++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index db8d83b..8627f7c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1953,7 +1953,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
return kvm_set_irq(s, route->kroute.gsi, 1);
}
-int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector, PCIDevice *dev)
{
struct kvm_irq_routing_entry kroute = {};
int virq;
@@ -1996,7 +1996,18 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector,
PCIDevice *dev)
kvm_add_routing_entry(s, &kroute);
kvm_arch_add_msi_route_post(&kroute, vector, dev);
-kvm_irqchip_commit_routes(s);
+
+return virq;
+}
+
+int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
+{
+int virq;
+
+virq = kvm_irqchip_add_deferred_msi_route(s, vector, dev);
+if (virq >= 0) {
+kvm_irqchip_commit_routes(s);
+}
return virq;
}
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee..8de0d9a 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -476,6 +476,12 @@ void kvm_init_cpu_signals(CPUState *cpu);
* @return: virq (>=0) when success, errno (<0) when failed.
*/
int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev);
+/**
+ * Add MSI route for specific vector but does not commit to KVM
+ * immediately
+ */
+int kvm_irqchip_add_deferred_msi_route(KVMState *s, int vector,
+ PCIDevice *dev);
int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
PCIDevice *dev);
void kvm_irqchip_commit_routes(KVMState *s);
[PATCH v5 6/6] net/vmnet: update qemu-options.hx
Signed-off-by: Vladislav Yaroshchuk
---
qemu-options.hx | 25 +
1 file changed, 25 insertions(+)
diff --git a/qemu-options.hx b/qemu-options.hx
index 7749f59300..350d43bbc0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2677,6 +2677,25 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
#ifdef __linux__
"-netdev vhost-vdpa,id=str,vhostdev=/path/to/dev\n"
"configure a vhost-vdpa network,Establish a vhost-vdpa
netdev\n"
+#endif
+#ifdef CONFIG_VMNET
+"-netdev vmnet-host,id=str[,isolated=on|off][,net-uuid=uuid]\n"
+" [,dhcpstart=addr,dhcpend=addr,subnet-mask=mask]\n"
+"configure a vmnet network backend in host mode with ID
'str',\n"
+"isolate this interface from others with 'isolated',\n"
+"configure its DHCP server and choose a subnet mask\n"
+"or specify network UUID 'uuid' to disable DHCP and
interact with\n"
+"vmnet-host interfaces within this isolated network\n"
+"-netdev vmnet-shared,id=str[,isolated=on|off][,nat66-prefix=addr]\n"
+" [,dhcpstart=addr,dhcpend=addr,subnet-mask=mask]\n"
+"configure a vmnet network backend in shared mode with ID
'str',\n"
+"configure its DHCP server and choose a subnet mask,\n"
+"set IPv6 ULA prefix (of length 64) to use for internal
network,\n"
+"isolate this interface from others with 'isolated'\n"
+"-netdev vmnet-bridged,id=str,ifname=name[,isolated=on|off]\n"
+"configure a vmnet network backend in bridged mode with ID
'str',\n"
+"use 'ifname=name' to select a physical network interface
to be bridged,\n"
+"isolate this interface from others with 'isolated'\n"
#endif
"-netdev hubport,id=str,hubid=n[,netdev=nd]\n"
"configure a hub port on the hub with ID 'n'\n",
QEMU_ARCH_ALL)
@@ -2696,6 +2715,9 @@ DEF("nic", HAS_ARG, QEMU_OPTION_nic,
#endif
#ifdef CONFIG_POSIX
"vhost-user|"
+#endif
+#ifdef CONFIG_VMNET
+"vmnet-host|vmnet-shared|vmnet-bridged|"
#endif
"socket][,option][,...][mac=macaddr]\n"
"initialize an on-board / default host NIC (using MAC
address\n"
@@ -2718,6 +2740,9 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
#endif
#ifdef CONFIG_NETMAP
"netmap|"
+#endif
+#ifdef CONFIG_VMNET
+"vmnet-host|vmnet-shared|vmnet-bridged|"
#endif
"socket][,option][,option][,...]\n"
"old way to initialize a host network interface\n"
--
2.23.0
[PATCH v5 5/6] net/vmnet: implement bridged mode (vmnet-bridged)
Signed-off-by: Vladislav Yaroshchuk
---
net/vmnet-bridged.m | 98 ++---
1 file changed, 92 insertions(+), 6 deletions(-)
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
index 4e42a90391..3c9da9dc8b 100644
--- a/net/vmnet-bridged.m
+++ b/net/vmnet-bridged.m
@@ -10,16 +10,102 @@
#include "qemu/osdep.h"
#include "qapi/qapi-types-net.h"
-#include "vmnet_int.h"
-#include "clients.h"
-#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "clients.h"
+#include "vmnet_int.h"
#include
+typedef struct VmnetBridgedState {
+ VmnetCommonState cs;
+} VmnetBridgedState;
+
+static bool validate_ifname(const char *ifname)
+{
+xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
+__block bool match = false;
+
+xpc_array_apply(
+shared_if_list,
+^bool(size_t index, xpc_object_t value) {
+ if (strcmp(xpc_string_get_string_ptr(value), ifname) == 0) {
+ match = true;
+ return false;
+ }
+ return true;
+});
+
+return match;
+}
+
+static const char *get_valid_ifnames(void)
+{
+xpc_object_t shared_if_list = vmnet_copy_shared_interface_list();
+__block char *if_list = NULL;
+
+xpc_array_apply(
+shared_if_list,
+^bool(size_t index, xpc_object_t value) {
+ if_list = g_strconcat(xpc_string_get_string_ptr(value),
+" ",
+if_list,
+NULL);
+ return true;
+});
+
+if (if_list) {
+return if_list;
+}
+return "[no interfaces]";
+}
+
+static xpc_object_t create_if_desc(const Netdev *netdev, Error **errp)
+{
+const NetdevVmnetBridgedOptions *options = &(netdev->u.vmnet_bridged);
+xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
+
+xpc_dictionary_set_uint64(
+if_desc,
+vmnet_operation_mode_key,
+VMNET_BRIDGED_MODE
+);
+
+xpc_dictionary_set_bool(
+if_desc,
+vmnet_enable_isolation_key,
+options->isolated
+);
+
+if (validate_ifname(options->ifname)) {
+xpc_dictionary_set_string(if_desc,
+ vmnet_shared_interface_name_key,
+ options->ifname);
+} else {
+return NULL;
+}
+return if_desc;
+}
+
+static NetClientInfo net_vmnet_bridged_info = {
+.type = NET_CLIENT_DRIVER_VMNET_BRIDGED,
+.size = sizeof(VmnetBridgedState),
+.receive = vmnet_receive_common,
+.cleanup = vmnet_cleanup_common,
+};
+
int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
- error_setg(errp, "vmnet-bridged is not implemented yet");
- return -1;
-}
+NetClientState *nc = qemu_new_net_client(&net_vmnet_bridged_info,
+ peer, "vmnet-bridged", name);
+xpc_object_t if_desc = create_if_desc(netdev, errp);;
+
+if (!if_desc) {
+error_setg(errp,
+ "unsupported ifname, should be one of: %s",
+ get_valid_ifnames());
+return -1;
+}
+
+return vmnet_if_create(nc, if_desc, errp, NULL);
+}
\ No newline at end of file
--
2.23.0
[PATCH v5 3/6] net/vmnet: implement shared mode (vmnet-shared)
Signed-off-by: Phillip Tennen
Signed-off-by: Vladislav Yaroshchuk
---
net/vmnet-common.m | 305 +
net/vmnet-shared.c | 75 ++-
net/vmnet_int.h| 23
3 files changed, 399 insertions(+), 4 deletions(-)
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
index 532d152840..b058e1b846 100644
--- a/net/vmnet-common.m
+++ b/net/vmnet-common.m
@@ -10,6 +10,8 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "qemu/log.h"
#include "qapi/qapi-types-net.h"
#include "vmnet_int.h"
#include "clients.h"
@@ -17,4 +19,307 @@
#include "qapi/error.h"
#include
+#include
+#ifdef DEBUG
+#define D(x) x
+#define D_LOG(...) qemu_log(__VA_ARGS__)
+#else
+#define D(x) do { } while (0)
+#define D_LOG(...) do { } while (0)
+#endif
+
+typedef struct vmpktdesc vmpktdesc_t;
+typedef struct iovec iovec_t;
+
+static void vmnet_set_send_enabled(VmnetCommonState *s, bool enable)
+{
+s->send_enabled = enable;
+}
+
+
+static void vmnet_send_completed(NetClientState *nc, ssize_t len)
+{
+VmnetCommonState *s = DO_UPCAST(VmnetCommonState, nc, nc);
+vmnet_set_send_enabled(s, true);
+}
+
+
+static void vmnet_send(NetClientState *nc,
+ interface_event_t event_id,
+ xpc_object_t event)
+{
+assert(event_id == VMNET_INTERFACE_PACKETS_AVAILABLE);
+
+VmnetCommonState *s;
+uint64_t packets_available;
+
+struct iovec *iov;
+struct vmpktdesc *packets;
+int pkt_cnt;
+int i;
+
+vmnet_return_t if_status;
+ssize_t size;
+
+s = DO_UPCAST(VmnetCommonState, nc, nc);
+
+packets_available = xpc_dictionary_get_uint64(
+event,
+vmnet_estimated_packets_available_key
+);
+
+pkt_cnt = (packets_available < VMNET_PACKETS_LIMIT) ?
+ packets_available :
+ VMNET_PACKETS_LIMIT;
+
+
+iov = s->iov_buf;
+packets = s->packets_buf;
+
+for (i = 0; i < pkt_cnt; ++i) {
+packets[i].vm_pkt_size = s->max_packet_size;
+packets[i].vm_pkt_iovcnt = 1;
+packets[i].vm_flags = 0;
+}
+
+if_status = vmnet_read(s->vmnet_if, packets, &pkt_cnt);
+if (if_status != VMNET_SUCCESS) {
+error_printf("vmnet: read failed: %s\n",
+ vmnet_status_map_str(if_status));
+}
+qemu_mutex_lock_iothread();
+for (i = 0; i < pkt_cnt; ++i) {
+size = qemu_send_packet_async(nc,
+ iov[i].iov_base,
+ packets[i].vm_pkt_size,
+ vmnet_send_completed);
+if (size == 0) {
+vmnet_set_send_enabled(s, false);
+} else if (size < 0) {
+break;
+}
+}
+qemu_mutex_unlock_iothread();
+
+}
+
+
+static void vmnet_register_event_callback(VmnetCommonState *s)
+{
+dispatch_queue_t avail_pkt_q = dispatch_queue_create(
+"org.qemu.vmnet.if_queue",
+DISPATCH_QUEUE_SERIAL
+);
+
+vmnet_interface_set_event_callback(
+s->vmnet_if,
+VMNET_INTERFACE_PACKETS_AVAILABLE,
+avail_pkt_q,
+^(interface_event_t event_id, xpc_object_t event) {
+ if (s->send_enabled) {
+ vmnet_send(&s->nc, event_id, event);
+ }
+});
+}
+
+
+static void vmnet_bufs_init(VmnetCommonState *s)
+{
+int i;
+struct vmpktdesc *packets;
+struct iovec *iov;
+
+packets = s->packets_buf;
+iov = s->iov_buf;
+
+for (i = 0; i < VMNET_PACKETS_LIMIT; ++i) {
+iov[i].iov_len = s->max_packet_size;
+iov[i].iov_base = g_malloc0(iov[i].iov_len);
+packets[i].vm_pkt_iov = iov + i;
+}
+}
+
+
+const char *vmnet_status_map_str(vmnet_return_t status)
+{
+switch (status) {
+case VMNET_SUCCESS:
+return "success";
+case VMNET_FAILURE:
+return "general failure";
+case VMNET_MEM_FAILURE:
+return "memory allocation failure";
+case VMNET_INVALID_ARGUMENT:
+return "invalid argument specified";
+case VMNET_SETUP_INCOMPLETE:
+return "interface setup is not complete";
+case VMNET_INVALID_ACCESS:
+return "invalid access, permission denied";
+case VMNET_PACKET_TOO_BIG:
+return "packet size is larger than MTU";
+case VMNET_BUFFER_EXHAUSTED:
+return "buffers exhausted in kernel";
+case VMNET_TOO_MANY_PACKETS:
+return "packet count exceeds limit";
+case VMNET_SHARING_SERVICE_BUSY:
+return "conflict, sharing service is in use";
+default:
+return "unknown vmnet error";
+}
+}
+
+
+int vmnet_if_create(NetClientState *nc,
+xpc_object_t if_desc,
+Error **errp,
+void (*completion_callback)(xpc_object_t interface_param))
+{
+VmnetCommonState *s;
+
+dispatch_queue_t if_create_q;
+dispatch_semaphore_t if_created_sem;
+
+__block vmnet_return_t if_status;
+
+if_create_q =
[PATCH v5 4/6] net/vmnet: implement host mode (vmnet-host)
Signed-off-by: Vladislav Yaroshchuk
---
net/vmnet-host.c | 99 +---
1 file changed, 93 insertions(+), 6 deletions(-)
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
index 4a5ef99dc7..f7dbd3f9bf 100644
--- a/net/vmnet-host.c
+++ b/net/vmnet-host.c
@@ -9,16 +9,103 @@
*/
#include "qemu/osdep.h"
+#include "qemu/uuid.h"
#include "qapi/qapi-types-net.h"
-#include "vmnet_int.h"
-#include "clients.h"
-#include "qemu/error-report.h"
#include "qapi/error.h"
+#include "clients.h"
+#include "vmnet_int.h"
#include
+typedef struct VmnetHostState {
+ VmnetCommonState cs;
+ QemuUUID network_uuid;
+} VmnetHostState;
+
+static xpc_object_t create_if_desc(const Netdev *netdev,
+ NetClientState *nc,
+ Error **errp)
+{
+const NetdevVmnetHostOptions *options = &(netdev->u.vmnet_host);
+VmnetCommonState *cs = DO_UPCAST(VmnetCommonState, nc, nc);
+VmnetHostState *hs = DO_UPCAST(VmnetHostState, cs, cs);
+
+xpc_object_t if_desc = xpc_dictionary_create(NULL, NULL, 0);
+
+xpc_dictionary_set_uint64(
+if_desc,
+vmnet_operation_mode_key,
+VMNET_HOST_MODE
+);
+
+xpc_dictionary_set_bool(
+if_desc,
+vmnet_enable_isolation_key,
+options->isolated
+);
+
+if (options->has_net_uuid) {
+if (qemu_uuid_parse(options->net_uuid, &hs->network_uuid) < 0) {
+error_setg(errp, "Invalid UUID provided in 'net-uuid'");
+}
+
+xpc_dictionary_set_uuid(
+if_desc,
+vmnet_network_identifier_key,
+hs->network_uuid.data
+);
+}
+
+if (options->has_dhcpstart ||
+options->has_dhcpend ||
+options->has_subnet_mask) {
+
+if (options->has_dhcpstart &&
+options->has_dhcpend &&
+options->has_subnet_mask) {
+
+if (options->has_net_uuid) {
+error_setg(errp,
+ "DHCP disabled for this interface "
+ "because 'net-uuid' is provided");
+}
+
+xpc_dictionary_set_string(if_desc,
+ vmnet_start_address_key,
+ options->dhcpstart);
+xpc_dictionary_set_string(if_desc,
+ vmnet_end_address_key,
+ options->dhcpend);
+xpc_dictionary_set_string(if_desc,
+ vmnet_subnet_mask_key,
+ options->subnet_mask);
+} else {
+error_setg(
+errp,
+"'dhcpstart', 'dhcpend', 'subnet_mask' "
+"should be provided together"
+);
+}
+}
+
+return if_desc;
+}
+
+static NetClientInfo net_vmnet_host_info = {
+.type = NET_CLIENT_DRIVER_VMNET_HOST,
+.size = sizeof(VmnetHostState),
+.receive = vmnet_receive_common,
+.cleanup = vmnet_cleanup_common,
+};
+
int net_init_vmnet_host(const Netdev *netdev, const char *name,
-NetClientState *peer, Error **errp) {
- error_setg(errp, "vmnet-host is not implemented yet");
- return -1;
+NetClientState *peer, Error **errp)
+{
+NetClientState *nc;
+xpc_object_t if_desc;
+
+nc = qemu_new_net_client(&net_vmnet_host_info,
+ peer, "vmnet-host", name);
+if_desc = create_if_desc(netdev, nc, errp);
+return vmnet_if_create(nc, if_desc, errp, NULL);
}
--
2.23.0
[PATCH v5 2/6] net/vmnet: add vmnet backends to qapi/net
Create separate netdevs for each vmnet operating mode:
- vmnet-host
- vmnet-shared
- vmnet-bridged
Signed-off-by: Vladislav Yaroshchuk
---
net/clients.h | 11
net/meson.build | 7 +++
net/net.c | 10
net/vmnet-bridged.m | 25 +
net/vmnet-common.m | 20 +++
net/vmnet-host.c| 24 +
net/vmnet-shared.c | 25 +
net/vmnet_int.h | 25 +
qapi/net.json | 127 +++-
9 files changed, 272 insertions(+), 2 deletions(-)
create mode 100644 net/vmnet-bridged.m
create mode 100644 net/vmnet-common.m
create mode 100644 net/vmnet-host.c
create mode 100644 net/vmnet-shared.c
create mode 100644 net/vmnet_int.h
diff --git a/net/clients.h b/net/clients.h
index 92f9b59aed..c9157789f2 100644
--- a/net/clients.h
+++ b/net/clients.h
@@ -63,4 +63,15 @@ int net_init_vhost_user(const Netdev *netdev, const char
*name,
int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp);
+#ifdef CONFIG_VMNET
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+
+int net_init_vmnet_shared(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp);
+#endif /* CONFIG_VMNET */
+
#endif /* QEMU_NET_CLIENTS_H */
diff --git a/net/meson.build b/net/meson.build
index 847bc2ac85..00a88c4951 100644
--- a/net/meson.build
+++ b/net/meson.build
@@ -42,4 +42,11 @@ softmmu_ss.add(when: 'CONFIG_POSIX', if_true:
files(tap_posix))
softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('tap-win32.c'))
softmmu_ss.add(when: 'CONFIG_VHOST_NET_VDPA', if_true: files('vhost-vdpa.c'))
+vmnet_files = files(
+ 'vmnet-common.m',
+ 'vmnet-bridged.m',
+ 'vmnet-host.c',
+ 'vmnet-shared.c'
+)
+softmmu_ss.add(when: vmnet, if_true: vmnet_files)
subdir('can')
diff --git a/net/net.c b/net/net.c
index f0d14dbfc1..1dbb64b935 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1021,6 +1021,11 @@ static int (* const
net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
#ifdef CONFIG_L2TPV3
[NET_CLIENT_DRIVER_L2TPV3]= net_init_l2tpv3,
#endif
+#ifdef CONFIG_VMNET
+[NET_CLIENT_DRIVER_VMNET_HOST] = net_init_vmnet_host,
+[NET_CLIENT_DRIVER_VMNET_SHARED] = net_init_vmnet_shared,
+[NET_CLIENT_DRIVER_VMNET_BRIDGED] = net_init_vmnet_bridged,
+#endif /* CONFIG_VMNET */
};
@@ -1106,6 +,11 @@ void show_netdevs(void)
#endif
#ifdef CONFIG_VHOST_VDPA
"vhost-vdpa",
+#endif
+#ifdef CONFIG_VMNET
+"vmnet-host",
+"vmnet-shared",
+"vmnet-bridged",
#endif
};
diff --git a/net/vmnet-bridged.m b/net/vmnet-bridged.m
new file mode 100644
index 00..4e42a90391
--- /dev/null
+++ b/net/vmnet-bridged.m
@@ -0,0 +1,25 @@
+/*
+ * vmnet-bridged.m
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include
+
+int net_init_vmnet_bridged(const Netdev *netdev, const char *name,
+ NetClientState *peer, Error **errp)
+{
+ error_setg(errp, "vmnet-bridged is not implemented yet");
+ return -1;
+}
diff --git a/net/vmnet-common.m b/net/vmnet-common.m
new file mode 100644
index 00..532d152840
--- /dev/null
+++ b/net/vmnet-common.m
@@ -0,0 +1,20 @@
+/*
+ * vmnet-common.m - network client wrapper for Apple vmnet.framework
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk
+ * Copyright(c) 2021 Phillip Tennen
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include
+
diff --git a/net/vmnet-host.c b/net/vmnet-host.c
new file mode 100644
index 00..4a5ef99dc7
--- /dev/null
+++ b/net/vmnet-host.c
@@ -0,0 +1,24 @@
+/*
+ * vmnet-host.c
+ *
+ * Copyright(c) 2021 Vladislav Yaroshchuk
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-types-net.h"
+#include "vmnet_int.h"
+#include "clients.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+
+#include
+
+int net_init_vmnet_host(const Netdev *netdev, const char *name,
+NetClientState *peer, Error **errp) {
+ error_setg(errp, "vmnet-host is not implemented yet");
+ return -1;
+
[PATCH v5 1/6] net/vmnet: add vmnet dependency and customizable option
Signed-off-by: Vladislav Yaroshchuk
---
meson.build | 4
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 3 +++
3 files changed, 9 insertions(+)
diff --git a/meson.build b/meson.build
index 2ece4fe088..202e04af31 100644
--- a/meson.build
+++ b/meson.build
@@ -477,6 +477,8 @@ if cocoa.found() and get_option('gtk').enabled()
error('Cocoa and GTK+ cannot be enabled at the same time')
endif
+vmnet = dependency('appleframeworks', modules: 'vmnet', required:
get_option('vmnet'))
+
seccomp = not_found
if not get_option('seccomp').auto() or have_system or have_tools
seccomp = dependency('libseccomp', version: '>=2.3.0',
@@ -1455,6 +1457,7 @@ config_host_data.set('CONFIG_SECCOMP', seccomp.found())
config_host_data.set('CONFIG_SNAPPY', snappy.found())
config_host_data.set('CONFIG_USB_LIBUSB', libusb.found())
config_host_data.set('CONFIG_VDE', vde.found())
+config_host_data.set('CONFIG_VMNET', vmnet.found())
config_host_data.set('CONFIG_VHOST_USER_BLK_SERVER',
have_vhost_user_blk_server)
config_host_data.set('CONFIG_VNC', vnc.found())
config_host_data.set('CONFIG_VNC_JPEG', jpeg.found())
@@ -3383,6 +3386,7 @@ endif
summary_info += {'JACK support': jack}
summary_info += {'brlapi support':brlapi}
summary_info += {'vde support': vde}
+summary_info += {'vmnet.framework support': vmnet}
summary_info += {'netmap support':have_netmap}
summary_info += {'l2tpv3 support':have_l2tpv3}
summary_info += {'Linux AIO support': libaio}
diff --git a/meson_options.txt b/meson_options.txt
index 411952bc91..bdcd9674ea 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -147,6 +147,8 @@ option('netmap', type : 'feature', value : 'auto',
description: 'netmap network backend support')
option('vde', type : 'feature', value : 'auto',
description: 'vde network backend support')
+option('vmnet', type : 'feature', value : 'auto',
+ description: 'vmnet.framework network backend support')
option('virglrenderer', type : 'feature', value : 'auto',
description: 'virgl rendering support')
option('vnc', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 45e1f2e20d..f75375c795 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -81,6 +81,7 @@ meson_options_help() {
printf "%s\n" ' u2f U2F emulation support'
printf "%s\n" ' usb-redir libusbredir support'
printf "%s\n" ' vde vde network backend support'
+ printf "%s\n" ' vmnet vmnet.framework network backend support'
printf "%s\n" ' vhost-user-blk-server'
printf "%s\n" ' build vhost-user-blk server'
printf "%s\n" ' virglrenderer virgl rendering support'
@@ -239,6 +240,8 @@ _meson_option_parse() {
--disable-usb-redir) printf "%s" -Dusb_redir=disabled ;;
--enable-vde) printf "%s" -Dvde=enabled ;;
--disable-vde) printf "%s" -Dvde=disabled ;;
+--enable-vmnet) printf "%s" -Dvmnet=enabled ;;
+--disable-vmnet) printf "%s" -Dvmnet=disabled ;;
--enable-vhost-user-blk-server) printf "%s"
-Dvhost_user_blk_server=enabled ;;
--disable-vhost-user-blk-server) printf "%s"
-Dvhost_user_blk_server=disabled ;;
--enable-virglrenderer) printf "%s" -Dvirglrenderer=enabled ;;
--
2.23.0
[PATCH v5 0/6] Add vmnet.framework based network backend
macOS provides networking API for VMs called 'vmnet.framework': https://developer.apple.com/documentation/vmnet We can provide its support as the new QEMU network backends which represent three different vmnet.framework interface usage modes: * `vmnet-shared`: allows the guest to communicate with other guests in shared mode and also with external network (Internet) via NAT. Has (macOS-provided) DHCP server; subnet mask and IP range can be configured; * `vmnet-host`: allows the guest to communicate with other guests in host mode. By default has enabled DHCP as `vmnet-shared`, but providing network unique id (uuid) can make `vmnet-host` interfaces isolated from each other and also disables DHCP. * `vmnet-bridged`: bridges the guest with a physical network interface. This backends cannot work on macOS Catalina 10.15 cause we use vmnet.framework API provided only with macOS 11 and newer. Seems that it is not a problem, because QEMU guarantees to work on two most recent versions of macOS which now are Big Sur (11) and Monterey (12). Also, we have one inconvenient restriction: vmnet.framework interfaces can create only privileged user: `$ sudo qemu-system-x86_64 -nic vmnet-shared` Attempt of `vmnet-*` netdev creation being unprivileged user fails with vmnet's 'general failure'. This happens because vmnet.framework requires `com.apple.vm.networking` entitlement which is: "restricted to developers of virtualization software. To request this entitlement, contact your Apple representative." as Apple documentation says: https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_vm_networking One more note: we still have quite useful but not supported 'vmnet.framework' features as creating port forwarding rules, IPv6 NAT prefix specifying and so on. Nevertheless, new backends work fine and tested within `qemu-system-x86-64` on macOS Bir Sur 11.5.2 host with such nic models: * e1000-82545em * virtio-net-pci * vmxnet3 The guests were: * macOS 10.15.7 * Ubuntu Bionic (server cloudimg) This series partially reuses patches by Phillip Tennen: https://patchew.org/QEMU/20210218134947.1860-1-phillip.en...@gmail.com/ So I included his signed-off line into one of the commit messages and also here. v1 -> v2: Since v1 minor typos were fixed, patches rebased onto latest master, redundant changes removed (small commits squashed) v2 -> v3: - QAPI style fixes - Typos fixes in comments - `#include`'s updated to be in sync with recent master v3 -> v4: - Support vmnet interfaces isolation feature - Support vmnet-host network uuid setting feature - Refactored sources a bit v4 -> v5: - Missed 6.2 boat, now 7.0 candidate - Fix qapi netdev descriptions and styles (@subnetmask -> @subnet-mask) - Support vmnet-shared IPv6 prefix setting feature Signed-off-by: Phillip Tennen Signed-off-by: Vladislav Yaroshchuk Vladislav Yaroshchuk (6): net/vmnet: add vmnet dependency and customizable option net/vmnet: add vmnet backends to qapi/net net/vmnet: implement shared mode (vmnet-shared) net/vmnet: implement host mode (vmnet-host) net/vmnet: implement bridged mode (vmnet-bridged) net/vmnet: update qemu-options.hx meson.build | 4 + meson_options.txt | 2 + net/clients.h | 11 ++ net/meson.build | 7 + net/net.c | 10 ++ net/vmnet-bridged.m | 111 net/vmnet-common.m| 325 ++ net/vmnet-host.c | 111 net/vmnet-shared.c| 92 ++ net/vmnet_int.h | 48 + qapi/net.json | 127 - qemu-options.hx | 25 +++ scripts/meson-buildoptions.sh | 3 + 13 files changed, 874 insertions(+), 2 deletions(-) create mode 100644 net/vmnet-bridged.m create mode 100644 net/vmnet-common.m create mode 100644 net/vmnet-host.c create mode 100644 net/vmnet-shared.c create mode 100644 net/vmnet_int.h -- 2.23.0
Re: [PATCH for 6.2 v2 5/5] bios-tables-test: Update golden binaries
On Thu, 11 Nov 2021, Ani Sinha wrote:
>
>
> On Thu, 11 Nov 2021, Ani Sinha wrote:
>
> >
> >
> > On Thu, 11 Nov 2021, Michael S. Tsirkin wrote:
> >
> > > On Wed, Nov 10, 2021 at 04:11:40PM -0500, Igor Mammedov wrote:
> > > > From: Julia Suvorova
> > > >
> > > > The changes are the result of
> > > > 'hw/i386/acpi-build: Deny control on PCIe Native Hot-Plug in
> > > > _OSC'
> > > > and listed here:
> > > >
> > > > Method (_OSC, 4, NotSerialized) // _OSC: Operating System Capabilities
> > > > {
> > > > CreateDWordField (Arg3, Zero, CDW1)
> > > > If ((Arg0 == ToUUID
> > > > ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> > > > {
> > > > CreateDWordField (Arg3, 0x04, CDW2)
> > > > CreateDWordField (Arg3, 0x08, CDW3)
> > > > Local0 = CDW3 /* \_SB_.PCI0._OSC.CDW3 */
> > > > -Local0 &= 0x1F
> > > > +Local0 &= 0x1E
> > > >
> > > > Signed-off-by: Julia Suvorova
> > > > Signed-off-by: Igor Mammedov
> > > > ---
> > > > tests/qtest/bios-tables-test-allowed-diff.h | 16
> > > > tests/data/acpi/q35/DSDT| Bin 8289 -> 8289 bytes
> > > > tests/data/acpi/q35/DSDT.acpihmat | Bin 9614 -> 9614 bytes
> > > > tests/data/acpi/q35/DSDT.bridge | Bin 11003 -> 11003 bytes
> > > > tests/data/acpi/q35/DSDT.cphp | Bin 8753 -> 8753 bytes
> > > > tests/data/acpi/q35/DSDT.dimmpxm| Bin 9943 -> 9943 bytes
> > > > tests/data/acpi/q35/DSDT.dmar | Bin 0 -> 8289 bytes
> > > > tests/data/acpi/q35/DSDT.ipmibt | Bin 8364 -> 8364 bytes
> > > > tests/data/acpi/q35/DSDT.ivrs | Bin 8306 -> 8306 bytes
> > > > tests/data/acpi/q35/DSDT.memhp | Bin 9648 -> 9648 bytes
> > > > tests/data/acpi/q35/DSDT.mmio64 | Bin 9419 -> 9419 bytes
> > > > tests/data/acpi/q35/DSDT.multi-bridge | Bin 8583 -> 8583 bytes
> > > > tests/data/acpi/q35/DSDT.nohpet | Bin 8147 -> 8147 bytes
> > > > tests/data/acpi/q35/DSDT.nosmm | Bin 0 -> 8289 bytes
> > > > tests/data/acpi/q35/DSDT.numamem| Bin 8295 -> 8295 bytes
> > > > tests/data/acpi/q35/DSDT.smm-compat | Bin 0 -> 8289 bytes
> > > > tests/data/acpi/q35/DSDT.smm-compat-nosmm | Bin 0 -> 8289 bytes
> > > > tests/data/acpi/q35/DSDT.tis.tpm12 | Bin 8894 -> 8894 bytes
> > > > tests/data/acpi/q35/DSDT.tis.tpm2 | Bin 8894 -> 8894 bytes
> > > > tests/data/acpi/q35/DSDT.xapic | Bin 35652 -> 35652 bytes
> > >
> > > Why do we have all the new files? What is going on here?
> >
> > Good catch. I saw those files even in my workspace and failed to notice
> > that they were being newly created in this patch and they did not exist
> > previously:
> > https://git.qemu.org/?p=qemu.git;a=tree;f=tests/data/acpi/q35;h=e9d1edd2671997a3e7fe278018313bcbfcfb0850;hb=HEAD
> >
> >
> > >
> > > > 20 files changed, 16 deletions(-)
> > > > create mode 100644 tests/data/acpi/q35/DSDT.dmar
> >
> > The corresponding change that adds the test is :
> >
> > commit 0ff92b6d99011c8de57321503c0eb655c461a217
> > Author: Igor Mammedov
> > Date: Thu Sep 2 07:35:43 2021 -0400
> >
> > tests: acpi: add testcase for intel_iommu (DMAR table)
> >
> > Igor has updated the DMAR table blob here:
> >
> > commit 44d3bdd8a6f1ae2a5ca417251736a033900d4c08
> > Author: Igor Mammedov
> > Date: Thu Sep 2 07:35:44 2021 -0400
> >
> > tests: acpi: add expected blob for DMAR table
> >
> > but maybe the test also introduced changes in DSDT table as well?
> > Needs more investigation.
>
> Actually I am pretty sure that no changes in DSDT was introduced with the
> above test. If it did, the tests would be broken because Igor did not
> update the DSDT table blob. So we should not commit the
> tests/data/acpi/q35/DSDT.dmar
>
> table blob here.
>
>
> >
> > > > create mode 100644 tests/data/acpi/q35/DSDT.nosmm
> > > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat
> > > > create mode 100644 tests/data/acpi/q35/DSDT.smm-compat-nosmm
> >
> > The corresponding tests for these files were added in this commit:
> >
> > commit 0dabb2e802437c2e578dc72bd0bdf3380a25ec96
> > Author: Isaku Yamahata
> > Date: Wed Feb 17 21:51:14 2021 -0800
> >
> > BUT it seems that by mistake the table blobs were not added when the
> > following commit was made:
> >
> > commit 7b630d937a6c73fb145746fb31e0fb4b08f0cf0e
> > Author: Isaku Yamahata
> > Date: Wed Feb 17 21:51:18 2021 -0800
> >
> > qtest/acpi/bios-tables-test: update acpi tables
> >
> > Sadly, the above commit does update tests/data/acpi/q35/DSDT which I
> > believe includes the changes for the above tests. The commit message
> > does not add the ASL diff which the tests introduces.
>
> We need to check this one. If the above commit did indeed introduce
> changes in DSDT blob whi
Re: [PATCH 02/10] chardev/char-socket: tcp_chr_recv: don't clobber errno
On Thu, Nov 11, 2021 at 7:38 PM Roman Kagan wrote:
> tcp_chr_recv communicates the specific error condition to the caller via
> errno. However, after setting it, it may call into some system calls or
> library functions which can clobber the errno.
>
> Avoid this by moving the errno assignment to the end of the function.
>
> Signed-off-by: Roman Kagan
>
Reviewed-by: Marc-André Lureau
---
> chardev/char-socket.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 836cfa0bc2..90054ce58c 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -346,13 +346,6 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
> size_t len)
> NULL);
> }
>
> -if (ret == QIO_CHANNEL_ERR_BLOCK) {
> -errno = EAGAIN;
> -ret = -1;
> -} else if (ret == -1) {
> -errno = EIO;
> -}
> -
> if (msgfds_num) {
> /* close and clean read_msgfds */
> for (i = 0; i < s->read_msgfds_num; i++) {
> @@ -381,6 +374,13 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf,
> size_t len)
> #endif
> }
>
> +if (ret == QIO_CHANNEL_ERR_BLOCK) {
> +errno = EAGAIN;
> +ret = -1;
> +} else if (ret == -1) {
> +errno = EIO;
> +}
> +
> return ret;
> }
>
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 03/10] chardev/char-socket: tcp_chr_sync_read: don't clobber errno
On Thu, Nov 11, 2021 at 7:36 PM Roman Kagan wrote:
> After the return from tcp_chr_recv, tcp_chr_sync_read calls into a
> function which eventually makes a system call and may clobber errno.
>
> Make a copy of errno right after tcp_chr_recv and restore the errno on
> return from tcp_chr_sync_read.
>
> Signed-off-by: Roman Kagan
>
Reviewed-by: Marc-André Lureau
---
> chardev/char-socket.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 90054ce58c..cf7f2ba65a 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -581,6 +581,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> int size;
> +int saved_errno;
>
> if (s->state != TCP_CHARDEV_STATE_CONNECTED) {
> return 0;
> @@ -588,6 +589,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
>
> qio_channel_set_blocking(s->ioc, true, NULL);
> size = tcp_chr_recv(chr, (void *) buf, len);
> +saved_errno = errno;
> if (s->state != TCP_CHARDEV_STATE_DISCONNECTED) {
> qio_channel_set_blocking(s->ioc, false, NULL);
> }
> @@ -596,6 +598,7 @@ static int tcp_chr_sync_read(Chardev *chr, const
> uint8_t *buf, int len)
> tcp_chr_disconnect(chr);
> }
>
> +errno = saved_errno;
> return size;
> }
>
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH 1/2] hw/core/loader: return image sizes as ssize_t
On 14:11 Thu 11 Nov , Jamie Iles wrote:
> Various loader functions return an int which limits images to 2GB which
> is fine for things like a BIOS/kernel image, but if we want to be able
> to load memory images or large ramdisks then any file over 2GB would
> silently fail to load.
>
> Cc: Luc Michel
> Signed-off-by: Jamie Iles
Reviewed-by: Luc Michel
> ---
> hw/arm/armv7m.c | 2 +-
> hw/arm/boot.c| 8 ++--
> hw/core/generic-loader.c | 2 +-
> hw/core/loader.c | 81 +---
> hw/i386/x86.c| 2 +-
> hw/riscv/boot.c | 5 ++-
> include/hw/loader.h | 55 +--
> 7 files changed, 80 insertions(+), 75 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 8d08db80be83..a6393dce7276 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -552,7 +552,7 @@ static void armv7m_reset(void *opaque)
>
> void armv7m_load_kernel(ARMCPU *cpu, const char *kernel_filename, int
> mem_size)
> {
> -int image_size;
> +ssize_t image_size;
> uint64_t entry;
> int big_endian;
> AddressSpace *as;
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 74ad397b1ff9..3853203438ba 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -876,7 +876,7 @@ static int do_arm_linux_init(Object *obj, void *opaque)
> return 0;
> }
>
> -static int64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> +static ssize_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, AddressSpace *as)
> {
> @@ -887,7 +887,7 @@ static int64_t arm_load_elf(struct arm_boot_info *info,
> uint64_t *pentry,
> } elf_header;
> int data_swab = 0;
> bool big_endian;
> -int64_t ret = -1;
> +ssize_t ret = -1;
> Error *err = NULL;
>
>
> @@ -1009,7 +1009,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
> /* Set up for a direct boot of a kernel image file. */
> CPUState *cs;
> AddressSpace *as = arm_boot_address_space(cpu, info);
> -int kernel_size;
> +ssize_t kernel_size;
> int initrd_size;
> int is_linux = 0;
> uint64_t elf_entry;
> @@ -1098,7 +1098,7 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
>
> if (kernel_size > info->ram_size) {
> error_report("kernel '%s' is too large to fit in RAM "
> - "(kernel size %d, RAM size %" PRId64 ")",
> + "(kernel size %zd, RAM size %" PRId64 ")",
> info->kernel_filename, kernel_size, info->ram_size);
> exit(1);
> }
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea2e..bc1451da8f55 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -66,7 +66,7 @@ static void generic_loader_realize(DeviceState *dev, Error
> **errp)
> GenericLoaderState *s = GENERIC_LOADER(dev);
> hwaddr entry;
> int big_endian;
> -int size = 0;
> +ssize_t size = 0;
>
> s->set_pc = false;
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 052a0fd7198b..348bbf535bd9 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
> return did;
> }
>
> -int load_image_targphys(const char *filename,
> -hwaddr addr, uint64_t max_sz)
> +ssize_t load_image_targphys(const char *filename,
> +hwaddr addr, uint64_t max_sz)
> {
> return load_image_targphys_as(filename, addr, max_sz, NULL);
> }
>
> /* return the size or -1 if error */
> -int load_image_targphys_as(const char *filename,
> - hwaddr addr, uint64_t max_sz, AddressSpace *as)
> +ssize_t load_image_targphys_as(const char *filename,
> + hwaddr addr, uint64_t max_sz, AddressSpace
> *as)
> {
> -int size;
> +ssize_t size;
>
> size = get_image_size(filename);
> if (size < 0 || size > max_sz) {
> @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
> return size;
> }
>
> -int load_image_mr(const char *filename, MemoryRegion *mr)
> +ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
> {
> -int size;
> +ssize_t size;
>
> if (!memory_access_is_direct(mr, false)) {
> /* Can only load an image into RAM or ROM */
> @@ -223,8 +223,8 @@ static void bswap_ahdr(struct exec *e)
> : (_N_SEGMENT_ROUND (_N_TXTENDADDR(x, target_page_size),
> target_page_size)))
>
>
> -int load_aout(const char *filename, hwaddr addr, int max_sz,
> - int bswap_needed, hwaddr target_page_size)
> +ssize_t load_aout(const char *filename, hwaddr addr, int max_sz,
> + int bswap_needed, hwaddr target_page_size)
> {
> int fd;
> ssize_t size, ret;
> @@ -618,13 +618,14
Re: [PATCH 04/10] chardev/char-fe: don't allow EAGAIN from blocking read
Hi
On Thu, Nov 11, 2021 at 7:44 PM Roman Kagan wrote:
> As its name suggests, ChardevClass.chr_sync_read is supposed to do a
> blocking read. The only implementation of it, tcp_chr_sync_read, does
> set the underlying io channel to the blocking mode indeed.
>
> Therefore a failure return with EAGAIN is not expected from this call.
>
> So do not retry it in qemu_chr_fe_read_all; instead place an assertion
> that it doesn't fail with EAGAIN.
>
The code was introduced in :
commit 7b0bfdf52d694c9a3a96505aa42ce3f8d63acd35
Author: Nikolay Nikolaev
Date: Tue May 27 15:03:48 2014 +0300
Add chardev API qemu_chr_fe_read_all
Also touched later by Daniel in:
commit 53628efbc8aa7a7ab5354d24b971f4d69452151d
Author: Daniel P. Berrangé
Date: Thu Mar 31 16:29:27 2016 +0100
char: fix broken EAGAIN retry on OS-X due to errno clobbering
> Signed-off-by: Roman Kagan
> ---
> chardev/char-fe.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 7789f7be9c..f94efe928e 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -68,13 +68,10 @@ int qemu_chr_fe_read_all(CharBackend *be, uint8_t
> *buf, int len)
> }
>
> while (offset < len) {
> -retry:
> res = CHARDEV_GET_CLASS(s)->chr_sync_read(s, buf + offset,
>len - offset);
> -if (res == -1 && errno == EAGAIN) {
> -g_usleep(100);
> -goto retry;
> -}
> +/* ->chr_sync_read should block */
> +assert(!(res < 0 && errno == EAGAIN));
>
>
While I agree with the rationale to clean this code a bit, I am not so sure
about replacing it with an assert(). In the past, when we did such things
we had unexpected regressions :)
A slightly better approach perhaps is g_warn_if_fail(), although it's not
very popular in qemu.
> if (res == 0) {
> break;
> --
> 2.33.1
>
>
>
--
Marc-André Lureau
Re: [PATCH v4 2/6] net/vmnet: add vmnet backends to qapi/net
чт, 11 нояб. 2021 г., 11:49 PM Eric Blake : > On Thu, Nov 11, 2021 at 06:21:28PM +0300, Vladislav Yaroshchuk wrote: > > > > > +# > > > > +# Since: 6.2 > > > > > > Missed 6.2, please adjust. More of the same below. > > > > > > > > The next one is 6.3, isn't it? > > 7.0, actually, as it will be the first release in 2022. > > OK, thank you! > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > > -- Best Regards, Vladislav Yaroshchuk
Re: [PATCH 00/10] vhost: stick to -errno error return convention
On Thu, Nov 11, 2021 at 03:14:56PM -0500, Michael S. Tsirkin wrote: > On Thu, Nov 11, 2021 at 06:33:44PM +0300, Roman Kagan wrote: > > Error propagation between the generic vhost code and the specific backends > > is > > not quite consistent: some places follow "return -1 and set errno" > > convention, > > while others assume "return negated errno". Furthermore, not enough care is > > taken not to clobber errno. > > > > As a result, on certain code paths the errno resulting from a failure may > > get > > overridden by another function call, and then that zero errno inidicating > > success is propagated up the stack, leading to failures being lost. In > > particular, we've seen errors in the communication with a vhost-user-blk > > slave > > not trigger an immediate connection drop and reconnection, leaving it in a > > broken state. > > > > Rework error propagation to always return negated errno on errors and > > correctly pass it up the stack. > > Looks like something we want post release. I'll tag it > but pls ping me after the release to help make sure > it's not lost. It doesn't introduce new features so I guess it might qualify for rc0, but the churn is somewhat too big indeed. OK I'll reiterate once 6.2 is out; meanwhile if anyone has spare cycles to review it, it'll be much appreciated. Thanks, Roman. > > > > Roman Kagan (10): > > vhost-user-blk: reconnect on any error during realize > > chardev/char-socket: tcp_chr_recv: don't clobber errno > > chardev/char-socket: tcp_chr_sync_read: don't clobber errno > > chardev/char-fe: don't allow EAGAIN from blocking read > > vhost-backend: avoid overflow on memslots_limit > > vhost-backend: stick to -errno error return convention > > vhost-vdpa: stick to -errno error return convention > > vhost-user: stick to -errno error return convention > > vhost: stick to -errno error return convention > > vhost-user-blk: propagate error return from generic vhost > > > > chardev/char-fe.c | 7 +- > > chardev/char-socket.c | 17 +- > > hw/block/vhost-user-blk.c | 4 +- > > hw/virtio/vhost-backend.c | 4 +- > > hw/virtio/vhost-user.c| 401 +- > > hw/virtio/vhost-vdpa.c| 37 ++-- > > hw/virtio/vhost.c | 98 +- > > 7 files changed, 307 insertions(+), 261 deletions(-) > > > > -- > > 2.33.1 > > >
