Re: [PATCH for-6.1] hw/arm/boot: Report error if there is no fw_cfg device in the machine

[Leif Lindholm]

On Mon, Jul 26, 2021 at 17:33:51 +0100, Peter Maydell wrote:
> If the user provides both a BIOS/firmware image and also a guest
> kernel filename, arm_setup_firmware_boot() will pass the
> kernel image to the firmware via the fw_cfg device. However we
> weren't checking whether there really was a fw_cfg device present,
> and if there wasn't we would crash.
> 
> This crash can be provoked with a command line such as
>  qemu-system-aarch64 -M raspi3 -kernel /dev/null -bios /dev/null -display none
> 
> It is currently only possible on the raspi3 machine, because unless
> the machine sets info->firmware_loaded we won't call
> arm_setup_firmware_boot(), and the only machines which set that are:
>  * virt (has a fw-cfg device)
>  * sbsa-ref (checks itself for kernel_filename && firmware_loaded)
>  * raspi3 (crashes)
> 
> But this is an unfortunate beartrap to leave for future machine
> model implementors, so we should handle this situation in boot.c.
> 
> Check in arm_setup_firmware_boot() whether the fw-cfg device exists
> before trying to load files into it, and if it doesn't exist then
> exit with a hopefully helpful error message.
> 
> Because we now handle this check in a machine-agnostic way, we
> can remove the check from sbsa-ref.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/503
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

However, the subject line threw me slightly. How about:?
"Report error if trying to load kernel with no fw_cfg"

> ---
> I didn't reuse exactly the same wording sbsa-ref used to have,
> because the bit about loading an OS from hard disk might not
> apply to all machine types.
> ---
>  hw/arm/boot.c     | 9 +++++++++
>  hw/arm/sbsa-ref.c | 7 -------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d7b059225e6..57efb61ee41 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -1243,6 +1243,15 @@ static void arm_setup_firmware_boot(ARMCPU *cpu, 
> struct arm_boot_info *info)
>          bool try_decompressing_kernel;
>  
>          fw_cfg = fw_cfg_find();
> +
> +        if (!fw_cfg) {
> +            error_report("This machine type does not support loading both "
> +                         "a guest firmware/BIOS image and a guest kernel at "
> +                         "the same time. You should change your QEMU command 
> "
> +                         "line to specify one or the other, but not both.");
> +            exit(1);
> +        }
> +
>          try_decompressing_kernel = arm_feature(&cpu->env,
>                                                 ARM_FEATURE_AARCH64);
>  
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 43c19b49234..c1629df6031 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -691,13 +691,6 @@ static void sbsa_ref_init(MachineState *machine)
>  
>      firmware_loaded = sbsa_firmware_init(sms, sysmem, secure_sysmem);
>  
> -    if (machine->kernel_filename && firmware_loaded) {
> -        error_report("sbsa-ref: No fw_cfg device on this machine, "
> -                     "so -kernel option is not supported when firmware 
> loaded, "
> -                     "please load OS from hard disk instead");
> -        exit(1);
> -    }
> -
>      /*
>       * This machine has EL3 enabled, external firmware should supply PSCI
>       * implementation, so the QEMU's internal PSCI is disabled.
> -- 
> 2.20.1
>