[PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Ralf Ramsauer]

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.");
         g_free(machine->firmware);
         machine->firmware = g_strdup("none");
     }
-- 
2.32.0

Re: [PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Alistair Francis]

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
>

Re: [EXT] Re: [PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Ralf Ramsauer]

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
>>

[PATCH v2] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Ralf Ramsauer]

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index da50cbed43..0c477addbc 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1312,6 +1312,10 @@ static void virt_machine_init(MachineState *machine)
      * when KVM is enabled.
      */
     if (kvm_enabled()) {
+        if (machine->firmware && strcmp(machine->firmware, "none")) {
+            warn_report("Machine mode firmware is not supported in combination "
+                        "with KVM. Ignoring -bios.");
+        }
         g_free(machine->firmware);
         machine->firmware = g_strdup("none");
     }
-- 
2.35.1

Re: [PATCH v2] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Alistair Francis]

On Thu, Mar 24, 2022 at 8:31 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!

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/riscv/virt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index da50cbed43..0c477addbc 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1312,6 +1312,10 @@ static void virt_machine_init(MachineState *machine)
>       * when KVM is enabled.
>       */
>      if (kvm_enabled()) {
> +        if (machine->firmware && strcmp(machine->firmware, "none")) {
> +            warn_report("Machine mode firmware is not supported in combination "
> +                        "with KVM. Ignoring -bios.");
> +        }
>          g_free(machine->firmware);
>          machine->firmware = g_strdup("none");
>      }
> --
> 2.35.1
>

Re: [PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Daniel P. Berrangé]

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
-- 
|: 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 :|

Re: [PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Alistair Francis]

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 :|
>

Re: [EXT] Re: [PATCH] hw/riscv: virt: Warn the user if -bios is provided when using KVM

[Ralf Ramsauer]

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 :|
>>