Re: [Xen-devel] [PATCH 26/27 v12] arm/xen: vpl011: Fix the slow early console SBSA UART output

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 18/10/17 11:17, Bhupinder Thakur wrote:
> Hi Andre,
> 
> On 17 October 2017 at 15:21, Andre Przywara <andre.przywara@xxxxxxx> wrote:
>> Hi Bhupinder,
>>
>> first thing: As the bulk of the series has been merged now, please
>> restart your patch and version numbering, so a (potential) next post
>> should be prefixed [PATCH v3 1/2]. And please have a cover letter giving
>> a brief overview what this series fixes.
>>
> Should I resend the patch series with a cover letter? I will also add
> a reported-by tag.

Please wait until we have settled upon a solution, especially for that
other patch. We can talk about this in our meeting later today.

Cheers,
Andre.

>> On 13/10/17 11:40, Bhupinder Thakur wrote:
>>> The early console output uses pl011_early_write() to write data. This
>>> function waits for BUSY bit to get cleared before writing the next byte.
>>
>> ... which is questionable given the actual definition of the BUSY bit in
>> the PL011 TRM:
>>
>> ============
>> .... The BUSY signal goes HIGH as soon as data is written to the
>> transmit FIFO (that is, the FIFO is non-empty) and remains asserted
>> HIGH while data is being transmitted. BUSY is negated only when the
>> transmit FIFO is empty, and the last character has been transmitted from
>> the shift register, ....
>> ============
>>
>> (I take it you are talking about the Linux driver in a guest here).
>> I think the early_write routine tries to (deliberately?) ignore the
>> FIFO, possibly to make sure characters really get pushed out before a
>> system crashes, maybe.
>>
>>>
>>> In the SBSA UART emulation logic, the BUSY bit was set as soon one
>>> byte was written in the FIFO and it remained set until the FIFO was
>>> emptied.
>>
>> Which is correct behaviour, as this matches the PL011 TRM as quoted above.
>>
>>> This meant that the output was delayed as each character needed
>>> the BUSY to get cleared.
>>
>> But this is true as well!
>>
>>> Since the SBSA UART is getting emulated in Xen using ring buffers, it
>>> ensures that once the data is enqueued in the FIFO, it will be received
>>> by xenconsole so it is safe to set the BUSY bit only when FIFO becomes
>>> full. This will ensure that pl011_early_write() is not delayed unduly
>>> to write the data.
>>
>> So I can confirm that this patch fixes the very slow earlycon output
>> observed with the current staging HEAD.
>>
>> So while this is somewhat deviating from the spec, I can see the benefit
>> for an emulation scenario. I believe that emulations in general might
>> choose implementing things a bit differently, to cope with the
>> fundamental differences in their environment, like the virtually endless
>> "FIFO" and the lack of any timing restrictions on the emulated "wire".
>>
>> So unless someone comes up with a better solution, I would support
>> taking this patch, as this fixes a real problem.
>>
>> Cheers,
>> Andre
>>
>>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
>>> ---
>>> CC: Julien Grall <julien.grall@xxxxxxx>
>>> CC: Andre Przywara <andre.przywara@xxxxxxx>
>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>
>>>  xen/arch/arm/vpl011.c | 21 ++++++++++++++++-----
>>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
>>> index f7ddccb..0b07436 100644
>>> --- a/xen/arch/arm/vpl011.c
>>> +++ b/xen/arch/arm/vpl011.c
>>> @@ -159,9 +159,15 @@ static void vpl011_write_data(struct domain *d, 
>>> uint8_t data)
>>>      {
>>>          vpl011->uartfr |= TXFF;
>>>          vpl011->uartris &= ~TXI;
>>> -    }
>>>
>>> -    vpl011->uartfr |= BUSY;
>>> +        /*
>>> +         * This bit is set only when FIFO becomes full. This ensures that
>>> +         * the SBSA UART driver can write the early console data as fast as
>>> +         * possible, without waiting for the BUSY bit to get cleared before
>>> +         * writing each byte.
>>> +         */
>>> +        vpl011->uartfr |= BUSY;
>>> +    }
>>>
>>>      vpl011->uartfr &= ~TXFE;
>>>
>>> @@ -371,11 +377,16 @@ static void vpl011_data_avail(struct domain *d)
>>>      {
>>>          vpl011->uartfr &= ~TXFF;
>>>          vpl011->uartris |= TXI;
>>> +
>>> +        /*
>>> +         * Clear the BUSY bit as soon as space becomes available
>>> +         * so that the SBSA UART driver can start writing more data
>>> +         * without any further delay.
>>> +         */
>>> +        vpl011->uartfr &= ~BUSY;
>>> +
>>>          if ( out_ring_qsize == 0 )
>>> -        {
>>> -            vpl011->uartfr &= ~BUSY;
>>>              vpl011->uartfr |= TXFE;
>>> -        }
>>>      }
>>>
>>>      vpl011_update_interrupt_status(d);
>>>
> 
> Regards,
> Bhupinder
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel