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

From: Alex Williamson <alex.williamson@redhat.com>
To: Auger Eric <eric.auger@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 13:16:28 -0700	[thread overview]
Message-ID: <20190214131628.54ca06fc@w520.home> (raw)
In-Reply-To: <015103e3-c317-22f3-e209-f5ccf890bafb@redhat.com>

On Thu, 14 Feb 2019 19:27:15 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> 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?

Please take it, test it, and repost it, I haven't tested it at all.
Thanks,

Alex