On Tue, 2014-01-14 at 22:37 +0530, Bandan Das wrote:
> Ccing Markus for the *_once macros
>
> Alex Williamson <alex.william...@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 <b...@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
