[Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Peter Maydell]

Hi; I'm currently writing a UART device model, and I want to make
it avoid using the deprecated qemu_chr_fe_write_all() (in the same
way that the hw/char/cadence_uart.c code does, by using
qemu_chr_fe_write() and then registering a callback with
qemu_chr_fe_add_watch() if the write doesn't manage to send data).

I have some code that seems to work, but I'm not sure about how the
chardev frontend callbacks interact with VM start, stop and migration.

For instance, suppose my UART tries to write to the chardev but
can't, so it registers a callback with qemu_chr_fe_add_watch().
Then the user stops the VM. Does the chardev UI guarantee that
my callback won't get called until the user restarts the VM,
or does my callback function have to do something to say "don't
actually update the emulation state if we're stopped"?
If the user then restarts the VM, do I need to do something to
retry the chardev write?

For migration, if we're migrating in the state where I have a
pending character I'd like to write to the chardev, what do I need
to do on the destination side? Do I need to call qemu_chr_fe_add_watch
in the post-load function, or is this too early and I need to do
something to call it when the destination VM is actually restarted?

More generally, am I guaranteed that my qemu_chr_fe_add_watch()
callbacks are only called with the iothread mutex held (ie that
I don't need to worry about races between device model functions
called from the guest and the callbacks) ?

There are I think similar questions about the read handler and event
handler functions that every UART model registers with the
qemu_chr_fe_set_handlers function: are those only called when the
VM is actually running, or is special casing required for "called
when the VM is stopped?".

And one that I think is true but we don't document: is it the case
that the chardev layer guarantees not to call the chardev's IOReadHandler
unless its IOCanReadHandler has returned OK?

These all seem like things it would be nice to have documented in
include/chardev/char-fe.h :-)  What I would like the answer to be
(as far as is possible) is "the chardev layer handles these issues
so the frontends don't need to"...

thanks
-- PMM

Re: [Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Paolo Bonzini]

On 22/06/2017 12:09, Peter Maydell wrote:
> For instance, suppose my UART tries to write to the chardev but
> can't, so it registers a callback with qemu_chr_fe_add_watch().
> Then the user stops the VM. Does the chardev UI guarantee that
> my callback won't get called until the user restarts the VM,
> or does my callback function have to do something to say "don't
> actually update the emulation state if we're stopped"?
> If the user then restarts the VM, do I need to do something to
> retry the chardev write?

Currently, chardev code runs normally between stop and start.

> For migration, if we're migrating in the state where I have a
> pending character I'd like to write to the chardev, what do I need
> to do on the destination side? Do I need to call qemu_chr_fe_add_watch
> in the post-load function, or is this too early and I need to do
> something to call it when the destination VM is actually restarted?

Post-load is fine.  There could be interesting interactions with
interrupts, but these are not specific to character device backends
(real-time clocks are another example).  So boards should create
interrupt controllers and distributors as soon as possible.

On the save side, when migration is about to finish, the mig thread
takes the I/O thread lock, so chardevs cannot run anymore at that point.

> More generally, am I guaranteed that my qemu_chr_fe_add_watch()
> callbacks are only called with the iothread mutex held (ie that
> I don't need to worry about races between device model functions
> called from the guest and the callbacks) ?

Yes.  Chardev backends can be written to outside the iothread mutex, but
callbacks are always protected by the iothread mutex unless mentioned
specifically by the documentation and even then, only if you run in your
own IOThread.

> There are I think similar questions about the read handler and event
> handler functions that every UART model registers with the
> qemu_chr_fe_set_handlers function: are those only called when the
> VM is actually running, or is special casing required for "called
> when the VM is stopped?".

No special casing is currently done by the other backends.  It seems to
work fine in practice...

> And one that I think is true but we don't document: is it the case
> that the chardev layer guarantees not to call the chardev's IOReadHandler
> unless its IOCanReadHandler has returned OK?

Yes, and no more bytes than the can_read handler has told him to return.

Paolo

> These all seem like things it would be nice to have documented in
> include/chardev/char-fe.h :-)  What I would like the answer to be
> (as far as is possible) is "the chardev layer handles these issues
> so the frontends don't need to"...

Re: [Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Peter Maydell]

On 22 June 2017 at 11:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 22/06/2017 12:09, Peter Maydell wrote:
>> For instance, suppose my UART tries to write to the chardev but
>> can't, so it registers a callback with qemu_chr_fe_add_watch().
>> Then the user stops the VM. Does the chardev UI guarantee that
>> my callback won't get called until the user restarts the VM,
>> or does my callback function have to do something to say "don't
>> actually update the emulation state if we're stopped"?
>> If the user then restarts the VM, do I need to do something to
>> retry the chardev write?
>
> Currently, chardev code runs normally between stop and start.

I think that's rather counterintuitive -- as a user, if I've
stopped the VM I don't expect any of its device state to change.
Stopped should mean "simulation state doesn't change".

>> For migration, if we're migrating in the state where I have a
>> pending character I'd like to write to the chardev, what do I need
>> to do on the destination side? Do I need to call qemu_chr_fe_add_watch
>> in the post-load function, or is this too early and I need to do
>> something to call it when the destination VM is actually restarted?
>
> Post-load is fine.  There could be interesting interactions with
> interrupts, but these are not specific to character device backends
> (real-time clocks are another example).  So boards should create
> interrupt controllers and distributors as soon as possible.

This sounds wrong to me -- nobody should be asserting an
interrupt line during post-load, in the same way that nobody
should assert an interrupt line during reset. We don't have
any guarantees that the device on the other end of the line
has finished migration yet. I think we really need the
chardev backends to not start doing things until load has
finished and the VM has been restarted. (If we make "VM
stopped" mean "no chardev callbacks" then this would fall
out in the wash, though, since the callback registered in
post-load would only trigger on the following VM start.)

thanks
-- PMM

Re: [Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Paolo Bonzini]

On 22/06/2017 12:38, Peter Maydell wrote:
> On 22 June 2017 at 11:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 22/06/2017 12:09, Peter Maydell wrote:
>>> For instance, suppose my UART tries to write to the chardev but
>>> can't, so it registers a callback with qemu_chr_fe_add_watch().
>>> Then the user stops the VM. Does the chardev UI guarantee that
>>> my callback won't get called until the user restarts the VM,
>>> or does my callback function have to do something to say "don't
>>> actually update the emulation state if we're stopped"?
>>> If the user then restarts the VM, do I need to do something to
>>> retry the chardev write?
>>
>> Currently, chardev code runs normally between stop and start.
> 
> I think that's rather counterintuitive -- as a user, if I've
> stopped the VM I don't expect any of its device state to change.
> Stopped should mean "simulation state doesn't change".
> 
>>> For migration, if we're migrating in the state where I have a
>>> pending character I'd like to write to the chardev, what do I need
>>> to do on the destination side? Do I need to call qemu_chr_fe_add_watch
>>> in the post-load function, or is this too early and I need to do
>>> something to call it when the destination VM is actually restarted?
>>
>> Post-load is fine.  There could be interesting interactions with
>> interrupts, but these are not specific to character device backends
>> (real-time clocks are another example).  So boards should create
>> interrupt controllers and distributors as soon as possible.
> 
> This sounds wrong to me -- nobody should be asserting an
> interrupt line during post-load, in the same way that nobody
> should assert an interrupt line during reset. We don't have
> any guarantees that the device on the other end of the line
> has finished migration yet. I think we really need the
> chardev backends to not start doing things until load has
> finished and the VM has been restarted. (If we make "VM
> stopped" mean "no chardev callbacks" then this would fall
> out in the wash, though, since the callback registered in
> post-load would only trigger on the following VM start.)

I don't disagree, and if you found a way to fix all frontends at the
same time that would be great.

However, again this is not exclusive to character devices.  I mentioned
real-time clocks, and I don't see an easy way to fix that.

Paolo

Re: [Qemu-devel] qemu_chr_fe_add_watch interaction with VM start/stop/migrate

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 22 June 2017 at 11:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> > On 22/06/2017 12:09, Peter Maydell wrote:
> >> For instance, suppose my UART tries to write to the chardev but
> >> can't, so it registers a callback with qemu_chr_fe_add_watch().
> >> Then the user stops the VM. Does the chardev UI guarantee that
> >> my callback won't get called until the user restarts the VM,
> >> or does my callback function have to do something to say "don't
> >> actually update the emulation state if we're stopped"?
> >> If the user then restarts the VM, do I need to do something to
> >> retry the chardev write?
> >
> > Currently, chardev code runs normally between stop and start.
> 
> I think that's rather counterintuitive -- as a user, if I've
> stopped the VM I don't expect any of its device state to change.
> Stopped should mean "simulation state doesn't change".

Yes and stuff breaks unless you're very careful (e.g. a23a6d18)
- make sure you don't change RAM state when the guest is stopped
and you should at least survive migration.

> >> For migration, if we're migrating in the state where I have a
> >> pending character I'd like to write to the chardev, what do I need
> >> to do on the destination side? Do I need to call qemu_chr_fe_add_watch
> >> in the post-load function, or is this too early and I need to do
> >> something to call it when the destination VM is actually restarted?
> >
> > Post-load is fine.  There could be interesting interactions with
> > interrupts, but these are not specific to character device backends
> > (real-time clocks are another example).  So boards should create
> > interrupt controllers and distributors as soon as possible.
> 
> This sounds wrong to me -- nobody should be asserting an
> interrupt line during post-load, in the same way that nobody
> should assert an interrupt line during reset. We don't have
> any guarantees that the device on the other end of the line
> has finished migration yet.

I remember someone added something to impose an order to get interrupt
controllers loaded before PCI devices etc - I thought it was ARM interrupt
controller related but I can't find it.

But yes, we've had bugs associated with this type of misordering
in the past;  they're normally terrible to debug because
they can depend on timing, especially if the thing that's
loaded in the post-load starts a timer and the issue is whether
the timer fires before or after the other devices or loaded
and or before the VM starts running. (Replace 'timer' with
'chardev' and 'fires' with 'a character is received)

> I think we really need the
> chardev backends to not start doing things until load has
> finished and the VM has been restarted. (If we make "VM
> stopped" mean "no chardev callbacks" then this would fall
> out in the wash, though, since the callback registered in
> post-load would only trigger on the following VM start.)

That depends on what the chardev is doing;
if it's a chardev to a monitor or something not directly
updating guest state then you'd probably want it to start.
What about chardev's for networking stuff?

Dave

> thanks
> -- PMM
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK