[Qemu-devel] [PATCH 0/2] serial: cleanups for physical serial port passthrough

[Qemu-devel] [PATCH 0/2] serial: cleanups for physical serial port passthrough

[Paolo Bonzini]

Patch 1 ensures that the serial port state is the same at VM startup
and after reset.

Patch 2 ensures that the poll_msl field is computed at reset time
(rather than arbitrarily later), so that it becomes -1 for serial ports
backed by PTYs, sockets, etc.

Please review!

Paolo

Paolo Bonzini (2):
  serial: reset state at startup
  serial: check if backed by a physical serial port at realize time

 hw/char/serial.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] serial: reset state at startup

[Paolo Bonzini]

When a serial port is started, its initial state is all zero.  Make
it consistent with reset state instead.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 764e184..4523ccb 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -668,6 +668,7 @@ void serial_realize_core(SerialState *s, Error **errp)
                           serial_event, s);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
+    serial_reset(s);
 }
 
 void serial_exit_core(SerialState *s)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] serial: reset state at startup

[Chen, Tiejun]

On 2014/9/19 16:54, Paolo Bonzini wrote:
> When a serial port is started, its initial state is all zero.  Make
> it consistent with reset state instead.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/char/serial.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 764e184..4523ccb 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -668,6 +668,7 @@ void serial_realize_core(SerialState *s, Error **errp)
>                             serial_event, s);

It should just follow qemu_register_reset(serial_reset, s).

>       fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
>       fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
> +    serial_reset(s);

Or at least we should push this before this pair of fifo8_create() since

static void serial_reset(void *opaque)
{
     ...
     fifo8_reset(&s->recv_fifo);
     fifo8_reset(&s->xmit_fifo);


Thanks
Tiejun

>   }
>
>   void serial_exit_core(SerialState *s)
>

Re: [Qemu-devel] [PATCH 1/2] serial: reset state at startup

[Paolo Bonzini]

Il 19/09/2014 11:17, Chen, Tiejun ha scritto:
> On 2014/9/19 16:54, Paolo Bonzini wrote:
>> When a serial port is started, its initial state is all zero.  Make
>> it consistent with reset state instead.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   hw/char/serial.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>> index 764e184..4523ccb 100644
>> --- a/hw/char/serial.c
>> +++ b/hw/char/serial.c
>> @@ -668,6 +668,7 @@ void serial_realize_core(SerialState *s, Error
>> **errp)
>>                             serial_event, s);
> 
> It should just follow qemu_register_reset(serial_reset, s).
> 
>>       fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
>>       fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
>> +    serial_reset(s);
> 
> Or at least we should push this before this pair of fifo8_create() since

No, it should be _after_ the fifo8_create() pair.  With the current
implementation it doesn't matter, but first you create something and
then you initialize it, not the other way round.

Paolo

> static void serial_reset(void *opaque)
> {
>     ...
>     fifo8_reset(&s->recv_fifo);
>     fifo8_reset(&s->xmit_fifo);
> 
> 
> Thanks
> Tiejun
> 
>>   }
>>
>>   void serial_exit_core(SerialState *s)
>>
> 
> 

Re: [Qemu-devel] [PATCH 1/2] serial: reset state at startup

[Chen, Tiejun]

On 2014/9/19 20:57, Paolo Bonzini wrote:
> Il 19/09/2014 11:17, Chen, Tiejun ha scritto:
>> On 2014/9/19 16:54, Paolo Bonzini wrote:
>>> When a serial port is started, its initial state is all zero.  Make
>>> it consistent with reset state instead.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    hw/char/serial.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/char/serial.c b/hw/char/serial.c
>>> index 764e184..4523ccb 100644
>>> --- a/hw/char/serial.c
>>> +++ b/hw/char/serial.c
>>> @@ -668,6 +668,7 @@ void serial_realize_core(SerialState *s, Error
>>> **errp)
>>>                              serial_event, s);
>>
>> It should just follow qemu_register_reset(serial_reset, s).
>>
>>>        fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
>>>        fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
>>> +    serial_reset(s);
>>
>> Or at least we should push this before this pair of fifo8_create() since
>
> No, it should be _after_ the fifo8_create() pair.  With the current
> implementation it doesn't matter, but first you create something and

Yes, I took a look at this pair,

void fifo8_create(Fifo8 *fifo, uint32_t capacity)
{
     fifo->data = g_new(uint8_t, capacity);
     fifo->capacity = capacity;
     fifo->head = 0;
     fifo->num = 0;
}

and

void fifo8_reset(Fifo8 *fifo)
{
     fifo->num = 0;
     fifo->head = 0;
}

> then you initialize it, not the other way round.
>

Thanks for your explanation in this case.

Thanks
Tiejun

> Paolo
>
>> static void serial_reset(void *opaque)
>> {
>>      ...
>>      fifo8_reset(&s->recv_fifo);
>>      fifo8_reset(&s->xmit_fifo);
>>
>>
>> Thanks
>> Tiejun
>>
>>>    }
>>>
>>>    void serial_exit_core(SerialState *s)
>>>
>>
>>
>
>

[Qemu-devel] [PATCH 2/2] serial: check if backed by a physical serial port at realize time

[Paolo Bonzini]

Right now, s->poll_msl may linger at "0" value for an arbitrarily long
time, until serial_update_msl is called for the first time.  This is
unnecessary, and will lead to the s->poll_msl field being unnecessarily
migrated.

We can call serial_update_msl immediately at realize time (via
serial_reset) and be done with it.  The memory-mapped UART was already
doing that, but not the ISA and PCI variants.

Regarding the delta bits, be consistent with what serial_reset does when
the serial port is not backed by a physical serial port, and always clear
them at reset time.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 4523ccb..e1dd0c9 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -650,6 +650,9 @@ static void serial_reset(void *opaque)
     s->thr_ipending = 0;
     s->last_break_enable = 0;
     qemu_irq_lower(s->irq);
+
+    serial_update_msl(s);
+    s->msr &= ~UART_MSR_ANY_DELTA;
 }
 
 void serial_realize_core(SerialState *s, Error **errp)
@@ -780,7 +783,5 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
     memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s,
                           "serial", 8 << it_shift);
     memory_region_add_subregion(address_space, base, &s->io);
-
-    serial_update_msl(s);
     return s;
 }
-- 
2.1.0