Re: [PATCH v3 kvmtool 12/32] vfio/pci: Ignore expansion ROM BAR writes

From: "André Przywara" <andre.przywara@arm.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>, kvm@vger.kernel.org
Cc: will@kernel.org, julien.thierry.kdev@gmail.com,
	sami.mujawar@arm.com, lorenzo.pieralisi@arm.com
Subject: Re: [PATCH v3 kvmtool 12/32] vfio/pci: Ignore expansion ROM BAR writes
Date: Mon, 30 Mar 2020 10:29:51 +0100	[thread overview]
Message-ID: <751b91a9-dfe2-fb9f-8485-dac9e9a62cd3@arm.com> (raw)
In-Reply-To: <20200326152438.6218-13-alexandru.elisei@arm.com>

On 26/03/2020 15:24, Alexandru Elisei wrote:
> To get the size of the expansion ROM, software writes 0xfffff800 to the
> expansion ROM BAR in the PCI configuration space. PCI emulation executes
> the optional configuration space write callback that a device can implement
> before emulating this write.
> 
> kvmtool's implementation of VFIO doesn't have support for emulating
> expansion ROMs. However, the callback writes the guest value to the
> hardware BAR, and then it reads it back to the emulated BAR to make sure
> the write has completed successfully.
> 
> After this, we return to regular PCI emulation and because the BAR is no
> longer 0, we write back to the BAR the value that the guest used to get the
> size. As a result, the guest will think that the ROM size is 0x800 after
> the subsequent read and we end up unintentionally exposing to the guest a
> BAR which we don't emulate.
> 
> Let's fix this by ignoring writes to the expansion ROM BAR.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

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

Thanks,
Andre

> ---
>  vfio/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/vfio/pci.c b/vfio/pci.c
> index 1bdc20038411..1f38f90c3ae9 100644
> --- a/vfio/pci.c
> +++ b/vfio/pci.c
> @@ -472,6 +472,9 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd
>  	struct vfio_device *vdev;
>  	void *base = pci_hdr;
>  
> +	if (offset == PCI_ROM_ADDRESS)
> +		return;
> +
>  	pdev = container_of(pci_hdr, struct vfio_pci_device, hdr);
>  	vdev = container_of(pdev, struct vfio_device, pci);
>  	info = &vdev->regions[VFIO_PCI_CONFIG_REGION_INDEX].info;
>