| Message ID | 5A6B5091.8030601@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Qemu-arm] pl011: do not put into fifo before enabled the interruption | expand |
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
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
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
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 > > . >
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 > > . >
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 > > > > . > > > >
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
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 {
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(+) -- 2.11.0