[Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom

[Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom

[Bandan Das]

Minor changes to print a message on the monitor
console in case of a rom read failure. Please see individual
patches for more details.

Bandan Das (2):
  vfio: warn if host device rom can't be read
  vfio: Do not reattempt a failed rom read

 hw/misc/vfio.c              | 12 +++++++++++-
 include/qemu/error-report.h | 20 ++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read

[Bandan Das]

If the device rom can't be read, report an error to the
user. The guest might try to read the rom contents more than
once, so introduce macros that print a message only once and
not clutter up the console. This is to alert the user
that the device has a bad state that is causing rom read
failure or option rom loading has been disabled from the device
boot menu (among other reasons).

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/misc/vfio.c              |  7 +++++++
 include/qemu/error-report.h | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 9aecaa8..e5b2826 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -1125,6 +1125,13 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
     vdev->rom_offset = reg_info.offset;
 
     if (!vdev->rom_size) {
+        error_report_once("vfio-pci: Cannot read device rom at "
+                    "%04x:%02x:%02x.%x\n",
+                    vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                    vdev->host.function);
+        error_printf_once("Device option ROM contents are probably invalid "
+                    "(check dmesg).\nSkip option ROM probe with rombar=0, "
+                    "or load from file with romfile=\n");
         return;
     }
 
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3b098a9..7d24e4c 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -43,4 +43,24 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;
 
+#define error_printf_once(fmt, ...)             \
+({                                              \
+        static bool __printf_once;              \
+                                                \
+        if (!__printf_once) {                   \
+            __printf_once = true;               \
+            error_printf(fmt, ##__VA_ARGS__);   \
+        }                                       \
+})                                              \
+
+#define error_report_once(fmt, ...)             \
+({                                              \
+        static bool __report_once;              \
+                                                \
+        if (!__report_once) {                   \
+            __report_once = true;               \
+            error_report(fmt, ##__VA_ARGS__);   \
+        }                                       \
+})                                              \
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] vfio: Do not reattempt a failed rom read

[Bandan Das]

During lazy rom loading, if rom read fails, and the
guest attempts a read again, vfio_rom_read will again attempt it.
Add a boolean to prevent this. There could be a case where
a failed rom read might succeed the next time because of
a device reset or such, but it's best to exclude unpredictable
behavior

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/misc/vfio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index e5b2826..c500067 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -191,6 +191,7 @@ typedef struct VFIODevice {
     bool has_flr;
     bool has_pm_reset;
     bool needs_reset;
+    bool rom_read_failed;
 } VFIODevice;
 
 typedef struct VFIOGroup {
@@ -1125,6 +1126,7 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
     vdev->rom_offset = reg_info.offset;
 
     if (!vdev->rom_size) {
+        vdev->rom_read_failed = true;
         error_report_once("vfio-pci: Cannot read device rom at "
                     "%04x:%02x:%02x.%x\n",
                     vdev->host.domain, vdev->host.bus, vdev->host.slot,
@@ -1161,7 +1163,7 @@ static uint64_t vfio_rom_read(void *opaque, hwaddr addr, unsigned size)
     uint64_t val = ((uint64_t)1 << (size * 8)) - 1;
 
     /* Load the ROM lazily when the guest tries to read it */
-    if (unlikely(!vdev->rom)) {
+    if (unlikely(!vdev->rom_read_failed && !vdev->rom)) {
         vfio_pci_load_rom(vdev);
     }
 
@@ -1230,6 +1232,7 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
                      PCI_BASE_ADDRESS_SPACE_MEMORY, &vdev->pdev.rom);
 
     vdev->pdev.has_rom = true;
+    vdev->rom_read_failed = false;
 }
 
 static void vfio_vga_write(void *opaque, hwaddr addr,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read

[Alex Williamson]

On Tue, 2014-01-14 at 21:45 +0530, Bandan Das wrote:
> If the device rom can't be read, report an error to the
> user. The guest might try to read the rom contents more than
> once, so introduce macros that print a message only once and
> not clutter up the console. This is to alert the user
> that the device has a bad state that is causing rom read
> failure or option rom loading has been disabled from the device
> boot menu (among other reasons).
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/misc/vfio.c              |  7 +++++++
>  include/qemu/error-report.h | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 9aecaa8..e5b2826 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -1125,6 +1125,13 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
>      vdev->rom_offset = reg_info.offset;
>  
>      if (!vdev->rom_size) {
> +        error_report_once("vfio-pci: Cannot read device rom at "
> +                    "%04x:%02x:%02x.%x\n",
> +                    vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +                    vdev->host.function);
> +        error_printf_once("Device option ROM contents are probably invalid "
> +                    "(check dmesg).\nSkip option ROM probe with rombar=0, "
> +                    "or load from file with romfile=\n");
>          return;
>      }
>  
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3b098a9..7d24e4c 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -43,4 +43,24 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  const char *error_get_progname(void);
>  extern bool enable_timestamp_msg;
>  
> +#define error_printf_once(fmt, ...)             \
> +({                                              \
> +        static bool __printf_once;              \
> +                                                \
> +        if (!__printf_once) {                   \
> +            __printf_once = true;               \
> +            error_printf(fmt, ##__VA_ARGS__);   \
> +        }                                       \
> +})                                              \
> +
> +#define error_report_once(fmt, ...)             \
> +({                                              \
> +        static bool __report_once;              \
> +                                                \
> +        if (!__report_once) {                   \
> +            __report_once = true;               \
> +            error_report(fmt, ##__VA_ARGS__);   \
> +        }                                       \
> +})                                              \
> +
>  #endif

Why do we need these if patch 2/2 comes along and only calls
vfio_pci_load_rom() once?  If we do need these, they should be a
separate patch.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read

[Bandan Das]

Ccing Markus for the *_once macros

Alex Williamson <alex.williamson@redhat.com> writes:

> On Tue, 2014-01-14 at 21:45 +0530, Bandan Das wrote:
>> If the device rom can't be read, report an error to the
>> user. The guest might try to read the rom contents more than
>> once, so introduce macros that print a message only once and
>> not clutter up the console. This is to alert the user
>> that the device has a bad state that is causing rom read
>> failure or option rom loading has been disabled from the device
>> boot menu (among other reasons).
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/misc/vfio.c              |  7 +++++++
>>  include/qemu/error-report.h | 20 ++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 9aecaa8..e5b2826 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -1125,6 +1125,13 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
>>      vdev->rom_offset = reg_info.offset;
>>  
>>      if (!vdev->rom_size) {
>> +        error_report_once("vfio-pci: Cannot read device rom at "
>> +                    "%04x:%02x:%02x.%x\n",
>> +                    vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +                    vdev->host.function);
>> +        error_printf_once("Device option ROM contents are probably invalid "
>> +                    "(check dmesg).\nSkip option ROM probe with rombar=0, "
>> +                    "or load from file with romfile=\n");
>>          return;
>>      }
>>  
>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
>> index 3b098a9..7d24e4c 100644
>> --- a/include/qemu/error-report.h
>> +++ b/include/qemu/error-report.h
>> @@ -43,4 +43,24 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>>  const char *error_get_progname(void);
>>  extern bool enable_timestamp_msg;
>>  
>> +#define error_printf_once(fmt, ...)             \
>> +({                                              \
>> +        static bool __printf_once;              \
>> +                                                \
>> +        if (!__printf_once) {                   \
>> +            __printf_once = true;               \
>> +            error_printf(fmt, ##__VA_ARGS__);   \
>> +        }                                       \
>> +})                                              \
>> +
>> +#define error_report_once(fmt, ...)             \
>> +({                                              \
>> +        static bool __report_once;              \
>> +                                                \
>> +        if (!__report_once) {                   \
>> +            __report_once = true;               \
>> +            error_report(fmt, ##__VA_ARGS__);   \
>> +        }                                       \
>> +})                                              \
>> +
>>  #endif
>
> Why do we need these if patch 2/2 comes along and only calls
> vfio_pci_load_rom() once?  If we do need these, they should be a
> separate patch.  Thanks,

I was in and out on this until I decided to include it for cases 
where we vfio assign a number of functions from the same card - if rom 
loading fails for one, it will most probably fail for others as 
well and this will make sure to print it just once at bootup. 
However, this also means that it will print once for unrelated assignments
too, I kind of half-convinced myself that that's probably ok :)

Would you rather have this get printed for each assigned device if loading 
fails ? 

> Alex

Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read

[Alex Williamson]

On Tue, 2014-01-14 at 22:37 +0530, Bandan Das wrote:
> Ccing Markus for the *_once macros
> 
> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > On Tue, 2014-01-14 at 21:45 +0530, Bandan Das wrote:
> >> If the device rom can't be read, report an error to the
> >> user. The guest might try to read the rom contents more than
> >> once, so introduce macros that print a message only once and
> >> not clutter up the console. This is to alert the user
> >> that the device has a bad state that is causing rom read
> >> failure or option rom loading has been disabled from the device
> >> boot menu (among other reasons).
> >> 
> >> Signed-off-by: Bandan Das <bsd@redhat.com>
> >> ---
> >>  hw/misc/vfio.c              |  7 +++++++
> >>  include/qemu/error-report.h | 20 ++++++++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >> 
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 9aecaa8..e5b2826 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -1125,6 +1125,13 @@ static void vfio_pci_load_rom(VFIODevice *vdev)
> >>      vdev->rom_offset = reg_info.offset;
> >>  
> >>      if (!vdev->rom_size) {
> >> +        error_report_once("vfio-pci: Cannot read device rom at "
> >> +                    "%04x:%02x:%02x.%x\n",
> >> +                    vdev->host.domain, vdev->host.bus, vdev->host.slot,
> >> +                    vdev->host.function);
> >> +        error_printf_once("Device option ROM contents are probably invalid "
> >> +                    "(check dmesg).\nSkip option ROM probe with rombar=0, "
> >> +                    "or load from file with romfile=\n");
> >>          return;
> >>      }
> >>  
> >> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> >> index 3b098a9..7d24e4c 100644
> >> --- a/include/qemu/error-report.h
> >> +++ b/include/qemu/error-report.h
> >> @@ -43,4 +43,24 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> >>  const char *error_get_progname(void);
> >>  extern bool enable_timestamp_msg;
> >>  
> >> +#define error_printf_once(fmt, ...)             \
> >> +({                                              \
> >> +        static bool __printf_once;              \
> >> +                                                \
> >> +        if (!__printf_once) {                   \
> >> +            __printf_once = true;               \
> >> +            error_printf(fmt, ##__VA_ARGS__);   \
> >> +        }                                       \
> >> +})                                              \
> >> +
> >> +#define error_report_once(fmt, ...)             \
> >> +({                                              \
> >> +        static bool __report_once;              \
> >> +                                                \
> >> +        if (!__report_once) {                   \
> >> +            __report_once = true;               \
> >> +            error_report(fmt, ##__VA_ARGS__);   \
> >> +        }                                       \
> >> +})                                              \
> >> +
> >>  #endif
> >
> > Why do we need these if patch 2/2 comes along and only calls
> > vfio_pci_load_rom() once?  If we do need these, they should be a
> > separate patch.  Thanks,
> 
> I was in and out on this until I decided to include it for cases 
> where we vfio assign a number of functions from the same card - if rom 
> loading fails for one, it will most probably fail for others as 
> well and this will make sure to print it just once at bootup. 
> However, this also means that it will print once for unrelated assignments
> too, I kind of half-convinced myself that that's probably ok :)
> 
> Would you rather have this get printed for each assigned device if loading 
> fails ? 

The error_report is going to list a specific device, so yes, it's
probably best to be explicit about all the devices that are having
problems.  Thanks,

Alex