| Message ID | 20220323171346.792572-1-ralf.ramsauer@oth-regensburg.de |
|---|---|
| State | New |
| Headers | show |
| Series | hw/riscv: virt: Warn the user if -bios is provided when using KVM | expand |
On Thu, Mar 24, 2022 at 3:13 AM Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> wrote: > > The -bios option is silently ignored if used in combination with -enable-kvm. > The reason is that the machine starts in S-Mode, and the bios typically runs in > M-Mode. > > Warn the user that the bios won't be loaded. > > Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> Thanks for the patch. > --- > hw/riscv/virt.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 4496a15346..a4d13114ee 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) > * when KVM is enabled. > */ > if (kvm_enabled()) { > + if (machine->firmware && strcmp(machine->firmware, "none")) You need curly braces around the if statement. You can run checkpatch on the patch to catch issues like this with: git show | ./scripts/checkpatch.pl - > + warn_report("BIOS is not supported in combination with KVM. " > + "Ignoring BIOS."); Maybe say "Machine mode firmware is not supported in combination with KVM. Ignoring -bios" Alistair > g_free(machine->firmware); > machine->firmware = g_strdup("none"); > } > -- > 2.32.0 >
On 23/03/2022 22:05, Alistair Francis wrote: > On Thu, Mar 24, 2022 at 3:13 AM Ralf Ramsauer > <ralf.ramsauer@oth-regensburg.de> wrote: >> >> The -bios option is silently ignored if used in combination with -enable-kvm. >> The reason is that the machine starts in S-Mode, and the bios typically runs in >> M-Mode. >> >> Warn the user that the bios won't be loaded. >> >> Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> > > Thanks for the patch. > >> --- >> hw/riscv/virt.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >> index 4496a15346..a4d13114ee 100644 >> --- a/hw/riscv/virt.c >> +++ b/hw/riscv/virt.c >> @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) >> * when KVM is enabled. >> */ >> if (kvm_enabled()) { >> + if (machine->firmware && strcmp(machine->firmware, "none")) > > You need curly braces around the if statement. You can run checkpatch > on the patch to catch issues like this with: > > git show | ./scripts/checkpatch.pl - total: 0 errors, 0 warnings, 9 lines checked Your patch has no obvious style problems and is ready for submission. > >> + warn_report("BIOS is not supported in combination with KVM. " >> + "Ignoring BIOS."); > > Maybe say > > "Machine mode firmware is not supported in combination with KVM. Ignoring -bios" Anyway, will provide a V2 with an improved warning message. Thanks Ralf > > Alistair > >> g_free(machine->firmware); >> machine->firmware = g_strdup("none"); >> } >> -- >> 2.32.0 >>
On Wed, Mar 23, 2022 at 06:13:46PM +0100, Ralf Ramsauer wrote: > The -bios option is silently ignored if used in combination with -enable-kvm. > The reason is that the machine starts in S-Mode, and the bios typically runs in > M-Mode. > > Warn the user that the bios won't be loaded. > > Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> > --- > hw/riscv/virt.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 4496a15346..a4d13114ee 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) > * when KVM is enabled. > */ > if (kvm_enabled()) { > + if (machine->firmware && strcmp(machine->firmware, "none")) > + warn_report("BIOS is not supported in combination with KVM. " > + "Ignoring BIOS."); If the usage scenario isn't supportable, then ultimately we should be raising an error and immediately exiting. If you know of common usage that is already mistakenly passing -bios, then we could start with a warning and list it as deprecated, then change to an error_report 2 releases later. If we don't thing people are often mistakenly passing -bios, then go straight for error_report and exit. > g_free(machine->firmware); > machine->firmware = g_strdup("none"); > } > -- > 2.32.0 > > With regards, Daniel
On Thu, Mar 24, 2022 at 7:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Mar 23, 2022 at 06:13:46PM +0100, Ralf Ramsauer wrote: > > The -bios option is silently ignored if used in combination with -enable-kvm. > > The reason is that the machine starts in S-Mode, and the bios typically runs in > > M-Mode. > > > > Warn the user that the bios won't be loaded. > > > > Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> > > --- > > hw/riscv/virt.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > > index 4496a15346..a4d13114ee 100644 > > --- a/hw/riscv/virt.c > > +++ b/hw/riscv/virt.c > > @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) > > * when KVM is enabled. > > */ > > if (kvm_enabled()) { > > + if (machine->firmware && strcmp(machine->firmware, "none")) > > + warn_report("BIOS is not supported in combination with KVM. " > > + "Ignoring BIOS."); > > If the usage scenario isn't supportable, then ultimately we should be > raising an error and immediately exiting. > > If you know of common usage that is already mistakenly passing -bios, > then we could start with a warning and list it as deprecated, then > change to an error_report 2 releases later. If we don't thing people > are often mistakenly passing -bios, then go straight for error_report > and exit. That's a good point. The original thinking was that we did support -bios and so we should warn the user that it's unlikely they want to use it. This would still allow S mode UEFI loaders to be used (they don't exist today). Considering we are currently just ignoring the option I agree it's better to report an error. Do you mind sending a v2 Ralf? Alistair > > > g_free(machine->firmware); > > machine->firmware = g_strdup("none"); > > } > > -- > > 2.32.0 > > > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On 31/03/2022 02:11, Alistair Francis wrote: > On Thu, Mar 24, 2022 at 7:08 PM Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> On Wed, Mar 23, 2022 at 06:13:46PM +0100, Ralf Ramsauer wrote: >>> The -bios option is silently ignored if used in combination with -enable-kvm. >>> The reason is that the machine starts in S-Mode, and the bios typically runs in >>> M-Mode. >>> >>> Warn the user that the bios won't be loaded. >>> >>> Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> >>> --- >>> hw/riscv/virt.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c >>> index 4496a15346..a4d13114ee 100644 >>> --- a/hw/riscv/virt.c >>> +++ b/hw/riscv/virt.c >>> @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) >>> * when KVM is enabled. >>> */ >>> if (kvm_enabled()) { >>> + if (machine->firmware && strcmp(machine->firmware, "none")) >>> + warn_report("BIOS is not supported in combination with KVM. " >>> + "Ignoring BIOS."); >> >> If the usage scenario isn't supportable, then ultimately we should be >> raising an error and immediately exiting. >> >> If you know of common usage that is already mistakenly passing -bios, >> then we could start with a warning and list it as deprecated, then >> change to an error_report 2 releases later. If we don't thing people >> are often mistakenly passing -bios, then go straight for error_report >> and exit. > > That's a good point. The original thinking was that we did support > -bios and so we should warn the user that it's unlikely they want to > use it. This would still allow S mode UEFI loaders to be used (they > don't exist today). > > Considering we are currently just ignoring the option I agree it's > better to report an error. > > Do you mind sending a v2 Ralf? Yes, will return with another revision. Anyway, I'll choose to exit immediately, as I doubt that there are any non-development users of this particular feature (RISCV/Qemu + KVM) due to the lack of physical hardware. Thanks Ralf > > Alistair > >> >>> g_free(machine->firmware); >>> machine->firmware = g_strdup("none"); >>> } >>> -- >>> 2.32.0 >>> >>> >> >> With regards, >> Daniel >> -- >> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >> |: https://libvirt.org -o- https://fstop138.berrange.com :| >> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >>
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 4496a15346..a4d13114ee 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -1312,6 +1312,9 @@ static void virt_machine_init(MachineState *machine) * when KVM is enabled. */ if (kvm_enabled()) { + if (machine->firmware && strcmp(machine->firmware, "none")) + warn_report("BIOS is not supported in combination with KVM. " + "Ignoring BIOS."); g_free(machine->firmware); machine->firmware = g_strdup("none"); }
The -bios option is silently ignored if used in combination with -enable-kvm. The reason is that the machine starts in S-Mode, and the bios typically runs in M-Mode. Warn the user that the bios won't be loaded. Signed-off-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> --- hw/riscv/virt.c | 3 +++ 1 file changed, 3 insertions(+)