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

[Peter Maydell]

[oops, forgot to update Peter C's email address in the From line;
apologies to everybody else for the duplicate mail.]

On 18 July 2015 at 10:00, Peter Maydell <address@hidden> wrote:
> 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.

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

Any further comment on this? As I said, I'm unconvinced about
making this method more general than we really need. The other
patches have been reviewed so consensus on this API is I think
the only blocker.

thanks
-- PMM