Re: [PATCH v4 kvmtool 06/12] vfio/pci: Don't write configuration value twice

["André Przywara" <andre.przywara@arm.com>]

On 14/05/2020 16:38, Alexandru Elisei wrote:
> After writing to the device fd as part of the PCI configuration space
> emulation, we read back from the device to make sure that the write
> finished. The value is read back into the PCI configuration space and
> afterwards, the same value is copied by the PCI emulation code. Let's
> read from the device fd into a temporary variable, to prevent this
> double write.
> 
> The double write is harmless in itself. But when we implement
> reassignable BARs, we need to keep track of the old BAR value, and the
> VFIO code is overwritting it.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks for the changes!

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  vfio/pci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index 0b548e4bf9e2..2de893407574 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -3,6 +3,8 @@
>  #include "kvm/kvm-cpu.h"
>  #include "kvm/vfio.h"
>  
> +#include <assert.h>
> +
>  #include <sys/ioctl.h>
>  #include <sys/eventfd.h>
>  #include <sys/resource.h>
> @@ -478,7 +480,10 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
>  	struct vfio_region_info *info;
>  	struct vfio_pci_device *pdev;
>  	struct vfio_device *vdev;
> -	void *base = pci_hdr;
> +	u32 tmp;
> +
> +	/* Make sure a larger size will not overrun tmp on the stack. */
> +	assert(sz <= 4);
>  
>  	if (offset == PCI_ROM_ADDRESS)
>  		return;
> @@ -498,7 +503,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
>  	if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI)
>  		vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz);
>  
> -	if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz)
> +	if (pread(vdev->fd, &tmp, sz, info->offset + offset) != sz)
>  		vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x",
>  			      sz, offset);
>  }
>