[Qemu-devel] [PATCH] vfio: allow to disable MMAP per device with -x-mmap=off option

[Qemu-devel] [PATCH] vfio: allow to disable MMAP per device with -x-mmap=off option

[Samuel Pitoiset]

Disabling MMAP support uses the slower read/write accesses but allows to
trace all MMIO accesses, which is not good for performance, but very
useful for reverse engineering PCI drivers. This option allows to
disable MMAP per device without a compile-time change.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 hw/vfio/common.c              | 2 +-
 hw/vfio/pci.c                 | 1 +
 include/hw/vfio/vfio-common.h | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index c5d1551..bb87974 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -493,7 +493,7 @@ int vfio_mmap_region(Object *obj, VFIORegion *region,
     int ret = 0;
     VFIODevice *vbasedev = region->vbasedev;
 
-    if (VFIO_ALLOW_MMAP && size && region->flags &
+    if (vbasedev->allow_mmap && size && region->flags &
         VFIO_REGION_INFO_FLAG_MMAP) {
         int prot = 0;
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 84e9d99..3c71de3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3456,6 +3456,7 @@ static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_VGA_BIT, false),
     DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
+    DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
     /*
      * TODO - support passed fds... is this necessary?
      * DEFINE_PROP_STRING("vfiofd", VFIOPCIDevice, vfiofd_name),
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 5f3679b..be860ca 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -36,7 +36,6 @@
 #endif
 
 /* Extra debugging, trap acceleration paths for more logging */
-#define VFIO_ALLOW_MMAP 1
 #define VFIO_ALLOW_KVM_INTX 1
 #define VFIO_ALLOW_KVM_MSI 1
 #define VFIO_ALLOW_KVM_MSIX 1
@@ -102,6 +101,7 @@ typedef struct VFIODevice {
     int type;
     bool reset_works;
     bool needs_reset;
+    bool allow_mmap;
     VFIODeviceOps *ops;
     unsigned int num_irqs;
     unsigned int num_regions;
-- 
2.3.0

Re: [Qemu-devel] [PATCH] vfio: allow to disable MMAP per device with -x-mmap=off option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On 02/24/2015 01:38 PM, Samuel Pitoiset wrote:
> Disabling MMAP support uses the slower read/write accesses but allows to
> trace all MMIO accesses, which is not good for performance, but very
> useful for reverse engineering PCI drivers. This option allows to
> disable MMAP per device without a compile-time change.
> 
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  hw/vfio/common.c              | 2 +-
>  hw/vfio/pci.c                 | 1 +
>  include/hw/vfio/vfio-common.h | 2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)

> +++ b/hw/vfio/pci.c
> @@ -3456,6 +3456,7 @@ static Property vfio_pci_dev_properties[] = {
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
>      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> +    DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),

Naming it 'x-mmap' implies it is experimental and may be removed
someday.  Is there any reason why you are not proposing it as a
permanent knob?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] vfio: allow to disable MMAP per device with -x-mmap=off option

[Alex Williamson]

On Tue, 2015-02-24 at 14:40 -0700, Eric Blake wrote:
> On 02/24/2015 01:38 PM, Samuel Pitoiset wrote:
> > Disabling MMAP support uses the slower read/write accesses but allows to
> > trace all MMIO accesses, which is not good for performance, but very
> > useful for reverse engineering PCI drivers. This option allows to
> > disable MMAP per device without a compile-time change.
> > 
> > Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> > ---
> >  hw/vfio/common.c              | 2 +-
> >  hw/vfio/pci.c                 | 1 +
> >  include/hw/vfio/vfio-common.h | 2 +-
> >  3 files changed, 3 insertions(+), 2 deletions(-)
> 
> > +++ b/hw/vfio/pci.c
> > @@ -3456,6 +3456,7 @@ static Property vfio_pci_dev_properties[] = {
> >      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> >                      VFIO_FEATURE_ENABLE_VGA_BIT, false),
> >      DEFINE_PROP_INT32("bootindex", VFIOPCIDevice, bootindex, -1),
> > +    DEFINE_PROP_BOOL("x-mmap", VFIOPCIDevice, vbasedev.allow_mmap, true),
> 
> Naming it 'x-mmap' implies it is experimental and may be removed
> someday.  Is there any reason why you are not proposing it as a
> permanent knob?

I actually suggested the x- prefix to Samuel because I don't think this
is a mode that we want to officially support.  It's useful for enabling
tracing, but it's possible that doing this will break timing sensitive
devices.  I therefore don't really want to put it at the fingertips of
the average user.  If we have a reason to call it supported at some
point later, we can always change it.  Thanks,

Alex

Re: [Qemu-devel] [PATCH] vfio: allow to disable MMAP per device with -x-mmap=off option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On 02/24/2015 02:53 PM, Alex Williamson wrote:

>>
>> Naming it 'x-mmap' implies it is experimental and may be removed
>> someday.  Is there any reason why you are not proposing it as a
>> permanent knob?
> 
> I actually suggested the x- prefix to Samuel because I don't think this
> is a mode that we want to officially support.  It's useful for enabling
> tracing, but it's possible that doing this will break timing sensitive
> devices.  I therefore don't really want to put it at the fingertips of
> the average user.

Works for me :)

>  If we have a reason to call it supported at some
> point later, we can always change it.  Thanks,

Yep, that's always an option.  Easier to make something stable later
when it proved worthwhile, than it is to regret making it stable now.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]