* [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom
@ 2019-02-13 11:06 Eric Auger
2019-02-13 17:52 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Eric Auger @ 2019-02-13 11:06 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, linux-kernel, kvm, alex.williamson
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).
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>
---
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:
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-13 11:06 [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom Eric Auger
@ 2019-02-13 17:52 ` Alex Williamson
2019-02-14 18:27 ` Auger Eric
0 siblings, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2019-02-13 17:52 UTC (permalink / raw)
To: Eric Auger; +Cc: eric.auger.pro, linux-kernel, kvm
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.
> 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")
>
> ---
>
> 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:
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:
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-13 17:52 ` Alex Williamson
@ 2019-02-14 18:27 ` Auger Eric
2019-02-14 20:16 ` Alex Williamson
0 siblings, 1 reply; 4+ messages in thread
From: Auger Eric @ 2019-02-14 18:27 UTC (permalink / raw)
To: Alex Williamson; +Cc: eric.auger.pro, linux-kernel, kvm
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:
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom
2019-02-14 18:27 ` Auger Eric
@ 2019-02-14 20:16 ` Alex Williamson
0 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2019-02-14 20:16 UTC (permalink / raw)
To: Auger Eric; +Cc: eric.auger.pro, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-14 20:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-13 11:06 [PATCH v2] vfio_pci: Enable memory accesses before calling pci_map_rom Eric Auger
2019-02-13 17:52 ` Alex Williamson
2019-02-14 18:27 ` Auger Eric
2019-02-14 20:16 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox