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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> Signed-off-by: Xin Zeng <address@hidden>
> ---
>   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