From: Peter Xu <peterx@redhat.com> To: Alex Williamson <alex.williamson@redhat.com> Cc: eric.auger@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org Subject: Re: [PATCH v2 4/4] vfio/quirks: Enable ioeventfd quirks to be handled by vfio directly Date: Fri, 4 May 2018 10:16:36 +0800 [thread overview] Message-ID: <20180504021635.GH29580@xz-mi> (raw) In-Reply-To: <20180503102942.7fcc1211@w520.home> On Thu, May 03, 2018 at 10:29:42AM -0600, Alex Williamson wrote: > On Thu, 3 May 2018 12:56:03 +0800 > Peter Xu <peterx@redhat.com> wrote: > > > On Tue, May 01, 2018 at 10:43:46AM -0600, Alex Williamson wrote: > > > > [...] > > > > > -static void vfio_ioeventfd_exit(VFIOIOEventFD *ioeventfd) > > > +static void vfio_ioeventfd_exit(VFIOPCIDevice *vdev, VFIOIOEventFD *ioeventfd) > > > { > > > QLIST_REMOVE(ioeventfd, next); > > > + > > > memory_region_del_eventfd(ioeventfd->mr, ioeventfd->addr, ioeventfd->size, > > > ioeventfd->match_data, ioeventfd->data, > > > &ioeventfd->e); > > > - qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), NULL, NULL, NULL); > > > + > > > + if (ioeventfd->vfio) { > > > + struct vfio_device_ioeventfd vfio_ioeventfd; > > > + > > > + vfio_ioeventfd.argsz = sizeof(vfio_ioeventfd); > > > + vfio_ioeventfd.flags = ioeventfd->size; > > > + vfio_ioeventfd.data = ioeventfd->data; > > > + vfio_ioeventfd.offset = ioeventfd->region->fd_offset + > > > + ioeventfd->region_addr; > > > + vfio_ioeventfd.fd = -1; > > > + > > > + ioctl(vdev->vbasedev.fd, VFIO_DEVICE_IOEVENTFD, &vfio_ioeventfd); > > > > (If the series is going to respin, I am thinking whether it would > > worth it to capture this error to dump something. But it's for sure > > optional since even error happened we should have something in dmesg > > so it only matters on whether we also want something to be dumped > > from QEMU side too... After all there aren't much we can do) > > I'm torn whether to use QEMU standard error handling here, ie. > abort(). If we failed to remove the KVM ioeventfd, we'd abort before > we get here, so there's no chance that the vfio ioeventfd will continue > to be triggered. Obviously leaving a vfio ioeventfd that we can't > trigger and might interfere with future ioeventfds is not good, but do > we really want to kill the VM because we possibly can't add an > accelerator here later? I'm inclined to say no, so I think I'll just > error_report() unless there are objections. I must be misleading when I said "dump something"... :) Yes the error_report is exactly what I meant. (Even an "error_report_once" but we don't have that yet) > > > > + > > > + } else { > > > + qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), > > > + NULL, NULL, NULL); > > > + } > > > + > > > event_notifier_cleanup(&ioeventfd->e); > > > trace_vfio_ioeventfd_exit(memory_region_name(ioeventfd->mr), > > > (uint64_t)ioeventfd->addr, ioeventfd->size, > > > > [...] > > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > > index ba1239551115..84e27c7bb2d1 100644 > > > --- a/hw/vfio/pci.c > > > +++ b/hw/vfio/pci.c > > > @@ -3177,6 +3177,8 @@ static Property vfio_pci_dev_properties[] = { > > > no_geforce_quirks, false), > > > DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", VFIOPCIDevice, no_kvm_ioeventfd, > > > false), > > > + DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", VFIOPCIDevice, no_vfio_ioeventfd, > > > + false), > > > > Here it looks more like a tri-state for me, so we can either: > > > > - disable the acceleration in general, or > > - enable QEMU-side acceleration only, or > > - enable kernel-side acceleration > > So you're looking for a Auto/Off/KVM-only option? Do you really think > it's worth defining a new tristate property for this sort of debugging > option... > > > In other words, IIUC x-no-vfio-ioeventfd won't matter much if > > x-no-kvm-ioeventfd is already set. So not sure whether a single > > parameter would be nicer. > > That's correct, but who do we expect to be using this option and why? > I added enum OffAutoPCIBAR and the property to enable it for MSI-x > relocation because it is an option that a normal user might reasonably > need to use, given the right hardware on the right host, but it's an > unsupported option because we cannot programatically validate it. > Support rests with the individual user, if it doesn't work, don't use > it, if it helps, great. Here we have options that are really only for > debugging, to test whether something has gone wrong in this code, > disable this bypass to make all device interactions visible through > QEMU, or specifically to evaluate the performance of this path. Is it > reasonable to impose yet another property type on the common code for > this use case when a couple bools work just fine, if perhaps not > absolutely ideal? Am I overlooking an existing tri-state that might be > a reasonable match? Oh so it's only for debugging. Then I would be perfectly fine with two parameters. Actually I wasn't thinking about any tri-state property, I was thinking about e.g. string-typed that can satisfy things like tri-state. But again now I don't think it'll worth it to repost with that if only for debugging purpose. Thanks! -- Peter Xu
WARNING: multiple messages have this Message-ID (diff)
From: Peter Xu <peterx@redhat.com> To: Alex Williamson <alex.williamson@redhat.com> Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org, eric.auger@redhat.com Subject: Re: [Qemu-devel] [PATCH v2 4/4] vfio/quirks: Enable ioeventfd quirks to be handled by vfio directly Date: Fri, 4 May 2018 10:16:36 +0800 [thread overview] Message-ID: <20180504021635.GH29580@xz-mi> (raw) In-Reply-To: <20180503102942.7fcc1211@w520.home> On Thu, May 03, 2018 at 10:29:42AM -0600, Alex Williamson wrote: > On Thu, 3 May 2018 12:56:03 +0800 > Peter Xu <peterx@redhat.com> wrote: > > > On Tue, May 01, 2018 at 10:43:46AM -0600, Alex Williamson wrote: > > > > [...] > > > > > -static void vfio_ioeventfd_exit(VFIOIOEventFD *ioeventfd) > > > +static void vfio_ioeventfd_exit(VFIOPCIDevice *vdev, VFIOIOEventFD *ioeventfd) > > > { > > > QLIST_REMOVE(ioeventfd, next); > > > + > > > memory_region_del_eventfd(ioeventfd->mr, ioeventfd->addr, ioeventfd->size, > > > ioeventfd->match_data, ioeventfd->data, > > > &ioeventfd->e); > > > - qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), NULL, NULL, NULL); > > > + > > > + if (ioeventfd->vfio) { > > > + struct vfio_device_ioeventfd vfio_ioeventfd; > > > + > > > + vfio_ioeventfd.argsz = sizeof(vfio_ioeventfd); > > > + vfio_ioeventfd.flags = ioeventfd->size; > > > + vfio_ioeventfd.data = ioeventfd->data; > > > + vfio_ioeventfd.offset = ioeventfd->region->fd_offset + > > > + ioeventfd->region_addr; > > > + vfio_ioeventfd.fd = -1; > > > + > > > + ioctl(vdev->vbasedev.fd, VFIO_DEVICE_IOEVENTFD, &vfio_ioeventfd); > > > > (If the series is going to respin, I am thinking whether it would > > worth it to capture this error to dump something. But it's for sure > > optional since even error happened we should have something in dmesg > > so it only matters on whether we also want something to be dumped > > from QEMU side too... After all there aren't much we can do) > > I'm torn whether to use QEMU standard error handling here, ie. > abort(). If we failed to remove the KVM ioeventfd, we'd abort before > we get here, so there's no chance that the vfio ioeventfd will continue > to be triggered. Obviously leaving a vfio ioeventfd that we can't > trigger and might interfere with future ioeventfds is not good, but do > we really want to kill the VM because we possibly can't add an > accelerator here later? I'm inclined to say no, so I think I'll just > error_report() unless there are objections. I must be misleading when I said "dump something"... :) Yes the error_report is exactly what I meant. (Even an "error_report_once" but we don't have that yet) > > > > + > > > + } else { > > > + qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), > > > + NULL, NULL, NULL); > > > + } > > > + > > > event_notifier_cleanup(&ioeventfd->e); > > > trace_vfio_ioeventfd_exit(memory_region_name(ioeventfd->mr), > > > (uint64_t)ioeventfd->addr, ioeventfd->size, > > > > [...] > > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > > index ba1239551115..84e27c7bb2d1 100644 > > > --- a/hw/vfio/pci.c > > > +++ b/hw/vfio/pci.c > > > @@ -3177,6 +3177,8 @@ static Property vfio_pci_dev_properties[] = { > > > no_geforce_quirks, false), > > > DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", VFIOPCIDevice, no_kvm_ioeventfd, > > > false), > > > + DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", VFIOPCIDevice, no_vfio_ioeventfd, > > > + false), > > > > Here it looks more like a tri-state for me, so we can either: > > > > - disable the acceleration in general, or > > - enable QEMU-side acceleration only, or > > - enable kernel-side acceleration > > So you're looking for a Auto/Off/KVM-only option? Do you really think > it's worth defining a new tristate property for this sort of debugging > option... > > > In other words, IIUC x-no-vfio-ioeventfd won't matter much if > > x-no-kvm-ioeventfd is already set. So not sure whether a single > > parameter would be nicer. > > That's correct, but who do we expect to be using this option and why? > I added enum OffAutoPCIBAR and the property to enable it for MSI-x > relocation because it is an option that a normal user might reasonably > need to use, given the right hardware on the right host, but it's an > unsupported option because we cannot programatically validate it. > Support rests with the individual user, if it doesn't work, don't use > it, if it helps, great. Here we have options that are really only for > debugging, to test whether something has gone wrong in this code, > disable this bypass to make all device interactions visible through > QEMU, or specifically to evaluate the performance of this path. Is it > reasonable to impose yet another property type on the common code for > this use case when a couple bools work just fine, if perhaps not > absolutely ideal? Am I overlooking an existing tri-state that might be > a reasonable match? Oh so it's only for debugging. Then I would be perfectly fine with two parameters. Actually I wasn't thinking about any tri-state property, I was thinking about e.g. string-typed that can satisfy things like tri-state. But again now I don't think it'll worth it to repost with that if only for debugging purpose. Thanks! -- Peter Xu
next prev parent reply other threads:[~2018-05-04 2:16 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-01 16:42 [PATCH v2 0/4] vfio/quirks: ioeventfd support Alex Williamson 2018-05-01 16:42 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:43 ` [PATCH v2 1/4] vfio/quirks: Add common quirk alloc helper Alex Williamson 2018-05-01 16:43 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:43 ` [PATCH v2 2/4] vfio/quirks: Add quirk reset callback Alex Williamson 2018-05-01 16:43 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:43 ` [PATCH v2 3/4] vfio/quirks: ioeventfd quirk acceleration Alex Williamson 2018-05-01 16:43 ` [Qemu-devel] " Alex Williamson 2018-05-03 3:36 ` Peter Xu 2018-05-03 3:36 ` [Qemu-devel] " Peter Xu 2018-05-03 4:20 ` Alex Williamson 2018-05-03 4:20 ` [Qemu-devel] " Alex Williamson 2018-05-03 14:33 ` Auger Eric 2018-05-03 14:33 ` [Qemu-devel] " Auger Eric 2018-05-03 14:48 ` Alex Williamson 2018-05-03 14:48 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:43 ` [PATCH v2 4/4] vfio/quirks: Enable ioeventfd quirks to be handled by vfio directly Alex Williamson 2018-05-01 16:43 ` [Qemu-devel] " Alex Williamson 2018-05-03 4:56 ` Peter Xu 2018-05-03 4:56 ` [Qemu-devel] " Peter Xu 2018-05-03 16:29 ` Alex Williamson 2018-05-03 16:29 ` [Qemu-devel] " Alex Williamson 2018-05-04 2:16 ` Peter Xu [this message] 2018-05-04 2:16 ` Peter Xu 2018-05-04 7:38 ` Auger Eric 2018-05-04 7:38 ` [Qemu-devel] " Auger Eric 2018-05-03 15:20 ` Auger Eric 2018-05-03 15:20 ` [Qemu-devel] " Auger Eric 2018-05-03 16:30 ` Alex Williamson 2018-05-03 16:30 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:56 ` [PATCH v2 0/4] vfio/quirks: ioeventfd support no-reply 2018-05-01 16:56 ` [Qemu-devel] " no-reply 2018-05-01 17:05 ` Alex Williamson 2018-05-01 17:05 ` [Qemu-devel] " Alex Williamson 2018-05-01 16:56 ` no-reply 2018-05-01 16:56 ` [Qemu-devel] " no-reply
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180504021635.GH29580@xz-mi \ --to=peterx@redhat.com \ --cc=alex.williamson@redhat.com \ --cc=eric.auger@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.