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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

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.

> 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