Re: [Qemu-devel] [PATCH v2 2/6] hw/arm: new interface for devices which need to behave differently for kernel boot

[Peter Maydell]

On 18 July 2015 at 04:55, Peter Crosthwaite
<address@hidden> wrote:
> On Thu, Jul 16, 2015 at 1:11 PM, Peter Maydell <address@hidden> wrote:
>> For ARM we have a little minimalist bootloader in hw/arm/boot.c which
>> takes the place of firmware if we're directly booting a Linux kernel.
>> Unfortunately a few devices need special case handling in this situation
>> to do the initialization which on real hardware would be done by
>> firmware. (In particular if we're booting a kernel in NonSecure state
>> then we need to make a TZ-aware GIC put all its interrupts into Group 1,
>> or the guest will be unable to use them.)
>>
>> Create a new QOM interface which can be implemented by devices which
>> need to do something different from their default reset behaviour.
>> The callback will be called after machine initialization and before
>> first reset.
>>
>> Suggested-by: Peter Crosthwaite <address@hidden>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/arm/boot.c                  | 34 +++++++++++++++++++++++++++++++++
>>  include/hw/arm/linux-boot-if.h | 43 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 77 insertions(+)
>>  create mode 100644 include/hw/arm/linux-boot-if.h
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 5b969cd..4bac6dc 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -10,6 +10,7 @@
>>  #include "config.h"
>>  #include "hw/hw.h"
>>  #include "hw/arm/arm.h"
>> +#include "hw/arm/linux-boot-if.h"
>>  #include "sysemu/sysemu.h"
>>  #include "hw/boards.h"
>>  #include "hw/loader.h"
>> @@ -555,6 +556,20 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, 
>> uint16_t size_key,
>>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>  }
>>
>> +static int do_arm_linux_init(Object *obj, void *opaque)
>> +{
>> +    if (object_dynamic_cast(obj, TYPE_ARM_LINUX_BOOT_IF)) {
>> +        ARMLinuxBootIf *albif = ARM_LINUX_BOOT_IF(obj);
>> +        ARMLinuxBootIfClass *albifc = ARM_LINUX_BOOT_IF_GET_CLASS(obj);
>> +        struct arm_boot_info *info = opaque;
>> +
>> +        if (albifc->arm_linux_init) {
>> +            albifc->arm_linux_init(albif, info->secure_boot);
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>>  static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>  {
>>      CPUState *cs;
>> @@ -778,6 +793,12 @@ static void arm_load_kernel_notify(Notifier *notifier, 
>> void *data)
>>          if (info->nb_cpus > 1) {
>>              info->write_secondary_boot(cpu, info);
>>          }
>> +
>> +        /* Notify devices which need to fake up firmware initialization
>> +         * that we'ro doing a direct kernel boot.
>> +         */
>
> "we're"
>
>> +        object_child_foreach_recursive(object_get_root(),
>> +                                       do_arm_linux_init, info);
>>      }
>>      info->is_linux = is_linux;
>>
>> @@ -803,3 +824,16 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
>> *info)
>>          qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
>>      }
>>  }
>> +
>> +static const TypeInfo arm_linux_boot_if_info = {
>> +    .name = TYPE_ARM_LINUX_BOOT_IF,
>> +    .parent = TYPE_INTERFACE,
>> +    .class_size = sizeof(ARMLinuxBootIfClass),
>> +};
>> +
>> +static void arm_linux_boot_register_types(void)
>> +{
>> +    type_register_static(&arm_linux_boot_if_info);
>> +}
>> +
>> +type_init(arm_linux_boot_register_types)
>> diff --git a/include/hw/arm/linux-boot-if.h b/include/hw/arm/linux-boot-if.h
>> new file mode 100644
>> index 0000000..aba4479
>> --- /dev/null
>> +++ b/include/hw/arm/linux-boot-if.h
>> @@ -0,0 +1,43 @@
>> +/*
>> + * hw/arm/linux-boot-if.h : interface for devices which need to behave
>> + * specially for direct boot of an ARM Linux kernel
>> + */
>> +
>> +#ifndef HW_ARM_LINUX_BOOT_IF_H
>> +#define HW_ARM_LINUX_BOOT_IF_H
>> +
>> +#include "qom/object.h"
>> +
>> +#define TYPE_ARM_LINUX_BOOT_IF "arm-linux-boot-if"
>> +#define ARM_LINUX_BOOT_IF_CLASS(klass) \
>> +    OBJECT_CLASS_CHECK(ARMLinuxBootIfClass, (klass), TYPE_ARM_LINUX_BOOT_IF)
>> +#define ARM_LINUX_BOOT_IF_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(ARMLinuxBootIfClass, (obj), TYPE_ARM_LINUX_BOOT_IF)
>> +#define ARM_LINUX_BOOT_IF(obj) \
>> +    INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
>> +
>> +typedef struct ARMLinuxBootIf {
>> +    /*< private >*/
>> +    Object parent_obj;
>> +} ARMLinuxBootIf;
>> +
>> +typedef struct ARMLinuxBootIfClass {
>> +    /*< private >*/
>> +    InterfaceClass parent_class;
>> +
>> +    /*< public >*/
>> +    /** arm_linux_init: configure the device for a direct boot
>> +     * of an ARM Linux kernel (so that device reset puts it into
>> +     * the state the kernel expects after firmware initialization,
>> +     * rather than the true hardware reset state). This callback is
>> +     * called once after machine construction is complete (before the
>> +     * first system reset).
>> +     *
>> +     * @obj: the object implementing this interface
>> +     * @secure_boot: true if we are booting Secure, false for NonSecure
>> +     * (or for a CPU which doesn't support TrustZone)
>> +     */
>> +    void (*arm_linux_init)(ARMLinuxBootIf *obj, bool secure_boot);
>
> Can we drop the "arm_"? ARM is always going to be there in the context
> as it is in the typename due to ARMLinuxBootIfClass.

Yeah, I guess so. I wasn't really sure what the best method
name here was.

> So If we are going for an ARM-specific thing, it might make sense to
> instead drop all the _linux_ stuff and have this call unconditional.
> Then the API has wider application than just Linux boots. The struct
> arm_boot_info can be made more widely visible as the one data argument
> the device accepts, from which security state as well and is_linux can
> be fished.

I was going to pass arm_boot_info in, but that struct requires
the target cpu.h so it can't be used in compiled-once objects
like the GIC code. Hence the single bool parameter.

I'm also not too keen on increasing the set of things we try
to handle in boot. Currently we do:
 * "firmware", ie the guest code gets to do all setup that it needs,
   just as on hardware
 * "linux kernel", where we provide the more-or-less documented
   boot environment etc for Linux kernels in particular

I think you're implying that we want to support a third thing here?

thanks
-- PMM