diff mbox series

[v3,2/3] hw/vfio: drop guest writes to ro regions

Message ID 20200413063737.84706-1-yan.y.zhao@intel.com (mailing list archive)
State New, archived
Headers show
Series drop writes to read-only ram device & vfio regions | expand

Commit Message

Yan Zhao April 13, 2020, 6:37 a.m. UTC 
for vfio regions that are without write permission,
drop guest writes to those regions.

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Xin Zeng <xin.zeng@intel.com>
---
 hw/vfio/common.c     | 8 +++++++-
 hw/vfio/trace-events | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé April 14, 2020, 9:34 a.m. UTC | #1 
Hi Yan,

On 4/13/20 8:37 AM, Yan Zhao wrote:
> for vfio regions that are without write permission,
> drop guest writes to those regions.
> 
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> ---
>   hw/vfio/common.c     | 8 +++++++-
>   hw/vfio/trace-events | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0b3593b3c0..fd6ee1fe3e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
>           uint64_t qword;
>       } buf;
>   

I'd move the trace here (trace always):

        trace_vfio_region_write(vbasedev->name, region->nr, addr, data, 
size);

> +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> +        trace_vfio_region_write(vbasedev->name, region->nr,
> +                                   addr, data, size, true);

And use qemu_log_mask(LOG_GUEST_ERROR, ...) here instead.

> +        return;
> +    }
>       switch (size) {
>       case 1:
>           buf.byte = data;
> @@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
>                        addr, data, size);
>       }
>   
> -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
> +                            false);
>   
>       /*
>        * A read or write to a BAR always signals an INTx EOI.  This will
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index b1ef55a33f..fb9ff604e6 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
>   vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
>   
>   # common.c
> -vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
> +vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
>   vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
>   vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
>   vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
>
Yan Zhao April 15, 2020, 8:19 a.m. UTC | #2 
On Tue, Apr 14, 2020 at 05:34:29PM +0800, Philippe Mathieu-Daudé wrote:
> Hi Yan,
> 
> On 4/13/20 8:37 AM, Yan Zhao wrote:
> > for vfio regions that are without write permission,
> > drop guest writes to those regions.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> >   hw/vfio/common.c     | 8 +++++++-
> >   hw/vfio/trace-events | 2 +-
> >   2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 0b3593b3c0..fd6ee1fe3e 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -190,6 +190,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >           uint64_t qword;
> >       } buf;
> >   
> 
> I'd move the trace here (trace always):
> 
>         trace_vfio_region_write(vbasedev->name, region->nr, addr, data, 
> size);
> 
> > +    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
> > +        trace_vfio_region_write(vbasedev->name, region->nr,
> > +                                   addr, data, size, true);
> 
> And use qemu_log_mask(LOG_GUEST_ERROR, ...) here instead.
>

ok. will change it. Thanks!

Yan
> > +        return;
> > +    }
> >       switch (size) {
> >       case 1:
> >           buf.byte = data;
> > @@ -215,7 +220,8 @@ void vfio_region_write(void *opaque, hwaddr addr,
> >                        addr, data, size);
> >       }
> >   
> > -    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
> > +    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
> > +                            false);
> >   
> >       /*
> >        * A read or write to a BAR always signals an INTx EOI.  This will
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> > index b1ef55a33f..fb9ff604e6 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -91,7 +91,7 @@ vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
> >   vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
> >   
> >   # common.c
> > -vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
> > +vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
> >   vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
> >   vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
> >   vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64
> > 
>
diff mbox series

Patch

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 0b3593b3c0..fd6ee1fe3e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -190,6 +190,11 @@  void vfio_region_write(void *opaque, hwaddr addr,
         uint64_t qword;
     } buf;
 
+    if (!(region->flags & VFIO_REGION_INFO_FLAG_WRITE)) {
+        trace_vfio_region_write(vbasedev->name, region->nr,
+                                   addr, data, size, true);
+        return;
+    }
     switch (size) {
     case 1:
         buf.byte = data;
@@ -215,7 +220,8 @@  void vfio_region_write(void *opaque, hwaddr addr,
                      addr, data, size);
     }
 
-    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size);
+    trace_vfio_region_write(vbasedev->name, region->nr, addr, data, size,
+                            false);
 
     /*
      * A read or write to a BAR always signals an INTx EOI.  This will
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index b1ef55a33f..fb9ff604e6 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -91,7 +91,7 @@  vfio_pci_nvlink2_setup_quirk_ssatgt(const char *name, uint64_t tgt, uint64_t siz
 vfio_pci_nvlink2_setup_quirk_lnkspd(const char *name, uint32_t link_speed) "%s link_speed=0x%x"
 
 # common.c
-vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)"
+vfio_region_write(const char *name, int index, uint64_t addr, uint64_t data, unsigned size, bool readonly) " (%s:region%d+0x%"PRIx64", 0x%"PRIx64 ", %d)" " is_readonly_region=%d."
 vfio_region_read(char *name, int index, uint64_t addr, unsigned size, uint64_t data) " (%s:region%d+0x%"PRIx64", %d) = 0x%"PRIx64
 vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64
 vfio_listener_region_add_skip(uint64_t start, uint64_t end) "SKIPPING region_add 0x%"PRIx64" - 0x%"PRIx64