pl011: do not put into fifo before enabled the interruption

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 <[email protected]>

---
 hw/char/pl011.c | 2 ++
 1 file changed, 2 insertions(+)

hw/char/pl011.c +0 -0

index 2aa277fc4f..6296de9527 100644

Quoted 20 lines · 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 <[email protected]>

---
 hw/char/pl011.c | 2 ++
 1 file changed, 2 insertions(+)

hw/char/pl011.c +0 -0

index 2aa277fc4f..6296de9527 100644

hw/char/pl011.c static int pl011_can_receive(void *opaque)

Quoted 9 lines

hw/char/pl011.c +2 -1

@@ -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

.

Quoted 5 lines · Peter Maydell

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

Quoted 9 lines

* 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.

Quoted 2 lines

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.

Quoted 34 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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

Quoted 9 lines

* 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.

Quoted 2 lines

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

Quoted 3 lines

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

.

Quoted 5 lines · Peter Maydell

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.

Quoted 12 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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.

Quoted 4 lines

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?

Quoted 22 lines · Peter Maydell

Quoted 12 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 18 lines · Peter Maydell

Quoted 12 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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.

Quoted 4 lines

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.

Quoted 7 lines

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

.

Hi Andrew,

Quoted 27 lines · Andrew Jones

Quoted 18 lines · Peter Maydell

Quoted 12 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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.

Quoted 4 lines

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.

Quoted 7 lines

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?

Quoted 8 lines

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!

Quoted 7 lines

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

Quoted 5 lines

Thanks,
drew

.

Quoted 12 lines · Wei Xu

Quoted 5 lines · Peter Maydell

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.

Quoted 4 lines

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

.

Quoted 7 lines · Peter Maydell

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);
        }