* [Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom @ 2014-01-14 16:15 Bandan Das 2014-01-14 16:15 ` [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read Bandan Das 2014-01-14 16:15 ` [Qemu-devel] [PATCH 2/2] vfio: Do not reattempt a failed rom read Bandan Das 0 siblings, 2 replies; 6+ messages in thread From: Bandan Das @ 2014-01-14 16:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read 2014-01-14 16:15 [Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom Bandan Das @ 2014-01-14 16:15 ` Bandan Das 2014-01-14 16:31 ` Alex Williamson 2014-01-14 16:15 ` [Qemu-devel] [PATCH 2/2] vfio: Do not reattempt a failed rom read Bandan Das 1 sibling, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-01-14 16:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read 2014-01-14 16:15 ` [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read Bandan Das @ 2014-01-14 16:31 ` Alex Williamson 2014-01-14 17:07 ` Bandan Das 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2014-01-14 16:31 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read 2014-01-14 16:31 ` Alex Williamson @ 2014-01-14 17:07 ` Bandan Das 2014-01-14 17:18 ` Alex Williamson 0 siblings, 1 reply; 6+ messages in thread From: Bandan Das @ 2014-01-14 17:07 UTC (permalink / raw) To: Alex Williamson; +Cc: qemu-devel, armbru 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read 2014-01-14 17:07 ` Bandan Das @ 2014-01-14 17:18 ` Alex Williamson 0 siblings, 0 replies; 6+ messages in thread From: Alex Williamson @ 2014-01-14 17:18 UTC (permalink / raw) To: Bandan Das; +Cc: qemu-devel, armbru 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] vfio: Do not reattempt a failed rom read 2014-01-14 16:15 [Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom Bandan Das 2014-01-14 16:15 ` [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read Bandan Das @ 2014-01-14 16:15 ` Bandan Das 1 sibling, 0 replies; 6+ messages in thread From: Bandan Das @ 2014-01-14 16:15 UTC (permalink / raw) To: qemu-devel; +Cc: alex.williamson 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-14 17:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-14 16:15 [Qemu-devel] [PATCH 0/2] vfio: Warn on failure to read device rom Bandan Das 2014-01-14 16:15 ` [Qemu-devel] [PATCH 1/2] vfio: warn if host device rom can't be read Bandan Das 2014-01-14 16:31 ` Alex Williamson 2014-01-14 17:07 ` Bandan Das 2014-01-14 17:18 ` Alex Williamson 2014-01-14 16:15 ` [Qemu-devel] [PATCH 2/2] vfio: Do not reattempt a failed rom read Bandan Das
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).