Re: [Qemu-arm] [Qemu-devel] [PATCH] pl011: do not put into fifo before enabled the interruption

[Peter Maydell]

On 29 January 2018 at 10:29, Andrew Jones <address@hidden> wrote:
> On Fri, Jan 26, 2018 at 06:01:33PM +0000, Peter Maydell wrote:
>> On 26 January 2018 at 17:33, Wei Xu <address@hidden> wrote:
>> > On 2018/1/26 17:15, Peter Maydell wrote:
>> >> The pl011 code should call qemu_set_irq(..., 1) when the
>> >> guest enables interrupts on the device by writing to the int_enabled
>> >> (UARTIMSC) register. That will be a 0-to-1 level change and the KVM
>> >> VGIC should report the interrupt to the guest.
>> >>
>> >
>> > Yes.
>> > And in the pl011_update, the irq level is set by s->int_level & 
>> > s->int_enabled.
>> > When writing to the int_enabled, not sure why the int_level is set to
>> > 0x20(PL011_INT_TX) but int_enabled is 0x50.
>> >
>> > It still call qemu_set_irq(..., 0).
>> >
>> > I added "s->int_level |= PL011_INT_RX" before calling pl011_update
>> > when writing to the int_enabled and tested it also works.
>>
>> No, that's not right either. int_level should already have the
>> RX bit set, because pl011_put_fifo() sets that bit when it gets a
>> character from QEMU and puts it into the FIFO.
>>
>> Does something else clear the int_level between the character
>> going into the FIFO from QEMU and the guest enabling
>> interrupts?
>
> As part of the boot process Linux restarts the UART a few times. When
> Linux drives the PL011 with the SBSA driver then the FIFO doesn't get
> reset prior to being used again, as the SBSA doesn't specify a way to
> do that.

Right, but the FIFO not being cleared shouldn't be a problem --
if the FIFO is still full then the int_level should still
indicate that so that when the Linux driver enables interrupts
it takes an interrupt (and the handler should then drain the
FIFO by reading characters from it).

It seems likely that there's a bug in QEMU's pl011 model (this doesn't
happen with real hardware PL011s, I assume) -- but we need to find
out what the divergence from the hardware is, rather than making
changes which happen to fix the symptoms without having first
nailed down what the underlying cause is.

thanks
-- PMM