This introduces an Error object based implementation of virtio_error(). It
allows to implement virtio_error() wrappers in device-specific code.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/virtio/virtio.c | 21 ++++++++++++++++-----
include/hw/virtio/virtio.h | 1 +
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03592c542a55..4036f4816038 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2443,6 +2443,16 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
vdev->bus_name = g_strdup(bus_name);
}
+static void virtio_device_set_broken(VirtIODevice *vdev)
+{
+ vdev->broken = true;
+
+ if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+ virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
+ virtio_notify_config(vdev);
+ }
+}
+
void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
{
va_list ap;
@@ -2451,12 +2461,13 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
error_vreport(fmt, ap);
va_end(ap);
- vdev->broken = true;
+ virtio_device_set_broken(vdev);
+}
- if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
- virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
- virtio_notify_config(vdev);
- }
+void virtio_error_err(VirtIODevice *vdev, Error *err)
+{
+ error_report_err(err);
+ virtio_device_set_broken(vdev);
}
static void virtio_memory_listener_commit(MemoryListener *listener)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 15efcf205711..5b13c5f67b63 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -150,6 +150,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
void virtio_cleanup(VirtIODevice *vdev);
void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+void virtio_error_err(VirtIODevice *vdev, Error *err);
/* Set the child bus name. */
void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote:
> This introduces an Error object based implementation of virtio_error(). It
> allows to implement virtio_error() wrappers in device-specific code.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> hw/virtio/virtio.c | 21 ++++++++++++++++-----
> include/hw/virtio/virtio.h | 1 +
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 03592c542a55..4036f4816038 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2443,6 +2443,16 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
> vdev->bus_name = g_strdup(bus_name);
> }
>
> +static void virtio_device_set_broken(VirtIODevice *vdev)
> +{
> + vdev->broken = true;
> +
> + if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> + virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> + virtio_notify_config(vdev);
> + }
> +}
> +
> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> {
> va_list ap;
It's worth pondering whether we can set this for versions < 1.0 too.
> @@ -2451,12 +2461,13 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> error_vreport(fmt, ap);
> va_end(ap);
>
> - vdev->broken = true;
> + virtio_device_set_broken(vdev);
> +}
>
> - if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> - virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> - virtio_notify_config(vdev);
> - }
> +void virtio_error_err(VirtIODevice *vdev, Error *err)
> +{
> + error_report_err(err);
> + virtio_device_set_broken(vdev);
> }
>
> static void virtio_memory_listener_commit(MemoryListener *listener)
Should this skip error report if device is already broken?
Otherwise we'll get a ton of errors in the log.
Also, whether to stop the device, or the VM, or just warn,
seems like a policy decision. Why not set it on command line
like we do for other storage?
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 15efcf205711..5b13c5f67b63 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -150,6 +150,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
> void virtio_cleanup(VirtIODevice *vdev);
>
> void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> +void virtio_error_err(VirtIODevice *vdev, Error *err);
>
> /* Set the child bus name. */
> void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
On Mon, 27 Mar 2017 21:20:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote:
> > This introduces an Error object based implementation of virtio_error(). It
> > allows to implement virtio_error() wrappers in device-specific code.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/virtio/virtio.c | 21 ++++++++++++++++-----
> > include/hw/virtio/virtio.h | 1 +
> > 2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 03592c542a55..4036f4816038 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2443,6 +2443,16 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
> > vdev->bus_name = g_strdup(bus_name);
> > }
> >
> > +static void virtio_device_set_broken(VirtIODevice *vdev)
> > +{
> > + vdev->broken = true;
> > +
> > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > + virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> > + virtio_notify_config(vdev);
> > + }
> > +}
> > +
> > void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> > {
> > va_list ap;
>
> It's worth pondering whether we can set this for versions < 1.0 too.
I'm a bit torn there. In theory, setting an unknown status bit should
not really do harm; but we can't be sure that there aren't legacy
drivers out there that will crash when they notice an unknown status
bit, and I'm not sure we want that.
>
>
> > @@ -2451,12 +2461,13 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> > error_vreport(fmt, ap);
> > va_end(ap);
> >
> > - vdev->broken = true;
> > + virtio_device_set_broken(vdev);
> > +}
> >
> > - if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > - virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> > - virtio_notify_config(vdev);
> > - }
> > +void virtio_error_err(VirtIODevice *vdev, Error *err)
> > +{
> > + error_report_err(err);
> > + virtio_device_set_broken(vdev);
> > }
> >
> > static void virtio_memory_listener_commit(MemoryListener *listener)
>
> Should this skip error report if device is already broken?
> Otherwise we'll get a ton of errors in the log.
One would hope that qemu stops processing broken devices, but a check
might be better.
>
> Also, whether to stop the device, or the VM, or just warn,
> seems like a policy decision. Why not set it on command line
> like we do for other storage?
I would trust the device implementation to make the decision: Can we
recover, can we start using the device again after a reset, or are we
so broken that we want to terminate the vm?
Note that all of this already applies to the existing virtio_error(); I
think we should discuss this independently of this patch.
On Mon, 27 Mar 2017 21:20:56 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote:
> > This introduces an Error object based implementation of virtio_error(). It
> > allows to implement virtio_error() wrappers in device-specific code.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/virtio/virtio.c | 21 ++++++++++++++++-----
> > include/hw/virtio/virtio.h | 1 +
> > 2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 03592c542a55..4036f4816038 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2443,6 +2443,16 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
> > vdev->bus_name = g_strdup(bus_name);
> > }
> >
> > +static void virtio_device_set_broken(VirtIODevice *vdev)
> > +{
> > + vdev->broken = true;
> > +
> > + if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > + virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> > + virtio_notify_config(vdev);
> > + }
> > +}
> > +
> > void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> > {
> > va_list ap;
>
> It's worth pondering whether we can set this for versions < 1.0 too.
>
I don't understand this question... Are you talking of the NEEDS_RESET
status bit (we may expose this flag to non-virtio1 drivers?) or the
broken flag itself (we should not implement broken legacy devices) ?
>
> > @@ -2451,12 +2461,13 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> > error_vreport(fmt, ap);
> > va_end(ap);
> >
> > - vdev->broken = true;
> > + virtio_device_set_broken(vdev);
> > +}
> >
> > - if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > - virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
> > - virtio_notify_config(vdev);
> > - }
> > +void virtio_error_err(VirtIODevice *vdev, Error *err)
> > +{
> > + error_report_err(err);
> > + virtio_device_set_broken(vdev);
> > }
> >
> > static void virtio_memory_listener_commit(MemoryListener *listener)
>
> Should this skip error report if device is already broken?
> Otherwise we'll get a ton of errors in the log.
>
I don't think so: if the device is broken, it stops processing virtqueues
in and out, until it gets reset. And even though we would have a stubborn
guest that keeps breaking the device again and again, libvirt has size
limited and rotating log files.
> Also, whether to stop the device, or the VM, or just warn,
> seems like a policy decision. Why not set it on command line
> like we do for other storage?
>
Huh? This patch simply introduces a new API to a feature that underwent
several rounds of discussion and reached a reasonable consensus (even
your R-b).
I'm not sure this 9pfs series is the right place to talk about all the
behavior changes you're suggesting for virtio_error()... I'd rather
drop this patch and duplicate code in virtio-9p instead if I want the
fixes to go to 2.9.
Cc'ing Connie and Stefanha for insights.
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 15efcf205711..5b13c5f67b63 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -150,6 +150,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
> > void virtio_cleanup(VirtIODevice *vdev);
> >
> > void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> > +void virtio_error_err(VirtIODevice *vdev, Error *err);
> >
> > /* Set the child bus name. */
> > void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
On Tue, 28 Mar 2017 10:14:09 +0200 Greg Kurz <groug@kaod.org> wrote: > On Mon, 27 Mar 2017 21:20:56 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote: > > > This introduces an Error object based implementation of virtio_error(). It > > > allows to implement virtio_error() wrappers in device-specific code. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/virtio/virtio.c | 21 ++++++++++++++++----- > > > include/hw/virtio/virtio.h | 1 + > > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > > Also, whether to stop the device, or the VM, or just warn, > > seems like a policy decision. Why not set it on command line > > like we do for other storage? > > > > Huh? This patch simply introduces a new API to a feature that underwent > several rounds of discussion and reached a reasonable consensus (even > your R-b). > > I'm not sure this 9pfs series is the right place to talk about all the > behavior changes you're suggesting for virtio_error()... I'd rather > drop this patch and duplicate code in virtio-9p instead if I want the > fixes to go to 2.9. I agree that we should discuss this outside of this patch series. It's not like it is introducing a new error case. > > Cc'ing Connie and Stefanha for insights. See my reply to Michael's mail.
On Tue, 28 Mar 2017 10:24:21 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 28 Mar 2017 10:14:09 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Mon, 27 Mar 2017 21:20:56 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote: > > > > This introduces an Error object based implementation of virtio_error(). It > > > > allows to implement virtio_error() wrappers in device-specific code. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/virtio/virtio.c | 21 ++++++++++++++++----- > > > > include/hw/virtio/virtio.h | 1 + > > > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > > > > > Also, whether to stop the device, or the VM, or just warn, > > > seems like a policy decision. Why not set it on command line > > > like we do for other storage? > > > > > > > Huh? This patch simply introduces a new API to a feature that underwent > > several rounds of discussion and reached a reasonable consensus (even > > your R-b). > > > > I'm not sure this 9pfs series is the right place to talk about all the > > behavior changes you're suggesting for virtio_error()... I'd rather > > drop this patch and duplicate code in virtio-9p instead if I want the > > fixes to go to 2.9. > > I agree that we should discuss this outside of this patch series. It's > not like it is introducing a new error case. > > > > > Cc'ing Connie and Stefanha for insights. > > See my reply to Michael's mail. > Yeah, I saw that just after pressing the send button :) The points raised by Michael make a lot of sense anyway. Maybe we can discuss them for 2.10 ? Cheers. -- Greg
On Tue, 28 Mar 2017 11:34:15 +0200 Greg Kurz <groug@kaod.org> wrote: > On Tue, 28 Mar 2017 10:24:21 +0200 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > On Tue, 28 Mar 2017 10:14:09 +0200 > > Greg Kurz <groug@kaod.org> wrote: > > > > > On Mon, 27 Mar 2017 21:20:56 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote: > > > > > This introduces an Error object based implementation of virtio_error(). It > > > > > allows to implement virtio_error() wrappers in device-specific code. > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > --- > > > > > hw/virtio/virtio.c | 21 ++++++++++++++++----- > > > > > include/hw/virtio/virtio.h | 1 + > > > > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > > > > > > > > Also, whether to stop the device, or the VM, or just warn, > > > > seems like a policy decision. Why not set it on command line > > > > like we do for other storage? > > > > > > > > > > Huh? This patch simply introduces a new API to a feature that underwent > > > several rounds of discussion and reached a reasonable consensus (even > > > your R-b). > > > > > > I'm not sure this 9pfs series is the right place to talk about all the > > > behavior changes you're suggesting for virtio_error()... I'd rather > > > drop this patch and duplicate code in virtio-9p instead if I want the > > > fixes to go to 2.9. > > > > I agree that we should discuss this outside of this patch series. It's > > not like it is introducing a new error case. > > > > > > > > Cc'ing Connie and Stefanha for insights. > > > > See my reply to Michael's mail. > > > > Yeah, I saw that just after pressing the send button :) :) > > The points raised by Michael make a lot of sense anyway. Maybe we can > discuss them for 2.10 ? Certainly. We should not delay any fixes for 2.9, though.
On Tue, 28 Mar 2017 12:14:59 +0200 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 28 Mar 2017 11:34:15 +0200 > Greg Kurz <groug@kaod.org> wrote: > > > On Tue, 28 Mar 2017 10:24:21 +0200 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > > > On Tue, 28 Mar 2017 10:14:09 +0200 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > > > On Mon, 27 Mar 2017 21:20:56 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Mon, Mar 27, 2017 at 07:46:03PM +0200, Greg Kurz wrote: > > > > > > This introduces an Error object based implementation of virtio_error(). It > > > > > > allows to implement virtio_error() wrappers in device-specific code. > > > > > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > > > --- > > > > > > hw/virtio/virtio.c | 21 ++++++++++++++++----- > > > > > > include/hw/virtio/virtio.h | 1 + > > > > > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > Also, whether to stop the device, or the VM, or just warn, > > > > > seems like a policy decision. Why not set it on command line > > > > > like we do for other storage? > > > > > > > > > > > > > Huh? This patch simply introduces a new API to a feature that underwent > > > > several rounds of discussion and reached a reasonable consensus (even > > > > your R-b). > > > > > > > > I'm not sure this 9pfs series is the right place to talk about all the > > > > behavior changes you're suggesting for virtio_error()... I'd rather > > > > drop this patch and duplicate code in virtio-9p instead if I want the > > > > fixes to go to 2.9. > > > > > > I agree that we should discuss this outside of this patch series. It's > > > not like it is introducing a new error case. > > > > > > > > > > > Cc'ing Connie and Stefanha for insights. > > > > > > See my reply to Michael's mail. > > > > > > > Yeah, I saw that just after pressing the send button :) > > :) > > > > > The points raised by Michael make a lot of sense anyway. Maybe we can > > discuss them for 2.10 ? > > Certainly. We should not delay any fixes for 2.9, though. > Michael, Your comments call for some more discussion to take place during 2.10 devel. Can you please ack this patch and I'll take it through my tree for 2.9 ? Cheers. -- Greg
© 2016 - 2026 Red Hat, Inc.