Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected

[Anthony Liguori <aliguori@us.ibm.com>]

Hans de Goede <hdegoede@redhat.com> writes:

> Hi,
>
> On 03/22/2013 02:50 PM, Anthony Liguori wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>  We should have never allowed that in the first place and
>> I object strongly to extending the concept without making it make sense
>> for everything else.
>>
>>> Frontends end inside the guest usually in the form of some piece of
>>> virtual hardware inside the guest. Just because the virtual hardware
>>> is there does not mean the guest has a driver,
>>
>> Okay, let's use your example here with a standard UART.  In the
>> following sequence, I should receive:
>>
>> 1) Starts guest
>> 2) When guest initializes the UART, qemu_chr_fe_open()
>> 3) Reboot guest
>> 4) Receive qemu_chr_fe_close()
>> 5) Boot new guest without a UART driver
>> 6) Nothing is received
>>
>> But this doesn't happen today because the only backend that cares
>> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write().
>
> 1) If the guest does not have an uart driver, nothing will be written
> to the uart, so it wont call qemu_chr_fe_write and we won't assume
> a qemu_chr_fe_open
>
> 2) But I agree the assuming of qemu_chr_fe_open on the first write is
> a hack, it stems from before we had qemu_chr_fe_open/_close and now that
> we do have we should remove it.

If the qemu-char.c code did:

int qemu_chr_fe_write(...) {
    if (!s->fe_open) {
        qemu_chr_fe_open(s);
    }
    ...
}

That would be one thing.  It's a hack, but a more reasonable hack than
doing this in the backend like we do today.

And in fact, herein lies exactly what the problem is.  There is no
"s->fe_open".  And if such a thing did exist, we would note that this is
in fact guest state and that it needs to be taken care of during
migration.

> Note btw that many backends also don't handle sending CHR_EVENT_OPENED /
> _CLOSED themselves, they simply use qemu_chr_generic_open and that generated
> the opened event once on creation of the device. So the concept is just
> as broken / not broken on the backend side.

I know :-/

> We could do the same and have a qemu_fe_generic_open for frontends which
> does the qemu_chr_fe_open call.

You mean, in qemu_chr_fe_write()?  Yes, I think not doing this bit in
the backend and making it part of qemu-char would be a significant
improvement and would also guide in solving this problem correctly.

Also, you can get reset handling for free by unconditionally clearly
s->fe_open with a reset handler.  That would also simplify the
virtio-serial code since it would no longer need the reset logic.

>> And for me, the most logical thing is to call qemu_chr_fe_open() in
>> post_load for the device.
>
> With the device I assume you mean the frontend? That is what we originally
> suggested and submitted a patch for (for virtio-console) but Amit didn't
> like it.

As Avi would say, this is "derived guest state" and derived guest state
has to be recomputed in post_load handlers.

Regards,

Anthony Liguori

>
> Regards,
>
> Hans