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 <address@hidden> 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 <address@hidden>
> >> ---
> >>  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