Re: [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom

From: Auger Eric <eric.auger@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org
Subject: Re: [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom
Date: Thu, 14 Feb 2019 19:27:15 +0100	[thread overview]
Message-ID: <015103e3-c317-22f3-e209-f5ccf890bafb@redhat.com> (raw)
In-Reply-To: <20190213105200.1deafb62@w520.home>

Hi Alex,

On 2/13/19 6:52 PM, Alex Williamson wrote:
> On Wed, 13 Feb 2019 12:06:10 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> pci_map_rom/pci_get_rom_size() performs memory access in the ROM.
>> In case the Memory Space accesses were disabled, readw() is likely to
>> crash the host with a synchronous external abort (aarch64).
> 
> As implied in response to Konrad, the likeliness really depends on the
> whole platform, not just the CPU architecture.  It's a class of
> problems that depends on OS control or error handling, which we simply
> don't have on many systems.  But we can fix this instance of it.

Agreed, I just hit this issue on one specific aarch64 machine
> 
>> In case memory accesses were disabled, re-enable them before the call
>> and disable them back again just after.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> This has been around since the beginning, but maybe a Fixes tag would
> be useful:
> 
> Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
OK
> 
>>
>> ---
>>
>> v1 -> v2:
>> - also re-enable in case of error
>> ---
>>  drivers/vfio/pci/vfio_pci.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index ff60bd1ea587..721aa55424a4 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -706,8 +706,10 @@ static long vfio_pci_ioctl(void *device_data,
>>  			break;
>>  		case VFIO_PCI_ROM_REGION_INDEX:
>>  		{
>> +			bool mem_access_disabled;
>>  			void __iomem *io;
>>  			size_t size;
>> +			u16 cmd;
>>  
>>  			info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>>  			info.flags = 0;
>> @@ -723,15 +725,28 @@ static long vfio_pci_ioctl(void *device_data,
>>  					break;
>>  			}
>>  
>> +			pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>> +			mem_access_disabled = !(cmd & PCI_COMMAND_MEMORY);
>> +			if (mem_access_disabled) {
>> +				cmd |= PCI_COMMAND_MEMORY;
>> +				pci_write_config_word(pdev, PCI_COMMAND, cmd);
>> +			}
>> +
>>  			/* Is it really there? */
>>  			io = pci_map_rom(pdev, &size);
>>  			if (!io || !size) {
>>  				info.size = 0;
>> -				break;
>> +				goto rom_info_out;
>>  			}
>>  			pci_unmap_rom(pdev, io);
>>  
>>  			info.flags = VFIO_REGION_INFO_FLAG_READ;
>> +rom_info_out:
>> +			if (mem_access_disabled) {
>> +				cmd &= ~PCI_COMMAND_MEMORY;
>> +				pci_write_config_word(pdev, PCI_COMMAND, cmd);
>> +			}
>> +
>>  			break;
>>  		}
>>  		case VFIO_PCI_VGA_REGION_INDEX:
> 
> I don't think we need to be so timid about the command register and we
> can also avoid the goto by modifying the test (testing io and size in
> the original is probably overly paranoid), perhaps simply:
Yes looks fine.

Do you want to respin or do you prefer I do?

Thanks

Eric
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index ff60bd1ea587..659b7c1ea8fb 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -708,6 +708,7 @@ static long vfio_pci_ioctl(void *device_data,
>  		{
>  			void __iomem *io;
>  			size_t size;
> +			u16 orig_cmd;
>  
>  			info.offset = VFIO_PCI_INDEX_TO_OFFSET(info.index);
>  			info.flags = 0;
> @@ -723,15 +724,23 @@ static long vfio_pci_ioctl(void *device_data,
>  					break;
>  			}
>  
> -			/* Is it really there? */
> +			/*
> +			 * Is it really there?  Enable memory decode for
> +			 * implicit access in pci_map_rom().
> +			 */
> +			pci_read_config_word(pdev, PCI_COMMAND, &orig_cmd);
> +			pci_write_config_word(pdev, PCI_COMMAND,
> +					      orig_cmd | PCI_COMMAND_MEMORY);
> +
>  			io = pci_map_rom(pdev, &size);
> -			if (!io || !size) {
> +			if (io) {
> +				info.flags = VFIO_REGION_INFO_FLAG_READ;
> +				pci_unmap_rom(pdev, io);
> +			} else
>  				info.size = 0;
> -				break;
> -			}
> -			pci_unmap_rom(pdev, io);
>  
> -			info.flags = VFIO_REGION_INFO_FLAG_READ;
> +			pci_write_config_word(pdev, PCI_COMMAND, orig_cmd);
> +
>  			break;
>  		}
>  		case VFIO_PCI_VGA_REGION_INDEX:
> 