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

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

[Wei Xu]

If the user pressed some keys in the console during the guest booting,
the console will be hanged after entering the shell.
Because in the above case the pl011_can_receive will return 0 that
the pl011_receive will not be called. That means no interruption will
be injected in to the kernel and the pl011 state could not be driven
further.

This patch fixed that issue by checking the interruption is enabled or
not before putting into the fifo.

Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
 hw/char/pl011.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 2aa277fc4f..6296de9527 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -229,6 +229,8 @@ static int pl011_can_receive(void *opaque)
     PL011State *s = (PL011State *)opaque;
     int r;

+    if (!s->int_enabled)
+	return 0;
     if (s->lcr & 0x10) {
         r = s->read_count < 16;
     } else {
-- 
2.11.0

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

[Peter Maydell]

On 26 January 2018 at 16:00, Wei Xu <xuwei5@hisilicon.com> wrote:
> If the user pressed some keys in the console during the guest booting,
> the console will be hanged after entering the shell.
> Because in the above case the pl011_can_receive will return 0 that
> the pl011_receive will not be called. That means no interruption will
> be injected in to the kernel and the pl011 state could not be driven
> further.
>
> This patch fixed that issue by checking the interruption is enabled or
> not before putting into the fifo.
>
> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> ---
>  hw/char/pl011.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 2aa277fc4f..6296de9527 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -229,6 +229,8 @@ static int pl011_can_receive(void *opaque)
>      PL011State *s = (PL011State *)opaque;
>      int r;
>
> +    if (!s->int_enabled)
> +       return 0;
>      if (s->lcr & 0x10) {
>          r = s->read_count < 16;
>      } else {
> --

This doesn't look right. You should be able to use the PL011
in a strictly polling mode, without ever enabling interrupts.
Returning false in can_receive if interrupts are disabled
would break that.

If the user presses keys before interrupts are enabled,
what ought to happen is:
 * we put the key in the FIFO, and update the int_level flags
 * when the FIFO is full, can_receive starts returning 0 and
   QEMU stops passing us new characters
 * when the guest driver for the pl011 initializes the
   device and enables interrupts then either:
    (a) it does something that clears the FIFO, which will
    mean can_receive starts allowing new chars again, or
    (b) it leaves the FIFO as it is, and we should thus
    immediately raise an interrupt for the characters still
    in the FIFO; when the guest handles this interrupt and
    gets the characters, can_receive will permit new ones

What is happening in your situation that means this is not
working as expected ?

thanks
-- PMM

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

[Wei Xu]

Hi Peter,

On 2018/1/26 16:36, Peter Maydell wrote:
> On 26 January 2018 at 16:00, Wei Xu <xuwei5@hisilicon.com> wrote:
>> If the user pressed some keys in the console during the guest booting,
>> the console will be hanged after entering the shell.
>> Because in the above case the pl011_can_receive will return 0 that
>> the pl011_receive will not be called. That means no interruption will
>> be injected in to the kernel and the pl011 state could not be driven
>> further.
>>
>> This patch fixed that issue by checking the interruption is enabled or
>> not before putting into the fifo.
>>
>> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
>> ---
>>  hw/char/pl011.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 2aa277fc4f..6296de9527 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -229,6 +229,8 @@ static int pl011_can_receive(void *opaque)
>>      PL011State *s = (PL011State *)opaque;
>>      int r;
>>
>> +    if (!s->int_enabled)
>> +       return 0;
>>      if (s->lcr & 0x10) {
>>          r = s->read_count < 16;
>>      } else {
>> --
> 
> This doesn't look right. You should be able to use the PL011
> in a strictly polling mode, without ever enabling interrupts.
> Returning false in can_receive if interrupts are disabled
> would break that.
> 
> If the user presses keys before interrupts are enabled,
> what ought to happen is:
>  * we put the key in the FIFO, and update the int_level flags
>  * when the FIFO is full, can_receive starts returning 0 and
>    QEMU stops passing us new characters
>  * when the guest driver for the pl011 initializes the
>    device and enables interrupts then either:
>     (a) it does something that clears the FIFO, which will
>     mean can_receive starts allowing new chars again, or
>     (b) it leaves the FIFO as it is, and we should thus
>     immediately raise an interrupt for the characters still
>     in the FIFO; when the guest handles this interrupt and
>     gets the characters, can_receive will permit new ones
>

Yes, now it is handled like b.

> What is happening in your situation that means this is not
> working as expected ?

But in the kernel side, the pll011 is triggered as a level interruption.
During the booting, if any key is pressed ,the call stack is as below:
QEMU side:
pl011_update
-->qemu_set_irq(level as 0)
---->kvm_arm_gic_set_irq

Kernel side:
kvm_vm_ioctl_irq_line
-->kvm_vgic_inject_irq
---->vgic_validate_injection (if level did not change, return)
---->vgic_queue_irq_unlock

Without above changes, in the vgic_validate_injection, because the
interruption level is always 0, this irq will not be queued into vgic.
And the guest will not read the pl011 fifo.

Best Regards,
Wei

> 
> thanks
> -- PMM
> 
> .
> 

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

[Peter Maydell]

On 26 January 2018 at 17:05, Wei Xu <xuwei5@hisilicon.com> wrote:
> On 2018/1/26 16:36, Peter Maydell wrote:
>> If the user presses keys before interrupts are enabled,
>> what ought to happen is:
>>  * we put the key in the FIFO, and update the int_level flags
>>  * when the FIFO is full, can_receive starts returning 0 and
>>    QEMU stops passing us new characters
>>  * when the guest driver for the pl011 initializes the
>>    device and enables interrupts then either:
>>     (a) it does something that clears the FIFO, which will
>>     mean can_receive starts allowing new chars again, or
>>     (b) it leaves the FIFO as it is, and we should thus
>>     immediately raise an interrupt for the characters still
>>     in the FIFO; when the guest handles this interrupt and
>>     gets the characters, can_receive will permit new ones
>>
>
> Yes, now it is handled like b.
>
>> What is happening in your situation that means this is not
>> working as expected ?
>
> But in the kernel side, the pll011 is triggered as a level interruption.
> During the booting, if any key is pressed ,the call stack is as below:
> QEMU side:
> pl011_update
> -->qemu_set_irq(level as 0)
> ---->kvm_arm_gic_set_irq
>
> Kernel side:
> kvm_vm_ioctl_irq_line
> -->kvm_vgic_inject_irq
> ---->vgic_validate_injection (if level did not change, return)
> ---->vgic_queue_irq_unlock
>
> Without above changes, in the vgic_validate_injection, because the
> interruption level is always 0, this irq will not be queued into vgic.
> And the guest will not read the pl011 fifo.

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.

thanks
-- PMM

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

[Wei Xu]

Hi Peter,

On 2018/1/26 17:15, Peter Maydell wrote:
> On 26 January 2018 at 17:05, Wei Xu <xuwei5@hisilicon.com> wrote:
>> On 2018/1/26 16:36, Peter Maydell wrote:
>>> If the user presses keys before interrupts are enabled,
>>> what ought to happen is:
>>>  * we put the key in the FIFO, and update the int_level flags
>>>  * when the FIFO is full, can_receive starts returning 0 and
>>>    QEMU stops passing us new characters
>>>  * when the guest driver for the pl011 initializes the
>>>    device and enables interrupts then either:
>>>     (a) it does something that clears the FIFO, which will
>>>     mean can_receive starts allowing new chars again, or
>>>     (b) it leaves the FIFO as it is, and we should thus
>>>     immediately raise an interrupt for the characters still
>>>     in the FIFO; when the guest handles this interrupt and
>>>     gets the characters, can_receive will permit new ones
>>>
>>
>> Yes, now it is handled like b.
>>
>>> What is happening in your situation that means this is not
>>> working as expected ?
>>
>> But in the kernel side, the pll011 is triggered as a level interruption.
>> During the booting, if any key is pressed ,the call stack is as below:
>> QEMU side:
>> pl011_update
>> -->qemu_set_irq(level as 0)
>> ---->kvm_arm_gic_set_irq
>>
>> Kernel side:
>> kvm_vm_ioctl_irq_line
>> -->kvm_vgic_inject_irq
>> ---->vgic_validate_injection (if level did not change, return)
>> ---->vgic_queue_irq_unlock
>>
>> Without above changes, in the vgic_validate_injection, because the
>> interruption level is always 0, this irq will not be queued into vgic.
>> And the guest will not read the pl011 fifo.
> 
> 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.
How do you think about like that?
Thanks!

Best Regards,
Wei

> thanks
> -- PMM
> 
> .
> 

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

[Peter Maydell]

On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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?

thanks
-- PMM

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 5A6B5091.8030601@hisilicon.com
Subject: [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo before enabled the interruption

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/5A6B5091.8030601@hisilicon.com -> patchew/5A6B5091.8030601@hisilicon.com
Switched to a new branch 'test'
aac404070b pl011: do not put into fifo before enabled the interruption

=== OUTPUT BEGIN ===
Checking PATCH 1/1: pl011: do not put into fifo before enabled the interruption...
ERROR: braces {} are necessary for all arms of this statement
#27: FILE: hw/char/pl011.c:232:
+    if (!s->int_enabled)
[...]

ERROR: code indent should never use tabs
#28: FILE: hw/char/pl011.c:233:
+^Ireturn 0;$

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Andrew Jones]

On Fri, Jan 26, 2018 at 06:01:33PM +0000, Peter Maydell wrote:
> On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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. I'm not sure if this issue is due to the SBSA attempting to
be overly simple, or something the Linux driver can deal with. See
this thread for a discussion I started once.

https://www.spinics.net/lists/linux-serial/msg23163.html

Wei,

I assume you're using UEFI/ACPI when booting, as I don't recall this
problem occurring with the Linux PL011 driver which would be used
when booting with DT.

Thanks,
drew

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

[Peter Maydell]

On 29 January 2018 at 10:29, Andrew Jones <drjones@redhat.com> wrote:
> On Fri, Jan 26, 2018 at 06:01:33PM +0000, Peter Maydell wrote:
>> On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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

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

[Wei Xu]

Hi Andrew,

On 2018/1/29 10:29, Andrew Jones wrote:
> On Fri, Jan 26, 2018 at 06:01:33PM +0000, Peter Maydell wrote:
>> On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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. I'm not sure if this issue is due to the SBSA attempting to
> be overly simple, or something the Linux driver can deal with. See
> this thread for a discussion I started once.
> 
> https://www.spinics.net/lists/linux-serial/msg23163.html

I am not sure it is the same problem or not.
I will check that.
Thanks!

> 
> Wei,
> 
> I assume you're using UEFI/ACPI when booting, as I don't recall this
> problem occurring with the Linux PL011 driver which would be used
> when booting with DT.
>

I am using an ARM64 board, the guest is booted *without* UEFI but the
host is booted with UEFI/ACPI.
The command I am using is as below:
	"qemu-system-aarch64 -enable-kvm -m 1024 -cpu host -M virt \
	-nographic --kernel Image --initrd roofs.cpio.gz"

Thanks!

Best Regards,
Wei

> Thanks,
> drew
> 
> .
> 

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

[Wei Xu]

Hi Peter,

On 2018/1/26 18:01, Peter Maydell wrote:
> On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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?

Yes. When the guest enabling the interrupts, the pl011 driver in
the kernel will clear the RX interrupts[1].
And pasted the code below to make it easy to read.

	static void pl011_enable_interrupts(struct uart_amba_port *uap)
	{
		spin_lock_irq(&uap->port.lock);

		/* Clear out any spuriously appearing RX interrupts */
		pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
		uap->im = UART011_RTIM;
		if (!pl011_dma_rx_running(uap))
		uap->im |= UART011_RXIM;
		pl011_write(uap->im, uap, REG_IMSC);
		spin_unlock_irq(&uap->port.lock);
	}

I tried kept the RXIS in the kernel side to test and found the issue is still there.
A little confused now :(

[1]: https://elixir.free-electrons.com/linux/latest/source/drivers/tty/serial/amba-pl011.c#L1732

Best Regards,
Wei

> 
> thanks
> -- PMM
> 
> .
> 

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

[Andrew Jones]

On Mon, Jan 29, 2018 at 11:37:05AM +0000, Wei Xu wrote:
> Hi Andrew,
> 
> On 2018/1/29 10:29, Andrew Jones wrote:
> > On Fri, Jan 26, 2018 at 06:01:33PM +0000, Peter Maydell wrote:
> >> On 26 January 2018 at 17:33, Wei Xu <xuwei5@hisilicon.com> 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. I'm not sure if this issue is due to the SBSA attempting to
> > be overly simple, or something the Linux driver can deal with. See
> > this thread for a discussion I started once.
> > 
> > https://www.spinics.net/lists/linux-serial/msg23163.html
> 
> I am not sure it is the same problem or not.
> I will check that.
> Thanks!
> 
> > 
> > Wei,
> > 
> > I assume you're using UEFI/ACPI when booting, as I don't recall this
> > problem occurring with the Linux PL011 driver which would be used
> > when booting with DT.
> >
> 
> I am using an ARM64 board, the guest is booted *without* UEFI but the
> host is booted with UEFI/ACPI.
> The command I am using is as below:
> 	"qemu-system-aarch64 -enable-kvm -m 1024 -cpu host -M virt \
> 	-nographic --kernel Image --initrd roofs.cpio.gz"

Hmm. This means you're booting the guest with DT, which means the
Linux driver is the PL011 driver, not the SBSA driver. I didn't
recall that driver having the issue, but maybe something changed.

Thanks,
drew

> 
> Thanks!
> 
> Best Regards,
> Wei
> 
> > Thanks,
> > drew
> > 
> > .
> > 
> 
> 

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

[Peter Maydell]

On 29 January 2018 at 12:18, Wei Xu <xuwei5@hisilicon.com> wrote:
> On 2018/1/26 18:01, Peter Maydell wrote:
>> 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?
>
> Yes. When the guest enabling the interrupts, the pl011 driver in
> the kernel will clear the RX interrupts[1].
> And pasted the code below to make it easy to read.
>
>         static void pl011_enable_interrupts(struct uart_amba_port *uap)
>         {
>                 spin_lock_irq(&uap->port.lock);
>
>                 /* Clear out any spuriously appearing RX interrupts */
>                 pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
>                 uap->im = UART011_RTIM;
>                 if (!pl011_dma_rx_running(uap))
>                 uap->im |= UART011_RXIM;
>                 pl011_write(uap->im, uap, REG_IMSC);
>                 spin_unlock_irq(&uap->port.lock);
>         }
>

This seems at first glance like a kernel driver bug to me -- if it is
explicitly killing off the pending interrupt without handling
it then it's naturally going to get stuck if characters come
in in the window before it does that.

(It wouldn't surprise me if there's something in the driver
init sequence that has the effect of clearing the FIFO on
real hardware but not QEMU, which would mean that on real
h/w the window where this can happen would be small, but it's
still there.)

thanks
-- PMM