| Message ID | 20210726163351.32086-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-6.1] hw/arm/boot: Report error if there is no fw_cfg device in the machine | expand |
On 7/26/21 6:33 AM, 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: Richard Henderson <richard.henderson@linaro.org> r~
On 7/26/21 6:33 PM, 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> > --- > 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(-) Thanks for fixing this. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 >
On Tue, 3 Aug 2021 at 15:29, Leif Lindholm <leif@nuviainc.com> wrote: > > 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" Yeah, in retrospect that would have been a better subject. However, the commit is in master already (dae257394ae5) so it is what it is :-/ thanks -- PMM
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.
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> --- 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(-)