Re: [Qemu-devel] [PATCH v8 4/5] generic-loader: Add a generic loader

[Alistair Francis]

On Wed, Jul 13, 2016 at 12:44 PM, Peter Maydell
<address@hidden> wrote:
> On 13 July 2016 at 18:45, Alistair Francis <address@hidden> wrote:
>> On Tue, Jul 12, 2016 at 9:39 AM, Peter Maydell <address@hidden> wrote:
>>> On 2 July 2016 at 02:07, Alistair Francis <address@hidden> wrote:
>>>> Add a generic loader to QEMU which can be used to load images or set
>>>> memory values.
>>>>
>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>> ---
>
>>>> +    if (s->cpu) {
>>>> +        CPUClass *cc = CPU_GET_CLASS(s->cpu);
>>>> +        cpu_reset(s->cpu);
>>>
>>> If something else sets the PC later on this gets lost, which is
>>> ugly and fragile. It's kinda-sorta OK that we bodge this for things
>>> like the hw/arm/boot.c code because all the pieces are inside QEMU,
>>> but when you start adding in command line devices I'm a bit
>>> nervous. Maybe we should actually figure out our reset order
>>> woes properly ?
>>
>> This is up to the user to specify the commands in the order they want
>> them applied. What other method should we be using?
>
> Well, if it works it works, I guess, but one day we
> will need to figure out an actual model for reset order
> rather than having it work by chance (in this case,
> because command line devices get reset after ones in
> the board model proper, I think.)

Agreed, but I think that is out of scope of this series. That would
require a overhaul of the reset infrastructure to allow a more
configurable system or at least some stricter ordering rules to
follow. I thought I saw some patches on the list to start to improve
the reset functionality?

>
>>>> +    qemu_register_reset(generic_loader_reset, dev);
>>>
>>> What's wrong with a device reset function set via dc->reset ?
>>
>> This allows setting values from the HMP command line interface once
>> the machine is running. The dc->reset isn't applied in that case.
>
> I don't understand this -- could you explain in a bit
> more detail, please?

So this loading system is just a device, which means that it can be
loaded whenever a normal device would be loaded. The more interesting
option is using this via the command line but it is also possible to
use the QEMU Monitor once QEMU is running to call this.

A user can use the device_add function to add set values once QEMU is
running. The problem with that though is that the qdev_device_add()
function doesn't connect the reset function. I initially had a patch
to do this "qdev-monitor.c: Register reset function if the device has
one" but it was decided to just register the reset function in the
realise instead.

At the moment rom loading is only supported at startup, but in the
future maybe that could be supported as well, adding even more value
to this.

>
>>> Changing that after this goes into the tree would be a command
>>> line compatibility break, so this inclines me to saying that this
>>> isn't baked enough for 2.7 and we should aim to put it into 2.8.
>>
>> That's fair, but upsetting.
>
> I'm not really happy about pushing this back yet another
> release either, and there are two weeks left til hard
> freeze. So it's not impossible that we could fit it in,
> but we're still discussing the semantics of the device
> at this point, so if it did get into 2.7 I think it
> would be ending up going in really fairly late, and I
> tend by preference to be conservative about freezes.
> Wider review (ie somebody else as well as me) would help
> in providing reassurance that the interface is correct.

That is fair, I can see that it is risky.

I have the next version ready to send, I'll send it by the end of
today (PST time) if no other issues come up. Maybe it can still make
it in.

Thanks,

Alistair

>
> thanks
> -- PMM
>