| Message ID | 1473416072-7063-3-git-send-email-maxime.coquelin@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 9 Sep 2016 12:14:32 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > This patch makes pci devices plugging more robust, by not confusing > guest with modern interface when the backend doesn't support > VIRTIO_F_VERSION_1. > > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: qemu-stable@nongnu.org > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > hw/virtio/virtio-pci.c | 15 +++++++++++++++ > hw/virtio/virtio-pci.h | 5 +++++ > 2 files changed, 20 insertions(+) Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for ccw") fixes this issue for ccw via the introduction of a ->post_plugged() callback. Unfortunately, we did not find a good way to make it work for pci back then. Two comments: - ->post_plugged() handles the dependencies (as noticed for your first patch) - and this is due to it being called after the plugging is already done. - I don't really like pci and ccw being too different. We have probably more flexibility with the handling for ccw, so I could probably convert ccw to the same mechanism that pci uses. Maybe there are other uses for ->post_plugged()?
On 09/09/2016 01:40 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 12:14:32 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> This patch makes pci devices plugging more robust, by not confusing >> guest with modern interface when the backend doesn't support >> VIRTIO_F_VERSION_1. >> >> Cc: Marcel Apfelbaum <marcel@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> hw/virtio/virtio-pci.c | 15 +++++++++++++++ >> hw/virtio/virtio-pci.h | 5 +++++ >> 2 files changed, 20 insertions(+) > > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > ccw") fixes this issue for ccw via the introduction of a > ->post_plugged() callback. Unfortunately, we did not find a good way to > make it work for pci back then. It seems that for ccw is enough to rewind dev->rev_max, sadly for pci we need to rewind a lot of settings/resources. Thanks, Marcel > > Two comments: > - ->post_plugged() handles the dependencies (as noticed for your first > patch) - and this is due to it being called after the plugging is > already done. > - I don't really like pci and ccw being too different. We have probably > more flexibility with the handling for ccw, so I could probably convert > ccw to the same mechanism that pci uses. Maybe there are other uses for > ->post_plugged()? >
On Fri, 9 Sep 2016 14:04:55 +0300 Marcel Apfelbaum <marcel@redhat.com> wrote: > On 09/09/2016 01:40 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 12:14:32 +0200 > > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > > > >> This patch makes pci devices plugging more robust, by not confusing > >> guest with modern interface when the backend doesn't support > >> VIRTIO_F_VERSION_1. > >> > >> Cc: Marcel Apfelbaum <marcel@redhat.com> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Cc: qemu-stable@nongnu.org > >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >> --- > >> hw/virtio/virtio-pci.c | 15 +++++++++++++++ > >> hw/virtio/virtio-pci.h | 5 +++++ > >> 2 files changed, 20 insertions(+) > > > > Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > > ccw") fixes this issue for ccw via the introduction of a > > ->post_plugged() callback. Unfortunately, we did not find a good way to > > make it work for pci back then. > > It seems that for ccw is enough to rewind dev->rev_max, > sadly for pci we need to rewind a lot of settings/resources. Yes, that what I meant with 'more flexibility for ccw'. > > Thanks, > Marcel > > > > > Two comments: > > - ->post_plugged() handles the dependencies (as noticed for your first > > patch) - and this is due to it being called after the plugging is > > already done. > > - I don't really like pci and ccw being too different. We have probably > > more flexibility with the handling for ccw, so I could probably convert > > ccw to the same mechanism that pci uses. Maybe there are other uses for > > ->post_plugged()? > > >
On 09/09/2016 01:20 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 14:04:55 +0300 > Marcel Apfelbaum <marcel@redhat.com> wrote: > >> On 09/09/2016 01:40 PM, Cornelia Huck wrote: >>> On Fri, 9 Sep 2016 12:14:32 +0200 >>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: >>> >>>> This patch makes pci devices plugging more robust, by not confusing >>>> guest with modern interface when the backend doesn't support >>>> VIRTIO_F_VERSION_1. >>>> >>>> Cc: Marcel Apfelbaum <marcel@redhat.com> >>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>> Cc: qemu-stable@nongnu.org >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> --- >>>> hw/virtio/virtio-pci.c | 15 +++++++++++++++ >>>> hw/virtio/virtio-pci.h | 5 +++++ >>>> 2 files changed, 20 insertions(+) >>> >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for >>> ccw") fixes this issue for ccw via the introduction of a >>> ->post_plugged() callback. Unfortunately, we did not find a good way to >>> make it work for pci back then. >> >> It seems that for ccw is enough to rewind dev->rev_max, >> sadly for pci we need to rewind a lot of settings/resources. > > Yes, that what I meant with 'more flexibility for ccw'. Maybe we could replace post_plugged with a pre_plugged approach? In ->pre_plugged(), cww and pci would specify which features it can support using virtio_add_feature(). Then we could call get_features() before ->device_plugged(). Doing this, both ccw and pci would have the needed information without having to rewind any settings. Does that make sense? But for now, I think it would be better to merge something in the spirit of this series (taking into account to remarks). Indeed, I think we want this fixed in stable, but the above proposal would be too huge for stable. Thanks, Maxime
On Fri, 9 Sep 2016 13:44:35 +0200 Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > On 09/09/2016 01:20 PM, Cornelia Huck wrote: > > On Fri, 9 Sep 2016 14:04:55 +0300 > > Marcel Apfelbaum <marcel@redhat.com> wrote: > > > >> On 09/09/2016 01:40 PM, Cornelia Huck wrote: > >>> On Fri, 9 Sep 2016 12:14:32 +0200 > >>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >>> > >>>> This patch makes pci devices plugging more robust, by not confusing > >>>> guest with modern interface when the backend doesn't support > >>>> VIRTIO_F_VERSION_1. > >>>> > >>>> Cc: Marcel Apfelbaum <marcel@redhat.com> > >>>> Cc: Michael S. Tsirkin <mst@redhat.com> > >>>> Cc: qemu-stable@nongnu.org > >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > >>>> --- > >>>> hw/virtio/virtio-pci.c | 15 +++++++++++++++ > >>>> hw/virtio/virtio-pci.h | 5 +++++ > >>>> 2 files changed, 20 insertions(+) > >>> > >>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for > >>> ccw") fixes this issue for ccw via the introduction of a > >>> ->post_plugged() callback. Unfortunately, we did not find a good way to > >>> make it work for pci back then. > >> > >> It seems that for ccw is enough to rewind dev->rev_max, > >> sadly for pci we need to rewind a lot of settings/resources. > > > > Yes, that what I meant with 'more flexibility for ccw'. > Maybe we could replace post_plugged with a pre_plugged approach? > > In ->pre_plugged(), cww and pci would specify which features it can > support using virtio_add_feature(). > Then we could call get_features() before ->device_plugged(). I think that would work for ccw (haven't looked at pci). > > Doing this, both ccw and pci would have the needed information without > having to rewind any settings. > > Does that make sense? > > But for now, I think it would be better to merge something in the spirit > of this series (taking into account to remarks). > Indeed, I think we want this fixed in stable, but the above proposal > would be too huge for stable. A 'just check for VERSION_1' approach would probably be best for stable.
On 09/09/2016 01:49 PM, Cornelia Huck wrote: > On Fri, 9 Sep 2016 13:44:35 +0200 > Maxime Coquelin <maxime.coquelin@redhat.com> wrote: > >> On 09/09/2016 01:20 PM, Cornelia Huck wrote: >>> On Fri, 9 Sep 2016 14:04:55 +0300 >>> Marcel Apfelbaum <marcel@redhat.com> wrote: >>> >>>> On 09/09/2016 01:40 PM, Cornelia Huck wrote: >>>>> On Fri, 9 Sep 2016 12:14:32 +0200 >>>>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote: >>>>> >>>>>> This patch makes pci devices plugging more robust, by not confusing >>>>>> guest with modern interface when the backend doesn't support >>>>>> VIRTIO_F_VERSION_1. >>>>>> >>>>>> Cc: Marcel Apfelbaum <marcel@redhat.com> >>>>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>>>> Cc: qemu-stable@nongnu.org >>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>>>> --- >>>>>> hw/virtio/virtio-pci.c | 15 +++++++++++++++ >>>>>> hw/virtio/virtio-pci.h | 5 +++++ >>>>>> 2 files changed, 20 insertions(+) >>>>> >>>>> Note that 11380b361 ("virtio: handle non-virtio-1-capable backend for >>>>> ccw") fixes this issue for ccw via the introduction of a >>>>> ->post_plugged() callback. Unfortunately, we did not find a good way to >>>>> make it work for pci back then. >>>> >>>> It seems that for ccw is enough to rewind dev->rev_max, >>>> sadly for pci we need to rewind a lot of settings/resources. >>> >>> Yes, that what I meant with 'more flexibility for ccw'. >> Maybe we could replace post_plugged with a pre_plugged approach? >> >> In ->pre_plugged(), cww and pci would specify which features it can >> support using virtio_add_feature(). >> Then we could call get_features() before ->device_plugged(). > > I think that would work for ccw (haven't looked at pci). Good, once quick fix accepted, I'll try this solution. > >> >> Doing this, both ccw and pci would have the needed information without >> having to rewind any settings. >> >> Does that make sense? >> >> But for now, I think it would be better to merge something in the spirit >> of this series (taking into account to remarks). >> Indeed, I think we want this fixed in stable, but the above proposal >> would be too huge for stable. > > A 'just check for VERSION_1' approach would probably be best for stable. > Ok, thanks. I will send a v2 replacing the generic function with a VERISON_1 specfic: bool virtio_test_backend_virtio_1(VirtIODevice *vdev, Error **errp); Maxime
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 755f921..0e5d59c 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1581,6 +1581,21 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp) uint32_t size; VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + /* + * Virtio capabilities present without + * VIRTIO_F_VERSION_1 confuses guests + */ + if (!virtio_test_backend_feature(vdev, VIRTIO_F_VERSION_1, errp)) { + virtio_pci_disable_modern(proxy); + } + + legacy = virtio_pci_legacy(proxy); + modern = virtio_pci_modern(proxy); + if (!legacy && !modern) { + error_setg(errp, "PCI device is neither legacy nor modern."); + return; + } + config = proxy->pci_dev.config; if (proxy->class_code) { pci_config_set_class(config, proxy->class_code); diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index 25fbf8a..4e976b3 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -172,6 +172,11 @@ static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy) proxy->disable_legacy = ON_OFF_AUTO_ON; } +static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy) +{ + proxy->disable_modern = true; +} + /* * virtio-scsi-pci: This extends VirtioPCIProxy. */
This patch makes pci devices plugging more robust, by not confusing guest with modern interface when the backend doesn't support VIRTIO_F_VERSION_1. Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: qemu-stable@nongnu.org Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- hw/virtio/virtio-pci.c | 15 +++++++++++++++ hw/virtio/virtio-pci.h | 5 +++++ 2 files changed, 20 insertions(+)