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

[Peter Crosthwaite]

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.

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.

void (*fw_init)(ARMLinuxBootif *obj, struct arm_boot_info info);

Regards,
Peter

> +} ARMLinuxBootIfClass;
> +
> +#endif
> --
> 1.9.1
>
>