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

[Bandan Das <bsd@redhat.com>]

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