Re: [PATCH 3/3] hw/watchdog/wdt_imx2: Remove redundant assignment
On 5/13/24 03:11, Bernhard Beschow wrote:
The same statement is executed unconditionally right before the if statement.
Cc: Guenter Roeck
Signed-off-by: Bernhard Beschow
---
The duplicate line may indicate a bug. I'm not familiar with the code, so this
patch may go into the wrong direction. Please check!
Should be ok. Technically the function should not be called to start with
if the watchdog isn't running. If it is, it might be useful to trace the content
of wcr and try to determine why the timer isn't stopped if / when the watchdog
is disabled.
Reviewed-by: Guenter Roeck
Thanks,
Guenter
---
hw/watchdog/wdt_imx2.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/watchdog/wdt_imx2.c b/hw/watchdog/wdt_imx2.c
index 6452fc4721..f9a7ea287f 100644
--- a/hw/watchdog/wdt_imx2.c
+++ b/hw/watchdog/wdt_imx2.c
@@ -39,7 +39,6 @@ static void imx2_wdt_expired(void *opaque)
/* Perform watchdog action if watchdog is enabled */
if (s->wcr & IMX2_WDT_WCR_WDE) {
-s->wrsr = IMX2_WDT_WRSR_TOUT;
watchdog_perform_action();
}
}
Re: [PATCH v3 07/11] hw/sh4/r2d: Realize IDE controller before accessing it
Hi, On Thu, Feb 08, 2024 at 07:12:40PM +0100, Philippe Mathieu-Daudé wrote: > We should not wire IRQs on unrealized device. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Peter Maydell > Reviewed-by: Yoshinori Sato qemu 9.0 fails to boot Linux from ide/ata drives with the sh4 and sh4eb emulations. Error log is as follows. ata1.00: ATA-7: QEMU HARDDISK, 2.5+, max UDMA/100 ata1.00: 16384 sectors, multi 16: LBA48 ata1.00: configured for PIO scsi 0:0:0:0: Direct-Access ATA QEMU HARDDISK2.5+ PQ: 0 ANSI: 5 sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB) sd 0:0:0:0: [sda] Write Protect is off sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA ata1: lost interrupt (Status 0x58) [ and more similar errors ] qemu command line: qemu-system-sh4eb -M r2d -kernel arch/sh/boot/zImage \ -snapshot -drive file=rootfs.ext2,format=raw,if=ide \ -append "root=/dev/sda console=ttySC1,115200 noiotrap" \ -serial null -serial stdio -monitor null -nographic -no-reboot Bisect points to this patch (see below). Reverting it fixes the problem. Guenter --- bisect log: # bad: [c25df57ae8f9fe1c72eee2dab37d76d904ac382e] Update version for 9.0.0 release # good: [1600b9f46b1bd08b00fe86c46ef6dbb48cbe10d6] Update version for v8.2.0 release git bisect start 'v9.0.0' 'v8.2.0' # good: [62357c047a5abc6ede992159ed7c0aaaeb50617a] Merge tag 'qemu-sparc-20240213' of https://github.com/mcayland/qemu into staging git bisect good 62357c047a5abc6ede992159ed7c0aaaeb50617a # bad: [d65f1ed7de1559534d0a1fabca5bdd81c594c7ca] docs/acpi/bits: add some clarity and details while also improving formating git bisect bad d65f1ed7de1559534d0a1fabca5bdd81c594c7ca # bad: [99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c] hw/i386/pc: Populate RTC attribute directly git bisect bad 99e1c1137b6f339be1e4b76e243ad7b7c3d3cb8c # bad: [760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0] Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging git bisect bad 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0 # good: [f2b4a98930c122648e9dc494e49cea5dffbcc2be] target/arm: Allow access to SPSR_hyp from hyp mode git bisect good f2b4a98930c122648e9dc494e49cea5dffbcc2be # bad: [1a8e2f58c5dd721086284f827326b370d19ad9eb] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object git bisect bad 1a8e2f58c5dd721086284f827326b370d19ad9eb # good: [59ae6bcddc3651b55b96c2bf05a6cd4312e46d10] hw/ppc/prep: Realize ISA bridge before accessing it git bisect good 59ae6bcddc3651b55b96c2bf05a6cd4312e46d10 # bad: [7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef] hw/intc/grlib_irqmp: implements the multiprocessor status register git bisect bad 7ed9a5f626a6c932a8c869a91e6a8b3e2029f5ef # bad: [d08b7af3f7f27f6f3da8446756bf0b9352026b1d] target/sparc: Provide hint about CPUSPARCState::irq_manager member git bisect bad d08b7af3f7f27f6f3da8446756bf0b9352026b1d # bad: [5e37bc4997c32a1c9a6621a060462c84df9f1b8f] hw/dma: Pass parent object to i8257_dma_init() git bisect bad 5e37bc4997c32a1c9a6621a060462c84df9f1b8f # bad: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it git bisect bad 3c5f86a22686ef475a8259c0d8ee714f61c770c9 # good: [fc432ba0f58343c8912b80e9056315bb9bd8df92] hw/misc/macio: Realize IDE controller before accessing it git bisect good fc432ba0f58343c8912b80e9056315bb9bd8df92 # first bad commit: [3c5f86a22686ef475a8259c0d8ee714f61c770c9] hw/sh4/r2d: Realize IDE controller before accessing it
Re: Problems (timeouts) when testing usb-ohci with qemu
On 4/24/24 08:23, Guenter Roeck wrote:
On 4/24/24 04:16, Gerd Hoffmann wrote:
qemu hack:
hw/usb/hcd-ohci.c | 11 +++
hw/usb/hcd-ohci.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index fc8fc91a1d..99e52ad13a 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -267,6 +267,10 @@ static inline void ohci_intr_update(OHCIState *ohci)
(ohci->intr_status & ohci->intr))
level = 1;
+ if (level && ohci->level)
+ qemu_set_irq(ohci->irq, 0);
+
+ ohci->level = level;
qemu_set_irq(ohci->irq, level);
}
diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index e1827227ac..6f82e72bd9 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -52,6 +52,7 @@ struct OHCIState {
uint32_t ctl, status;
uint32_t intr_status;
uint32_t intr;
+ int level;
/* memory pointer partition */
uint32_t hcca;
Phew. Disclaimer: Havn't looked at the ohci emulation code for years.
It should not be needed to store the interrupt level that way. It is
possible to calculate what the interrupt level should be, based on the
interrupt status register and the interrupt mask register, and the code
above seems to do exactly that (the "ohci->intr_status & ohci->intr"
bit).
You are correct. For the purpose of this kludge a simpler
+ qemu_set_irq(ohci->irq, 0);
qemu_set_irq(ohci->irq, level);
would have been sufficient. My original code added tracing,
where this generated a lot of noise. I didn't completely simplify
the kludge. Sorry for that and for any confusion it may have caused.
ohci_intr_update() must be called if one of these two registers changes,
i.e. if the guest changes the mask, if the guest acks an IRQ by clearing
an status bit, if the device raises an IRQ by setting an status bit.
Might be the ohci emulation has a bug here.
Another possible trouble spot is that the timing behavior is different
on virtual vs. physical hardware. Specifically with the emulated
hardware some actions appear to complete instantly (when the vmexit to
handle the mmio register write returns it's finished already), which
will never complete that quickly on physical hardware. So drivers can
have race conditions which only trigger on virtual hardware. The error
pattern you are describing sounds like this could be the case here.
I think the underlying problem is that both the qemu emulation and
the Linux kernel driver expect that the interrupt is level triggered.
It looks like some entity in between handles the interrupts as edge
triggered. This makes the kludge necessary: All it does is to generate
an artificial interrupt edge.
This can be worked around in the Linux interrupt handler by checking
if another interrupt arrived while the original interrupt was handled.
This will ensure that all interrupts are handled and cleared when the
handler exits, and that a later arriving interrupt will generate the
necessary edge and thus another interrupt. That doesn't fix the
edge<->level triggered interrupt confusion (if that is indeed the root
cause of the problem), but it does address its consequences.
If anyone has an idea how to find out where the interrupt confusion
happens, please let me know, and I'll be happy to do some more testing.
I am quite curious myself, and it would make sense to solve the problem
at its root.
Update:
I found upstream commit 0b60557230ad ("usb: ehci: Prevent missed ehci
interrupts with edge-triggered MSI") which I think explains the problem.
Guenter
Re: Problems (timeouts) when testing usb-ohci with qemu
On 4/24/24 04:16, Gerd Hoffmann wrote:
qemu hack:
hw/usb/hcd-ohci.c | 11 +++
hw/usb/hcd-ohci.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index fc8fc91a1d..99e52ad13a 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -267,6 +267,10 @@ static inline void ohci_intr_update(OHCIState *ohci)
(ohci->intr_status & ohci->intr))
level = 1;
+if (level && ohci->level)
+qemu_set_irq(ohci->irq, 0);
+
+ohci->level = level;
qemu_set_irq(ohci->irq, level);
}
diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index e1827227ac..6f82e72bd9 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -52,6 +52,7 @@ struct OHCIState {
uint32_t ctl, status;
uint32_t intr_status;
uint32_t intr;
+int level;
/* memory pointer partition */
uint32_t hcca;
Phew. Disclaimer: Havn't looked at the ohci emulation code for years.
It should not be needed to store the interrupt level that way. It is
possible to calculate what the interrupt level should be, based on the
interrupt status register and the interrupt mask register, and the code
above seems to do exactly that (the "ohci->intr_status & ohci->intr"
bit).
You are correct. For the purpose of this kludge a simpler
+ qemu_set_irq(ohci->irq, 0);
qemu_set_irq(ohci->irq, level);
would have been sufficient. My original code added tracing,
where this generated a lot of noise. I didn't completely simplify
the kludge. Sorry for that and for any confusion it may have caused.
ohci_intr_update() must be called if one of these two registers changes,
i.e. if the guest changes the mask, if the guest acks an IRQ by clearing
an status bit, if the device raises an IRQ by setting an status bit.
Might be the ohci emulation has a bug here.
Another possible trouble spot is that the timing behavior is different
on virtual vs. physical hardware. Specifically with the emulated
hardware some actions appear to complete instantly (when the vmexit to
handle the mmio register write returns it's finished already), which
will never complete that quickly on physical hardware. So drivers can
have race conditions which only trigger on virtual hardware. The error
pattern you are describing sounds like this could be the case here.
I think the underlying problem is that both the qemu emulation and
the Linux kernel driver expect that the interrupt is level triggered.
It looks like some entity in between handles the interrupts as edge
triggered. This makes the kludge necessary: All it does is to generate
an artificial interrupt edge.
This can be worked around in the Linux interrupt handler by checking
if another interrupt arrived while the original interrupt was handled.
This will ensure that all interrupts are handled and cleared when the
handler exits, and that a later arriving interrupt will generate the
necessary edge and thus another interrupt. That doesn't fix the
edge<->level triggered interrupt confusion (if that is indeed the root
cause of the problem), but it does address its consequences.
If anyone has an idea how to find out where the interrupt confusion
happens, please let me know, and I'll be happy to do some more testing.
I am quite curious myself, and it would make sense to solve the problem
at its root.
Thanks,
Guenter
Re: Problems (timeouts) when testing usb-ohci with qemu
On 4/23/24 19:11, Alan Stern wrote: [ ... ] To avoid the overhead of repeated interrupts, it would be best to check the interrupt status at the end of the routine and restart if any of the enabled bits are set, as in your first patch. If you would like to clean it up (get rid of the debugging stuff) and submit it, I'll review it. Sure, I'll do that. Thanks, Guenter
Re: Problems (timeouts) when testing usb-ohci with qemu
Hi Alan,
On 4/23/24 10:30, Alan Stern wrote:
On Tue, Apr 23, 2024 at 10:04:17AM -0700, Guenter Roeck wrote:
Hi,
when testing usb-ohci
What is usb-ohci? Do you mean ohci-hcd?
with qemu's pci-ohci emulation, I keep getting
random usb interface timeouts. Sometimes the usb_hub_wq times out.
...
Sometimes there is an i/o scheduling timeout such as
...
This is not a new problem; I have seen it forever. Recently I spent some
time trying to understand the problem and found that the linux driver does
not always handle all ohci interupts.
Please be more specific: _Which_ interrupts aren't being handled? That
is, which flags remain set in the intrstatus register when the handler
returns?
Sorry, I didn't think it was relevant.
Here is the result of repeating the test with the linux patch several times
and passing the log output through "sort | uniq".
1 pending 0x2 mask 0x805a
9 pending 0x4 mask 0x801e
208 pending 0x4 mask 0x805e
So it is almost always OHCI_INTR_SF and rarely OHCI_INTR_WDH.
For reference, this is repeatedly running
CMDLINE="panic=-1 kunit.stats_enabled=2 kunit.filter=speed>slow root=/dev/sda
rootwait console=ttyS0,115200 earlycon=uart8250,mmio,0x1000,115200"
qemu-system-riscv32 -M virt -m 512M \
-no-reboot -bios default -kernel arch/riscv/boot/Image -snapshot \
-device virtio-net-device,netdev=net0 -netdev user,id=net0 -usb \
-device pci-ohci,id=ohci -device usb-storage,bus=ohci.0,drive=d0 \
-drive file=rootfs.ext2,if=none,id=d0,format=raw \
-append "${CMDLINE}" \
-nographic -monitor none \
--trace 'usb*' -D "/tmp/usb-ohci.$$.${sequence}.trace.log"
Tracing isn't really necessary but it changes timing enough that
the problem is more likely to be seen if it is active. The problem
is seen with various emulations; I just picked one of them.
Since the interrupt is shared and
thus level triggered, that means that interrupts are still pending when
ohci_irq() exits. The interrupt core in Linux does not re-enter the
interrupt handler, presumably because it is marked as shared interrupt
and returns that the interrupt has been handled.
Isn't that behavior mistaken? A level-triggered IRQ that remains set when
it is re-enabled (when the various shared handlers return) should cause
another interrupt to occur right away.
Edged-triggered interrupts would be a different story, of course.
Maybe I got it wrong; I thought that shared interrupts would have to be
level triggered. But then you are correct: one would think that level
triggered interrupts would keep executing interrupt handlers until the
interrupt is completely handled. I guess that means that I don't really
understand what is happening. Sorry for jumping to conclusions.
I found two possible fixes for the problem. One essentially mirrors the
code from ehci_irq(), the other adds a (bad) kludge into qemu. Both "fix"
or work around the problem.
Question is: What is actually wrong ? Something in the generic interrupt
handling code in Linux, something in the Linux usb-ohci driver, or
something in qemu ? Any idea how a proper fix might look like ?
To answer these questions we need more information.
What else would you need ?
Thanks,
Guenter
Problems (timeouts) when testing usb-ohci with qemu
Hi,
when testing usb-ohci with qemu's pci-ohci emulation, I keep getting
random usb interface timeouts. Sometimes the usb_hub_wq times out.
[9.555666] Waiting for root device /dev/sda...
[ 62.452625] INFO: task kworker/0:2:42 blocked for more than 30 seconds.
[ 62.453036] Tainted: G N 6.9.0-rc1-00305-geae7a41d2233
#1
[ 62.453393] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[ 62.453778] task:kworker/0:2 state:D stack:0 pid:42tgid:42
ppid:2 flags:0x
[ 62.454700] Workqueue: usb_hub_wq hub_event
[ 62.455137] Call Trace:
[ 62.455416] [] __schedule+0x35c/0xe00
[ 62.455708] [] schedule+0x32/0x178
[ 62.455906] [] usb_kill_urb+0xa8/0xda
[ 62.456220] [] usb_start_wait_urb+0xca/0xe2
[ 62.456441] [] usb_control_msg+0x9a/0x102
[ 62.456648] [] hub_port_init+0x5de/0xb40
[ 62.456851] [] hub_event+0xb90/0x1364
[ 62.457049] [] process_one_work+0x200/0x564
Sometimes there is an i/o scheduling timeout such as
[6.361811] Run /sbin/init as init process
[ 93.167039] INFO: task kworker/u4:0:10 blocked for more than 30 seconds.
[ 93.167715] Tainted: G N 6.9.0-rc5-00036-gaece0dd54838
#4
[ 93.168169] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[ 93.168607] task:kworker/u4:0state:D stack:0 pid:10tgid:10
ppid:2 flags:0x
[ 93.169602] Workqueue: scsi_tmf_0 scmd_eh_abort_handler
[ 93.170278] Call Trace:
[ 93.170584] [] __schedule+0x358/0xd4e
[ 93.170904] [] schedule+0x32/0x166
[ 93.171161] [] schedule_timeout+0xd8/0x10a
[ 93.171420] [] __wait_for_common+0xce/0x1ce
[ 93.171604] [] wait_for_completion+0x1c/0x24
[ 93.171716] [] command_abort_matching.part.0+0x38/0x52
[ 93.171841] [] command_abort+0x36/0x70
[ 93.171946] [] scmd_eh_abort_handler+0xa6/0x192
...
This is not a new problem; I have seen it forever. Recently I spent some
time trying to understand the problem and found that the linux driver does
not always handle all ohci interupts. Since the interrupt is shared and
thus level triggered, that means that interrupts are still pending when
ohci_irq() exits. The interrupt core in Linux does not re-enter the
interrupt handler, presumably because it is marked as shared interrupt
and returns that the interrupt has been handled.
I found two possible fixes for the problem. One essentially mirrors the
code from ehci_irq(), the other adds a (bad) kludge into qemu. Both "fix"
or work around the problem.
Question is: What is actually wrong ? Something in the generic interrupt
handling code in Linux, something in the Linux usb-ohci driver, or
something in qemu ? Any idea how a proper fix might look like ?
Thanks,
Guenter
---
Linux hack:
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 4f9982ecfb58..48d523e71ea0 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -883,6 +883,7 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
* optimization of checking the LSB of hcca->done_head; it doesn't
* work on all systems (edge triggering for OHCI can be a factor).
*/
+retry:
ints = ohci_readl(ohci, >intrstatus);
/* Check for an all 1's result which is a typical consequence
@@ -982,6 +983,14 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
}
spin_unlock(>lock);
+ ints = ohci_readl(ohci, >intrstatus);
+ ints &= ohci_readl(ohci, >intrenable);
+ if (ints) {
+ pr_err(" Interrupts still pending 0x%x mask 0x%x\n",
ints,
+ ohci_readl(ohci, >intrenable));
+ goto retry;
+ }
+
return IRQ_HANDLED;
}
---
qemu hack:
hw/usb/hcd-ohci.c | 11 +++
hw/usb/hcd-ohci.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index fc8fc91a1d..99e52ad13a 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -267,6 +267,10 @@ static inline void ohci_intr_update(OHCIState *ohci)
(ohci->intr_status & ohci->intr))
level = 1;
+if (level && ohci->level)
+qemu_set_irq(ohci->irq, 0);
+
+ohci->level = level;
qemu_set_irq(ohci->irq, level);
}
diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
index e1827227ac..6f82e72bd9 100644
--- a/hw/usb/hcd-ohci.h
+++ b/hw/usb/hcd-ohci.h
@@ -52,6 +52,7 @@ struct OHCIState {
uint32_t ctl, status;
uint32_t intr_status;
uint32_t intr;
+int level;
/* memory pointer partition */
uint32_t hcca;
--
2.39.2
Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
On Fri, Mar 08, 2024 at 03:41:48PM +, Peter Maydell wrote: > On Tue, 13 Feb 2024 at 15:36, Guenter Roeck wrote: > > > > On Tue, Feb 13, 2024 at 03:14:21PM +, Peter Maydell wrote: > > > On Mon, 12 Feb 2024 at 14:36, Guenter Roeck wrote: > > > > On 2/12/24 04:32, Peter Maydell wrote: > > > > > The machines I have in mind are: > > > > > > > > > > PXA2xx machines: > > > > > > > > > > akitaSharp SL-C1000 (Akita) PDA (PXA270) > > > > > borzoi Sharp SL-C3100 (Borzoi) PDA (PXA270) > > > > > connex Gumstix Connex (PXA255) > > > > > mainstoneMainstone II (PXA27x) > > > > > spitzSharp SL-C3000 (Spitz) PDA (PXA270) > > > > > terrier Sharp SL-C3200 (Terrier) PDA (PXA270) > > > > > tosa Sharp SL-6000 (Tosa) PDA (PXA255) > > > > > verdex Gumstix Verdex Pro XL6P COMs (PXA270) > > > > > z2 Zipit Z2 (PXA27x) > > > > > > OMAP1 machines: > > > > > > > > > > cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) > > > > > sx1 Siemens SX1 (OMAP310) V2 > > > > > sx1-v1 Siemens SX1 (OMAP310) V1 > > > > > > OMAP2 machines: > > > > > > > > > > n800 Nokia N800 tablet aka. RX-34 (OMAP2420) > > > > > n810 Nokia N810 tablet aka. RX-44 (OMAP2420) > > > > > > The one SA1110 machine: > > > > > > > > > > collie Sharp SL-5500 (Collie) PDA (SA-1110) > > > I am copying Arnd, the OMAP1 Linux kernel maintainers, PXA2 maintainers, > > and the Linux omap mailing list for input. Sorry for the noise for those > > who don't care, but I think it is useful to have your voices heard. > > Thanks to everybody for your input on this thread. My > proposal is to drop from QEMU: > * all the PXA2xx machines > * all the OMAP2 machines > * the cheetah OMAP1 machine > > leaving (at least for now) sx1, sx1-v1, collie. > > Rationale: > * for QEMU dropping individual machines is much less beneficial >than if we can drop support for an entire SoC > * the OMAP2 QEMU code in particular is large, old and unmaintained, >and none of the OMAP2 kernel maintainers said they were using >QEMU in any of their testing/development > * although Guenter is currently booting test kernels on some >of the PXA2xx machines, nobody seemed to be using them as part >of their active kernel development and my impression from the >thread is that PXA is the closest of all these SoC families to >being dropped from the kernel soon > * nobody said they were using cheetah, so it's entirely >untested and quite probably broken > * on the other hand the OMAP1 sx1 model does seem to be being >used as part of kernel development, and there was interest >in keeping collie around > > I'm going to mark these as deprecated for the QEMU 9.0 release, > which by our deprecate-and-drop policy means they will be > still present in 9.0 (due out in April) and 9.1 (August-ish), > and removed in 9.2 (December). > > I'm potentially open to persuasion if anybody thinks I'm > being too drastic here; persuasion that came attached to > a desire to help modernise the QEMU code for the relevant > machines would be the most effective :-) > sgtm Guenter
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
On 2/17/24 01:06, Michael Tokarev wrote:
28.02.2023 20:11, Guenter Roeck wrote:
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Has this been lost/forgotten?
Not sure. My understanding was that I could not prove that this is how
real hardware handles the situation, thus it wasn't applied. I carry it
locally in my builds of qemu, so it is not a problem for me. Note that
I didn't check if the problem has since been fixed differently. Maybe
that is the case and the problem no longer exists in the upstream version
of qemu.
Guenter
[PATCH v2] target: hppa: Fix unaligned double word accesses for hppa64
Unaligned 64-bit accesses were found in Linux to clobber carry bits,
resulting in bad results if an arithmetic operation involving a
carry bit was executed after an unaligned 64-bit operation.
hppa 2.0 defines additional carry bits in PSW register bits 32..39.
When restoring PSW after executing an unaligned instruction trap, those
bits were not cleared and ended up to be active all the time. Since there
are no bits other than the upper carry bits needed in the upper 32 bit of
env->psw and since those are stored in env->psw_cb, just clear the entire
upper 32 bit when storing psw to solve the problem unconditionally.
Fixes: 931adff31478 ("target/hppa: Update cpu_hppa_get/put_psw for hppa64")
Cc: Richard Henderson
Cc: Charlie Jenkins
Cc: Helge Deller
Reviewed-by: Richard Henderson
Signed-off-by: Guenter Roeck
---
v2: Rework to not require conditional code [Richard]
Add Richard's Reviewed-by: tag
target/hppa/helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/target/hppa/helper.c b/target/hppa/helper.c
index 859644c47a..9d217d051c 100644
--- a/target/hppa/helper.c
+++ b/target/hppa/helper.c
@@ -76,7 +76,8 @@ void cpu_hppa_put_psw(CPUHPPAState *env, target_ulong psw)
}
psw &= ~reserved;
-env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+env->psw = psw & (uint32_t)~(PSW_N | PSW_V | PSW_CB);
+
env->psw_n = (psw / PSW_N) & 1;
env->psw_v = -((psw / PSW_V) & 1);
--
2.39.2
Re: [PATCH] target: hppa: Fix unaligned double word accesses for hppa64
On 2/15/24 22:16, Richard Henderson wrote:
On 2/15/24 19:34, Guenter Roeck wrote:
- env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+ if (hppa_is_pa20(env)) {
+ env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xffull);
+ } else {
+ env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+ }
There are never any bits above 31 set in env->psw, because all of the CB bits are
supposed to be stored in env->psw_cb. Thus
env->psw = psw & (uint32_t)~(...)
with no need for the pa20 check.
With that,
Reviewed-by: Richard Henderson
sgtm. I'll test that and send v2 tomorrow (it is getting late).
Thanks,
Guenter
[PATCH] target: hppa: Fix unaligned double word accesses for hppa64
Unaligned 64-bit accesses were found in Linux to clobber carry bits,
resulting in bad results if an arithmetic operation involving a
carry bit was executed after an unaligned 64-bit operation.
hppa 2.0 defines additional carry bits in PSW register bits 32..39.
When restoring PSW after executing an unaligned instruction trap,
those bits were not cleared and ended up to be active all the time.
Clearing bit 32..39 in psw prior to restoring it solves the problem.
Fixes: 931adff31478 ("target/hppa: Update cpu_hppa_get/put_psw for hppa64")
Cc: Richard Henderson
Cc: Charlie Jenkins
Cc: Helge Deller
Signed-off-by: Guenter Roeck
---
target/hppa/helper.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/target/hppa/helper.c b/target/hppa/helper.c
index 859644c47a..7b798d1227 100644
--- a/target/hppa/helper.c
+++ b/target/hppa/helper.c
@@ -76,7 +76,12 @@ void cpu_hppa_put_psw(CPUHPPAState *env, target_ulong psw)
}
psw &= ~reserved;
-env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+if (hppa_is_pa20(env)) {
+env->psw = psw & ~(PSW_N | PSW_V | PSW_CB | 0xffull);
+} else {
+env->psw = psw & ~(PSW_N | PSW_V | PSW_CB);
+}
+
env->psw_n = (psw / PSW_N) & 1;
env->psw_v = -((psw / PSW_V) & 1);
--
2.39.2
Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
On Tue, Feb 13, 2024 at 03:14:21PM +, Peter Maydell wrote: > On Mon, 12 Feb 2024 at 14:36, Guenter Roeck wrote: > > On 2/12/24 04:32, Peter Maydell wrote: > > > The machines I have in mind are: > > > > > > PXA2xx machines: > > > > > > akitaSharp SL-C1000 (Akita) PDA (PXA270) > > > borzoi Sharp SL-C3100 (Borzoi) PDA (PXA270) > > > connex Gumstix Connex (PXA255) > > > mainstoneMainstone II (PXA27x) > > > spitzSharp SL-C3000 (Spitz) PDA (PXA270) > > > terrier Sharp SL-C3200 (Terrier) PDA (PXA270) > > > tosa Sharp SL-6000 (Tosa) PDA (PXA255) > > > verdex Gumstix Verdex Pro XL6P COMs (PXA270) > > > z2 Zipit Z2 (PXA27x) > > > > > I test akita, borzoi, spitz, and terrier. Upstream Linux removed support > > for mainstone, tosa, and z2 from the Linux kernel as of version 6.0, so > > I am no longer testing those. > > > > I never managed to boot connex or verdex. > > > > > OMAP1 machines: > > > > > > cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) > > > sx1 Siemens SX1 (OMAP310) V2 > > > sx1-v1 Siemens SX1 (OMAP310) V1 > > > > > I test sx1. I don't think I ever tried cheetah, and I could not get sx1-v1 > > to work. > > > > > OMAP2 machines: > > > > > > n800 Nokia N800 tablet aka. RX-34 (OMAP2420) > > > n810 Nokia N810 tablet aka. RX-44 (OMAP2420) > > > > > I never managed to get those to boot the Linux kernel. > > > > > The one SA1110 machine: > > > > > > collie Sharp SL-5500 (Collie) PDA (SA-1110) > > > > > I do test collie. > > > > All the ones I use still boot the latest Linux kernel. > > > > > Obviously if we can remove all the machines that used a given > > > SoC, that's much more effective than if we just delete one or two. > > > > > > I don't have any test images for the SA1110 or OMAP1 machines, > > > so those are the ones I am most keen to be able to drop. > > > I do have test images for a few of the pxa2xx and the OMAP2 machines. > > > > > I don't mind dropping them, just listing what I use for testing the > > Linux kernel. I suspect I may be the only "user" of those boards, > > though, both in Linux and qemu. > > Mmm; there's not much point in both QEMU and the kernel > maintaining code that nobody's using. Are you considering > dropping support for any of these SoC families from the kernel? > Not me personally. Arnd is the one mostly involved in dropping support of obsolete hardware from the kernel. > It sounds like between the two of us we do have at least one > test image per SoC type if we do keep any of these, but > if it isn't going to inconvenience kernel testing I'm > inclined to go ahead with deprecate-and-drop for the whole lot. > (With QEMU's deprecate-and-drop policy, that would be "announce > deprecation now for 9.0, keep in 9.1, remove before 9.2 release > at the end of the year".) At a minimum I would like to drop > the OMAP1 and OMAP2 boards, as that's the biggest code burden. > I am copying Arnd, the OMAP1 Linux kernel maintainers, PXA2 maintainers, and the Linux omap mailing list for input. Sorry for the noise for those who don't care, but I think it is useful to have your voices heard. Personally I think it very unlikely that anyone is using the latest Linux kernel on any of the affected machines, but I may be wrong. Thanks, Guenter
Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
Hi, On Tue, Feb 13, 2024 at 02:42:29PM +0100, Marcin Juszkiewicz wrote: > > > > The one SA1110 machine: > > > > > > collie Sharp SL-5500 (Collie) PDA (SA-1110) > > > > > I do test collie. > > Can you share kernel/initrd/config? I wanted to boot something at 20th > anniversary of buying one but was unable to build anything bootable on > either QEMU/collie or physical one. > You'll find it at https://github.com/groeck/linux-build-test/tree/master/rootfs/arm Essentially it is collie_defconfig plus some additional configuration options (BLK_DEV_INITRD and EABI are essential), and the rootfs is rootfs-sa110.cpio.gz. It boots from initrd. I never managed to get it booting from flash. Guenter
Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
[ sorry for the earlier noise; accidentally hit "send" ] On 2/12/24 04:32, Peter Maydell wrote: QEMU includes some models of old Arm machine types which are a bit problematic for us because: * they're written in a very old way that uses numerous APIs that we would like to get away from (eg they don't use qdev, they use qemu_system_reset_request(), they use vmstate_register(), etc) * they've been that way for a decade plus and nobody particularly has stepped up to try to modernise the code (beyond some occasional work here and there) * we often don't have test cases for them, which means that if we do try to do the necessary refactoring work on them we have no idea if they even still work at all afterwards All these machine types are also of hardware that has largely passed away into history and where I would not be surprised to find that e.g. the Linux kernel support was never tested on real hardware any more. So I would like to explore whether we can deprecate-and-drop some or all of them. This would let us delete the code entirely rather than spending a long time trying to bring it up to scratch for a probably very small to nonexistent userbase. The aim of this email is to see if anybody is still using any of these and would be upset if they went away. Reports of "I tried to use this machine type and it's just broken" are also interesting as they would strongly suggest that the machine has no real users and can be removed. The machines I have in mind are: PXA2xx machines: akitaSharp SL-C1000 (Akita) PDA (PXA270) borzoi Sharp SL-C3100 (Borzoi) PDA (PXA270) connex Gumstix Connex (PXA255) mainstoneMainstone II (PXA27x) spitzSharp SL-C3000 (Spitz) PDA (PXA270) terrier Sharp SL-C3200 (Terrier) PDA (PXA270) tosa Sharp SL-6000 (Tosa) PDA (PXA255) verdex Gumstix Verdex Pro XL6P COMs (PXA270) z2 Zipit Z2 (PXA27x) I test akita, borzoi, spitz, and terrier. Upstream Linux removed support for mainstone, tosa, and z2 from the Linux kernel as of version 6.0, so I am no longer testing those. I never managed to boot connex or verdex. OMAP1 machines: cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) sx1 Siemens SX1 (OMAP310) V2 sx1-v1 Siemens SX1 (OMAP310) V1 I test sx1. I don't think I ever tried cheetah, and I could not get sx1-v1 to work. OMAP2 machines: n800 Nokia N800 tablet aka. RX-34 (OMAP2420) n810 Nokia N810 tablet aka. RX-44 (OMAP2420) I never managed to get those to boot the Linux kernel. The one SA1110 machine: collie Sharp SL-5500 (Collie) PDA (SA-1110) I do test collie. All the ones I use still boot the latest Linux kernel. Obviously if we can remove all the machines that used a given SoC, that's much more effective than if we just delete one or two. I don't have any test images for the SA1110 or OMAP1 machines, so those are the ones I am most keen to be able to drop. I do have test images for a few of the pxa2xx and the OMAP2 machines. I don't mind dropping them, just listing what I use for testing the Linux kernel. I suspect I may be the only "user" of those boards, though, both in Linux and qemu. Guenter
Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
On 2/12/24 04:32, Peter Maydell wrote: QEMU includes some models of old Arm machine types which are a bit problematic for us because: * they're written in a very old way that uses numerous APIs that we would like to get away from (eg they don't use qdev, they use qemu_system_reset_request(), they use vmstate_register(), etc) * they've been that way for a decade plus and nobody particularly has stepped up to try to modernise the code (beyond some occasional work here and there) * we often don't have test cases for them, which means that if we do try to do the necessary refactoring work on them we have no idea if they even still work at all afterwards All these machine types are also of hardware that has largely passed away into history and where I would not be surprised to find that e.g. the Linux kernel support was never tested on real hardware any more. So I would like to explore whether we can deprecate-and-drop some or all of them. This would let us delete the code entirely rather than spending a long time trying to bring it up to scratch for a probably very small to nonexistent userbase. The aim of this email is to see if anybody is still using any of these and would be upset if they went away. Reports of "I tried to use this machine type and it's just broken" are also interesting as they would strongly suggest that the machine has no real users and can be removed. The machines I have in mind are: PXA2xx machines: akitaSharp SL-C1000 (Akita) PDA (PXA270) borzoi Sharp SL-C3100 (Borzoi) PDA (PXA270) connex Gumstix Connex (PXA255) mainstoneMainstone II (PXA27x) spitzSharp SL-C3000 (Spitz) PDA (PXA270) terrier Sharp SL-C3200 (Terrier) PDA (PXA270) tosa Sharp SL-6000 (Tosa) PDA (PXA255) verdex Gumstix Verdex Pro XL6P COMs (PXA270) z2 Zipit Z2 (PXA27x) OMAP1 machines: cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) sx1 Siemens SX1 (OMAP310) V2 sx1-v1 Siemens SX1 (OMAP310) V1 OMAP2 machines: n800 Nokia N800 tablet aka. RX-34 (OMAP2420) n810 Nokia N810 tablet aka. RX-44 (OMAP2420) The one SA1110 machine: collie Sharp SL-5500 (Collie) PDA (SA-1110) Obviously if we can remove all the machines that used a given SoC, that's much more effective than if we just delete one or two. I don't have any test images for the SA1110 or OMAP1 machines, so those are the ones I am most keen to be able to drop. I do have test images for a few of the pxa2xx and the OMAP2 machines. thanks -- PMM
Re: [PULL 02/10] hw/hppa/machine: Disable default devices with --nodefaults option
Hi Helge,
On Thu, Feb 01, 2024 at 08:22:58PM -0800, Guenter Roeck wrote:
> Hi,
>
> On Sat, Jan 13, 2024 at 06:57:20AM +0100, del...@kernel.org wrote:
> > From: Helge Deller
> >
> > Recognize the qemu --nodefaults option, which will disable the
> > following default devices on hppa:
> > - lsi53c895a SCSI controller,
> > - artist graphics card,
> > - LASI 82596 NIC,
> > - tulip PCI NIC,
> > - second serial PCI card,
> > - USB OHCI controller.
> >
> > Adding this option is very useful to allow manual testing and
> > debugging of the other possible devices on the command line.
> >
>
> With this patch in the tree, I get some interesting crashes in Seabios
> if I provide a somewhat unusual command line option. For example,
> something like
>
> -usb -device usb-ehci,id=ehci \
> -device usb-uas,bus=ehci.0,id=uas \
> -device scsi-hd,bus=uas.0,scsi-id=0,lun=0,drive=d0 \
> -drive file= ...
>
> is accepted as command line option but results in
>
> SeaBIOS PA-RISC 32-bit Firmware Version 15 (QEMU 8.2.1)
> Duplex Console IO Dependent Code (IODC) revision 1
> --
> (c) Copyright 2017-2024 Helge Deller and SeaBIOS developers.
> --
> Processor SpeedState Coprocessor State Cache Size
> - - - --
> 0 250 MHzActive Functional0 KB
> 1 250 MHzIdle Functional0 KB
> 2 250 MHzIdle Functional0 KB
> 3 250 MHzIdle Functional0 KB
> Emulated machine: HP C3700 (64-bit PA2.0) with 32-bit PDC
> Available memory: 1024 MB
> Good memory required: 16 MB
> Primary boot path:FWSCSI.0.0
> Alternate boot path: FWSCSI.0.0
> Console path: SERIAL_2.9600.8.none
> Keyboard path:SERIAL_2.9600.8.none
> *ERROR* in SeaBIOS-hppa-v15:
> prepare_boot_path:2898
> SeaBIOS wants SYSTEM HALT.
Just in case it matters, the following partial revert fixes the problem
for me.
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index eb78c46ff1..cc903bc323 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -346,10 +346,8 @@ static void machine_HP_common_init_tail(MachineState
*machine, PCIBus *pci_bus,
SysBusDevice *s;
/* SCSI disk setup. */
-if (drive_get_max_bus(IF_SCSI) >= 0) {
-dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-lsi53c8xx_handle_legacy_cmdline(dev);
-}
+dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
+lsi53c8xx_handle_legacy_cmdline(dev);
It seems that SeaBIOS doesn't like it if the SCSI interface is missing.
Guenter
Re: [PULL 02/10] hw/hppa/machine: Disable default devices with --nodefaults option
On Fri, Feb 02, 2024 at 10:54:20AM +0100, Helge Deller wrote:
> Hi Guenter,
>
> On 2/2/24 05:22, Guenter Roeck wrote:
> > On Sat, Jan 13, 2024 at 06:57:20AM +0100, del...@kernel.org wrote:
> > > From: Helge Deller
> > >
> > > Recognize the qemu --nodefaults option, which will disable the
> > > following default devices on hppa:
> > > - lsi53c895a SCSI controller,
> > > - artist graphics card,
> > > - LASI 82596 NIC,
> > > - tulip PCI NIC,
> > > - second serial PCI card,
> > > - USB OHCI controller.
> > >
> > > Adding this option is very useful to allow manual testing and
> > > debugging of the other possible devices on the command line.
> > >
> >
> > With this patch in the tree, I get some interesting crashes in Seabios
> > if I provide a somewhat unusual command line option. For example,
> > something like
> >
> > -usb -device usb-ehci,id=ehci \
> > -device usb-uas,bus=ehci.0,id=uas \
> > -device scsi-hd,bus=uas.0,scsi-id=0,lun=0,drive=d0 \
> > -drive file= ...
> >
> > is accepted as command line option but results in
> >
> > SeaBIOS PA-RISC 32-bit Firmware Version 15 (QEMU 8.2.1)
> > Duplex Console IO Dependent Code (IODC) revision 1
> > --
> >(c) Copyright 2017-2024 Helge Deller and SeaBIOS
> > developers.
> > --
> >Processor SpeedState Coprocessor State Cache
> > Size
> >- - -
> > --
> >0 250 MHzActive Functional0 KB
> >1 250 MHzIdle Functional0 KB
> >2 250 MHzIdle Functional0 KB
> >3 250 MHzIdle Functional0 KB
> >Emulated machine: HP C3700 (64-bit PA2.0) with 32-bit PDC
> >Available memory: 1024 MB
> >Good memory required: 16 MB
> >Primary boot path:FWSCSI.0.0
> >Alternate boot path: FWSCSI.0.0
> >Console path: SERIAL_2.9600.8.none
> >Keyboard path:SERIAL_2.9600.8.none
> > *ERROR* in SeaBIOS-hppa-v15:
> > prepare_boot_path:2898
> > SeaBIOS wants SYSTEM HALT.
> >
> > This is without --nodefaults, and it used to work. Is that intentional ?
>
> This should now be fixed in the upcoming SeaBIOS-hppa-v16 version ("devel"
> branch):
> https://github.com/hdeller/seabios-hppa/tree/devel
> Could you test?
I was able to build from the 'master' branch, but 'devel' gives me
hppa64-linux-ld: target elf32-hppa-linux not found
Do you have a binary seabios image, by any chance ?
> If it doesn't work, please give me the full command line.
>
qemu-system-hppa -M C3700 -smp 4 \
-kernel vmlinux -no-reboot -snapshot \
-usb -device usb-ehci,id=ehci \
-device usb-uas,bus=ehci.0,id=uas \
-device scsi-hd,bus=uas.0,scsi-id=0,lun=0,drive=d0 \
-drive file=rootfs.ext2,if=none,format=raw,id=d0 \
-append "root=/dev/sda rootwait console=ttyS0,115200" \
-nographic -monitor null
This is with qemu 8.2.1. Note that the number of CPUs doesn't make a
difference. It turns out this also crashes/aborts immediately with
"nodefaults".
> > If I do use the --nodefaults parameter, I was unable to figure out how
> > to configure the serial console. What command line parameter(s) do I need to
> > get it ?
>
> You need to add:
> -serial mon:stdio
> This will create a serial port if it's not yet there.
>
And there was me trying all variants of "-device pci-serial-4x..." I could
think of ;-).
Guenter
Re: [PULL 02/10] hw/hppa/machine: Disable default devices with --nodefaults option
Hi, On Sat, Jan 13, 2024 at 06:57:20AM +0100, del...@kernel.org wrote: > From: Helge Deller > > Recognize the qemu --nodefaults option, which will disable the > following default devices on hppa: > - lsi53c895a SCSI controller, > - artist graphics card, > - LASI 82596 NIC, > - tulip PCI NIC, > - second serial PCI card, > - USB OHCI controller. > > Adding this option is very useful to allow manual testing and > debugging of the other possible devices on the command line. > With this patch in the tree, I get some interesting crashes in Seabios if I provide a somewhat unusual command line option. For example, something like -usb -device usb-ehci,id=ehci \ -device usb-uas,bus=ehci.0,id=uas \ -device scsi-hd,bus=uas.0,scsi-id=0,lun=0,drive=d0 \ -drive file= ... is accepted as command line option but results in SeaBIOS PA-RISC 32-bit Firmware Version 15 (QEMU 8.2.1) Duplex Console IO Dependent Code (IODC) revision 1 -- (c) Copyright 2017-2024 Helge Deller and SeaBIOS developers. -- Processor SpeedState Coprocessor State Cache Size - - - -- 0 250 MHzActive Functional0 KB 1 250 MHzIdle Functional0 KB 2 250 MHzIdle Functional0 KB 3 250 MHzIdle Functional0 KB Emulated machine: HP C3700 (64-bit PA2.0) with 32-bit PDC Available memory: 1024 MB Good memory required: 16 MB Primary boot path:FWSCSI.0.0 Alternate boot path: FWSCSI.0.0 Console path: SERIAL_2.9600.8.none Keyboard path:SERIAL_2.9600.8.none *ERROR* in SeaBIOS-hppa-v15: prepare_boot_path:2898 SeaBIOS wants SYSTEM HALT. This is without --nodefaults, and it used to work. Is that intentional ? If I do use the --nodefaults parameter, I was unable to figure out how to configure the serial console. What command line parameter(s) do I need to get it ? Thanks, Guenter
Re: [PATCH] pci-host: designware: Limit value range of iATU viewport register
On Thu, Feb 01, 2024 at 02:58:40PM +, Peter Maydell wrote: > On Mon, 29 Jan 2024 at 06:00, Guenter Roeck wrote: > > > > The latest version of qemu (v8.2.0-869-g7a1dc45af5) crashes when booting > > the mcimx7d-sabre emulation with Linux v5.11 and later. > > > > qemu-system-arm: ../system/memory.c:2750: memory_region_set_alias_offset: > > Assertion `mr->alias' failed. > > > > Problem is that the Designware PCIe emulation accepts the full value range > > for the iATU Viewport Register. However, both hardware and emulation only > > support four inbound and four outbound viewports. > > > > The Linux kernel determines the number of supported viewports by writing > > 0xff into the viewport register and reading the value back. The expected > > value when reading the register is the highest supported viewport index. > > This behaviour by the kernel seems to me to be out of spec. > Looking at the "i.MX6 6Dual/6Quad Applications Processor Referenc > Manual IMXDQRM" it says about the PCIE_PL_iATUVR register field > Region_Index: "Must not be set to a value greater than 3" > (there being 4 regions in this case). > Plus it says elsewhere that software "should" write all-0s to > reserved fields, and bits [7:4] are reserved in this register. > Yes, I have seen that, but apparently it works with real hardware. > > Match that code by masking the supported viewport value range when the > > register is written. With this change, the Linux kernel reports > > > > imx6q-pcie 3380.pcie: iATU: unroll F, 4 ob, 4 ib, align 0K, limit 4G > > > > as expected and supported. > > However given this is presumably what the hardware does in this > case where the guest does something out of spec, and we definitely > need to do something to avoid asserting, we should take this patch. > Another option might be to define some flag to enable the behavior, in case other designware based implementations handle it differently. At the very least the code will have to be changed to handle situations where the register is set to a value outside the valid range. I have no idea, though, how that should or could be handled differently, given that we don't really know how the underlying designware IP handles it (such as: maybe it is a mask in the IP configuration file, and the mask for i.MX is really 0x03, not 0x0f). On a side note, I have not been able to actually instantiate a PCIe device behind the root bridge. If anyone knows how to do that, please let me know. Thanks, Guenter
[PATCH] pci-host: designware: Limit value range of iATU viewport register
The latest version of qemu (v8.2.0-869-g7a1dc45af5) crashes when booting
the mcimx7d-sabre emulation with Linux v5.11 and later.
qemu-system-arm: ../system/memory.c:2750: memory_region_set_alias_offset:
Assertion `mr->alias' failed.
Problem is that the Designware PCIe emulation accepts the full value range
for the iATU Viewport Register. However, both hardware and emulation only
support four inbound and four outbound viewports.
The Linux kernel determines the number of supported viewports by writing
0xff into the viewport register and reading the value back. The expected
value when reading the register is the highest supported viewport index.
Match that code by masking the supported viewport value range when the
register is written. With this change, the Linux kernel reports
imx6q-pcie 3380.pcie: iATU: unroll F, 4 ob, 4 ib, align 0K, limit 4G
as expected and supported.
Fixes: d64e5eabc4c7 ("pci: Add support for Designware IP block")
Cc: Andrey Smirnov
Cc: Nikita Ostrenkov
Signed-off-by: Guenter Roeck
---
hw/pci-host/designware.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index dd9e389c07..c25d50f1c6 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -340,6 +340,8 @@ static void designware_pcie_root_config_write(PCIDevice *d,
uint32_t address,
break;
case DESIGNWARE_PCIE_ATU_VIEWPORT:
+val &= DESIGNWARE_PCIE_ATU_REGION_INBOUND |
+(DESIGNWARE_PCIE_NUM_VIEWPORTS - 1);
root->atu_viewport = val;
break;
--
2.39.2
Re: [PATCH v2] hw/arm: add PCIe to Freescale i.MX6
On Sat, Jan 27, 2024 at 11:11:58AM -0800, Guenter Roeck wrote: > Hi, > > On Mon, Jan 08, 2024 at 02:03:25PM +, Nikita Ostrenkov wrote: > > Signed-off-by: Nikita Ostrenkov > > --- > > This patch, with the "sabrelite" emulation and the Linux upstream kernel > (v6.8-rc1, using imx_v6_v7_defconfig), results in: > > qemu-system-arm: ../system/memory.c:2750: memory_region_set_alias_offset: > Assertion `mr->alias' failed. > > with the backtrace below. Any idea what might be wrong ? > Never mind. I found the problem. I'll send a patch. Guenter
Re: [PATCH v2] hw/arm: add PCIe to Freescale i.MX6
On Sat, Jan 27, 2024 at 11:11:58AM -0800, Guenter Roeck wrote: > Hi, > > On Mon, Jan 08, 2024 at 02:03:25PM +, Nikita Ostrenkov wrote: > > Signed-off-by: Nikita Ostrenkov > > --- > > This patch, with the "sabrelite" emulation and the Linux upstream kernel > (v6.8-rc1, using imx_v6_v7_defconfig), results in: > > qemu-system-arm: ../system/memory.c:2750: memory_region_set_alias_offset: > Assertion `mr->alias' failed. > > with the backtrace below. Any idea what might be wrong ? > Additional information: The only reason for not seing the same problem with mcimx7d-sabre is that Linux needs the PF3000 PMIC to instantiate the PCIe controller on that system. After implementing PF3000 support in qemu and instantiating it for mcimx7d-sabre, I get exactly the same assertion when trying to run that simulation with Linux v6.8-rc1. Guenter > Thanks, > Guenter > > --- > #0 __pthread_kill_implementation (no_tid=0, signo=6, > threadid=140737237087808) at ./nptl/pthread_kill.c:44 > #1 __pthread_kill_internal (signo=6, threadid=140737237087808) at > ./nptl/pthread_kill.c:78 > #2 __GI___pthread_kill (threadid=140737237087808, signo=signo@entry=6) at > ./nptl/pthread_kill.c:89 > #3 0x76242476 in __GI_raise (sig=sig@entry=6) at > ../sysdeps/posix/raise.c:26 > #4 0x762287f3 in __GI_abort () at ./stdlib/abort.c:79 > #5 0x7622871b in __assert_fail_base > (fmt=0x763dd130 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", > assertion=0x565712bd "mr->alias", file=0x56570da4 > "../system/memory.c", line=2750, function=) > at ./assert/assert.c:92 > #6 0x76239e96 in __GI___assert_fail > (assertion=0x565712bd "mr->alias", file=0x56570da4 > "../system/memory.c", line=2750, function=0x56571bf0 > <__PRETTY_FUNCTION__.8> "memory_region_set_alias_offset") > at ./assert/assert.c:101 > #7 0x560192ce in memory_region_set_alias_offset (mr=0x57a77ce0, > offset=286326784) at ../system/memory.c:2750 > #8 0x55b8bc9f in designware_pcie_update_viewport > (root=0x57a74d50, viewport=0x57a77bc0) at > ../hw/pci-host/designware.c:280 > #9 0x55b8c06d in designware_pcie_root_config_write > (d=0x57a74d50, address=2312, val=0, len=4) at > ../hw/pci-host/designware.c:375 > #10 0x55b78488 in pci_host_config_write_common > (pci_dev=0x57a74d50, addr=2312, limit=4096, val=0, len=4) at > ../hw/pci/pci_host.c:96 > #11 0x55b8c7ee in designware_pcie_host_mmio_write > (opaque=0x57a746c0, addr=2312, val=0, size=4) at > ../hw/pci-host/designware.c:635 > #12 0x56012388 in memory_region_write_accessor (mr=0x57a780a0, > addr=2312, value=0x7105a628, size=4, shift=0, mask=4294967295, attrs=...) > at ../system/memory.c:497 > #13 0x560126c1 in access_with_adjusted_size > (addr=2312, value=0x7105a628, size=4, access_size_min=4, > access_size_max=4, access_fn=0x5601228e , > mr=0x57a780a0, attrs=...) at ../system/memory.c:573 > #14 0x560159cd in memory_region_dispatch_write (mr=0x57a780a0, > addr=2312, data=0, op=MO_32, attrs=...) at ../system/memory.c:1521 > #15 0x5607351f in int_st_mmio_leN (cpu=0x57a47150, > full=0x7fffe841fe80, val_le=0, addr=3500312840, size=4, mmu_idx=7, > ra=140734908258821, mr=0x57a780a0, mr_offset=2312) > at ../accel/tcg/cputlb.c:2545 > #16 0x56073697 in do_st_mmio_leN (cpu=0x57a47150, > full=0x7fffe841fe80, val_le=0, addr=3500312840, size=4, mmu_idx=7, > ra=140734908258821) at ../accel/tcg/cputlb.c:2581 > #17 0x56073f14 in do_st_4 (cpu=0x57a47150, p=0x7105a7c0, > val=0, mmu_idx=7, memop=1282, ra=140734908258821) at > ../accel/tcg/cputlb.c:2758 > #18 0x560742d7 in do_st4_mmu (cpu=0x57a47150, addr=3500312840, > val=0, oi=20519, ra=140734908258821) at ../accel/tcg/cputlb.c:2834 > #19 0x56074df3 in helper_stl_mmu (env=0x57a49910, > addr=3500312840, val=0, oi=20519, retaddr=140734908258821) at > ../accel/tcg/ldst_common.c.inc:100 > #20 0x7fff6636da46 in code_gen_buffer () > #21 0x560587e8 in cpu_tb_exec (cpu=0x57a47150, > itb=0x7fffa636d900, tb_exit=0x7105adf0) at ../accel/tcg/cpu-exec.c:458 > #22 0x56059565 in cpu_loop_exec_tb (cpu=0x57a47150, > tb=0x7fffa636d900, pc=3230581304, last_tb=0x7105ae00, > tb_exit=0x7105adf0) at ../accel/tcg/cpu-exec.c:920 > #23 0x560598da in cpu_exec_loop (cpu=0x57a47150, > sc=0x7105ae80) at ../accel/tcg/cpu-exec.c:1041 > #24 0x560599ab in cpu_exec_setjmp (cpu=0x57a47150, > sc=0x7105ae80) at ../accel/tcg/c
Re: [PATCH v2] hw/arm: add PCIe to Freescale i.MX6
Hi, On Mon, Jan 08, 2024 at 02:03:25PM +, Nikita Ostrenkov wrote: > Signed-off-by: Nikita Ostrenkov > --- This patch, with the "sabrelite" emulation and the Linux upstream kernel (v6.8-rc1, using imx_v6_v7_defconfig), results in: qemu-system-arm: ../system/memory.c:2750: memory_region_set_alias_offset: Assertion `mr->alias' failed. with the backtrace below. Any idea what might be wrong ? Thanks, Guenter --- #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737237087808) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=140737237087808) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=140737237087808, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x76242476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x762287f3 in __GI_abort () at ./stdlib/abort.c:79 #5 0x7622871b in __assert_fail_base (fmt=0x763dd130 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x565712bd "mr->alias", file=0x56570da4 "../system/memory.c", line=2750, function=) at ./assert/assert.c:92 #6 0x76239e96 in __GI___assert_fail (assertion=0x565712bd "mr->alias", file=0x56570da4 "../system/memory.c", line=2750, function=0x56571bf0 <__PRETTY_FUNCTION__.8> "memory_region_set_alias_offset") at ./assert/assert.c:101 #7 0x560192ce in memory_region_set_alias_offset (mr=0x57a77ce0, offset=286326784) at ../system/memory.c:2750 #8 0x55b8bc9f in designware_pcie_update_viewport (root=0x57a74d50, viewport=0x57a77bc0) at ../hw/pci-host/designware.c:280 #9 0x55b8c06d in designware_pcie_root_config_write (d=0x57a74d50, address=2312, val=0, len=4) at ../hw/pci-host/designware.c:375 #10 0x55b78488 in pci_host_config_write_common (pci_dev=0x57a74d50, addr=2312, limit=4096, val=0, len=4) at ../hw/pci/pci_host.c:96 #11 0x55b8c7ee in designware_pcie_host_mmio_write (opaque=0x57a746c0, addr=2312, val=0, size=4) at ../hw/pci-host/designware.c:635 #12 0x56012388 in memory_region_write_accessor (mr=0x57a780a0, addr=2312, value=0x7105a628, size=4, shift=0, mask=4294967295, attrs=...) at ../system/memory.c:497 #13 0x560126c1 in access_with_adjusted_size (addr=2312, value=0x7105a628, size=4, access_size_min=4, access_size_max=4, access_fn=0x5601228e , mr=0x57a780a0, attrs=...) at ../system/memory.c:573 #14 0x560159cd in memory_region_dispatch_write (mr=0x57a780a0, addr=2312, data=0, op=MO_32, attrs=...) at ../system/memory.c:1521 #15 0x5607351f in int_st_mmio_leN (cpu=0x57a47150, full=0x7fffe841fe80, val_le=0, addr=3500312840, size=4, mmu_idx=7, ra=140734908258821, mr=0x57a780a0, mr_offset=2312) at ../accel/tcg/cputlb.c:2545 #16 0x56073697 in do_st_mmio_leN (cpu=0x57a47150, full=0x7fffe841fe80, val_le=0, addr=3500312840, size=4, mmu_idx=7, ra=140734908258821) at ../accel/tcg/cputlb.c:2581 #17 0x56073f14 in do_st_4 (cpu=0x57a47150, p=0x7105a7c0, val=0, mmu_idx=7, memop=1282, ra=140734908258821) at ../accel/tcg/cputlb.c:2758 #18 0x560742d7 in do_st4_mmu (cpu=0x57a47150, addr=3500312840, val=0, oi=20519, ra=140734908258821) at ../accel/tcg/cputlb.c:2834 #19 0x56074df3 in helper_stl_mmu (env=0x57a49910, addr=3500312840, val=0, oi=20519, retaddr=140734908258821) at ../accel/tcg/ldst_common.c.inc:100 #20 0x7fff6636da46 in code_gen_buffer () #21 0x560587e8 in cpu_tb_exec (cpu=0x57a47150, itb=0x7fffa636d900, tb_exit=0x7105adf0) at ../accel/tcg/cpu-exec.c:458 #22 0x56059565 in cpu_loop_exec_tb (cpu=0x57a47150, tb=0x7fffa636d900, pc=3230581304, last_tb=0x7105ae00, tb_exit=0x7105adf0) at ../accel/tcg/cpu-exec.c:920 #23 0x560598da in cpu_exec_loop (cpu=0x57a47150, sc=0x7105ae80) at ../accel/tcg/cpu-exec.c:1041 #24 0x560599ab in cpu_exec_setjmp (cpu=0x57a47150, sc=0x7105ae80) at ../accel/tcg/cpu-exec.c:1058 #25 0x56059a47 in cpu_exec (cpu=0x57a47150) at ../accel/tcg/cpu-exec.c:1084 #26 0x560838c2 in tcg_cpus_exec (cpu=0x57a47150) at ../accel/tcg/tcg-accel-ops.c:76 #27 0x5608403d in mttcg_cpu_thread_fn (arg=0x57a47150) at ../accel/tcg/tcg-accel-ops-mttcg.c:95 #28 0x562b32d7 in qemu_thread_start (args=0x57c06360) at ../util/qemu-thread-posix.c:541 #29 0x76294ac3 in start_thread (arg=) at ./nptl/pthread_create.c:442 #30 0x76326850 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
Re: [PATCH 0/4] esp-pci: fixes for Linux and MS-DOS
On 1/20/24 05:09, Michael Tokarev wrote: 12.01.2024 16:15, Mark Cave-Ayland: This series contains fixes for the esp-pci device (am53c974 or dc390) for a few issues spotted whilst testing the previous ESP series. Patches 1-3 are fixes for issues found by Helge/Guenter whilst testing the hppa C3700 machine with the amd53c974/dc390 devices under Linux, whilst patch 4 fixes an issue that was exposed by testing MS-DOS and Windows drivers. With this series applied on top of the reworked ESP device, it is possible to boot Linux under qemu-system-hppa without any errors and also boot and install Win98SE from a DC390 PCI SCSI controller (no IDE!) using an MS-DOS boot floppy. Signed-off-by: Mark Cave-Ayland Based-on: 20240112125420.514425-1-mark.cave-ayl...@ilande.co.uk Mark Cave-Ayland (4): esp-pci.c: use correct address register for PCI DMA transfers esp-pci.c: generate PCI interrupt from separate ESP and PCI sources esp-pci.c: synchronise setting of DMA_STAT_DONE with ESP completion interrupt esp-pci.c: set DMA_STAT_BCMBLT when BLAST command issued Is it worth to pick up for stable? Especially the first one. It's interesting this bug is here for a very long time.. :) FWIW, I never observed the first one with Linux. I had carried variants of the other three in my tree for a long time, but they were never in a shape to be sent upstream and I never bothered trying to find the root cause. All those _can_ be observed when booting Linux. So, if anything, I'd argue that they should all be taken into stable releases. Thanks, Guenter
[PATCH] fsl-imx6ul: Add various missing unimplemented devices
Add MMDC, OCOTP, SQPI, CAAM, and USBMISC as unimplemented devices.
This allows operating systems such as Linux to run emulations such as
mcimx6ul-evk.
Before commit 0cd4926b85 ("Refactor i.MX6UL processor code"), the affected
memory ranges were covered by the unimplemented DAP device. The commit
reduced the DAP address range from 0x10 to 4kB, and the emulation
thus no longer covered the various unimplemented devices in the affected
address range.
Fixes: 0cd4926b85 ("Refactor i.MX6UL processor code")
Cc: Jean-Christophe Dubois
Signed-off-by: Guenter Roeck
---
This patch is necessary to boot mcimx6ul-evk with Linux v6.7.
My apologies for the noise if a similar patch has already been submitted
and I missed it.
hw/arm/fsl-imx6ul.c | 30 ++
include/hw/arm/fsl-imx6ul.h | 2 ++
2 files changed, 32 insertions(+)
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index e37b69a5e1..7d2ee0df6e 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -192,6 +192,36 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error
**errp)
create_unimplemented_device("a7mpcore-dap", FSL_IMX6UL_A7MPCORE_DAP_ADDR,
FSL_IMX6UL_A7MPCORE_DAP_SIZE);
+/*
+ * MMDC
+ */
+create_unimplemented_device("a7mpcore-mmdc", FSL_IMX6UL_MMDC_CFG_ADDR,
+FSL_IMX6UL_MMDC_CFG_SIZE);
+
+/*
+ * OCOTP
+ */
+create_unimplemented_device("a7mpcore-ocotp", FSL_IMX6UL_OCOTP_CTRL_ADDR,
+FSL_IMX6UL_OCOTP_CTRL_SIZE);
+
+/*
+ * QSPI
+ */
+create_unimplemented_device("a7mpcore-qspi", FSL_IMX6UL_QSPI_ADDR,
+FSL_IMX6UL_QSPI_SIZE);
+
+/*
+ * CAAM
+ */
+create_unimplemented_device("a7mpcore-qspi", FSL_IMX6UL_CAAM_ADDR,
+FSL_IMX6UL_CAAM_SIZE);
+
+/*
+ * USBMISC
+ */
+create_unimplemented_device("a7mpcore-usbmisc",
FSL_IMX6UL_USBO2_USBMISC_ADDR,
+FSL_IMX6UL_USBO2_USBMISC_SIZE);
+
/*
* GPTs
*/
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 14390f6014..8277b0e8b2 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -182,6 +182,8 @@ enum FslIMX6ULMemoryMap {
FSL_IMX6UL_ENET1_ADDR = 0x02188000,
FSL_IMX6UL_USBO2_USBMISC_ADDR = 0x02184800,
+FSL_IMX6UL_USBO2_USBMISC_SIZE = 0x200,
+
FSL_IMX6UL_USBO2_USB1_ADDR = 0x02184000,
FSL_IMX6UL_USBO2_USB2_ADDR = 0x02184200,
--
2.39.2
Re: [PULL 39/41] hw/net/cadence_gem: use FIELD to describe PHYMNTNC register fields
On Fri, Jan 19, 2024 at 02:32:47PM -0800, Guenter Roeck wrote: > Hi, > > On Fri, Oct 27, 2023 at 03:39:40PM +0100, Peter Maydell wrote: > > From: Luc Michel > > > > Use the FIELD macro to describe the PHYMNTNC register fields. > > > > Signed-off-by: Luc Michel > > Reviewed-by: sai.pavan.bo...@amd.com > > Message-id: 20231017194422.4124691-10-luc.mic...@amd.com > > Signed-off-by: Peter Maydell > > With qemu v8.2.0 and this patch in place, I get the following error when > trying > to enable the network interface on the xilinx-zynq-a9 emulation. > > macb e000b000.ethernet eth0: validation of with support > 00,,5000,6000 and advertisement 00,,, > failed: -EINVAL > macb e000b000.ethernet eth0: Could not attach PHY (-22) > > The problem is gone after reverting this patch. Note that I also had > to revert "hw/net/cadence_gem: perform PHY access on write only", but > that alone did not fix the problem. > Never mind, it looks like the problem was fixed with commit 0c7ffc977195c1. Sorry for the noise. Guenter
Re: [PULL 39/41] hw/net/cadence_gem: use FIELD to describe PHYMNTNC register fields
Hi, On Fri, Oct 27, 2023 at 03:39:40PM +0100, Peter Maydell wrote: > From: Luc Michel > > Use the FIELD macro to describe the PHYMNTNC register fields. > > Signed-off-by: Luc Michel > Reviewed-by: sai.pavan.bo...@amd.com > Message-id: 20231017194422.4124691-10-luc.mic...@amd.com > Signed-off-by: Peter Maydell With qemu v8.2.0 and this patch in place, I get the following error when trying to enable the network interface on the xilinx-zynq-a9 emulation. macb e000b000.ethernet eth0: validation of with support 00,,5000,6000 and advertisement 00,,, failed: -EINVAL macb e000b000.ethernet eth0: Could not attach PHY (-22) The problem is gone after reverting this patch. Note that I also had to revert "hw/net/cadence_gem: perform PHY access on write only", but that alone did not fix the problem. Guenter
Re: [PATCH v2 3/3] hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board
On 1/16/24 02:04, Philippe Mathieu-Daudé wrote: Hi, (Cc'ing Li, Strahinja and Niek) On 15/1/24 19:27, Guenter Roeck wrote: Add watchdog timer support to Allwinner-H40 and Bananapi. The watchdog timer is added as an overlay to the Timer module memory map. I'm confused by these registers from TYPE_AW_A10_PIT and the TYPE_AW_WDT implementation you are using: #define AW_A10_PIT_WDOG_CONTROL 0x90 #define AW_A10_PIT_WDOG_MODE 0x94 Do we have 2 implementations of the same peripheral? The watchdog core in A10 and H40 is the same. Linux devicetree uses allwinner,sun4i-a10-wdt for several chips. arch/arm/boot/dts/allwinner/sun4i-a10.dtsi: compatible = "allwinner,sun4i-a10-wdt"; arch/arm/boot/dts/allwinner/sun5i.dtsi: compatible = "allwinner,sun4i-a10-wdt"; arch/arm/boot/dts/allwinner/sun7i-a20.dtsi: compatible = "allwinner,sun4i-a10-wdt"; arch/arm/boot/dts/allwinner/sun8i-r40.dtsi: compatible = "allwinner,sun4i-a10-wdt"; Should we instanciate AW_WDT within AW_A10_PIT? As far as I can see, AW_A10_PIT_WDOG_CONTROL and AW_A10_PIT_WDOG_MODE are not really handled in hw/timer/allwinner-a10-pit.c because of the memory range overlay, and the defines might as well be dropped from there. It made some sense to have them when the watchdog wasn't implemented to avoid emulation errors, but afaics they are now obsolete. The wdt type (TYPE_AW_WDT_SUN4I) more accurately reflects the watchdog type. Control and mode registers are model specific. For example, sun6i and sun9i use the same timer and watchdog cores but with different register offsets and bit values for the watchdog registers. sun8i uses the same watchdog core (and same register offsets) as sun6i/sun9i, but the timer module itself is different. Implementing that with the current model (using memory map overlays) is straightforward. I don't know how one would do that from the timer modules; it seems more complex to me than the current implementation. On the other side, I don't know how the "proper" implementation in qemu would look like, so I don't really have a strong opinion either way. Guenter
[PATCH v2 1/3] hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board
Allwinner R40 supports two USB host ports shared between a USB 2.0 EHCI
host controller and a USB 1.1 OHCI host controller. Add support for both
of them.
If machine USB support is not enabled, create unimplemented devices
for the USB memory ranges to avoid crashes when booting Linux.
Signed-off-by: Guenter Roeck
---
v2: The USB Controllers are part of the chipset, so instantiate them
unconditionally
docs/system/arm/bananapi_m2u.rst | 2 +-
hw/arm/Kconfig | 2 ++
hw/arm/allwinner-r40.c | 47 ++--
include/hw/arm/allwinner-r40.h | 9 ++
4 files changed, 57 insertions(+), 3 deletions(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index b09ba5c548..e77c425e2c 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -23,6 +23,7 @@ The Banana Pi M2U machine supports the following devices:
* GMAC ethernet
* Clock Control Unit
* TWI (I2C)
+ * USB 2.0
Limitations
"""""""""""
@@ -33,7 +34,6 @@ Currently, Banana Pi M2U does *not* support the following
features:
- Audio output
- Hardware Watchdog
- Real Time Clock
-- USB 2.0 interfaces
Also see the 'unimplemented' array in the Allwinner R40 SoC module
for a complete list of unimplemented I/O devices: ``./hw/arm/allwinner-r40.c``
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 39d255425b..6b508780d3 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -415,6 +415,8 @@ config ALLWINNER_R40
select ARM_TIMER
select ARM_GIC
select UNIMP
+select USB_OHCI
+select USB_EHCI_SYSBUS
select SD
config RASPI
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index a0d367c60d..2e8943eff7 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -23,6 +23,7 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include "qemu/units.h"
+#include "hw/boards.h"
#include "hw/qdev-core.h"
#include "hw/sysbus.h"
#include "hw/char/serial.h"
@@ -45,6 +46,10 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_MMC1] = 0x01c1,
[AW_R40_DEV_MMC2] = 0x01c11000,
[AW_R40_DEV_MMC3] = 0x01c12000,
+[AW_R40_DEV_EHCI1] = 0x01c19000,
+[AW_R40_DEV_OHCI1] = 0x01c19400,
+[AW_R40_DEV_EHCI2] = 0x01c1c000,
+[AW_R40_DEV_OHCI2] = 0x01c1c400,
[AW_R40_DEV_CCU]= 0x01c2,
[AW_R40_DEV_PIT]= 0x01c20c00,
[AW_R40_DEV_UART0] = 0x01c28000,
@@ -89,9 +94,9 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
{ "crypto", 0x01c15000, 4 * KiB },
{ "spi2", 0x01c17000, 4 * KiB },
{ "sata", 0x01c18000, 4 * KiB },
-{ "usb1-host", 0x01c19000, 4 * KiB },
+{ "usb1-phy", 0x01c19800, 2 * KiB },
{ "sid",0x01c1b000, 4 * KiB },
-{ "usb2-host", 0x01c1c000, 4 * KiB },
+{ "usb2-phy", 0x01c1c800, 2 * KiB },
{ "cs1",0x01c1d000, 4 * KiB },
{ "spi3", 0x01c1f000, 4 * KiB },
{ "rtc",0x01c20400, 1 * KiB },
@@ -181,6 +186,10 @@ enum {
AW_R40_GIC_SPI_MMC2 = 34,
AW_R40_GIC_SPI_MMC3 = 35,
AW_R40_GIC_SPI_EMAC = 55,
+AW_R40_GIC_SPI_OHCI1 = 64,
+AW_R40_GIC_SPI_OHCI2 = 65,
+AW_R40_GIC_SPI_EHCI1 = 76,
+AW_R40_GIC_SPI_EHCI2 = 78,
AW_R40_GIC_SPI_GMAC = 85,
};
@@ -276,6 +285,13 @@ static void allwinner_r40_init(Object *obj)
TYPE_AW_SDHOST_SUN50I_A64);
}
+for (size_t i = 0; i < AW_R40_NUM_USB; i++) {
+object_initialize_child(obj, "ehci[*]", >ehci[i],
+TYPE_PLATFORM_EHCI);
+object_initialize_child(obj, "ohci[*]", >ohci[i],
+TYPE_SYSBUS_OHCI);
+}
+
object_initialize_child(obj, "twi0", >i2c0, TYPE_AW_I2C_SUN6I);
object_initialize_child(obj, "emac", >emac, TYPE_AW_EMAC);
@@ -407,6 +423,33 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_realize(SYS_BUS_DEVICE(>ccu), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>ccu), 0, s->memmap[AW_R40_DEV_CCU]);
+/* USB */
+for (size_t i = 0; i < AW_R40_NUM_USB; i++) {
+g_autofree char *bus = g_strdup_printf("usb-bus.%zu", i);
+
+object_property_set_bool(OBJECT(>ehci[i]), "companion-enable", true,
+ _fatal);
+sysbus_realize(SYS_BUS_DEVICE(>ehci[i]), _fatal);
+sysbus_mmio_map(SYS_BUS_DEVICE(>ehci[i]), 0,
+allwinner_r40_memmap[i ? AW_R40_DEV_EHCI2
+ : AW_R40_DEV_EHCI1]
[PATCH v2 2/3] hw/arm: Add AHCI/SATA controller to Allwinner R40 and Bananapi board
Allwinner R40 supports an AHCI compliant SATA controller.
Add support for it.
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Guenter Roeck
---
docs/system/arm/bananapi_m2u.rst | 1 +
hw/arm/Kconfig | 1 +
hw/arm/allwinner-r40.c | 12 +++-
include/hw/arm/allwinner-r40.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index e77c425e2c..542310591d 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -22,6 +22,7 @@ The Banana Pi M2U machine supports the following devices:
* EMAC ethernet
* GMAC ethernet
* Clock Control Unit
+ * SATA
* TWI (I2C)
* USB 2.0
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 6b508780d3..98ca5ebc7d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -408,6 +408,7 @@ config ALLWINNER_H3
config ALLWINNER_R40
bool
default y if TCG && ARM
+select AHCI
select ALLWINNER_SRAMC
select ALLWINNER_A10_PIT
select AXP2XX_PMU
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index 2e8943eff7..534be4a735 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -46,6 +46,7 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_MMC1] = 0x01c1,
[AW_R40_DEV_MMC2] = 0x01c11000,
[AW_R40_DEV_MMC3] = 0x01c12000,
+[AW_R40_DEV_AHCI] = 0x01c18000,
[AW_R40_DEV_EHCI1] = 0x01c19000,
[AW_R40_DEV_OHCI1] = 0x01c19400,
[AW_R40_DEV_EHCI2] = 0x01c1c000,
@@ -93,7 +94,6 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
{ "usb0-host", 0x01c14000, 4 * KiB },
{ "crypto", 0x01c15000, 4 * KiB },
{ "spi2", 0x01c17000, 4 * KiB },
-{ "sata", 0x01c18000, 4 * KiB },
{ "usb1-phy", 0x01c19800, 2 * KiB },
{ "sid",0x01c1b000, 4 * KiB },
{ "usb2-phy", 0x01c1c800, 2 * KiB },
@@ -186,6 +186,7 @@ enum {
AW_R40_GIC_SPI_MMC2 = 34,
AW_R40_GIC_SPI_MMC3 = 35,
AW_R40_GIC_SPI_EMAC = 55,
+AW_R40_GIC_SPI_AHCI = 56,
AW_R40_GIC_SPI_OHCI1 = 64,
AW_R40_GIC_SPI_OHCI2 = 65,
AW_R40_GIC_SPI_EHCI1 = 76,
@@ -285,6 +286,8 @@ static void allwinner_r40_init(Object *obj)
TYPE_AW_SDHOST_SUN50I_A64);
}
+object_initialize_child(obj, "sata", >sata, TYPE_ALLWINNER_AHCI);
+
for (size_t i = 0; i < AW_R40_NUM_USB; i++) {
object_initialize_child(obj, "ehci[*]", >ehci[i],
TYPE_PLATFORM_EHCI);
@@ -423,6 +426,13 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_realize(SYS_BUS_DEVICE(>ccu), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>ccu), 0, s->memmap[AW_R40_DEV_CCU]);
+/* SATA / AHCI */
+sysbus_realize(SYS_BUS_DEVICE(>sata), _fatal);
+sysbus_mmio_map(SYS_BUS_DEVICE(>sata), 0,
+allwinner_r40_memmap[AW_R40_DEV_AHCI]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>sata), 0,
+ qdev_get_gpio_in(DEVICE(>gic), AW_R40_GIC_SPI_AHCI));
+
/* USB */
for (size_t i = 0; i < AW_R40_NUM_USB; i++) {
g_autofree char *bus = g_strdup_printf("usb-bus.%zu", i);
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index ae82822d42..c589fcc1c1 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -22,6 +22,7 @@
#include "qom/object.h"
#include "hw/timer/allwinner-a10-pit.h"
+#include "hw/ide/ahci.h"
#include "hw/intc/arm_gic.h"
#include "hw/sd/allwinner-sdhost.h"
#include "hw/misc/allwinner-r40-ccu.h"
@@ -46,6 +47,7 @@ enum {
AW_R40_DEV_MMC1,
AW_R40_DEV_MMC2,
AW_R40_DEV_MMC3,
+AW_R40_DEV_AHCI,
AW_R40_DEV_EHCI1,
AW_R40_DEV_OHCI1,
AW_R40_DEV_EHCI2,
@@ -112,6 +114,7 @@ struct AwR40State {
const hwaddr *memmap;
AwSRAMCState sramc;
AwA10PITState timer;
+AllwinnerAHCIState sata;
AwSdHostState mmc[AW_R40_NUM_MMCS];
EHCISysBusState ehci[AW_R40_NUM_USB];
OHCISysBusState ohci[AW_R40_NUM_USB];
--
2.39.2
[PATCH v2 3/3] hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board
Add watchdog timer support to Allwinner-H40 and Bananapi.
The watchdog timer is added as an overlay to the Timer
module memory map.
Signed-off-by: Guenter Roeck
---
docs/system/arm/bananapi_m2u.rst | 2 +-
hw/arm/Kconfig | 1 +
hw/arm/allwinner-r40.c | 8
include/hw/arm/allwinner-r40.h | 3 +++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index 542310591d..587b488655 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -25,6 +25,7 @@ The Banana Pi M2U machine supports the following devices:
* SATA
* TWI (I2C)
* USB 2.0
+ * Hardware Watchdog
Limitations
"""""""""""
@@ -33,7 +34,6 @@ Currently, Banana Pi M2U does *not* support the following
features:
- Graphical output via HDMI, GPU and/or the Display Engine
- Audio output
-- Hardware Watchdog
- Real Time Clock
Also see the 'unimplemented' array in the Allwinner R40 SoC module
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 98ca5ebc7d..386edbae15 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -411,6 +411,7 @@ config ALLWINNER_R40
select AHCI
select ALLWINNER_SRAMC
select ALLWINNER_A10_PIT
+select ALLWINNER_WDT
select AXP2XX_PMU
select SERIAL
select ARM_TIMER
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index 534be4a735..a28e5b3886 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -53,6 +53,7 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_OHCI2] = 0x01c1c400,
[AW_R40_DEV_CCU]= 0x01c2,
[AW_R40_DEV_PIT]= 0x01c20c00,
+[AW_R40_DEV_WDT]= 0x01c20c90,
[AW_R40_DEV_UART0] = 0x01c28000,
[AW_R40_DEV_UART1] = 0x01c28400,
[AW_R40_DEV_UART2] = 0x01c28800,
@@ -279,6 +280,8 @@ static void allwinner_r40_init(Object *obj)
object_property_add_alias(obj, "clk1-freq", OBJECT(>timer),
"clk1-freq");
+object_initialize_child(obj, "wdt", >wdt, TYPE_AW_WDT_SUN4I);
+
object_initialize_child(obj, "ccu", >ccu, TYPE_AW_R40_CCU);
for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
@@ -545,6 +548,11 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_connect_irq(SYS_BUS_DEVICE(>emac), 0,
qdev_get_gpio_in(DEVICE(>gic), AW_R40_GIC_SPI_EMAC));
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(>wdt), _fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>wdt), 0,
+allwinner_r40_memmap[AW_R40_DEV_WDT], 1);
+
/* Unimplemented devices */
for (unsigned i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
create_unimplemented_device(r40_unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index c589fcc1c1..66c38e7d90 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -33,6 +33,7 @@
#include "hw/net/allwinner-sun8i-emac.h"
#include "hw/usb/hcd-ohci.h"
#include "hw/usb/hcd-ehci.h"
+#include "hw/watchdog/allwinner-wdt.h"
#include "target/arm/cpu.h"
#include "sysemu/block-backend.h"
@@ -54,6 +55,7 @@ enum {
AW_R40_DEV_OHCI2,
AW_R40_DEV_CCU,
AW_R40_DEV_PIT,
+AW_R40_DEV_WDT,
AW_R40_DEV_UART0,
AW_R40_DEV_UART1,
AW_R40_DEV_UART2,
@@ -114,6 +116,7 @@ struct AwR40State {
const hwaddr *memmap;
AwSRAMCState sramc;
AwA10PITState timer;
+AwWdtState wdt;
AllwinnerAHCIState sata;
AwSdHostState mmc[AW_R40_NUM_MMCS];
EHCISysBusState ehci[AW_R40_NUM_USB];
--
2.39.2
[PATCH v2 0/3] hw/arm: Add support for USB, SATA, and watchdog to Allwinner R40
Add support for - USB 2.0 EHCI/OHCI - SATA/AHCI - Watchdog to Allwinner R40. The hardware is quite similar to Allwinner A10 and H3, so the code is derived from the implementations for those SOCs. Tested with bpim2u emulation by instantiating EHCI and OHCI keyboards, by booting from USB, by booting from ATA/SATA drive, and by manually testing watchdog operation. v2: - The USB Controllers are part of the chipset, so instantiate them unconditionally - Add Reviewed-by: tag to patch 2/3 Guenter Roeck (3): hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board hw/arm: Add AHCI/SATA controller to Allwinner R40 and Bananapi board hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board docs/system/arm/bananapi_m2u.rst | 5 +-- hw/arm/Kconfig | 4 +++ hw/arm/allwinner-r40.c | 67 ++-- include/hw/arm/allwinner-r40.h | 15 + 4 files changed, 86 insertions(+), 5 deletions(-)
Re: [PATCH 1/2] hw/arm/allwinner-a10: Unconditionally map the USB Host controllers
On Mon, Jan 15, 2024 at 05:56:14PM +0100, Philippe Mathieu-Daudé wrote: > The USB Controllers are part of the chipset, thus are > always present and mapped in memory. > > Reported-by: Guenter Roeck > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Guenter Roeck Tested-by: Guenter Roeck
Re: [PATCH 1/3] hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board
On 1/15/24 08:30, Philippe Mathieu-Daudé wrote:
On 15/1/24 17:12, Guenter Roeck wrote:
On 1/15/24 03:02, Philippe Mathieu-Daudé wrote:
Hi,
On 13/1/24 20:16, Guenter Roeck wrote:
Allwinner R40 supports two USB host ports shared between a USB 2.0 EHCI
host controller and a USB 1.1 OHCI host controller. Add support for both
of them.
If machine USB support is not enabled, create unimplemented devices
for the USB memory ranges to avoid crashes when booting Linux.
I never really understood the reason for machine_usb() and had on my
TODO to do some archeology research to figure it out since quite some
time. Having to map an UnimpDevice due to CLI options seems like an
anti-pattern when the device is indeed implemented in the repository.
Me not either. I copied the code from aw_a10_init(), trying to use the
same pattern. I am perfectly fine with making it unconditional, but then
I would argue that it should be unconditional for Allwinner A10 as well
(not that I really care much, just for consistency).
Certainly, but I'd rather have a global pattern cleanup, not just
Allwinner based machines. Looking at the repository:
$ git grep -w machine_usb hw/
hw/arm/allwinner-a10.c:82: if (machine_usb(current_machine)) {
hw/arm/allwinner-a10.c:168: if (machine_usb(current_machine)) {
hw/arm/nseries.c:1356: if (machine_usb(machine)) {
hw/arm/realview.c:288: if (machine_usb(machine)) {
hw/arm/realview.c-289- pci_create_simple(pci_bus, -1, "pci-ohci");
hw/arm/versatilepb.c:276: if (machine_usb(machine)) {
hw/arm/versatilepb.c-277- pci_create_simple(pci_bus, -1, "pci-ohci");
hw/core/machine.c:1175:bool machine_usb(MachineState *machine)
hw/i386/acpi-microvm.c:88: if (machine_usb(MACHINE(mms))) {
hw/i386/acpi-microvm.c-89- xhci_sysbus_build_aml(scope,
MICROVM_XHCI_BASE, MICROVM_XHCI_IRQ);
hw/i386/microvm.c:218: if (x86_machine_is_acpi_enabled(x86ms) &&
machine_usb(MACHINE(mms))) {
hw/i386/microvm.c-225- sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
MICROVM_XHCI_BASE);
hw/i386/pc_piix.c:267: machine_usb(machine), _abort);
hw/i386/pc_q35.c:321: if (machine_usb(machine)) {
hw/i386/pc_q35.c-323- ehci_create_ich9_with_companions(host_bus, 0x1d);
hw/ppc/mac_oldworld.c:300: if (machine_usb(machine)) {
hw/ppc/mac_oldworld.c-301- pci_create_simple(pci_bus, -1, "pci-ohci");
I'd classify that as "USB controller over MMIO / over some bus (PCI)".
The "plug a PCI-to-USB card by default" seems a valid use case (except
for Q35 which is a Frankenstein case, ICH9 chipset is like ARM SoC,
USB bus is always there).
IMHO all the MMIO uses should be corrected.
The "-usb" option says "enable on-board USB host controller (if not
enabled by default)". Unfortunately, that doesn't tell me much,
and most specifically it doesn't tell me how to enable it by default.
One option I can think of would be to enable it on the machine level,
i.e., from bananapi_m2u.c, but then, again, I don't see if/how
that is done for other boards. Any suggestions ?
Of course, I could discuss this with the person who implemented this
code for A10, but it turns out that was me, for no good reason than
that I tried to follow the pattern I had seen elsewhere without really
understanding what I was doing.
So should I drop the conditional from H40 and send a separate patch
to drop it from the A10 code as well, following your line of argument ?
Or drop it and leave A10 alone ?
Well your patch isn't invalid, so:
Reviewed-by: Philippe Mathieu-Daudé
I'll send v2 anyway, especially taking your other patch series into account.
Thanks,
Guenter
Re: [PATCH 1/3] hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board
On 1/15/24 03:02, Philippe Mathieu-Daudé wrote: Hi, On 13/1/24 20:16, Guenter Roeck wrote: Allwinner R40 supports two USB host ports shared between a USB 2.0 EHCI host controller and a USB 1.1 OHCI host controller. Add support for both of them. If machine USB support is not enabled, create unimplemented devices for the USB memory ranges to avoid crashes when booting Linux. I never really understood the reason for machine_usb() and had on my TODO to do some archeology research to figure it out since quite some time. Having to map an UnimpDevice due to CLI options seems like an anti-pattern when the device is indeed implemented in the repository. Me not either. I copied the code from aw_a10_init(), trying to use the same pattern. I am perfectly fine with making it unconditional, but then I would argue that it should be unconditional for Allwinner A10 as well (not that I really care much, just for consistency). The "-usb" option says "enable on-board USB host controller (if not enabled by default)". Unfortunately, that doesn't tell me much, and most specifically it doesn't tell me how to enable it by default. One option I can think of would be to enable it on the machine level, i.e., from bananapi_m2u.c, but then, again, I don't see if/how that is done for other boards. Any suggestions ? Of course, I could discuss this with the person who implemented this code for A10, but it turns out that was me, for no good reason than that I tried to follow the pattern I had seen elsewhere without really understanding what I was doing. So should I drop the conditional from H40 and send a separate patch to drop it from the A10 code as well, following your line of argument ? Or drop it and leave A10 alone ? Thanks, Guenter
[PATCH 2/3] hw/arm: Add AHCI/SATA controller to Allwinner R40 and Bananapi board
Allwinner R40 supports an AHCI compliant SATA controller.
Add support for it.
Signed-off-by: Guenter Roeck
---
docs/system/arm/bananapi_m2u.rst | 1 +
hw/arm/Kconfig | 1 +
hw/arm/allwinner-r40.c | 12 +++-
include/hw/arm/allwinner-r40.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index e77c425e2c..542310591d 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -22,6 +22,7 @@ The Banana Pi M2U machine supports the following devices:
* EMAC ethernet
* GMAC ethernet
* Clock Control Unit
+ * SATA
* TWI (I2C)
* USB 2.0
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 6b508780d3..98ca5ebc7d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -408,6 +408,7 @@ config ALLWINNER_H3
config ALLWINNER_R40
bool
default y if TCG && ARM
+select AHCI
select ALLWINNER_SRAMC
select ALLWINNER_A10_PIT
select AXP2XX_PMU
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index f42b0fa0ce..f90d59fb5e 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -46,6 +46,7 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_MMC1] = 0x01c1,
[AW_R40_DEV_MMC2] = 0x01c11000,
[AW_R40_DEV_MMC3] = 0x01c12000,
+[AW_R40_DEV_AHCI] = 0x01c18000,
[AW_R40_DEV_EHCI1] = 0x01c19000,
[AW_R40_DEV_OHCI1] = 0x01c19400,
[AW_R40_DEV_EHCI2] = 0x01c1c000,
@@ -93,7 +94,6 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
{ "usb0-host", 0x01c14000, 4 * KiB },
{ "crypto", 0x01c15000, 4 * KiB },
{ "spi2", 0x01c17000, 4 * KiB },
-{ "sata", 0x01c18000, 4 * KiB },
{ "usb1-phy", 0x01c19800, 2 * KiB },
{ "sid",0x01c1b000, 4 * KiB },
{ "usb2-phy", 0x01c1c800, 2 * KiB },
@@ -186,6 +186,7 @@ enum {
AW_R40_GIC_SPI_MMC2 = 34,
AW_R40_GIC_SPI_MMC3 = 35,
AW_R40_GIC_SPI_EMAC = 55,
+AW_R40_GIC_SPI_AHCI = 56,
AW_R40_GIC_SPI_OHCI1 = 64,
AW_R40_GIC_SPI_OHCI2 = 65,
AW_R40_GIC_SPI_EHCI1 = 76,
@@ -285,6 +286,8 @@ static void allwinner_r40_init(Object *obj)
TYPE_AW_SDHOST_SUN50I_A64);
}
+object_initialize_child(obj, "sata", >sata, TYPE_ALLWINNER_AHCI);
+
if (machine_usb(current_machine)) {
int i;
@@ -427,6 +430,13 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_realize(SYS_BUS_DEVICE(>ccu), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>ccu), 0, s->memmap[AW_R40_DEV_CCU]);
+/* SATA / AHCI */
+sysbus_realize(SYS_BUS_DEVICE(>sata), _fatal);
+sysbus_mmio_map(SYS_BUS_DEVICE(>sata), 0,
+allwinner_r40_memmap[AW_R40_DEV_AHCI]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>sata), 0,
+ qdev_get_gpio_in(DEVICE(>gic), AW_R40_GIC_SPI_AHCI));
+
/* USB */
if (machine_usb(current_machine)) {
int i;
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index ae82822d42..c589fcc1c1 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -22,6 +22,7 @@
#include "qom/object.h"
#include "hw/timer/allwinner-a10-pit.h"
+#include "hw/ide/ahci.h"
#include "hw/intc/arm_gic.h"
#include "hw/sd/allwinner-sdhost.h"
#include "hw/misc/allwinner-r40-ccu.h"
@@ -46,6 +47,7 @@ enum {
AW_R40_DEV_MMC1,
AW_R40_DEV_MMC2,
AW_R40_DEV_MMC3,
+AW_R40_DEV_AHCI,
AW_R40_DEV_EHCI1,
AW_R40_DEV_OHCI1,
AW_R40_DEV_EHCI2,
@@ -112,6 +114,7 @@ struct AwR40State {
const hwaddr *memmap;
AwSRAMCState sramc;
AwA10PITState timer;
+AllwinnerAHCIState sata;
AwSdHostState mmc[AW_R40_NUM_MMCS];
EHCISysBusState ehci[AW_R40_NUM_USB];
OHCISysBusState ohci[AW_R40_NUM_USB];
--
2.39.2
[PATCH 3/3] hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board
Add watchdog timer support to Allwinner-H40 and Bananapi.
The watchdog timer is added as an overlay to the Timer
module memory map.
Signed-off-by: Guenter Roeck
---
docs/system/arm/bananapi_m2u.rst | 2 +-
hw/arm/Kconfig | 1 +
hw/arm/allwinner-r40.c | 8
include/hw/arm/allwinner-r40.h | 3 +++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index 542310591d..587b488655 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -25,6 +25,7 @@ The Banana Pi M2U machine supports the following devices:
* SATA
* TWI (I2C)
* USB 2.0
+ * Hardware Watchdog
Limitations
"""""""""""
@@ -33,7 +34,6 @@ Currently, Banana Pi M2U does *not* support the following
features:
- Graphical output via HDMI, GPU and/or the Display Engine
- Audio output
-- Hardware Watchdog
- Real Time Clock
Also see the 'unimplemented' array in the Allwinner R40 SoC module
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 98ca5ebc7d..386edbae15 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -411,6 +411,7 @@ config ALLWINNER_R40
select AHCI
select ALLWINNER_SRAMC
select ALLWINNER_A10_PIT
+select ALLWINNER_WDT
select AXP2XX_PMU
select SERIAL
select ARM_TIMER
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index f90d59fb5e..334692ef97 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -53,6 +53,7 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_OHCI2] = 0x01c1c400,
[AW_R40_DEV_CCU]= 0x01c2,
[AW_R40_DEV_PIT]= 0x01c20c00,
+[AW_R40_DEV_WDT]= 0x01c20c90,
[AW_R40_DEV_UART0] = 0x01c28000,
[AW_R40_DEV_UART1] = 0x01c28400,
[AW_R40_DEV_UART2] = 0x01c28800,
@@ -279,6 +280,8 @@ static void allwinner_r40_init(Object *obj)
object_property_add_alias(obj, "clk1-freq", OBJECT(>timer),
"clk1-freq");
+object_initialize_child(obj, "wdt", >wdt, TYPE_AW_WDT_SUN4I);
+
object_initialize_child(obj, "ccu", >ccu, TYPE_AW_R40_CCU);
for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
@@ -553,6 +556,11 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_connect_irq(SYS_BUS_DEVICE(>emac), 0,
qdev_get_gpio_in(DEVICE(>gic), AW_R40_GIC_SPI_EMAC));
+/* WDT */
+sysbus_realize(SYS_BUS_DEVICE(>wdt), _fatal);
+sysbus_mmio_map_overlap(SYS_BUS_DEVICE(>wdt), 0,
+allwinner_r40_memmap[AW_R40_DEV_WDT], 1);
+
/* Unimplemented devices */
for (unsigned i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
create_unimplemented_device(r40_unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index c589fcc1c1..66c38e7d90 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -33,6 +33,7 @@
#include "hw/net/allwinner-sun8i-emac.h"
#include "hw/usb/hcd-ohci.h"
#include "hw/usb/hcd-ehci.h"
+#include "hw/watchdog/allwinner-wdt.h"
#include "target/arm/cpu.h"
#include "sysemu/block-backend.h"
@@ -54,6 +55,7 @@ enum {
AW_R40_DEV_OHCI2,
AW_R40_DEV_CCU,
AW_R40_DEV_PIT,
+AW_R40_DEV_WDT,
AW_R40_DEV_UART0,
AW_R40_DEV_UART1,
AW_R40_DEV_UART2,
@@ -114,6 +116,7 @@ struct AwR40State {
const hwaddr *memmap;
AwSRAMCState sramc;
AwA10PITState timer;
+AwWdtState wdt;
AllwinnerAHCIState sata;
AwSdHostState mmc[AW_R40_NUM_MMCS];
EHCISysBusState ehci[AW_R40_NUM_USB];
--
2.39.2
[PATCH 1/3] hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board
Allwinner R40 supports two USB host ports shared between a USB 2.0 EHCI
host controller and a USB 1.1 OHCI host controller. Add support for both
of them.
If machine USB support is not enabled, create unimplemented devices
for the USB memory ranges to avoid crashes when booting Linux.
Signed-off-by: Guenter Roeck
---
docs/system/arm/bananapi_m2u.rst | 2 +-
hw/arm/Kconfig | 2 +
hw/arm/allwinner-r40.c | 70 +++-
include/hw/arm/allwinner-r40.h | 9
4 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index b09ba5c548..e77c425e2c 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -23,6 +23,7 @@ The Banana Pi M2U machine supports the following devices:
* GMAC ethernet
* Clock Control Unit
* TWI (I2C)
+ * USB 2.0
Limitations
"""""""""""
@@ -33,7 +34,6 @@ Currently, Banana Pi M2U does *not* support the following
features:
- Audio output
- Hardware Watchdog
- Real Time Clock
-- USB 2.0 interfaces
Also see the 'unimplemented' array in the Allwinner R40 SoC module
for a complete list of unimplemented I/O devices: ``./hw/arm/allwinner-r40.c``
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 39d255425b..6b508780d3 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -415,6 +415,8 @@ config ALLWINNER_R40
select ARM_TIMER
select ARM_GIC
select UNIMP
+select USB_OHCI
+select USB_EHCI_SYSBUS
select SD
config RASPI
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index a0d367c60d..f42b0fa0ce 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -23,6 +23,7 @@
#include "qemu/bswap.h"
#include "qemu/module.h"
#include "qemu/units.h"
+#include "hw/boards.h"
#include "hw/qdev-core.h"
#include "hw/sysbus.h"
#include "hw/char/serial.h"
@@ -45,6 +46,10 @@ const hwaddr allwinner_r40_memmap[] = {
[AW_R40_DEV_MMC1] = 0x01c1,
[AW_R40_DEV_MMC2] = 0x01c11000,
[AW_R40_DEV_MMC3] = 0x01c12000,
+[AW_R40_DEV_EHCI1] = 0x01c19000,
+[AW_R40_DEV_OHCI1] = 0x01c19400,
+[AW_R40_DEV_EHCI2] = 0x01c1c000,
+[AW_R40_DEV_OHCI2] = 0x01c1c400,
[AW_R40_DEV_CCU]= 0x01c2,
[AW_R40_DEV_PIT]= 0x01c20c00,
[AW_R40_DEV_UART0] = 0x01c28000,
@@ -89,9 +94,9 @@ static struct AwR40Unimplemented r40_unimplemented[] = {
{ "crypto", 0x01c15000, 4 * KiB },
{ "spi2", 0x01c17000, 4 * KiB },
{ "sata", 0x01c18000, 4 * KiB },
-{ "usb1-host", 0x01c19000, 4 * KiB },
+{ "usb1-phy", 0x01c19800, 2 * KiB },
{ "sid",0x01c1b000, 4 * KiB },
-{ "usb2-host", 0x01c1c000, 4 * KiB },
+{ "usb2-phy", 0x01c1c800, 2 * KiB },
{ "cs1",0x01c1d000, 4 * KiB },
{ "spi3", 0x01c1f000, 4 * KiB },
{ "rtc",0x01c20400, 1 * KiB },
@@ -181,6 +186,10 @@ enum {
AW_R40_GIC_SPI_MMC2 = 34,
AW_R40_GIC_SPI_MMC3 = 35,
AW_R40_GIC_SPI_EMAC = 55,
+AW_R40_GIC_SPI_OHCI1 = 64,
+AW_R40_GIC_SPI_OHCI2 = 65,
+AW_R40_GIC_SPI_EHCI1 = 76,
+AW_R40_GIC_SPI_EHCI2 = 78,
AW_R40_GIC_SPI_GMAC = 85,
};
@@ -276,6 +285,17 @@ static void allwinner_r40_init(Object *obj)
TYPE_AW_SDHOST_SUN50I_A64);
}
+if (machine_usb(current_machine)) {
+int i;
+
+for (i = 0; i < AW_R40_NUM_USB; i++) {
+object_initialize_child(obj, "ehci[*]", >ehci[i],
+TYPE_PLATFORM_EHCI);
+object_initialize_child(obj, "ohci[*]", >ohci[i],
+TYPE_SYSBUS_OHCI);
+}
+}
+
object_initialize_child(obj, "twi0", >i2c0, TYPE_AW_I2C_SUN6I);
object_initialize_child(obj, "emac", >emac, TYPE_AW_EMAC);
@@ -407,6 +427,37 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_realize(SYS_BUS_DEVICE(>ccu), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>ccu), 0, s->memmap[AW_R40_DEV_CCU]);
+/* USB */
+if (machine_usb(current_machine)) {
+int i;
+
+for (i = 0; i < AW_R40_NUM_USB; i++) {
+g_autofree char *bus = g_strdup_printf("usb-bus.%d", i);
+
+object_property_set_bool(OBJECT(>ehci[i]), "companion-enable",
+ true, _fatal);
+sysbus_realize(SYS_BUS_DEVICE(>ehci[i]), _fatal);
+sysbus_mmio_map(SYS_BUS_DEVICE(>ehci[i]), 0,
+allwinner_r40_memmap[i ? AW_R40_DEV_EHCI2
+
[PATCH 0/3] hw/arm: Add support for USB, SATA, and watchdog to Allwinner R40
Add support for - USB 2.0 EHCI/OHCI - SATA/AHCI - Watchdog to Allwinner R40. The hardware is quite similar to Allwinner A10 and H3, so the code is derived from the implementations for those SOCs. Tested with bpim2u emulation by instantiating EHCI and OHCI keyboards, by booting from USB, by booting from ATA/SATA drive, and by manually testing watchdog operation. Guenter Roeck (3): hw/arm: Add EHCI/OHCI controllers to Allwinner R40 and Bananapi board hw/arm: Add AHCI/SATA controller to Allwinner R40 and Bananapi board hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board docs/system/arm/bananapi_m2u.rst | 5 ++- hw/arm/Kconfig | 4 ++ hw/arm/allwinner-r40.c | 90 ++-- include/hw/arm/allwinner-r40.h | 15 +++ 4 files changed, 109 insertions(+), 5 deletions(-)
Re: [PATCH 3/4] esp-pci.c: synchronise setting of DMA_STAT_DONE with ESP completion interrupt
On Fri, Jan 12, 2024 at 01:15:28PM +, Mark Cave-Ayland wrote:
> The setting of DMA_STAT_DONE at the end of a DMA transfer can be configured to
> generate an interrupt, however the Linux driver manually checks for
> DMA_STAT_DONE
> being set and if it is, considers that a DMA transfer has completed.
>
> If DMA_STAT_DONE is set but the ESP device isn't indicating an interrupt then
> the Linux driver considers this to be a spurious interrupt. However this can
> occur in QEMU as there is a delay between the end of DMA transfer where
> DMA_STAT_DONE is set, and the ESP device raising its completion interrupt.
>
> This appears to be an incorrect assumption in the Linux driver as the ESP and
> PCI DMA interrupt sources are separate (and may not be raised exactly
> together), however we can work around this by synchronising the setting of
> DMA_STAT_DONE at the end of a DMA transfer with the ESP completion interrupt.
>
> In conjunction with the previous commit Linux is now able to correctly boot
> from an am53c974 PCI SCSI device on the hppa C3700 machine without emitting
> "iget: checksum invalid" and "Spurious irq, sreg=10" errors.
>
> Signed-off-by: Mark Cave-Ayland
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
> ---
> hw/scsi/esp-pci.c | 28 +---
> 1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 15dc3c004d..875a49199d 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -93,6 +93,18 @@ static void esp_irq_handler(void *opaque, int irq_num, int
> level)
>
> if (level) {
> pci->dma_regs[DMA_STAT] |= DMA_STAT_SCSIINT;
> +
> +/*
> + * If raising the ESP IRQ to indicate end of DMA transfer, set
> + * DMA_STAT_DONE at the same time. In theory this should be done in
> + * esp_pci_dma_memory_rw(), however there is a delay between setting
> + * DMA_STAT_DONE and the ESP IRQ arriving which is visible to the
> + * guest that can cause confusion e.g. Linux
> + */
> +if ((pci->dma_regs[DMA_CMD] & DMA_CMD_MASK) == 0x3 &&
> +pci->dma_regs[DMA_WBC] == 0) {
> +pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> +}
> } else {
> pci->dma_regs[DMA_STAT] &= ~DMA_STAT_SCSIINT;
> }
> @@ -306,9 +318,6 @@ static void esp_pci_dma_memory_rw(PCIESPState *pci,
> uint8_t *buf, int len,
> /* update status registers */
> pci->dma_regs[DMA_WBC] -= len;
> pci->dma_regs[DMA_WAC] += len;
> -if (pci->dma_regs[DMA_WBC] == 0) {
> -pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> -}
> }
>
> static void esp_pci_dma_memory_read(void *opaque, uint8_t *buf, int len)
> @@ -363,24 +372,13 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
> }
> };
>
> -static void esp_pci_command_complete(SCSIRequest *req, size_t resid)
> -{
> -ESPState *s = req->hba_private;
> -PCIESPState *pci = container_of(s, PCIESPState, esp);
> -
> -esp_command_complete(req, resid);
> -pci->dma_regs[DMA_WBC] = 0;
> -pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> -esp_pci_update_irq(pci);
> -}
> -
> static const struct SCSIBusInfo esp_pci_scsi_info = {
> .tcq = false,
> .max_target = ESP_MAX_DEVS,
> .max_lun = 7,
>
> .transfer_data = esp_transfer_data,
> -.complete = esp_pci_command_complete,
> +.complete = esp_command_complete,
> .cancel = esp_request_cancelled,
> };
>
> --
> 2.39.2
>
Re: [PATCH 4/4] esp-pci.c: set DMA_STAT_BCMBLT when BLAST command issued
On Fri, Jan 12, 2024 at 01:15:29PM +, Mark Cave-Ayland wrote:
> Even though the BLAST command isn't fully implemented in QEMU, the
> DMA_STAT_BCMBLT
> bit should be set after the command has been issued to indicate that the
> command
> has completed.
>
> This fixes an issue with the DC390 DOS driver which issues the BLAST command
> as
> part of its normal error recovery routine at startup, and otherwise sits in a
> tight loop waiting for DMA_STAT_BCMBLT to be set before continuing.
>
> Signed-off-by: Mark Cave-Ayland
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
> ---
> hw/scsi/esp-pci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 875a49199d..42d9d2e483 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -124,6 +124,7 @@ static void esp_pci_handle_blast(PCIESPState *pci,
> uint32_t val)
> {
> trace_esp_pci_dma_blast(val);
> qemu_log_mask(LOG_UNIMP, "am53c974: cmd BLAST not implemented\n");
> +pci->dma_regs[DMA_STAT] |= DMA_STAT_BCMBLT;
> }
>
> static void esp_pci_handle_abort(PCIESPState *pci, uint32_t val)
> --
> 2.39.2
>
Re: [PATCH 2/4] esp-pci.c: generate PCI interrupt from separate ESP and PCI sources
On Fri, Jan 12, 2024 at 01:15:27PM +, Mark Cave-Ayland wrote:
> The am53c974/dc390 PCI interrupt has two separate sources: the first is from
> the
> internal ESP device, and the second is from the PCI DMA transfer logic.
>
> Update the ESP interrupt handler so that it sets DMA_STAT_SCSIINT rather than
> driving the PCI IRQ directly, and introduce a new esp_pci_update_irq()
> function
> to generate the correct PCI IRQ level. In particular this fixes spurious
> interrupts
> being generated by setting DMA_STAT_DONE at the end of a transfer if
> DMA_CMD_INTE_D
> isn't set in the DMA_CMD register.
>
> Signed-off-by: Mark Cave-Ayland
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
> ---
> hw/scsi/esp-pci.c | 32 +++-
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 7117725371..15dc3c004d 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -77,6 +77,29 @@ struct PCIESPState {
> ESPState esp;
> };
>
> +static void esp_pci_update_irq(PCIESPState *pci)
> +{
> +int scsi_level = !!(pci->dma_regs[DMA_STAT] & DMA_STAT_SCSIINT);
> +int dma_level = (pci->dma_regs[DMA_CMD] & DMA_CMD_INTE_D) ?
> +!!(pci->dma_regs[DMA_STAT] & DMA_STAT_DONE) : 0;
> +int level = scsi_level || dma_level;
> +
> +pci_set_irq(PCI_DEVICE(pci), level);
> +}
> +
> +static void esp_irq_handler(void *opaque, int irq_num, int level)
> +{
> +PCIESPState *pci = PCI_ESP(opaque);
> +
> +if (level) {
> +pci->dma_regs[DMA_STAT] |= DMA_STAT_SCSIINT;
> +} else {
> +pci->dma_regs[DMA_STAT] &= ~DMA_STAT_SCSIINT;
> +}
> +
> +esp_pci_update_irq(pci);
> +}
> +
> static void esp_pci_handle_idle(PCIESPState *pci, uint32_t val)
> {
> ESPState *s = >esp;
> @@ -151,6 +174,7 @@ static void esp_pci_dma_write(PCIESPState *pci, uint32_t
> saddr, uint32_t val)
> /* clear some bits on write */
> uint32_t mask = DMA_STAT_ERROR | DMA_STAT_ABORT | DMA_STAT_DONE;
> pci->dma_regs[DMA_STAT] &= ~(val & mask);
> +esp_pci_update_irq(pci);
> }
> break;
> default:
> @@ -161,17 +185,14 @@ static void esp_pci_dma_write(PCIESPState *pci,
> uint32_t saddr, uint32_t val)
>
> static uint32_t esp_pci_dma_read(PCIESPState *pci, uint32_t saddr)
> {
> -ESPState *s = >esp;
> uint32_t val;
>
> val = pci->dma_regs[saddr];
> if (saddr == DMA_STAT) {
> -if (s->rregs[ESP_RSTAT] & STAT_INT) {
> -val |= DMA_STAT_SCSIINT;
> -}
> if (!(pci->sbac & SBAC_STATUS)) {
> pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_ERROR | DMA_STAT_ABORT |
> DMA_STAT_DONE);
> +esp_pci_update_irq(pci);
> }
> }
>
> @@ -350,6 +371,7 @@ static void esp_pci_command_complete(SCSIRequest *req,
> size_t resid)
> esp_command_complete(req, resid);
> pci->dma_regs[DMA_WBC] = 0;
> pci->dma_regs[DMA_STAT] |= DMA_STAT_DONE;
> +esp_pci_update_irq(pci);
> }
>
> static const struct SCSIBusInfo esp_pci_scsi_info = {
> @@ -386,7 +408,7 @@ static void esp_pci_scsi_realize(PCIDevice *dev, Error
> **errp)
>"esp-io", 0x80);
>
> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >io);
> -s->irq = pci_allocate_irq(dev);
> +s->irq = qemu_allocate_irq(esp_irq_handler, pci, 0);
>
> scsi_bus_init(>bus, sizeof(s->bus), d, _pci_scsi_info);
> }
> --
> 2.39.2
>
Re: [PATCH 1/4] esp-pci.c: use correct address register for PCI DMA transfers
On Fri, Jan 12, 2024 at 01:15:26PM +, Mark Cave-Ayland wrote:
> The current code in esp_pci_dma_memory_rw() sets the DMA address to the value
> of the DMA_SPA (Starting Physical Address) register which is incorrect: this
> means that for each callback from the SCSI layer the DMA address is set back
> to the starting address.
>
> In the case where only a single SCSI callback occurs (currently for transfer
> lengths < 128kB) this works fine, however for larger transfers the DMA address
> wraps back to the initial starting address, corrupting the buffer holding the
> data transferred to the guest.
>
> Fix esp_pci_dma_memory_rw() to use the DMA_WAC (Working Address Counter) for
> the DMA address which is correctly incremented across multiple SCSI layer
> transfers.
>
> Signed-off-by: Mark Cave-Ayland
Reviewed-by: Guenter Roeck
Tested-by: Guenter Roeck
> ---
> hw/scsi/esp-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 93b3429e0f..7117725371 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -275,7 +275,7 @@ static void esp_pci_dma_memory_rw(PCIESPState *pci,
> uint8_t *buf, int len,
> qemu_log_mask(LOG_UNIMP, "am53c974: MDL transfer not implemented\n");
> }
>
> -addr = pci->dma_regs[DMA_SPA];
> +addr = pci->dma_regs[DMA_WAC];
> if (pci->dma_regs[DMA_WBC] < len) {
> len = pci->dma_regs[DMA_WBC];
> }
> --
> 2.39.2
>
Re: [PATCH] hw/sd/pxa2xx_mmci: Disable reentrancy detection
On 12/13/23 09:19, Philippe Mathieu-Daudé wrote:
Hi Guenter,
On 13/12/23 18:12, Peter Maydell wrote:
On Wed, 13 Dec 2023 at 01:49, Guenter Roeck wrote:
All tests using pxa2xx_mmc to access mmc cards on pxa2xx platforms
such as borzoi fail starting with commit a2e1753b80 ("memory: prevent
dma-reentracy issues"). Disable reentrancy guard to fix the problem.
Fixes: a2e1753b80 ("memory: prevent dma-reentracy issues")
Signed-off-by: Guenter Roeck
---
hw/sd/pxa2xx_mmci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 5e8ea69188..27ae8f2888 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -555,6 +555,8 @@ static void pxa2xx_mmci_instance_init(Object *obj)
qdev_init_gpio_out_named(dev, >rx_dma, "rx-dma", 1);
qdev_init_gpio_out_named(dev, >tx_dma, "tx-dma", 1);
+ s->iomem.disable_reentrancy_guard = true;
+
All patches that set this flag should include a comment which
explains what the device access path that triggers the reentrancy
is, please.
Can we get a reproducer or backtrace please?
qemu-system-arm: warning: Blocked re-entrant IO on MemoryRegion: pxa2xx-mmci at
addr: 0x40
[0.770246] mmc0: invalid bus width
[0.770962] mmc0: error -22 whilst initialising SD card
[0.828179] mmc0: invalid bus width
[0.828445] mmc0: error -22 whilst initialising SD card
with:
qemu-system-arm -M borzoi -kernel arch/arm/boot/zImage -no-reboot -snapshot \
-device sd-card,drive=d0 -drive
file=/tmp/flash,format=raw,if=none,id=d0 \
-usb -device usb-net,netdev=net0 -netdev user,id=net0 \
--append "root=/dev/mmcblk0 rootwait console=ttyS0"
Guenter
Re: [PATCH] hw/sd/pxa2xx_mmci: Disable reentrancy detection
On 12/13/23 09:12, Peter Maydell wrote:
On Wed, 13 Dec 2023 at 01:49, Guenter Roeck wrote:
All tests using pxa2xx_mmc to access mmc cards on pxa2xx platforms
such as borzoi fail starting with commit a2e1753b80 ("memory: prevent
dma-reentracy issues"). Disable reentrancy guard to fix the problem.
Fixes: a2e1753b80 ("memory: prevent dma-reentracy issues")
Signed-off-by: Guenter Roeck
---
hw/sd/pxa2xx_mmci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 5e8ea69188..27ae8f2888 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -555,6 +555,8 @@ static void pxa2xx_mmci_instance_init(Object *obj)
qdev_init_gpio_out_named(dev, >rx_dma, "rx-dma", 1);
qdev_init_gpio_out_named(dev, >tx_dma, "tx-dma", 1);
+s->iomem.disable_reentrancy_guard = true;
+
All patches that set this flag should include a comment which
explains what the device access path that triggers the reentrancy
is, please.
No idea what that would be, sorry. I noticed that the reentrancy guard
causes the affected emulations to fail, but I have no understanding or
knowledge of the code itself. NP if this is insufficient to apply the patch.
I am carrying it locally anyway, so for me it doesn't make a difference.
Maybe someone with better understanding of the underlying code can pick
it up at some point in the future and provide the necessary context.
Thanks,
Guenter
Re: [RFC 1/1] hw: tpmtisspi: add SPI support to QEMU TPM implementation
Hi,
On Tue, Aug 02, 2022 at 07:32:41PM -0700, Iris Chen wrote:
> From: Iris Chen
>
> Signed-off-by: Iris Chen
> ---
Are there any plans to submit a new version of this patch ?
Thanks,
Guenter
> configs/devices/arm-softmmu/default.mak | 1 +
> hw/arm/Kconfig | 5 +
> hw/tpm/Kconfig | 5 +
> hw/tpm/meson.build | 1 +
> hw/tpm/tpm_tis_spi.c| 311
> include/sysemu/tpm.h| 3 +
> 6 files changed, 326 insertions(+)
> create mode 100644 hw/tpm/tpm_tis_spi.c
>
> diff --git a/configs/devices/arm-softmmu/default.mak
> b/configs/devices/arm-softmmu/default.mak
> index 6985a25377..80d2841568 100644
> --- a/configs/devices/arm-softmmu/default.mak
> +++ b/configs/devices/arm-softmmu/default.mak
> @@ -42,3 +42,4 @@ CONFIG_FSL_IMX6UL=y
> CONFIG_SEMIHOSTING=y
> CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> CONFIG_ALLWINNER_H3=y
> +CONFIG_FBOBMC_AST=y
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 15fa79afd3..193decaec1 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -458,6 +458,11 @@ config ASPEED_SOC
> select PMBUS
> select MAX31785
>
> +config FBOBMC_AST
> +bool
> +select ASPEED_SOC
> +select TPM_TIS_SPI
> +
> config MPS2
> bool
> imply I2C_DEVICES
> diff --git a/hw/tpm/Kconfig b/hw/tpm/Kconfig
> index 29e82f3c92..370a43f045 100644
> --- a/hw/tpm/Kconfig
> +++ b/hw/tpm/Kconfig
> @@ -8,6 +8,11 @@ config TPM_TIS_SYSBUS
> depends on TPM
> select TPM_TIS
>
> +config TPM_TIS_SPI
> +bool
> +depends on TPM
> +select TPM_TIS
> +
> config TPM_TIS
> bool
> depends on TPM
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 1c68d81d6a..1a057f4e36 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -2,6 +2,7 @@ softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true:
> files('tpm_tis_common.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true:
> files('tpm_tis_sysbus.c'))
> softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_SPI', if_true: files('tpm_tis_spi.c'))
>
> specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_TIS'], if_true:
> files('tpm_ppi.c'))
> specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TPM_CRB'], if_true:
> files('tpm_ppi.c'))
> diff --git a/hw/tpm/tpm_tis_spi.c b/hw/tpm/tpm_tis_spi.c
> new file mode 100644
> index 00..c98ddcfddb
> --- /dev/null
> +++ b/hw/tpm/tpm_tis_spi.c
> @@ -0,0 +1,311 @@
> +#include "qemu/osdep.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +#include "hw/acpi/tpm.h"
> +#include "tpm_prop.h"
> +#include "tpm_tis.h"
> +#include "qom/object.h"
> +#include "hw/ssi/ssi.h"
> +#include "hw/ssi/spi_gpio.h"
> +
> +#define TPM_TIS_SPI_ADDR_BYTES 3
> +#define SPI_WRITE 0
> +
> +typedef enum {
> +TIS_SPI_PKT_STATE_DEACTIVATED = 0,
> +TIS_SPI_PKT_STATE_START,
> +TIS_SPI_PKT_STATE_ADDRESS,
> +TIS_SPI_PKT_STATE_DATA_WR,
> +TIS_SPI_PKT_STATE_DATA_RD,
> +TIS_SPI_PKT_STATE_DONE,
> +} TpmTisSpiPktState;
> +
> +union TpmTisRWSizeByte {
> +uint8_t byte;
> +struct {
> +uint8_t data_expected_size:6;
> +uint8_t resv:1;
> +uint8_t rwflag:1;
> +};
> +};
> +
> +union TpmTisSpiHwAddr {
> +hwaddr addr;
> +uint8_t bytes[sizeof(hwaddr)];
> +};
> +
> +union TpmTisSpiData {
> +uint32_t data;
> +uint8_t bytes[64];
> +};
> +
> +struct TpmTisSpiState {
> +/*< private >*/
> +SSIPeripheral parent_obj;
> +
> +/*< public >*/
> +TPMState tpm_state; /* not a QOM object */
> +TpmTisSpiPktState tpm_tis_spi_state;
> +
> +union TpmTisRWSizeByte first_byte;
> +union TpmTisSpiHwAddr addr;
> +union TpmTisSpiData data;
> +
> +uint32_t data_size;
> +uint8_t data_idx;
> +uint8_t addr_idx;
> +};
> +
> +struct TpmTisSpiClass {
> +SSIPeripheralClass parent_class;
> +};
> +
> +OBJECT_DECLARE_TYPE(TpmTisSpiState, TpmTisSpiClass, TPM_TIS_SPI)
> +
> +static void tpm_tis_spi_mmio_read(TpmTisSpiState *tts)
> +{
> +uint16_t offset = tts->addr.addr & 0xffc;
> +
> +switch (offset) {
> +case TPM_TIS_REG_DATA_FIFO:
> +for (uint8_t i = 0; i < tts->data_size; i++) {
> +tts->data.bytes[i] = (uint8_t)tpm_tis_memory_ops.read(
> + >tpm_state,
> + tts->addr.addr,
> + 1);
> +}
> +break;
> +default:
> +tts->data.data = (uint32_t)tpm_tis_memory_ops.read(
> + >tpm_state,
> + tts->addr.addr,
> + tts->data_size);
> +}
> +}
> +
> +static void tpm_tis_spi_mmio_write(TpmTisSpiState *tts)
> +{
> +uint16_t offset =
[PATCH] hw/sd/pxa2xx_mmci: Disable reentrancy detection
All tests using pxa2xx_mmc to access mmc cards on pxa2xx platforms
such as borzoi fail starting with commit a2e1753b80 ("memory: prevent
dma-reentracy issues"). Disable reentrancy guard to fix the problem.
Fixes: a2e1753b80 ("memory: prevent dma-reentracy issues")
Signed-off-by: Guenter Roeck
---
hw/sd/pxa2xx_mmci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 5e8ea69188..27ae8f2888 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -555,6 +555,8 @@ static void pxa2xx_mmci_instance_init(Object *obj)
qdev_init_gpio_out_named(dev, >rx_dma, "rx-dma", 1);
qdev_init_gpio_out_named(dev, >tx_dma, "tx-dma", 1);
+s->iomem.disable_reentrancy_guard = true;
+
qbus_init(>sdbus, sizeof(s->sdbus),
TYPE_PXA2XX_MMCI_BUS, DEVICE(obj), "sd-bus");
}
--
2.39.2
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
On 8/1/23 09:01, Peter Maydell wrote:
On Sat, 24 Jun 2023 at 16:02, Guenter Roeck wrote:
On 6/24/23 07:23, Guenter Roeck wrote:
On 6/24/23 03:40, Peter Maydell wrote:
On Fri, 23 Jun 2023 at 20:33, Guenter Roeck wrote:
On 6/23/23 10:44, Peter Maydell wrote:
On Sat, 17 Jun 2023 at 17:29, Guenter Roeck wrote:
Main problem is that the SD card gets instantiated randomly to
mmc0, mmc1, or mmc2, making it all but impossible to specify a
root file system device. The non-instantiated cards are always
reported as non-removable, including mmc0. Example:
mmc0: Failed to initialize a non-removable card
Do you mean that QEMU randomly connects the SD card to
a different MMC controller each time, or that Linux is
randomly assigning mmc0 to a different MMC controller each
time ?
Good question. Given the workaround (fix ?) I suggested is
in the devicetree file, I would assume it is the latter. I suspect
that Linux assigns drive names based on hardware detection order,
and that this is not deterministic for some reason. It is odd
because I have never experienced that with any other emulation.
Yeah, I don't really understand why it would be non-deterministic.
But it does make it sound like the right thing is for the
device tree file to explicitly say which MMC controller is
which -- presumably you might get unlucky with the timing
on real hardware too.
Agreed, only someone with real hardware would have to confirm
that this is the case.
Actually, the reason is quite simple. In the Linux kernel:
static struct platform_driver sunxi_mmc_driver = {
.driver = {
.name = "sunxi-mmc",
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
^
.of_match_table = sunxi_mmc_of_match,
.pm = _mmc_pm_ops,
},
.probe = sunxi_mmc_probe,
.remove = sunxi_mmc_remove,
};
All mmc devices instantiate at the same time, thus the
device name association is random. If I drop the probe_type
assignment, it becomes deterministic.
On top of that, Linux does know which drives are removable
from the devicetree file. However, since probe order is
random, the assignment of the one removable drive to device
names is random. Sometimes mmc0 shows up as removable,
sometimes it is mmc1 or mmc2.
So my conclusion is that qemu isn't doing anything wrong,
it is all happening in the Linux kernel.
Hi Guenter -- do you know if this "random MMC controller"
issue has been fixed in Linux ? If so, we might be able
to update our test case image to avoid the slightly ugly
"root=b300" workaround at some point.
No, it has not been fixed, or at least there is nothing in linux-next.
I don't have real hardware, so I am not in a position to submit, much
less test, a patch on it. Someone had mentioned that real hardware would
handle the problem in initramfs. That seems wrong to me, but it is what
it is.
I changed my own test to use the "root=b300" hack. That seems highly kludgy,
but at least it works.
Guenter
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On 7/24/23 00:18, Bernhard Beschow wrote:
Am 16. Juli 2023 19:53:37 UTC schrieb Bernhard Beschow :
Am 10. Juli 2023 16:01:46 UTC schrieb Bernhard Beschow :
Am 10. Juli 2023 10:16:35 UTC schrieb "Philippe Mathieu-Daudé"
:
On 9/7/23 10:09, Bernhard Beschow wrote:
Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller
interfaces" sdhci_common_realize() forces all SD card controllers to use either
sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property.
However, there are device models which use different MMIO ops: TYPE_IMX_USDHC
uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for
example. Fix this by defaulting the io_ops to little endian and switch to big
endian in sdhci_common_realize() only if there is a matchig big endian variant
available.
Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
interfaces")
Signed-off-by: Bernhard Beschow
---
hw/sd/sdhci.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6811f0f1a8..362c2c86aa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
sdhci_raise_insertion_irq, s);
s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
sdhci_data_transfer, s);
+
+s->io_ops = _mmio_le_ops;
}
void sdhci_uninitfn(SDHCIState *s)
@@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
What about simply keeping the same code guarded with 'if (!s->io_ops)'?
I chose below approach since it provides an error message when one attempts to
set one of the other device models to BE rather than just silently ignoring it.
Also, I didn't want to make the assumption that `s->io_ops == NULL` implied
that sdhci_mmio_*_ops is needed. That's similar material the bug fixed is made of,
so I wanted to prevent that in the first place by being more explicit.
In combination with the new error message the limitations of the current code
become hopefully very apparent now, and at the same time should provide enough
hints for adding BE support to the other device models if ever needed.
Best regards,
Bernhard
Ping
Ping^2
I would like to have the bug fixed in 8.1.
+1
Not that I care too much - I build qemu myself anyway and carry the patch
locally -
but this really should get fixed.
Guenter
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On Mon, Jul 10, 2023 at 12:16:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 9/7/23 10:09, Bernhard Beschow wrote:
> > Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host
> > controller
> > interfaces" sdhci_common_realize() forces all SD card controllers to use
> > either
> > sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness"
> > property.
> > However, there are device models which use different MMIO ops:
> > TYPE_IMX_USDHC
> > uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
> >
> > Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board,
> > for
> > example. Fix this by defaulting the io_ops to little endian and switch to
> > big
> > endian in sdhci_common_realize() only if there is a matchig big endian
> > variant
> > available.
> >
> > Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
> > interfaces")
> >
> > Signed-off-by: Bernhard Beschow
> > ---
> > hw/sd/sdhci.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 6811f0f1a8..362c2c86aa 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
> > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_raise_insertion_irq, s);
> > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_data_transfer, s);
> > +
> > +s->io_ops = _mmio_le_ops;
> > }
> > void sdhci_uninitfn(SDHCIState *s)
> > @@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error
> > **errp)
>
> What about simply keeping the same code guarded with 'if (!s->io_ops)'?
>
That was my quick fix which I considered a hack, and I didn't submit it
because I thought it was a hack ;-).
On the other side, this solution would probably break on big endian systems
which have their own io ops, so I am not sure if it is any better.
Guenter
> > switch (s->endianness) {
> > case DEVICE_LITTLE_ENDIAN:
> > -s->io_ops = _mmio_le_ops;
> > +/* s->io_ops is little endian by default */
> > break;
> > case DEVICE_BIG_ENDIAN:
> > +if (s->io_ops != _mmio_le_ops) {
> > +error_setg(errp, "SD controller doesn't support big
> > endianness");
> > +return;
> > +}
> > s->io_ops = _mmio_be_ops;
> > break;
> > default:
>
Re: [PATCH] hw/sd/sdhci: Do not force sdhci_mmio_*_ops onto all SD controllers
On 7/9/23 01:09, Bernhard Beschow wrote:
Since commit c0a55a0c9da2 "hw/sd/sdhci: Support big endian SD host controller
interfaces" sdhci_common_realize() forces all SD card controllers to use either
sdhci_mmio_le_ops or sdhci_mmio_be_ops, depending on the "endianness" property.
However, there are device models which use different MMIO ops: TYPE_IMX_USDHC
uses usdhc_mmio_ops and TYPE_S3C_SDHCI uses sdhci_s3c_mmio_ops.
Forcing sdhci_mmio_le_ops breaks SD card handling on the "sabrelite" board, for
example. Fix this by defaulting the io_ops to little endian and switch to big
endian in sdhci_common_realize() only if there is a matchig big endian variant
available.
Fixes: c0a55a0c9da2 ("hw/sd/sdhci: Support big endian SD host controller
interfaces")
Signed-off-by: Bernhard Beschow
Tested-by: Guenter Roeck
---
hw/sd/sdhci.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6811f0f1a8..362c2c86aa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1382,6 +1382,8 @@ void sdhci_initfn(SDHCIState *s)
s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer,
s);
+
+s->io_ops = _mmio_le_ops;
}
void sdhci_uninitfn(SDHCIState *s)
@@ -1399,9 +1401,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
switch (s->endianness) {
case DEVICE_LITTLE_ENDIAN:
-s->io_ops = _mmio_le_ops;
+/* s->io_ops is little endian by default */
break;
case DEVICE_BIG_ENDIAN:
+if (s->io_ops != _mmio_le_ops) {
+error_setg(errp, "SD controller doesn't support big endianness");
+return;
+}
s->io_ops = _mmio_be_ops;
break;
default:
[PATCH v2] riscv: Generate devicetree only after machine initialization is complete
If the devicetree is created before machine initialization is complete,
it misses dynamic devices. Specifically, the tpm device is not added
to the devicetree file and is therefore not instantiated in Linux.
Load/create devicetree in virt_machine_done() to solve the problem.
Cc: Daniel Henrique Barboza
Cc: Alistair Francis
Cc: Daniel Henrique Barboza
Fixes: 325b7c4e75 hw/riscv: Enable TPM backends
Signed-off-by: Guenter Roeck
---
v2: Handle devicetree (load & create) entirely in machine_done function.
hw/riscv/virt.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ed4c27487e..1c4bd823df 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1248,6 +1248,17 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint64_t kernel_entry = 0;
BlockBackend *pflash_blk0;
+/* load/create device tree */
+if (machine->dtb) {
+machine->fdt = load_device_tree(machine->dtb, >fdt_size);
+if (!machine->fdt) {
+error_report("load_device_tree() failed");
+exit(1);
+}
+} else {
+create_fdt(s, memmap);
+}
+
/*
* Only direct boot kernel is currently supported for KVM VM,
* so the "-bios" parameter is not supported when KVM is enabled.
@@ -1508,17 +1519,6 @@ static void virt_machine_init(MachineState *machine)
}
virt_flash_map(s, system_memory);
-/* load/create device tree */
-if (machine->dtb) {
-machine->fdt = load_device_tree(machine->dtb, >fdt_size);
-if (!machine->fdt) {
-error_report("load_device_tree() failed");
-exit(1);
-}
-} else {
-create_fdt(s, memmap);
-}
-
s->machine_done.notify = virt_machine_done;
qemu_add_machine_init_done_notifier(>machine_done);
}
--
2.39.2
Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete
On 7/3/23 12:25, Daniel Henrique Barboza wrote:
On 7/3/23 00:46, Guenter Roeck wrote:
If the devicetree is created before machine initialization is complete,
it misses dynamic devices. Specifically, the tpm device is not added
to the devicetree file and is therefore not instantiated in Linux.
Create devicetree in virt_machine_done() to solve the problem.
Cc: Alistair Francis
Fixes: 325b7c4e75 hw/riscv: Enable TPM backends
Signed-off-by: Guenter Roeck
---
hw/riscv/virt.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ed4c27487e..08876284f5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint64_t kernel_entry = 0;
BlockBackend *pflash_blk0;
+ /* create devicetree if not provided */
+ if (!machine->dtb) {
+ create_fdt(s, memmap);
+ }
+
I suggest moving the entire load/create DT code from virt_machine_init() to
the start of virt_machine_done():
/* load/create device tree */
if (machine->dtb) {
machine->fdt = load_device_tree(machine->dtb, >fdt_size);
if (!machine->fdt) {
error_report("load_device_tree() failed");
exit(1);
}
} else {
create_fdt(s, memmap);
}
This way we don't have to look in to 2 different functions to wonder what
happens
in case machine->dtb is NULL.
I can do that, but I don't know how to test it. Is there a working dtb/machine
combination for riscv which would let me test loading a devicetree file ?
Guenter
Re: [PATCH] riscv: Generate devicetree only after machine initialization is complete
On 7/3/23 00:46, Philippe Mathieu-Daudé wrote:
On 3/7/23 05:46, Guenter Roeck wrote:
If the devicetree is created before machine initialization is complete,
it misses dynamic devices. Specifically, the tpm device is not added
to the devicetree file and is therefore not instantiated in Linux.
Create devicetree in virt_machine_done() to solve the problem.
This makes sense, but what about the other archs/machines?
Shouldn't we fix this generically?
I had a brief look into various implementations. To me the code looks
all very machine specific. I don't think it can be solved generically
(if other machines are even affected), but then I am not a qemu expert
and may be wrong.
Guenter
Cc: Alistair Francis
Fixes: 325b7c4e75 hw/riscv: Enable TPM backends
Signed-off-by: Guenter Roeck
---
hw/riscv/virt.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ed4c27487e..08876284f5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint64_t kernel_entry = 0;
BlockBackend *pflash_blk0;
+ /* create devicetree if not provided */
+ if (!machine->dtb) {
+ create_fdt(s, memmap);
+ }
+
/*
* Only direct boot kernel is currently supported for KVM VM,
* so the "-bios" parameter is not supported when KVM is enabled.
@@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine)
}
virt_flash_map(s, system_memory);
- /* load/create device tree */
+ /* load device tree */
if (machine->dtb) {
machine->fdt = load_device_tree(machine->dtb, >fdt_size);
if (!machine->fdt) {
error_report("load_device_tree() failed");
exit(1);
}
- } else {
- create_fdt(s, memmap);
}
s->machine_done.notify = virt_machine_done;
[PATCH] riscv: Generate devicetree only after machine initialization is complete
If the devicetree is created before machine initialization is complete,
it misses dynamic devices. Specifically, the tpm device is not added
to the devicetree file and is therefore not instantiated in Linux.
Create devicetree in virt_machine_done() to solve the problem.
Cc: Alistair Francis
Fixes: 325b7c4e75 hw/riscv: Enable TPM backends
Signed-off-by: Guenter Roeck
---
hw/riscv/virt.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ed4c27487e..08876284f5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1248,6 +1248,11 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint64_t kernel_entry = 0;
BlockBackend *pflash_blk0;
+/* create devicetree if not provided */
+if (!machine->dtb) {
+create_fdt(s, memmap);
+}
+
/*
* Only direct boot kernel is currently supported for KVM VM,
* so the "-bios" parameter is not supported when KVM is enabled.
@@ -1508,15 +1513,13 @@ static void virt_machine_init(MachineState *machine)
}
virt_flash_map(s, system_memory);
-/* load/create device tree */
+/* load device tree */
if (machine->dtb) {
machine->fdt = load_device_tree(machine->dtb, >fdt_size);
if (!machine->fdt) {
error_report("load_device_tree() failed");
exit(1);
}
-} else {
-create_fdt(s, memmap);
}
s->machine_done.notify = virt_machine_done;
--
2.39.2
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
On 6/24/23 07:23, Guenter Roeck wrote:
On 6/24/23 03:40, Peter Maydell wrote:
On Fri, 23 Jun 2023 at 20:33, Guenter Roeck wrote:
On 6/23/23 10:44, Peter Maydell wrote:
On Sat, 17 Jun 2023 at 17:29, Guenter Roeck wrote:
Hi,
On Tue, May 23, 2023 at 06:04:58PM +0800, qianfangui...@163.com wrote:
From: qianfan Zhao
Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
for In-Car Entertainment usage, A40i and A40pro are variants that
differ in applicable temperatures range (industrial and military).
Signed-off-by: qianfan Zhao
Reviewed-by: Niek Linnenbank
I tried this in mainline linux with the following command.
qemu-system-arm -M bpim2u \
-kernel arch/arm/boot/zImage -no-reboot \
-snapshot -drive file=rootfs-armv7a.ext2,format=raw,if=sd \
-nic user \
--append "root=/dev/mmcblk0 rootwait console=ttyS0,115200" \
-dtb arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dtb \
-nographic -monitor null -serial stdio
Main problem is that the SD card gets instantiated randomly to
mmc0, mmc1, or mmc2, making it all but impossible to specify a
root file system device. The non-instantiated cards are always
reported as non-removable, including mmc0. Example:
mmc0: Failed to initialize a non-removable card
Do you mean that QEMU randomly connects the SD card to
a different MMC controller each time, or that Linux is
randomly assigning mmc0 to a different MMC controller each
time ?
Good question. Given the workaround (fix ?) I suggested is
in the devicetree file, I would assume it is the latter. I suspect
that Linux assigns drive names based on hardware detection order,
and that this is not deterministic for some reason. It is odd
because I have never experienced that with any other emulation.
Yeah, I don't really understand why it would be non-deterministic.
But it does make it sound like the right thing is for the
device tree file to explicitly say which MMC controller is
which -- presumably you might get unlucky with the timing
on real hardware too.
Agreed, only someone with real hardware would have to confirm
that this is the case.
Actually, the reason is quite simple. In the Linux kernel:
static struct platform_driver sunxi_mmc_driver = {
.driver = {
.name = "sunxi-mmc",
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
^
.of_match_table = sunxi_mmc_of_match,
.pm = _mmc_pm_ops,
},
.probe = sunxi_mmc_probe,
.remove = sunxi_mmc_remove,
};
All mmc devices instantiate at the same time, thus the
device name association is random. If I drop the probe_type
assignment, it becomes deterministic.
On top of that, Linux does know which drives are removable
from the devicetree file. However, since probe order is
random, the assignment of the one removable drive to device
names is random. Sometimes mmc0 shows up as removable,
sometimes it is mmc1 or mmc2.
So my conclusion is that qemu isn't doing anything wrong,
it is all happening in the Linux kernel.
Thanks, and sorry for the noise.
Guenter
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
On 6/24/23 03:40, Peter Maydell wrote: On Fri, 23 Jun 2023 at 20:33, Guenter Roeck wrote: On 6/23/23 10:44, Peter Maydell wrote: On Sat, 17 Jun 2023 at 17:29, Guenter Roeck wrote: Hi, On Tue, May 23, 2023 at 06:04:58PM +0800, qianfangui...@163.com wrote: From: qianfan Zhao Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU, and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3 for In-Car Entertainment usage, A40i and A40pro are variants that differ in applicable temperatures range (industrial and military). Signed-off-by: qianfan Zhao Reviewed-by: Niek Linnenbank I tried this in mainline linux with the following command. qemu-system-arm -M bpim2u \ -kernel arch/arm/boot/zImage -no-reboot \ -snapshot -drive file=rootfs-armv7a.ext2,format=raw,if=sd \ -nic user \ --append "root=/dev/mmcblk0 rootwait console=ttyS0,115200" \ -dtb arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dtb \ -nographic -monitor null -serial stdio Main problem is that the SD card gets instantiated randomly to mmc0, mmc1, or mmc2, making it all but impossible to specify a root file system device. The non-instantiated cards are always reported as non-removable, including mmc0. Example: mmc0: Failed to initialize a non-removable card Do you mean that QEMU randomly connects the SD card to a different MMC controller each time, or that Linux is randomly assigning mmc0 to a different MMC controller each time ? Good question. Given the workaround (fix ?) I suggested is in the devicetree file, I would assume it is the latter. I suspect that Linux assigns drive names based on hardware detection order, and that this is not deterministic for some reason. It is odd because I have never experienced that with any other emulation. Yeah, I don't really understand why it would be non-deterministic. But it does make it sound like the right thing is for the device tree file to explicitly say which MMC controller is which -- presumably you might get unlucky with the timing on real hardware too. Agreed, only someone with real hardware would have to confirm that this is the case. A secondary problem may be that Linux thinks that the first drive is not removable, even though it is a SD drive. I think that is a problem with qemu, but I don't understand the qemu code well enough to understand why. It seems that the mmc capability register always has bit 8 set, even for the first drive, but I don't know where/how that is set and how to change it. SDHCI has the capareg property, but that isn't used here (or I don't know how to use/set it). Yeah, this seems likely to be something we're getting wrong. I assume on other QEMU boards the SD card appears as removeable ? Yes. With some added debugging, sabrelite, and no drive provided in the qemu command: mmc0: SDHCI controller on 2198000.mmc [2198000.mmc] using ADMA # mmc0: No card inserted, removable=1 mmc1: SDHCI controller on 219c000.mmc [219c000.mmc] using ADMA # mmc1: No card inserted, removable=1 Thanks, Guenter
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
On 6/23/23 10:44, Peter Maydell wrote: On Sat, 17 Jun 2023 at 17:29, Guenter Roeck wrote: Hi, On Tue, May 23, 2023 at 06:04:58PM +0800, qianfangui...@163.com wrote: From: qianfan Zhao Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU, and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3 for In-Car Entertainment usage, A40i and A40pro are variants that differ in applicable temperatures range (industrial and military). Signed-off-by: qianfan Zhao Reviewed-by: Niek Linnenbank I tried this in mainline linux with the following command. qemu-system-arm -M bpim2u \ -kernel arch/arm/boot/zImage -no-reboot \ -snapshot -drive file=rootfs-armv7a.ext2,format=raw,if=sd \ -nic user \ --append "root=/dev/mmcblk0 rootwait console=ttyS0,115200" \ -dtb arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dtb \ -nographic -monitor null -serial stdio Main problem is that the SD card gets instantiated randomly to mmc0, mmc1, or mmc2, making it all but impossible to specify a root file system device. The non-instantiated cards are always reported as non-removable, including mmc0. Example: mmc0: Failed to initialize a non-removable card Do you mean that QEMU randomly connects the SD card to a different MMC controller each time, or that Linux is randomly assigning mmc0 to a different MMC controller each time ? Good question. Given the workaround (fix ?) I suggested is in the devicetree file, I would assume it is the latter. I suspect that Linux assigns drive names based on hardware detection order, and that this is not deterministic for some reason. It is odd because I have never experienced that with any other emulation. A secondary problem may be that Linux thinks that the first drive is not removable, even though it is a SD drive. I think that is a problem with qemu, but I don't understand the qemu code well enough to understand why. It seems that the mmc capability register always has bit 8 set, even for the first drive, but I don't know where/how that is set and how to change it. SDHCI has the capareg property, but that isn't used here (or I don't know how to use/set it). Guenter
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
On Sun, Jun 18, 2023 at 08:40:28AM +0800, qianfan wrote:
>
> 在 2023/6/18 0:29, Guenter Roeck 写道:
> > Hi,
> >
> > On Tue, May 23, 2023 at 06:04:58PM +0800, qianfangui...@163.com wrote:
> > > From: qianfan Zhao
> > >
> > > Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
> > > and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
> > > for In-Car Entertainment usage, A40i and A40pro are variants that
> > > differ in applicable temperatures range (industrial and military).
> > >
> > > Signed-off-by: qianfan Zhao
> > > Reviewed-by: Niek Linnenbank
> > I tried this in mainline linux with the following command.
> >
> > qemu-system-arm -M bpim2u \
> > -kernel arch/arm/boot/zImage -no-reboot \
> > -snapshot -drive file=rootfs-armv7a.ext2,format=raw,if=sd \
> > -nic user \
> > --append "root=/dev/mmcblk0 rootwait console=ttyS0,115200" \
> > -dtb arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dtb \
> > -nographic -monitor null -serial stdio
> >
> > Main problem is that the SD card gets instantiated randomly to
> > mmc0, mmc1, or mmc2, making it all but impossible to specify a
> > root file system device. The non-instantiated cards are always
> > reported as non-removable, including mmc0. Example:
> >
> > mmc0: Failed to initialize a non-removable card
> >
> > Using "-sd " instead of "-drive file=" does not
> > make a difference.
> >
> > I can fix (work around ?) the problem by adding the following information
> > to the devicetree file.
> >
> > aliases {
> > ethernet0 =
> > serial0 =
> > + mmc0 =
> > + mmc1 =
> > + mmc2 =
> > };
> >
> > Linux upstream commits fa2d0aa96941 and 2a43322ca7f3 describe the
> > logic behind this change.
> >
> > Is this a bug in the Linux kernel, or a problem with the qemu emulation ?
>
> On my work, the linux kenrel doesn't startup ext4 rootfs directly, it start
>
> a custom ramdisk and we can handle this in ramdisk scripts.
>
That won't help for automated testing.
I guess that means the answer to my question below is "no".
Thanks,
Guenter
> > Either case, is there a way to specify a qemu command line that doesn't
> > result in random assignments of the provided drive to mmc0/1/2 ?
> >
> > Thanks,
> > Guenter
>
Re: [PATCH v4 6/6] hw/riscv: Enable TPM backends
On Mon, Jun 19, 2023 at 01:32:34PM -0700, Guenter Roeck wrote:
> Hi Alistair,
>
> On Wed, Apr 20, 2022 at 03:52:48PM +1000, Alistair Francis wrote:
> > From: Alistair Francis
> >
> > Imply the TPM sysbus devices. This allows users to add TPM devices to
> > the RISC-V virt board.
> >
> > This was tested by first creating an emulated TPM device:
> >
> > swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \
> > --ctrl type=unixio,path=swtpm-sock
> >
> > Then launching QEMU with:
> >
> > -chardev socket,id=chrtpm,path=swtpm-sock \
> > -tpmdev emulator,id=tpm0,chardev=chrtpm \
> > -device tpm-tis-device,tpmdev=tpm0
> >
> > The TPM device can be seen in the memory tree and the generated device
> > tree.
> >
> I tried to get this working with qemu 8.0, but I did not have any success.
> I am quite sure I have the above command line correctly, and it does work
> with arm64. Any idea what I might be missing ?
>
Answering my own question: Nothing. The problem is that the devicetree
is created too early, before the tpm device is instantiated/realized in
qemu. The tpm device therefore does not show up in devicetree, and the
tom device does not instantiate in Linux. The patch below fixes the problem
for me.
Any comments / thoughts ? Is that change acceptable, or should it be
implemented differently ?
Thanks,
Guenter
---
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..ea259d7ade 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1247,6 +1247,11 @@ static void virt_machine_done(Notifier *notifier, void
*data)
uint32_t fdt_load_addr;
uint64_t kernel_entry;
+/* create devicetree if not provided */
+if (!machine->dtb) {
+create_fdt(s, memmap);
+}
+
/*
* Only direct boot kernel is currently supported for KVM VM,
* so the "-bios" parameter is not supported when KVM is enabled.
@@ -1519,15 +1524,13 @@ static void virt_machine_init(MachineState *machine)
}
virt_flash_map(s, system_memory);
-/* load/create device tree */
+/* load device tree */
if (machine->dtb) {
machine->fdt = load_device_tree(machine->dtb, >fdt_size);
if (!machine->fdt) {
error_report("load_device_tree() failed");
exit(1);
}
-} else {
-create_fdt(s, memmap);
}
s->machine_done.notify = virt_machine_done;
Re: [PATCH v4 6/6] hw/riscv: Enable TPM backends
Hi Alistair, On Wed, Apr 20, 2022 at 03:52:48PM +1000, Alistair Francis wrote: > From: Alistair Francis > > Imply the TPM sysbus devices. This allows users to add TPM devices to > the RISC-V virt board. > > This was tested by first creating an emulated TPM device: > > swtpm socket --tpm2 -t -d --tpmstate dir=/tmp/tpm \ > --ctrl type=unixio,path=swtpm-sock > > Then launching QEMU with: > > -chardev socket,id=chrtpm,path=swtpm-sock \ > -tpmdev emulator,id=tpm0,chardev=chrtpm \ > -device tpm-tis-device,tpmdev=tpm0 > > The TPM device can be seen in the memory tree and the generated device > tree. > I tried to get this working with qemu 8.0, but I did not have any success. I am quite sure I have the above command line correctly, and it does work with arm64. Any idea what I might be missing ? Thanks, Guenter > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/942 > Signed-off-by: Alistair Francis > Reviewed-by: Edgar E. Iglesias > --- > hw/riscv/virt.c | 4 > hw/riscv/Kconfig | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 6eed1f4d70..b62fd66a49 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -43,6 +43,7 @@ > #include "sysemu/device_tree.h" > #include "sysemu/sysemu.h" > #include "sysemu/kvm.h" > +#include "sysemu/tpm.h" > #include "hw/pci/pci.h" > #include "hw/pci-host/gpex.h" > #include "hw/display/ramfb.h" > @@ -1612,6 +1613,9 @@ static void virt_machine_class_init(ObjectClass *oc, > void *data) > hc->plug = virt_machine_device_plug_cb; > > machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE); > +#ifdef CONFIG_TPM > +machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS); > +#endif > > object_class_property_add_bool(oc, "aclint", virt_get_aclint, > virt_set_aclint); > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig > index da790f5936..79ff61c464 100644 > --- a/hw/riscv/Kconfig > +++ b/hw/riscv/Kconfig > @@ -34,6 +34,7 @@ config RISCV_VIRT > imply PCI_DEVICES > imply VIRTIO_VGA > imply TEST_DEVICES > +imply TPM_TIS_SYSBUS > select RISCV_NUMA > select GOLDFISH_RTC > select MSI_NONBROKEN > -- > 2.35.1 > >
Re: [PATCH v5 01/11] hw: arm: Add bananapi M2-Ultra and allwinner-r40 support
Hi,
On Tue, May 23, 2023 at 06:04:58PM +0800, qianfangui...@163.com wrote:
> From: qianfan Zhao
>
> Allwinner R40 (sun8i) SoC features a Quad-Core Cortex-A7 ARM CPU,
> and a Mali400 MP2 GPU from ARM. It's also known as the Allwinner T3
> for In-Car Entertainment usage, A40i and A40pro are variants that
> differ in applicable temperatures range (industrial and military).
>
> Signed-off-by: qianfan Zhao
> Reviewed-by: Niek Linnenbank
I tried this in mainline linux with the following command.
qemu-system-arm -M bpim2u \
-kernel arch/arm/boot/zImage -no-reboot \
-snapshot -drive file=rootfs-armv7a.ext2,format=raw,if=sd \
-nic user \
--append "root=/dev/mmcblk0 rootwait console=ttyS0,115200" \
-dtb arch/arm/boot/dts/sun8i-r40-bananapi-m2-ultra.dtb \
-nographic -monitor null -serial stdio
Main problem is that the SD card gets instantiated randomly to
mmc0, mmc1, or mmc2, making it all but impossible to specify a
root file system device. The non-instantiated cards are always
reported as non-removable, including mmc0. Example:
mmc0: Failed to initialize a non-removable card
Using "-sd " instead of "-drive file=" does not
make a difference.
I can fix (work around ?) the problem by adding the following information
to the devicetree file.
aliases {
ethernet0 =
serial0 =
+ mmc0 =
+ mmc1 =
+ mmc2 =
};
Linux upstream commits fa2d0aa96941 and 2a43322ca7f3 describe the
logic behind this change.
Is this a bug in the Linux kernel, or a problem with the qemu emulation ?
Either case, is there a way to specify a qemu command line that doesn't
result in random assignments of the provided drive to mmc0/1/2 ?
Thanks,
Guenter
Re: [PATCH 2/2] hw/sd/allwinner-sdhost: Don't send non-boolean IRQ line levels
On Tue, Jun 06, 2023 at 11:46:09AM +0100, Peter Maydell wrote: > QEMU allows qemu_irq lines to transfer arbitrary integers. However > the convention is that for a simple IRQ line the values transferred > are always 0 and 1. The A10 SD controller device instead assumes a > 0-vs-non-0 convention, which happens to work with the interrupt > controller it is wired up to. > > Coerce the value to boolean to follow our usual convention. > > Signed-off-by: Peter Maydell Tested-by: Guenter Roeck > --- > hw/sd/allwinner-sdhost.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/sd/allwinner-sdhost.c b/hw/sd/allwinner-sdhost.c > index 92a0f42708d..62df8f3d396 100644 > --- a/hw/sd/allwinner-sdhost.c > +++ b/hw/sd/allwinner-sdhost.c > @@ -191,7 +191,7 @@ static void allwinner_sdhost_update_irq(AwSdHostState *s) > } > > trace_allwinner_sdhost_update_irq(irq); > -qemu_set_irq(s->irq, irq); > +qemu_set_irq(s->irq, !!irq); > } > > static void allwinner_sdhost_update_transfer_cnt(AwSdHostState *s, > -- > 2.34.1 >
Re: [PATCH 1/2] hw/intc/allwinner-a10-pic: Handle IRQ levels other than 0 or 1
On Tue, Jun 06, 2023 at 11:46:08AM +0100, Peter Maydell wrote:
> In commit 2c5fa0778c3b430 we fixed an endianness bug in the Allwinner
> A10 PIC model; however in the process we introduced a regression.
> This is because the old code was robust against the incoming 'level'
> argument being something other than 0 or 1, whereas the new code was
> not.
>
> In particular, the allwinner-sdhost code treats its IRQ line
> as 0-vs-non-0 rather than 0-vs-1, so when the SD controller
> set its IRQ line for any reason other than transmit the
> interrupt controller would ignore it. The observed effect
> was a guest timeout when rebooting the guest kernel.
>
> Handle level values other than 0 or 1, to restore the old
> behaviour.
>
> Fixes: 2c5fa0778c3b430 ("hw/intc/allwinner-a10-pic: Don't use
> set_bit()/clear_bit()")
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Peter Maydell
Tested-by: Guenter Roeck
Tested on top of 8.0.2, both this patch alone as well as this
patch plus the second patch in the series.
Guenter
> ---
> hw/intc/allwinner-a10-pic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c
> index 4875e68ba6a..d0bf8d545ba 100644
> --- a/hw/intc/allwinner-a10-pic.c
> +++ b/hw/intc/allwinner-a10-pic.c
> @@ -51,7 +51,7 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int
> level)
> AwA10PICState *s = opaque;
> uint32_t *pending_reg = >irq_pending[irq / 32];
>
> -*pending_reg = deposit32(*pending_reg, irq % 32, 1, level);
> +*pending_reg = deposit32(*pending_reg, irq % 32, 1, !!level);
> aw_a10_pic_update(s);
> }
>
> --
> 2.34.1
>
Re: [PULL 31/35] hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()
On 6/5/23 02:40, Peter Maydell wrote: On Sat, 3 Jun 2023 at 19:06, Guenter Roeck wrote: On 6/3/23 10:46, Michael Tokarev wrote: 03.06.2023 18:03, Guenter Roeck wrote: Hi, On Tue, May 02, 2023 at 01:14:55PM +0100, Peter Maydell wrote: The Allwinner PIC model uses set_bit() and clear_bit() to update the values in its irq_pending[] array when an interrupt arrives. However it is using these functions wrongly: they work on an array of type 'long', and it is passing an array of type 'uint32_t'. Because the code manually figures out the right array element, this works on little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 in a 'long' are in the same place as they are in a 'uint32_t'. However it breaks on 64-bit big-endian hosts. Remove the use of set_bit() and clear_bit() in favour of using deposit32() on the array element. This fixes a bug where on big-endian 64-bit hosts the guest kernel would hang early on in bootup. Cc: qemu-sta...@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20230424152833.1334136-1-peter.mayd...@linaro.org In v8.0.2, the cubieboard emulation running Linux crashes during reboot with a hung task error. Tested with mainline Linux (v6.4-rc4-78-g929ed21dfdb6) and with v5.15.114. Host is AMD Ryzen 5900X. Requesting system reboot [ 61.927460] INFO: task kworker/0:1:13 blocked for more than 30 seconds. [ 61.927896] Not tainted 5.15.115-rc2-00038-g31e35d9f1b8d #1 [ 61.928144] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 61.928419] task:kworker/0:1 state:D stack:0 pid: 13 ppid: 2 flags:0x [ 61.928972] Workqueue: events_freezable mmc_rescan [ 61.929739] [] (__schedule) from [] (schedule+0x80/0x15c) [ 61.930041] [] (schedule) from [] (schedule_timeout+0xd4/0x12c) [ 61.930270] [] (schedule_timeout) from [] (do_wait_for_common+0xa0/0x154) [ 61.930523] [] (do_wait_for_common) from [] (wait_for_completion+0x40/0x4c) [ 61.930764] [] (wait_for_completion) from [] (mmc_wait_for_req_done+0x6c/0x90) [ 61.931012] [] (mmc_wait_for_req_done) from [] (mmc_wait_for_cmd+0x70/0xa8) [ 61.931252] [] (mmc_wait_for_cmd) from [] (sdio_reset+0x58/0x124) [ 61.931478] [] (sdio_reset) from [] (mmc_rescan+0x294/0x30c) [ 61.931692] [] (mmc_rescan) from [] (process_one_work+0x28c/0x720) [ 61.931924] [] (process_one_work) from [] (worker_thread+0x64/0x53c) [ 61.932153] [] (worker_thread) from [] (kthread+0x15c/0x180) [ 61.932365] [] (kthread) from [] (ret_from_fork+0x14/0x38) [ 61.932628] Exception stack(0xc31ddfb0 to 0xc31ddff8) This was not seen with v8.0.0. Bisect points to this patch. Reverting it fixes the problem. Does this happen on master too, or just on stable-8.0 ? It does. Tested with v8.0.0-1542-g848a6caa88. Here is my command line in case you want to give it a try: qemu-system-arm -M cubieboard -kernel arch/arm/boot/zImage -no-reboot \ -initrd rootfs-armv5.cpio -m 512 \ --append "panic=-1 rdinit=/sbin/init earlycon=uart8250,mmio32,0x1c28000,115200n8 console=ttyS0" \ -dtb arch/arm/boot/dts/sun4i-a10-cubieboard.dtb -nographic \ -monitor null -serial stdio initrd is https://github.com/groeck/linux-build-test/blob/master/rootfs/arm-v7/rootfs-armv5.cpio.gz This is with multi_v7_defconfig with some debug options added. If necessary I'll be happy to provide the exact configuration. If you can provide a link to the zImage and the dtb to reproduce as well, that would be helpful. Please see http://server.roeck-us.net/qemu/arm-v7/. There are also compiled versions of qemu v8.0.0 and v8.0.2 as well as scripts to run the test. Note that the initrd will auto-reboot. The cubieboard emulation does not support board reset, so the test will normally end with Requesting system reboot [ 22.020700] reboot: Restarting system In the failure case, the second line is not seen, and there will be a hung task crash about 30 seconds after the reboot request. Requesting system reboot [ 61.960821] INFO: task kworker/0:2:67 blocked for more than 30 seconds. [ 61.961406] Tainted: G N 6.4.0-rc5 #1 Hope this helps, Guenter
Re: [PULL 31/35] hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()
On 6/3/23 10:46, Michael Tokarev wrote: 03.06.2023 18:03, Guenter Roeck wrote: Hi, On Tue, May 02, 2023 at 01:14:55PM +0100, Peter Maydell wrote: The Allwinner PIC model uses set_bit() and clear_bit() to update the values in its irq_pending[] array when an interrupt arrives. However it is using these functions wrongly: they work on an array of type 'long', and it is passing an array of type 'uint32_t'. Because the code manually figures out the right array element, this works on little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 in a 'long' are in the same place as they are in a 'uint32_t'. However it breaks on 64-bit big-endian hosts. Remove the use of set_bit() and clear_bit() in favour of using deposit32() on the array element. This fixes a bug where on big-endian 64-bit hosts the guest kernel would hang early on in bootup. Cc: qemu-sta...@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20230424152833.1334136-1-peter.mayd...@linaro.org In v8.0.2, the cubieboard emulation running Linux crashes during reboot with a hung task error. Tested with mainline Linux (v6.4-rc4-78-g929ed21dfdb6) and with v5.15.114. Host is AMD Ryzen 5900X. Requesting system reboot [ 61.927460] INFO: task kworker/0:1:13 blocked for more than 30 seconds. [ 61.927896] Not tainted 5.15.115-rc2-00038-g31e35d9f1b8d #1 [ 61.928144] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 61.928419] task:kworker/0:1 state:D stack: 0 pid: 13 ppid: 2 flags:0x [ 61.928972] Workqueue: events_freezable mmc_rescan [ 61.929739] [] (__schedule) from [] (schedule+0x80/0x15c) [ 61.930041] [] (schedule) from [] (schedule_timeout+0xd4/0x12c) [ 61.930270] [] (schedule_timeout) from [] (do_wait_for_common+0xa0/0x154) [ 61.930523] [] (do_wait_for_common) from [] (wait_for_completion+0x40/0x4c) [ 61.930764] [] (wait_for_completion) from [] (mmc_wait_for_req_done+0x6c/0x90) [ 61.931012] [] (mmc_wait_for_req_done) from [] (mmc_wait_for_cmd+0x70/0xa8) [ 61.931252] [] (mmc_wait_for_cmd) from [] (sdio_reset+0x58/0x124) [ 61.931478] [] (sdio_reset) from [] (mmc_rescan+0x294/0x30c) [ 61.931692] [] (mmc_rescan) from [] (process_one_work+0x28c/0x720) [ 61.931924] [] (process_one_work) from [] (worker_thread+0x64/0x53c) [ 61.932153] [] (worker_thread) from [] (kthread+0x15c/0x180) [ 61.932365] [] (kthread) from [] (ret_from_fork+0x14/0x38) [ 61.932628] Exception stack(0xc31ddfb0 to 0xc31ddff8) This was not seen with v8.0.0. Bisect points to this patch. Reverting it fixes the problem. Does this happen on master too, or just on stable-8.0 ? It does. Tested with v8.0.0-1542-g848a6caa88. Here is my command line in case you want to give it a try: qemu-system-arm -M cubieboard -kernel arch/arm/boot/zImage -no-reboot \ -initrd rootfs-armv5.cpio -m 512 \ --append "panic=-1 rdinit=/sbin/init earlycon=uart8250,mmio32,0x1c28000,115200n8 console=ttyS0" \ -dtb arch/arm/boot/dts/sun4i-a10-cubieboard.dtb -nographic \ -monitor null -serial stdio initrd is https://github.com/groeck/linux-build-test/blob/master/rootfs/arm-v7/rootfs-armv5.cpio.gz This is with multi_v7_defconfig with some debug options added. If necessary I'll be happy to provide the exact configuration. Guenter
Re: [PULL 31/35] hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()
Hi, On Tue, May 02, 2023 at 01:14:55PM +0100, Peter Maydell wrote: > The Allwinner PIC model uses set_bit() and clear_bit() to update the > values in its irq_pending[] array when an interrupt arrives. However > it is using these functions wrongly: they work on an array of type > 'long', and it is passing an array of type 'uint32_t'. Because the > code manually figures out the right array element, this works on > little-endian hosts and on 32-bit big-endian hosts, where bits 0..31 > in a 'long' are in the same place as they are in a 'uint32_t'. > However it breaks on 64-bit big-endian hosts. > > Remove the use of set_bit() and clear_bit() in favour of using > deposit32() on the array element. This fixes a bug where on > big-endian 64-bit hosts the guest kernel would hang early on in > bootup. > > Cc: qemu-sta...@nongnu.org > Signed-off-by: Peter Maydell > Reviewed-by: Thomas Huth > Reviewed-by: Philippe Mathieu-Daudé > Message-id: 20230424152833.1334136-1-peter.mayd...@linaro.org In v8.0.2, the cubieboard emulation running Linux crashes during reboot with a hung task error. Tested with mainline Linux (v6.4-rc4-78-g929ed21dfdb6) and with v5.15.114. Host is AMD Ryzen 5900X. Requesting system reboot [ 61.927460] INFO: task kworker/0:1:13 blocked for more than 30 seconds. [ 61.927896] Not tainted 5.15.115-rc2-00038-g31e35d9f1b8d #1 [ 61.928144] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 61.928419] task:kworker/0:1 state:D stack:0 pid: 13 ppid: 2 flags:0x [ 61.928972] Workqueue: events_freezable mmc_rescan [ 61.929739] [] (__schedule) from [] (schedule+0x80/0x15c) [ 61.930041] [] (schedule) from [] (schedule_timeout+0xd4/0x12c) [ 61.930270] [] (schedule_timeout) from [] (do_wait_for_common+0xa0/0x154) [ 61.930523] [] (do_wait_for_common) from [] (wait_for_completion+0x40/0x4c) [ 61.930764] [] (wait_for_completion) from [] (mmc_wait_for_req_done+0x6c/0x90) [ 61.931012] [] (mmc_wait_for_req_done) from [] (mmc_wait_for_cmd+0x70/0xa8) [ 61.931252] [] (mmc_wait_for_cmd) from [] (sdio_reset+0x58/0x124) [ 61.931478] [] (sdio_reset) from [] (mmc_rescan+0x294/0x30c) [ 61.931692] [] (mmc_rescan) from [] (process_one_work+0x28c/0x720) [ 61.931924] [] (process_one_work) from [] (worker_thread+0x64/0x53c) [ 61.932153] [] (worker_thread) from [] (kthread+0x15c/0x180) [ 61.932365] [] (kthread) from [] (ret_from_fork+0x14/0x38) [ 61.932628] Exception stack(0xc31ddfb0 to 0xc31ddff8) This was not seen with v8.0.0. Bisect points to this patch. Reverting it fixes the problem. Bisect log is attached. Guenter --- # bad: [f7f686b61cf7ee142c9264d2e04ac2c6a96d37f8] Update version for 8.0.2 release # good: [c1eb2ddf0f8075faddc5f7c3d39feae3e8e9d6b4] Update version for v8.0.0 release git bisect start 'v8.0.2' 'v8.0.0' # bad: [21b54a683d14c0c6f9af35536d9059c60b7449ca] s390x/pv: Fix spurious warning with asynchronous teardown git bisect bad 21b54a683d14c0c6f9af35536d9059c60b7449ca # bad: [4dc5df865c482c6e8894964c7f300fa556c3b78e] softfloat: Fix the incorrect computation in float32_exp2 git bisect bad 4dc5df865c482c6e8894964c7f300fa556c3b78e # good: [f0c5a780292bd405bbce818b63757313cafcf262] target/arm: Initialize debug capabilities only once git bisect good f0c5a780292bd405bbce818b63757313cafcf262 # bad: [af08c70ef5204fedb2b974fbecaf65e1b6cc0a2f] hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit() git bisect bad af08c70ef5204fedb2b974fbecaf65e1b6cc0a2f # good: [168f193c5be54fc9a6d725dbb9974c0d2815792a] hw/arm/boot: Make write_bootloader() public as arm_write_bootloader() git bisect good 168f193c5be54fc9a6d725dbb9974c0d2815792a # good: [975f12aa528d6cab5cc41efebaf05d7eb7296d94] hw/arm/raspi: Use arm_write_bootloader() to write boot code git bisect good 975f12aa528d6cab5cc41efebaf05d7eb7296d94 # first bad commit: [af08c70ef5204fedb2b974fbecaf65e1b6cc0a2f] hw/intc/allwinner-a10-pic: Don't use set_bit()/clear_bit()
Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces
On Sat, Apr 29, 2023 at 01:46:26PM -0700, Guenter Roeck wrote:
> Hi,
>
> On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> > Some SDHCI IP can be synthetized in various endianness:
> > https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
> >
> > - CONFIG_SYS_FSL_ESDHC_BE
> >
> >ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
> >determined by ESDHC IP's endian mode or processor's endian mode.
> >
> > Our current implementation is little-endian. In order to support
> > big endianness:
> >
> > - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> > - Add an 'endianness' property to SDHCIState (default little endian)
> > - Set the 'io_ops' field in realize() after checking the property
> > - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
>
> With this patch in place (in qemu v8.0), I can no longer boot linux
> from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
> I get the following persistent errors.
>
> [ 12.210101] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.213222] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.215072] sdhci-esdhc-imx 2194000.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.218766] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.220441] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.221542] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.241544] sdhci-esdhc-imx 219.mmc:
> esdhc_wait_for_card_clock_gate_off: card clock still not gate off in 100us!.
> [ 12.242608] sdhci-esdhc-imx 219.mmc: card clock still not stable in
> 100us!.
>
> The emulations start to work again after reverting this patch.
>
Cause explained below.
> Guenter
>
> > ---
> > hw/sd/sdhci-internal.h | 1 +
> > hw/sd/sdhci.c | 32 +---
> > include/hw/sd/sdhci.h | 1 +
> > 3 files changed, 31 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> > index 964570f8e8..5f3765f12d 100644
> > --- a/hw/sd/sdhci-internal.h
> > +++ b/hw/sd/sdhci-internal.h
> > @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
> > #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
> >
> > #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> > +DEFINE_PROP_UINT8("endianness", _state, endianness,
> > DEVICE_LITTLE_ENDIAN), \
> > DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
> > DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
> > DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 22c758ad91..289baa879e 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t
> > val, unsigned size)
> > value >> shift, value >> shift);
> > }
> >
> > -static const MemoryRegionOps sdhci_mmio_ops = {
> > +static const MemoryRegionOps sdhci_mmio_le_ops = {
> > .read = sdhci_read,
> > .write = sdhci_write,
> > .impl = {
> > @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > +static const MemoryRegionOps sdhci_mmio_be_ops = {
> > +.read = sdhci_read,
> > +.write = sdhci_write,
> > +.impl = {
> > +.min_access_size = 4,
> > +.max_access_size = 4,
> > +},
> > +.valid = {
> > +.min_access_size = 1,
> > +.max_access_size = 4,
> > +.unaligned = false
> > +},
> > +.endianness = DEVICE_BIG_ENDIAN,
> > +};
> > +
> > static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> > {
> > ERRP_GUARD();
> > @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
> >
> > s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > sdhci_raise_insertion_irq, s);
> > s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
Re: [PATCH v6 2/3] hw/sd/sdhci: Support big endian SD host controller interfaces
Hi,
On Tue, Nov 01, 2022 at 11:29:33PM +0100, Philippe Mathieu-Daudé wrote:
> Some SDHCI IP can be synthetized in various endianness:
> https://github.com/u-boot/u-boot/blob/v2021.04/doc/README.fsl-esdhc
>
> - CONFIG_SYS_FSL_ESDHC_BE
>
>ESDHC IP is in big-endian mode. Accessing ESDHC registers can be
>determined by ESDHC IP's endian mode or processor's endian mode.
>
> Our current implementation is little-endian. In order to support
> big endianness:
>
> - Rename current MemoryRegionOps as sdhci_mmio_le_ops ('le')
> - Add an 'endianness' property to SDHCIState (default little endian)
> - Set the 'io_ops' field in realize() after checking the property
> - Add the sdhci_mmio_be_ops (big-endian) MemoryRegionOps.
>
> Signed-off-by: Philippe Mathieu-Daudé
With this patch in place (in qemu v8.0), I can no longer boot linux
from SD card with sabrelite, mcimx6ul-evk, and mcimx7d-sabre emulations.
I get the following persistent errors.
[ 12.210101] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.213222] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.215072] sdhci-esdhc-imx 2194000.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.218766] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.220441] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.221542] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.241544] sdhci-esdhc-imx 219.mmc: esdhc_wait_for_card_clock_gate_off:
card clock still not gate off in 100us!.
[ 12.242608] sdhci-esdhc-imx 219.mmc: card clock still not stable in
100us!.
The emulations start to work again after reverting this patch.
Guenter
> ---
> hw/sd/sdhci-internal.h | 1 +
> hw/sd/sdhci.c | 32 +---
> include/hw/sd/sdhci.h | 1 +
> 3 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
> index 964570f8e8..5f3765f12d 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -308,6 +308,7 @@ extern const VMStateDescription sdhci_vmstate;
> #define SDHC_CAPAB_REG_DEFAULT 0x057834b4
>
> #define DEFINE_SDHCI_COMMON_PROPERTIES(_state) \
> +DEFINE_PROP_UINT8("endianness", _state, endianness,
> DEVICE_LITTLE_ENDIAN), \
> DEFINE_PROP_UINT8("sd-spec-version", _state, sd_spec_version, 2), \
> DEFINE_PROP_UINT8("uhs", _state, uhs_mode, UHS_NOT_SUPPORTED), \
> DEFINE_PROP_UINT8("vendor", _state, vendor, SDHCI_VENDOR_NONE), \
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 22c758ad91..289baa879e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1329,7 +1329,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
> unsigned size)
> value >> shift, value >> shift);
> }
>
> -static const MemoryRegionOps sdhci_mmio_ops = {
> +static const MemoryRegionOps sdhci_mmio_le_ops = {
> .read = sdhci_read,
> .write = sdhci_write,
> .impl = {
> @@ -1344,6 +1344,21 @@ static const MemoryRegionOps sdhci_mmio_ops = {
> .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> +static const MemoryRegionOps sdhci_mmio_be_ops = {
> +.read = sdhci_read,
> +.write = sdhci_write,
> +.impl = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +},
> +.valid = {
> +.min_access_size = 1,
> +.max_access_size = 4,
> +.unaligned = false
> +},
> +.endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> static void sdhci_init_readonly_registers(SDHCIState *s, Error **errp)
> {
> ERRP_GUARD();
> @@ -1371,8 +1386,6 @@ void sdhci_initfn(SDHCIState *s)
>
> s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> sdhci_raise_insertion_irq, s);
> s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> sdhci_data_transfer, s);
> -
> -s->io_ops = _mmio_ops;
> }
>
> void sdhci_uninitfn(SDHCIState *s)
> @@ -1388,10 +1401,23 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
> {
> ERRP_GUARD();
>
> +switch (s->endianness) {
> +case DEVICE_LITTLE_ENDIAN:
> +s->io_ops = _mmio_le_ops;
> +break;
> +case DEVICE_BIG_ENDIAN:
> +s->io_ops = _mmio_be_ops;
> +break;
> +default:
> +error_setg(errp, "Incorrect endianness");
> +return;
> +}
> +
> sdhci_init_readonly_registers(s, errp);
> if (*errp) {
> return;
> }
> +
> s->buf_maxsz = sdhci_get_fifolen(s);
> s->fifo_buffer = g_malloc0(s->buf_maxsz);
>
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 01a64c5442..a989fca3b2 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -96,6 +96,7 @@ struct
Re: [PATCH 0/5] Support both Ethernet interfaces on i.MX6UL and i.MX7
On 4/18/23 08:32, Peter Maydell wrote: On Tue, 18 Apr 2023 at 16:18, Guenter Roeck wrote: On 4/18/23 07:46, Peter Maydell wrote: I guess I don't understand what the topology is for these specific SoCs, then. If there's only one master that might be connected to multiple PHYs, why does one ethernet device in QEMU need to know about the other one? Are the PHYs connected to just that first ethernet device, or to both? This bit in your cover letter makes it sound like "both ethernet interfaces connect to the same MDIO bus which has both PHYs on it": Yes, that is exactly how it is, similar to the configuration in the picture at prodigytechno.com. I don't recall what I wrote in the cover letter, but "Both Ethernet PHYs connect to the same MDIO bus which is connected to one of the Ethernet MACs" would be the most accurate description I can think of. Each MAC (Ethernet interface, instance of TYPE_IMX_FEC in qemu) has its own MDIO bus. Currently QEMU assumes that each PHY is connected to the MDIO bus on its associated MAC interface. That is not the case on the emulated boards, where all PHYs are connected to a single MDIO bus. So looking again at that diagram on that website, I think I understand now: for data transfer to/from the outside world, MAC1 talks only through PHY1 and MAC2 only through PHY2 (over the links marked "MII/GMII/XGMII"), but the "control" connection is via MDIO, and on these boards you have to configure PHY2 by doing the MDIO reads and writes via MAC1, even though MAC1 has nothing otherwise to do with PHY2 ? (And MAC2 has no devices on its MDIO bus at all.) Correct. Thanks, Guenter
Re: [PATCH 0/5] Support both Ethernet interfaces on i.MX6UL and i.MX7
On 4/18/23 07:46, Peter Maydell wrote: On Tue, 18 Apr 2023 at 15:42, Guenter Roeck wrote: On 4/18/23 05:10, Peter Maydell wrote: On Wed, 15 Mar 2023 at 14:52, Guenter Roeck wrote: So I was having a look at this to see if it was reasonably easy to split out the PHY into its own device object, and I'm a bit confused. I know basically 0 about MDIO, but wikipedia says that MDIO buses have one master (the ethernet MAC) and potentially multiple PHYs. However it looks like this patchset has configurations where multiple MACs talk to the same MDIO bus. Am I confused about the patchset, about the hardware, or about what MDIO supports? It is quite similar to I2C, a serial interface with one master/controller and a number of devices (PHYs) connected to it. There is a nice graphic example at https://prodigytechno.com/mdio-management-data-input-output/. Not sure I understand what is confusing about it. Can you explain ? I guess I don't understand what the topology is for these specific SoCs, then. If there's only one master that might be connected to multiple PHYs, why does one ethernet device in QEMU need to know about the other one? Are the PHYs connected to just that first ethernet device, or to both? This bit in your cover letter makes it sound like "both ethernet interfaces connect to the same MDIO bus which has both PHYs on it": Yes, that is exactly how it is, similar to the configuration in the picture at prodigytechno.com. I don't recall what I wrote in the cover letter, but "Both Ethernet PHYs connect to the same MDIO bus which is connected to one of the Ethernet MACs" would be the most accurate description I can think of. The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may be connected to separate MDIO busses, or both may be connected on the same MDIO bus using different PHY addresses. Each MAC (Ethernet interface, instance of TYPE_IMX_FEC in qemu) has its own MDIO bus. Currently QEMU assumes that each PHY is connected to the MDIO bus on its associated MAC interface. That is not the case on the emulated boards, where all PHYs are connected to a single MDIO bus. Userspace, when talking to the Ethernet controllers, knows that the PHY of the second Ethernet controller is connected to the MDIO bus on the first Ethernet controller. QEMU has to be told about that and otherwise misses that MDIO commands sent to the second PHY (on the first Ethernet controller) influence the second MAC interface. From this exchange I can only assume that my implementation is unacceptable. All I can say is that it works. Thanks, Guenter
Re: [PATCH 0/5] Support both Ethernet interfaces on i.MX6UL and i.MX7
On 4/18/23 05:10, Peter Maydell wrote:
On Wed, 15 Mar 2023 at 14:52, Guenter Roeck wrote:
The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may
be connected to separate MDIO busses, or both may be connected on the same
MDIO bus using different PHY addresses. Commit 461c51ad4275 ("Add a phy-num
property to the i.MX FEC emulator") added support for specifying PHY
addresses, but it did not provide support for linking the second PHY on
a given MDIO bus to the other Ethernet interface.
To be able to support two PHY instances on a single MDIO bus, two properties
are needed: First, there needs to be a flag indicating if the MDIO bus on
a given Ethernet interface is connected. If not, attempts to read from this
bus must always return 0x. Implement this property as phy-connected.
Second, if the MDIO bus on an interface is active, it needs a link to the
consumer interface to be able to provide PHY access for it. Implement this
property as phy-consumer.
So I was having a look at this to see if it was reasonably easy to
split out the PHY into its own device object, and I'm a bit confused.
I know basically 0 about MDIO, but wikipedia says that MDIO buses
have one master (the ethernet MAC) and potentially multiple PHYs.
However it looks like this patchset has configurations where
multiple MACs talk to the same MDIO bus. Am I confused about the
patchset, about the hardware, or about what MDIO supports?
It is quite similar to I2C, a serial interface with one master/controller
and a number of devices (PHYs) connected to it. There is a nice graphic
example at https://prodigytechno.com/mdio-management-data-input-output/.
Not sure I understand what is confusing about it. Can you explain ?
Thanks,
Guenter
Re: [PATCH 1/5] hw/net/imx_fec: Support two Ethernet interfaces connected to single MDIO bus
On Thu, Mar 30, 2023 at 05:31:13PM +0100, Peter Maydell wrote:
> On Wed, 15 Mar 2023 at 14:52, Guenter Roeck wrote:
> >
> > The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may
> > be connected to separate MDIO busses, or both may be connected on the same
> > MDIO bus using different PHY addresses. Commit 461c51ad4275 ("Add a phy-num
> > property to the i.MX FEC emulator") added support for specifying PHY
> > addresses, but it did not provide support for linking the second PHY on
> > a given MDIO bus to the other Ethernet interface.
> >
> > To be able to support two PHY instances on a single MDIO bus, two properties
> > are needed: First, there needs to be a flag indicating if the MDIO bus on
> > a given Ethernet interface is connected. If not, attempts to read from this
> > bus must always return 0x. Implement this property as phy-connected.
> > Second, if the MDIO bus on an interface is active, it needs a link to the
> > consumer interface to be able to provide PHY access for it. Implement this
> > property as phy-consumer.
> >
> > Signed-off-by: Guenter Roeck
>
> > @@ -282,11 +282,19 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
> > uint32_t val;
> > uint32_t phy = reg / 32;
> >
> > -if (phy != s->phy_num) {
> > -trace_imx_phy_read_num(phy, s->phy_num);
> > +if (!s->phy_connected) {
> > return 0x;
> > }
> >
> > +if (phy != s->phy_num) {
> > +if (s->phy_consumer && phy == s->phy_consumer->phy_num) {
> > +s = s->phy_consumer;
>
> This does work, but it leaves me wondering if we should really
> be modelling the phy as a separate device object, so that we
> can use link properties to connect the right phy to the right
> IMXFECState rather than having this odd "actually use the pointer
> to this other instance of the device"... A quick glance through
Possibly, but I don't understand well enough how this would work
to be able to implement it. I'll be happy to test patches from others,
of course.
Thanks,
Guenter
> the code suggests that the phy and the ethernet controller
> proper don't really care about each others' internals.
> (imx_phy_update_irq() does call imx_eth_update() but AFAICT
> that is unnecessary because imx_eth_update() doesn't care about
> any of the phy state...)
>
> thanks
> -- PMM
[PATCH] hw/usb/imx: Fix out of bounds access in imx_usbphy_read()
The i.MX USB Phy driver does not check register ranges, resulting in out of
bounds accesses if an attempt is made to access non-existing PHY registers.
Add range check and conditionally report bad accesses to fix the problem.
While at it, also conditionally log attempted writes to non-existing or
read-only registers.
Reported-by: Qiang Liu
Link: https://gitlab.com/qemu-project/qemu/-/issues/1408
Fixes: 0701a5efa015 ("hw/usb: Add basic i.MX USB Phy support")
Signed-off-by: Guenter Roeck
---
hw/usb/imx-usb-phy.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/hw/usb/imx-usb-phy.c b/hw/usb/imx-usb-phy.c
index 5d7a549e34..1a97b36a11 100644
--- a/hw/usb/imx-usb-phy.c
+++ b/hw/usb/imx-usb-phy.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "hw/usb/imx-usb-phy.h"
#include "migration/vmstate.h"
+#include "qemu/log.h"
#include "qemu/module.h"
static const VMStateDescription vmstate_imx_usbphy = {
@@ -90,7 +91,15 @@ static uint64_t imx_usbphy_read(void *opaque, hwaddr offset,
unsigned size)
value = s->usbphy[index - 3];
break;
default:
-value = s->usbphy[index];
+if (index < USBPHY_MAX) {
+value = s->usbphy[index];
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Read from non-existing USB PHY register 0x%"
+ HWADDR_PRIx "\n",
+ __func__, offset);
+value = 0;
+}
break;
}
return (uint64_t)value;
@@ -168,7 +177,13 @@ static void imx_usbphy_write(void *opaque, hwaddr offset,
uint64_t value,
s->usbphy[index - 3] ^= value;
break;
default:
-/* Other registers are read-only */
+/* Other registers are read-only or do not exist */
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Write to %s USB PHY register 0x%"
+ HWADDR_PRIx "\n",
+ __func__,
+ index >= USBPHY_MAX ? "non-existing" : "read-only",
+ offset);
break;
}
}
--
2.39.2
Re: [PATCH v3 1/5] hw/usb: Add basic i.MX USB Phy support
On Thu, Mar 16, 2023 at 02:51:23PM +, Peter Maydell wrote: > On Thu, 16 Mar 2023 at 14:12, Guenter Roeck wrote: > > > > Hi Peter, > > > > On 3/16/23 06:41, Peter Maydell wrote: > > > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck wrote: > > >> > > >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, > > >> and i.MX7 SoCs. > > >> > > >> The only support really needed - at least to boot Linux - is support > > >> for soft reset, which needs to reset various registers to their initial > > >> value. Otherwise, just record register values. > > >> > > >> Reviewed-by: Peter Maydell > > >> Signed-off-by: Guenter Roeck > > > > > > Hi Guenter; we've had a fuzzer report that this device model > > > accesses off the end of the usbphy[] array: > > > https://gitlab.com/qemu-project/qemu/-/issues/1408 > > > > > > > Good catch. And an obvious bug, sorry. > > > > > > > Do you know what the device is supposed to do with these > > > off-the-end acceses? We could either reduce the memory region > > > size or bounds check and RAZ/WI the out-of-range accesses. > > > > > > > I have no idea what the real hardware would do. The datasheets (at > > least the ones I checked) don't say, only that the region size is 4k. > > I would suggest a bounds check, ignore out-of-bounds writes (maybe > > with a log message), and return 0 for reads (which I think is what > > you suggest with RAZ/WI). > > > > Want me to send a patch ? > > If you have the time, that would be great. I expect you're > better set up to test it than I am... > I prepared a patch. Currently testing. Guenter
Re: [PATCH v3 1/5] hw/usb: Add basic i.MX USB Phy support
Hi Peter,
On 3/16/23 06:41, Peter Maydell wrote:
On Fri, 13 Mar 2020 at 01:45, Guenter Roeck wrote:
Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6,
and i.MX7 SoCs.
The only support really needed - at least to boot Linux - is support
for soft reset, which needs to reset various registers to their initial
value. Otherwise, just record register values.
Reviewed-by: Peter Maydell
Signed-off-by: Guenter Roeck
Hi Guenter; we've had a fuzzer report that this device model
accesses off the end of the usbphy[] array:
https://gitlab.com/qemu-project/qemu/-/issues/1408
Good catch. And an obvious bug, sorry.
+static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, unsigned size)
+{
+IMXUSBPHYState *s = (IMXUSBPHYState *)opaque;
+uint32_t index = offset >> 2;
+uint32_t value;
+default:
+value = s->usbphy[index];
No bounds check in the default case (or ditto in the write function)...
+break;
+}
+return (uint64_t)value;
+}
+static void imx_usbphy_realize(DeviceState *dev, Error **errp)
+{
+IMXUSBPHYState *s = IMX_USBPHY(dev);
+
+memory_region_init_io(>iomem, OBJECT(s), _usbphy_ops, s,
+ "imx-usbphy", 0x1000);
...and the memory region is created at size 0x1000 so the read/write
fns can be called with offsets up to that size...
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >iomem);
+}
+enum IMXUsbPhyRegisters {
+USBPHY_PWD,
+USBPHY_PWD_SET,
+USBPHY_PWD_CLR,
+USBPHY_PWD_TOG,
+USBPHY_TX,
+USBPHY_TX_SET,
+USBPHY_TX_CLR,
+USBPHY_TX_TOG,
+USBPHY_RX,
+USBPHY_RX_SET,
+USBPHY_RX_CLR,
+USBPHY_RX_TOG,
+USBPHY_CTRL,
+USBPHY_CTRL_SET,
+USBPHY_CTRL_CLR,
+USBPHY_CTRL_TOG,
+USBPHY_STATUS,
+USBPHY_DEBUG = 0x14,
+USBPHY_DEBUG_SET,
+USBPHY_DEBUG_CLR,
+USBPHY_DEBUG_TOG,
+USBPHY_DEBUG0_STATUS,
+USBPHY_DEBUG1 = 0x1c,
+USBPHY_DEBUG1_SET,
+USBPHY_DEBUG1_CLR,
+USBPHY_DEBUG1_TOG,
+USBPHY_VERSION,
+USBPHY_MAX
+};
+
+#define USBPHY_CTRL_SFTRST BIT(31)
+
+#define TYPE_IMX_USBPHY "imx.usbphy"
+#define IMX_USBPHY(obj) OBJECT_CHECK(IMXUSBPHYState, (obj), TYPE_IMX_USBPHY)
+
+typedef struct IMXUSBPHYState {
+/* */
+SysBusDevice parent_obj;
+
+/* */
+MemoryRegion iomem;
+
+uint32_t usbphy[USBPHY_MAX];
...but the array is only created with USBPHY_MAX entries.
Do you know what the device is supposed to do with these
off-the-end acceses? We could either reduce the memory region
size or bounds check and RAZ/WI the out-of-range accesses.
I have no idea what the real hardware would do. The datasheets (at
least the ones I checked) don't say, only that the region size is 4k.
I would suggest a bounds check, ignore out-of-bounds writes (maybe
with a log message), and return 0 for reads (which I think is what
you suggest with RAZ/WI).
Want me to send a patch ?
Thanks,
Guenter
[PATCH 1/5] hw/net/imx_fec: Support two Ethernet interfaces connected to single MDIO bus
The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may
be connected to separate MDIO busses, or both may be connected on the same
MDIO bus using different PHY addresses. Commit 461c51ad4275 ("Add a phy-num
property to the i.MX FEC emulator") added support for specifying PHY
addresses, but it did not provide support for linking the second PHY on
a given MDIO bus to the other Ethernet interface.
To be able to support two PHY instances on a single MDIO bus, two properties
are needed: First, there needs to be a flag indicating if the MDIO bus on
a given Ethernet interface is connected. If not, attempts to read from this
bus must always return 0x. Implement this property as phy-connected.
Second, if the MDIO bus on an interface is active, it needs a link to the
consumer interface to be able to provide PHY access for it. Implement this
property as phy-consumer.
Signed-off-by: Guenter Roeck
---
hw/net/imx_fec.c | 27 +++
include/hw/net/imx_fec.h | 2 ++
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index c862d96593..5d1f1f104c 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -282,11 +282,19 @@ static uint32_t imx_phy_read(IMXFECState *s, int reg)
uint32_t val;
uint32_t phy = reg / 32;
-if (phy != s->phy_num) {
-trace_imx_phy_read_num(phy, s->phy_num);
+if (!s->phy_connected) {
return 0x;
}
+if (phy != s->phy_num) {
+if (s->phy_consumer && phy == s->phy_consumer->phy_num) {
+s = s->phy_consumer;
+} else {
+trace_imx_phy_read_num(phy, s->phy_num);
+return 0x;
+}
+}
+
reg %= 32;
switch (reg) {
@@ -343,11 +351,19 @@ static void imx_phy_write(IMXFECState *s, int reg,
uint32_t val)
{
uint32_t phy = reg / 32;
-if (phy != s->phy_num) {
-trace_imx_phy_write_num(phy, s->phy_num);
+if (!s->phy_connected) {
return;
}
+if (phy != s->phy_num) {
+if (s->phy_consumer && phy == s->phy_consumer->phy_num) {
+s = s->phy_consumer;
+} else {
+trace_imx_phy_write_num(phy, s->phy_num);
+return;
+}
+}
+
reg %= 32;
trace_imx_phy_write(val, phy, reg);
@@ -1327,6 +1343,9 @@ static Property imx_eth_properties[] = {
DEFINE_NIC_PROPERTIES(IMXFECState, conf),
DEFINE_PROP_UINT32("tx-ring-num", IMXFECState, tx_ring_num, 1),
DEFINE_PROP_UINT32("phy-num", IMXFECState, phy_num, 0),
+DEFINE_PROP_BOOL("phy-connected", IMXFECState, phy_connected, true),
+DEFINE_PROP_LINK("phy-consumer", IMXFECState, phy_consumer, TYPE_IMX_FEC,
+ IMXFECState *),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
index e3a8755db9..2d13290c78 100644
--- a/include/hw/net/imx_fec.h
+++ b/include/hw/net/imx_fec.h
@@ -270,6 +270,8 @@ struct IMXFECState {
uint32_t phy_int;
uint32_t phy_int_mask;
uint32_t phy_num;
+bool phy_connected;
+struct IMXFECState *phy_consumer;
bool is_fec;
--
2.39.2
[PATCH 3/5] arm/mcimx6ul-evk: Set fec1-phy-connected property to false
On mcimx6ul-evk, the MDIO bus is connected to the second Ethernet interface. Set fec1-phy-connected to false to reflect this. Signed-off-by: Guenter Roeck --- hw/arm/mcimx6ul-evk.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c index d83c3c380e..3ac1e2ea9b 100644 --- a/hw/arm/mcimx6ul-evk.c +++ b/hw/arm/mcimx6ul-evk.c @@ -41,6 +41,8 @@ static void mcimx6ul_evk_init(MachineState *machine) object_property_add_child(OBJECT(machine), "soc", OBJECT(s)); object_property_set_uint(OBJECT(s), "fec1-phy-num", 2, _fatal); object_property_set_uint(OBJECT(s), "fec2-phy-num", 1, _fatal); +object_property_set_bool(OBJECT(s), "fec1-phy-connected", false, + _fatal); qdev_realize(DEVICE(s), NULL, _fatal); memory_region_add_subregion(get_system_memory(), FSL_IMX6UL_MMDC_ADDR, -- 2.39.2
[PATCH 2/5] fsl-imx6ul: Add fec[12]-phy-connected properties
Add fec[12]-phy-connected properties and use it to set phy-connected
and phy-consumer properties for imx_fec.
Signed-off-by: Guenter Roeck
---
hw/arm/fsl-imx6ul.c | 20
include/hw/arm/fsl-imx6ul.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index d88d6cc1c5..2189dcbb72 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -407,7 +407,23 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error
**errp)
/*
* Ethernet
+ *
+ * We must use two loops since phy_connected affects the other interface
+ * and we have to set all properties before calling sysbus_realize().
*/
+for (i = 0; i < FSL_IMX6UL_NUM_ETHS; i++) {
+object_property_set_bool(OBJECT(>eth[i]), "phy-connected",
+ s->phy_connected[i], _abort);
+/*
+ * If the MDIO bus on this controller is not connected, assume the
+ * other controller provides support for it.
+ */
+if (!s->phy_connected[i]) {
+object_property_set_link(OBJECT(>eth[1 - i]), "phy-consumer",
+ OBJECT(>eth[i]), _abort);
+}
+}
+
for (i = 0; i < FSL_IMX6UL_NUM_ETHS; i++) {
static const hwaddr FSL_IMX6UL_ENETn_ADDR[FSL_IMX6UL_NUM_ETHS] = {
FSL_IMX6UL_ENET1_ADDR,
@@ -620,6 +636,10 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error
**errp)
static Property fsl_imx6ul_properties[] = {
DEFINE_PROP_UINT32("fec1-phy-num", FslIMX6ULState, phy_num[0], 0),
DEFINE_PROP_UINT32("fec2-phy-num", FslIMX6ULState, phy_num[1], 1),
+DEFINE_PROP_BOOL("fec1-phy-connected", FslIMX6ULState, phy_connected[0],
+ true),
+DEFINE_PROP_BOOL("fec2-phy-connected", FslIMX6ULState, phy_connected[1],
+ true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index 1952cb984d..9ee15ae38d 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -89,6 +89,7 @@ struct FslIMX6ULState {
MemoryRegion ocram_alias;
uint32_t phy_num[FSL_IMX6UL_NUM_ETHS];
+bool phy_connected[FSL_IMX6UL_NUM_ETHS];
};
enum FslIMX6ULMemoryMap {
--
2.39.2
[PATCH 0/5] Support both Ethernet interfaces on i.MX6UL and i.MX7
The SOC on i.MX6UL and i.MX7 has 2 Ethernet interfaces. The PHY on each may
be connected to separate MDIO busses, or both may be connected on the same
MDIO bus using different PHY addresses. Commit 461c51ad4275 ("Add a phy-num
property to the i.MX FEC emulator") added support for specifying PHY
addresses, but it did not provide support for linking the second PHY on
a given MDIO bus to the other Ethernet interface.
To be able to support two PHY instances on a single MDIO bus, two properties
are needed: First, there needs to be a flag indicating if the MDIO bus on
a given Ethernet interface is connected. If not, attempts to read from this
bus must always return 0x. Implement this property as phy-connected.
Second, if the MDIO bus on an interface is active, it needs a link to the
consumer interface to be able to provide PHY access for it. Implement this
property as phy-consumer.
The first patch of the series implements support in hw/net/imx_fec.c.
Patches 2..5 set the necessary properties in i.MX6UL and i.MX7 emulations.
With this series in place, both Ethernet interfaces on affected emulations
are functional.
--------
Guenter Roeck (5):
hw/net/imx_fec: Support two Ethernet interfaces connected to single MDIO
bus
fsl-imx6ul: Add fec[12]-phy-connected properties
arm/mcimx6ul-evk: Set fec1-phy-connected property to false
fsl-imx7: Add fec[12]-phy-connected properties
arm/mcimx7d-sabre: Set fec2-phy-connected property to false
hw/arm/fsl-imx6ul.c | 20
hw/arm/fsl-imx7.c | 20
hw/arm/mcimx6ul-evk.c | 2 ++
hw/arm/mcimx7d-sabre.c | 2 ++
hw/net/imx_fec.c| 27 +++
include/hw/arm/fsl-imx6ul.h | 1 +
include/hw/arm/fsl-imx7.h | 1 +
include/hw/net/imx_fec.h| 2 ++
8 files changed, 71 insertions(+), 4 deletions(-)
[PATCH 5/5] arm/mcimx7d-sabre: Set fec2-phy-connected property to false
On mcimx7d-sabre, the MDIO bus is connected to the first Ethernet interface. Set fec2-phy-connected to false to reflect this. Signed-off-by: Guenter Roeck --- hw/arm/mcimx7d-sabre.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c index 6182b15f19..d1778122b6 100644 --- a/hw/arm/mcimx7d-sabre.c +++ b/hw/arm/mcimx7d-sabre.c @@ -41,6 +41,8 @@ static void mcimx7d_sabre_init(MachineState *machine) s = FSL_IMX7(object_new(TYPE_FSL_IMX7)); object_property_add_child(OBJECT(machine), "soc", OBJECT(s)); +object_property_set_bool(OBJECT(s), "fec2-phy-connected", false, + _fatal); qdev_realize(DEVICE(s), NULL, _fatal); memory_region_add_subregion(get_system_memory(), FSL_IMX7_MMDC_ADDR, -- 2.39.2
[PATCH 4/5] fsl-imx7: Add fec[12]-phy-connected properties
Add fec[12]-phy-connected properties and use it to set phy-connected
and phy-consumer properties for imx_fec.
Signed-off-by: Guenter Roeck
---
hw/arm/fsl-imx7.c | 20
include/hw/arm/fsl-imx7.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index afc7480799..9e41d4b677 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -395,7 +395,23 @@ static void fsl_imx7_realize(DeviceState *dev, Error
**errp)
/*
* Ethernet
+ *
+ * We must use two loops since phy_connected affects the other interface
+ * and we have to set all properties before calling sysbus_realize().
*/
+for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
+object_property_set_bool(OBJECT(>eth[i]), "phy-connected",
+ s->phy_connected[i], _abort);
+/*
+ * If the MDIO bus on this controller is not connected, assume the
+ * other controller provides support for it.
+ */
+if (!s->phy_connected[i]) {
+object_property_set_link(OBJECT(>eth[1 - i]), "phy-consumer",
+ OBJECT(>eth[i]), _abort);
+}
+}
+
for (i = 0; i < FSL_IMX7_NUM_ETHS; i++) {
static const hwaddr FSL_IMX7_ENETn_ADDR[FSL_IMX7_NUM_ETHS] = {
FSL_IMX7_ENET1_ADDR,
@@ -601,6 +617,10 @@ static void fsl_imx7_realize(DeviceState *dev, Error
**errp)
static Property fsl_imx7_properties[] = {
DEFINE_PROP_UINT32("fec1-phy-num", FslIMX7State, phy_num[0], 0),
DEFINE_PROP_UINT32("fec2-phy-num", FslIMX7State, phy_num[1], 1),
+DEFINE_PROP_BOOL("fec1-phy-connected", FslIMX7State, phy_connected[0],
+ true),
+DEFINE_PROP_BOOL("fec2-phy-connected", FslIMX7State, phy_connected[1],
+ true),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 355bd8ea83..54ea2f0890 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -82,6 +82,7 @@ struct FslIMX7State {
ChipideaState usb[FSL_IMX7_NUM_USBS];
DesignwarePCIEHost pcie;
uint32_t phy_num[FSL_IMX7_NUM_ETHS];
+bool phy_connected[FSL_IMX7_NUM_ETHS];
};
enum FslIMX7MemoryMap {
--
2.39.2
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
On 3/3/23 01:02, Fiona Ebner wrote:
Am 28.02.23 um 18:11 schrieb Guenter Roeck:
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Tested-by: Fiona Ebner
But I do have a question:
Signed-off-by: Guenter Roeck
---
hw/scsi/megasas.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..d624866bb6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
uint8_t cdb[16];
int len;
struct SCSIDevice *sdev = NULL;
-int target_id, lun_id, cdb_len;
+int target_id, lun_id;
lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
@@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
target_id = cmd->frame->header.target_id;
lun_id = cmd->frame->header.lun_id;
-cdb_len = cmd->frame->header.cdb_len;
if (target_id < MFI_MAX_LD && lun_id == 0) {
sdev = scsi_device_find(>bus, 0, target_id, lun_id);
@@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
return MFI_STAT_DEVICE_NOT_FOUND;
}
-if (cdb_len > 16) {
-trace_megasas_scsi_invalid_cdb_len(
-mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
-megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-cmd->frame->header.scsi_status = CHECK_CONDITION;
-s->event_count++;
-return MFI_STAT_SCSI_DONE_WITH_ERROR;
-}
Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
consequence of
Host drivers do not necessarily set cdb_len in megasas io commands.
that this can be uninitialized memory and we need to assume it was not
explicitly set?
I doubt that real hardware uses or checks the field for the affected commands
because that would be pointless, but it is really up to you to decide how
you want to handle it.
Guenter
Best Regards,
Fiona
-
cmd->iov_size = lba_count * sdev->blocksize;
if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
@@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
megasas_encode_lba(cdb, lba_start, lba_count, is_write);
cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cdb_len, cmd);
+lun_id, cdb, sizeof(cdb), cmd);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc(frame_cmd), target_id, lun_id);
[PATCH] scsi: megasas: Internal cdbs have 16-byte length
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("scsi: Reject commands if the CDB length
exceeds buf_len") and fe9d8927e2 ("scsi: Add buf_len parameter to
scsi_req_new()"), this results in failures to boot Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.
Signed-off-by: Guenter Roeck
---
hw/scsi/megasas.c | 14 ++
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..d624866bb6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
uint8_t cdb[16];
int len;
struct SCSIDevice *sdev = NULL;
-int target_id, lun_id, cdb_len;
+int target_id, lun_id;
lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
@@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
target_id = cmd->frame->header.target_id;
lun_id = cmd->frame->header.lun_id;
-cdb_len = cmd->frame->header.cdb_len;
if (target_id < MFI_MAX_LD && lun_id == 0) {
sdev = scsi_device_find(>bus, 0, target_id, lun_id);
@@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
return MFI_STAT_DEVICE_NOT_FOUND;
}
-if (cdb_len > 16) {
-trace_megasas_scsi_invalid_cdb_len(
-mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
-megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-cmd->frame->header.scsi_status = CHECK_CONDITION;
-s->event_count++;
-return MFI_STAT_SCSI_DONE_WITH_ERROR;
-}
-
cmd->iov_size = lba_count * sdev->blocksize;
if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
@@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd
*cmd, int frame_cmd)
megasas_encode_lba(cdb, lba_start, lba_count, is_write);
cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cdb_len, cmd);
+lun_id, cdb, sizeof(cdb), cmd);
if (!cmd->req) {
trace_megasas_scsi_req_alloc_failed(
mfi_frame_desc(frame_cmd), target_id, lun_id);
--
2.39.1
Re: [PATCH 02/25] aspeed: Add Supermicro X11 SPI machine type
On 1/31/23 23:49, Cédric Le Goater wrote: On 2/1/23 06:39, Joel Stanley wrote: On Thu, 19 Jan 2023 at 12:36, Cédric Le Goater wrote: From: Guenter Roeck supermicrox11-bmc is configured with ast2400-a1 SoC. This does not match the Supermicro documentation for X11 BMCs, and it does not match the devicetree file in the Linux kernel. I found this sentence confusing; AFAICT X11 doesn't name a specific motherboard. It appears to be a marketing term for a bunch of different things. As it turns out, some Supermicro X11 motherboards use AST2400 SoCs, while others use AST2500. Introduce new machine type supermicrox11-spi-bmc with AST2500 SoC How about supermicro-x11spi-bmc? It would match the product name: https://www.supermicro.com/en/products/motherboard/X11SPi-TF as well as the device tree filename. Indeed, model = "X11SPI BMC"; compatible = "supermicro,x11spi-bmc", "aspeed,ast2500"; I was the one who suggested the name. Let's change if Guenter agrees. Sure, any name is fine with me. Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/19/23 00:02, Klaus Jensen wrote: On Jan 19 08:28, Klaus Jensen wrote: On Jan 18 21:03, Keith Busch wrote: On Thu, Jan 19, 2023 at 01:10:57PM +1000, Alistair Francis wrote: On Thu, Jan 19, 2023 at 12:44 PM Keith Busch wrote: Further up, it says the "interrupt gateway" is responsible for forwarding new interrupt requests while the level remains asserted, but it doesn't look like anything is handling that, which essentially turns this into an edge interrupt. Am I missing something, or is this really not being handled? Yeah, that wouldn't be handled. In QEMU the PLIC relies on QEMUs internal GPIO lines to trigger an interrupt. So with the current setup we only support edge triggered interrupts. Thanks for confirming! Klaus, I think we can justify introducing a work-around in the emulated device now. My previous proposal with pci_irq_pulse() is no good since it does assert+deassert, but it needs to be the other way around, so please don't considert that one. Also, we ought to revisit the intms/intmc usage in the linux driver for threaded interrupts. +CC: qemu-riscv Keith, Thanks for digging into this! Yeah, you are probably right that we should only use the intms/intmc changes in the use_threaded_interrupts case, not in general. While my RFC patch does seem to "fix" this, it is just a workaround as your analysis indicate. > +CC: Philippe, I am observing these timeouts/aborts on mips as well, so I guess that emulation could suffer from the same issue? I suspect it does. In my case, I have an Ethernet controller on the same interrupt line as the NVME controller, and it looks like one of the interrupts gets lost if both controllers raise an interrupt at the same time. The suggested workarounds "fix" the problem, but that doesn't seem to be the correct solution. Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Tue, Jan 17, 2023 at 04:18:14PM +, Peter Maydell wrote: > On Tue, 17 Jan 2023 at 16:10, Guenter Roeck wrote: > > > > On Mon, Jan 16, 2023 at 09:58:13PM -0700, Keith Busch wrote: > > > On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote: > > > > I noticed that the Linux driver does not use the INTMS/INTMC registers > > > > to mask interrupts on the controller while processing CQEs. While not > > > > required by the spec, it is *recommended* in setups not using MSI-X to > > > > reduce the risk of spurious and/or missed interrupts. > > > > > > That's assuming completions are deferred to a bottom half. We don't do > > > that by default in Linux nvme, though you can ask the driver to do that > > > if you want. > > > > > > > With the patch below, running 100 boot iterations, no timeouts were > > > > observed on QEMU emulated riscv64 or mips64. > > > > > > > > No changes are required in the QEMU hw/nvme interrupt logic. > > > > > > Yeah, I can see why: it forces the irq line to deassert then assert, > > > just like we had forced to happen within the device side patches. Still, > > > none of that is supposed to be necessary, but this idea of using these > > > registers is probably fine. > > > > There is still no answer why this would be necessary in the first place, > > on either side. In my opinion, unless someone can confirm that the problem > > is seen with real hardware, we should assume that it happens on the qemu > > side and address it there. > > Sure, but that means identifying what the divergence > between QEMU's implementation and the hardware is first. I don't > want a fudged fix in QEMU's code any more than you want one in > the kernel's driver code :-) > I actually prefer it in qemu because that means I can test nvme support on all active LTS releases of the Linux kernel, but that is POV and secondary. This has been broken ever since I started testing nvme support with qemu, so it doesn't make much of a difference if fixing the problem for good takes a bit longer. Plus, I run my own version of qemu anyway, so carrying the fix (hack) in qemu doesn't make much of a difference for me. Anyway - any idea what to do to help figuring out what is happening ? Add tracing support to pci interrupt handling, maybe ? Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Mon, Jan 16, 2023 at 09:58:13PM -0700, Keith Busch wrote:
> On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote:
> > I noticed that the Linux driver does not use the INTMS/INTMC registers
> > to mask interrupts on the controller while processing CQEs. While not
> > required by the spec, it is *recommended* in setups not using MSI-X to
> > reduce the risk of spurious and/or missed interrupts.
>
> That's assuming completions are deferred to a bottom half. We don't do
> that by default in Linux nvme, though you can ask the driver to do that
> if you want.
>
> > With the patch below, running 100 boot iterations, no timeouts were
> > observed on QEMU emulated riscv64 or mips64.
> >
> > No changes are required in the QEMU hw/nvme interrupt logic.
>
> Yeah, I can see why: it forces the irq line to deassert then assert,
> just like we had forced to happen within the device side patches. Still,
> none of that is supposed to be necessary, but this idea of using these
> registers is probably fine.
There is still no answer why this would be necessary in the first place,
on either side. In my opinion, unless someone can confirm that the problem
is seen with real hardware, we should assume that it happens on the qemu
side and address it there.
Guenter
>
> > static irqreturn_t nvme_irq(int irq, void *data)
> > +{
> > + struct nvme_queue *nvmeq = data;
> > + struct nvme_dev *dev = nvmeq->dev;
> > + u32 mask = 1 << nvmeq->cq_vector;
> > + irqreturn_t ret = IRQ_NONE;
> > + DEFINE_IO_COMP_BATCH(iob);
> > +
> > + writel(mask, dev->bar + NVME_REG_INTMS);
> > +
> > + if (nvme_poll_cq(nvmeq, )) {
> > + if (!rq_list_empty(iob.req_list))
> > + nvme_pci_complete_batch();
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + writel(mask, dev->bar + NVME_REG_INTMC);
> > +
> > + return ret;
> > +}
>
> If threaded interrupts are used, you'll want to do the masking in
> nvme_irq_check(), then clear it in the threaded handler instead of doing
> both in the same callback.
>
> > +static irqreturn_t nvme_irq_msix(int irq, void *data)
> > {
> > struct nvme_queue *nvmeq = data;
> > DEFINE_IO_COMP_BATCH(iob);
> > @@ -1602,12 +1623,13 @@ static int queue_request_irq(struct nvme_queue
> > *nvmeq)
> > {
> > struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
> > int nr = nvmeq->dev->ctrl.instance;
> > + irq_handler_t handler = pdev->msix_enabled ? nvme_irq_msix :
> > nvme_irq;
> >
> > if (use_threaded_interrupts) {
> > return pci_request_irq(pdev, nvmeq->cq_vector,
> > nvme_irq_check,
> > - nvme_irq, nvmeq, "nvme%dq%d", nr,
> > nvmeq->qid);
> > + handler, nvmeq, "nvme%dq%d", nr,
> > nvmeq->qid);
> > } else {
> > - return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq,
> > + return pci_request_irq(pdev, nvmeq->cq_vector, handler,
> > NULL, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> > }
> > }
> >
> >
>
>
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On Mon, Jan 16, 2023 at 10:14:07PM +0100, Klaus Jensen wrote:
[ ... ]
>
> I noticed that the Linux driver does not use the INTMS/INTMC registers
> to mask interrupts on the controller while processing CQEs. While not
> required by the spec, it is *recommended* in setups not using MSI-X to
> reduce the risk of spurious and/or missed interrupts.
>
> With the patch below, running 100 boot iterations, no timeouts were
> observed on QEMU emulated riscv64 or mips64.
>
> No changes are required in the QEMU hw/nvme interrupt logic.
>
Yes, but isn't that technically similar to dropping the
interrupt request and raising it again, or in other words
pulsing the interrupt ?
I still don't understand why the (level triggered) interrupt
pin would require pulsing in the first place.
Thanks,
Guenter
>
> diff --git i/drivers/nvme/host/pci.c w/drivers/nvme/host/pci.c
> index b13baccedb4a..75f6b87c4c3f 100644
> --- i/drivers/nvme/host/pci.c
> +++ w/drivers/nvme/host/pci.c
> @@ -1128,6 +1128,27 @@ static inline int nvme_poll_cq(struct nvme_queue
> *nvmeq,
> }
>
> static irqreturn_t nvme_irq(int irq, void *data)
> +{
> + struct nvme_queue *nvmeq = data;
> + struct nvme_dev *dev = nvmeq->dev;
> + u32 mask = 1 << nvmeq->cq_vector;
> + irqreturn_t ret = IRQ_NONE;
> + DEFINE_IO_COMP_BATCH(iob);
> +
> + writel(mask, dev->bar + NVME_REG_INTMS);
> +
> + if (nvme_poll_cq(nvmeq, )) {
> + if (!rq_list_empty(iob.req_list))
> + nvme_pci_complete_batch();
> + ret = IRQ_HANDLED;
> + }
> +
> + writel(mask, dev->bar + NVME_REG_INTMC);
> +
> + return ret;
> +}
> +
> +static irqreturn_t nvme_irq_msix(int irq, void *data)
> {
> struct nvme_queue *nvmeq = data;
> DEFINE_IO_COMP_BATCH(iob);
> @@ -1602,12 +1623,13 @@ static int queue_request_irq(struct nvme_queue *nvmeq)
> {
> struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
> int nr = nvmeq->dev->ctrl.instance;
> + irq_handler_t handler = pdev->msix_enabled ? nvme_irq_msix : nvme_irq;
>
> if (use_threaded_interrupts) {
> return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq_check,
> - nvme_irq, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> + handler, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> } else {
> - return pci_request_irq(pdev, nvmeq->cq_vector, nvme_irq,
> + return pci_request_irq(pdev, nvmeq->cq_vector, handler,
> NULL, nvmeq, "nvme%dq%d", nr, nvmeq->qid);
> }
> }
>
>
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 11:27, Keith Busch wrote:
On Thu, Jan 12, 2023 at 06:45:55PM +0100, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
Could you try the below?
This works as well: 50 iterations with no failures after applying the patch
below on top of qemu v7.2.0. Tested with qemu-system-mipsel.
Guenter
---
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2c85de4700..521c3c80c1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -558,6 +558,20 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue *cq)
}
}
+static void nvme_irq_pulse(NvmeCtrl *n, NvmeCQueue *cq)
+{
+if (!cq->irq_enabled) {
+return;
+}
+
+if (msix_enabled(&(n->parent_obj))) {
+msix_notify(&(n->parent_obj), cq->vector);
+return;
+}
+
+pci_irq_pulse(>parent_obj);
+}
+
static void nvme_req_clear(NvmeRequest *req)
{
req->ns = NULL;
@@ -6917,6 +6931,12 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr,
int val)
}
nvme_irq_deassert(n, cq);
+} else {
+/*
+ * Retrigger the irq just to make sure the host has no excuse for
+ * not knowing there's more work to complete on this CQ.
+ */
+nvme_irq_pulse(n, cq);
}
} else {
/* Submission queue doorbell write */
--
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 09:45, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
So, for mipsel, two sets of results for the above:
First, qemu v7.2 is already much better than qemu v7.1. With qemu v7.1,
the boot test fails roughly every other test. Failure rate with qemu v7.2
is much less.
Second, my nvme boot test with qemu 7.2 fails after ~5-10 iterations.
After the above change, I did not see a single failure in 50 boot tests.
I'll test the other suggested change next.
Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 09:45, Klaus Jensen wrote:
On Jan 12 09:34, Keith Busch wrote:
On Thu, Jan 12, 2023 at 02:10:51PM +0100, Klaus Jensen wrote:
The pin-based interrupt logic in hw/nvme seems sound enough to me, so I
am wondering if there is something going on with the kernel driver (but
I certainly do not rule out that hw/nvme is at fault here, since
pin-based interrupts has also been a source of several issues in the
past).
Does it work if you change the pci_irq_assert() back to pci_irq_pulse()?
While probably not the "correct" thing to do, it has better results in
my testing.
A simple s/pci_irq_assert/pci_irq_pulse broke the device. However,
diff --git i/hw/nvme/ctrl.c w/hw/nvme/ctrl.c
index 03760ddeae8c..0fc46dcb9ec4 100644
--- i/hw/nvme/ctrl.c
+++ w/hw/nvme/ctrl.c
@@ -477,6 +477,7 @@ static void nvme_irq_check(NvmeCtrl *n)
return;
}
if (~intms & n->irq_status) {
+pci_irq_deassert(>parent_obj);
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
seems to do the trick (pulse is the other way around, assert, then
deassert).
Probably not the "correct" thing to do, but I'll take it since it seems
to fix it. On a simple boot loop I got the timeout about 1 out of 5. I'm
on ~20 runs now and have not encountered it.
I'll see if I can set up a mips rootfs and test that. Guenter, what MIPS
machine/board(s) are you testing?
See
https://github.com/groeck/linux-build-test/blob/master/rootfs/mips/run-qemu-mips.sh
and
https://github.com/groeck/linux-build-test/blob/master/rootfs/mipsel/run-qemu-mipsel.sh
I'll apply the change suggested above to my version of qemu (7.2.0)
and give it a try.
Guenter
Re: completion timeouts with pin-based interrupts in QEMU hw/nvme
On 1/12/23 05:10, Klaus Jensen wrote: Hi all (linux-nvme, qemu-devel, maintainers), On QEMU riscv64, which does not use MSI/MSI-X and thus relies on pin-based interrupts, I'm seeing occasional completion timeouts, i.e. nvme nvme0: I/O 333 QID 1 timeout, completion polled To rule out issues with shadow doorbells (which have been a source of frustration in the past), those are disabled. FWIW I'm also seeing the issue with shadow doorbells. diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index f25cc2c235e9..28d8e7f4b56c 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -7407,7 +7407,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev) id->mdts = n->params.mdts; id->ver = cpu_to_le32(NVME_SPEC_VER); id->oacs = -cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT | NVME_OACS_DBBUF); +cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT); id->cntrltype = 0x1; /* I captured a trace from QEMU when this happens: pci_nvme_mmio_write addr 0x1008 data 0x4e size 4 pci_nvme_mmio_doorbell_sq sqid 1 new_tail 78 pci_nvme_io_cmd cid 4428 nsid 0x1 sqid 1 opc 0x2 opname 'NVME_NVM_CMD_READ' pci_nvme_read cid 4428 nsid 1 nlb 32 count 16384 lba 0x1324 pci_nvme_map_prp trans_len 4096 len 16384 prp1 0x80aca000 prp2 0x82474100 num_prps 5 pci_nvme_map_addr addr 0x80aca000 len 4096 pci_nvme_map_addr addr 0x80ac9000 len 4096 pci_nvme_map_addr addr 0x80ac8000 len 4096 pci_nvme_map_addr addr 0x80ac7000 len 4096 pci_nvme_io_cmd cid 4429 nsid 0x1 sqid 1 opc 0x2 opname 'NVME_NVM_CMD_READ' pci_nvme_read cid 4429 nsid 1 nlb 224 count 114688 lba 0x1242 pci_nvme_map_prp trans_len 4096 len 114688 prp1 0x80ae6000 prp2 0x82474000 num_prps 29 pci_nvme_map_addr addr 0x80ae6000 len 4096 pci_nvme_map_addr addr 0x80ae5000 len 4096 pci_nvme_map_addr addr 0x80ae4000 len 4096 pci_nvme_map_addr addr 0x80ae3000 len 4096 pci_nvme_map_addr addr 0x80ae2000 len 4096 pci_nvme_map_addr addr 0x80ae1000 len 4096 pci_nvme_map_addr addr 0x80ae len 4096 pci_nvme_map_addr addr 0x80adf000 len 4096 pci_nvme_map_addr addr 0x80ade000 len 4096 pci_nvme_map_addr addr 0x80add000 len 4096 pci_nvme_map_addr addr 0x80adc000 len 4096 pci_nvme_map_addr addr 0x80adb000 len 4096 pci_nvme_map_addr addr 0x80ada000 len 4096 pci_nvme_map_addr addr 0x80ad9000 len 4096 pci_nvme_map_addr addr 0x80ad8000 len 4096 pci_nvme_map_addr addr 0x80ad7000 len 4096 pci_nvme_map_addr addr 0x80ad6000 len 4096 pci_nvme_map_addr addr 0x80ad5000 len 4096 pci_nvme_map_addr addr 0x80ad4000 len 4096 pci_nvme_map_addr addr 0x80ad3000 len 4096 pci_nvme_map_addr addr 0x80ad2000 len 4096 pci_nvme_map_addr addr 0x80ad1000 len 4096 pci_nvme_map_addr addr 0x80ad len 4096 pci_nvme_map_addr addr 0x80acf000 len 4096 pci_nvme_map_addr addr 0x80ace000 len 4096 pci_nvme_map_addr addr 0x80acd000 len 4096 pci_nvme_map_addr addr 0x80acc000 len 4096 pci_nvme_map_addr addr 0x80acb000 len 4096 pci_nvme_rw_cb cid 4428 blk 'd0' pci_nvme_rw_complete_cb cid 4428 blk 'd0' pci_nvme_enqueue_req_completion cid 4428 cqid 1 dw0 0x0 dw1 0x0 status 0x0 [1]: pci_nvme_irq_pin pulsing IRQ pin pci_nvme_rw_cb cid 4429 blk 'd0' pci_nvme_rw_complete_cb cid 4429 blk 'd0' pci_nvme_enqueue_req_completion cid 4429 cqid 1 dw0 0x0 dw1 0x0 status 0x0 [2]: pci_nvme_irq_pin pulsing IRQ pin [3]: pci_nvme_mmio_write addr 0x100c data 0x4d size 4 [4]: pci_nvme_mmio_doorbell_cq cqid 1 new_head 77 TIMEOUT HERE (30s) --- [5]: pci_nvme_mmio_read addr 0x1c size 4 [6]: pci_nvme_mmio_write addr 0x100c data 0x4e size 4 [7]: pci_nvme_mmio_doorbell_cq cqid 1 new_head 78 --- Interrupt deasserted (cq->tail == cq->head) [ 31.757821] nvme nvme0: I/O 333 QID 1 timeout, completion polled Following the timeout, everything returns to "normal" and device/driver happily continues. The pin-based interrupt logic in hw/nvme seems sound enough to me, so I am wondering if there is something going on with the kernel driver (but I certainly do not rule out that hw/nvme is at fault here, since pin-based interrupts has also been a source of several issues in the past). What I'm thinking is that following the interrupt in [1], the driver picks up completion for cid 4428 but does not find cid 4429 in the queue since it has not been posted yet. Before getting a cq head doorbell write (which would cause the pin to be deasserted), the device posts the completion for cid 4429 which just keeps the interrupt asserted in [2]. The trace then shows the cq head doorbell update in [3,4] for cid 4428 and then we hit the timeout since the driver is not aware that cid 4429 has been posted in between this (why is it not aware of this?) Timing out, the driver then polls the queue and notices cid 4429 and updates the cq head doorbell in [5-7], causing the device to deassert the interrupt and we are "back in shape". I'm observing this on 6.0 kernels and v6.2-rc3 (have not tested <6.0). Tested on QEMU v7.0.0 (to rule out all the
Re: [PATCH] m25p80: Add the is25wp256 SFPD table
On Mon, Jan 02, 2023 at 06:42:03PM +0100, Michael Walle wrote: > Am 2023-01-02 17:23, schrieb Guenter Roeck: > > On Mon, Jan 02, 2023 at 04:43:49PM +0100, Michael Walle wrote: > > > Am 2023-01-02 14:53, schrieb Cédric Le Goater: > > > > On 12/27/22 07:31, Tudor Ambarus wrote: > > > > > > > > > > > > > > > On 25.12.2022 14:18, Ben Dooks wrote: > > > > > > On Wed, Dec 21, 2022 at 06:36:02PM +0100, Cédric Le Goater wrote: > > > > > > > On 12/21/22 13:22, Guenter Roeck wrote: > > > > > > > > Generated from hardware using the following command and > > > > > > > > then padding > > > > > > > > with 0xff to fill out a power-of-2: > > > > > > > > xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp > > > > > > > > > > > > > > > > Cc: Michael Walle > > > > > > > > Cc: Tudor Ambarus > > > > > > > > Signed-off-by: Guenter Roeck > > > > > > > > > > > > > > Reviewed-by: Cédric Le Goater > > > > > > > > > > > > If SFDP is a standard, couldn't we have an function to generate > > > > > > it from > > > > > > the flash parameters? > > > > > > > > > > > > > > > > No, it's not practical as you have to specify all the flash parameters > > > > > at flash declaration. > > > > > > > > Indeed and the definition of flash models in QEMU is far to cover all > > > > the SFDP > > > > features. The known_devices array of m25p80 would be huge ! However, we > > > > could > > > > generate some of the SFDP tables if no raw data is provided. It could be > > > > useful > > > > for testing drivers. > > > > > > I don't think adding (incomplete) SFDP tables makes sense for any real > > > devices. E.g. sometimes our linux driver will look at specific bits in > > > SFDP to figure out what particular flash device is attached. For > > > example > > > when there are different flashes with the same jedec id. > > > > > > But since the last released kernel, we support a generic SFDP > > > driver, which > > > is used when there is no matching driver for the flash's jedec id. > > > Theoretically, you can build your own flash device (with a unique > > > id) and > > > generate the sfdp tables for that one. > > > > > How about older kernels versions ? Would those still support such > > (virtual ?) > > flash devices ? > > No with older kernels you'd be out of luck. They will just print "unknown > flash > id" and skip the device. That would mean that qemu versions including this change could no longer be used to test flash support on older kernel versions. That would be extremely undesirable. I'd rather live with the current code and still be able to test older kernels. Thanks, Guenter
Re: [PATCH] m25p80: Add the is25wp256 SFPD table
On Mon, Jan 02, 2023 at 04:43:49PM +0100, Michael Walle wrote: > Am 2023-01-02 14:53, schrieb Cédric Le Goater: > > On 12/27/22 07:31, Tudor Ambarus wrote: > > > > > > > > > On 25.12.2022 14:18, Ben Dooks wrote: > > > > On Wed, Dec 21, 2022 at 06:36:02PM +0100, Cédric Le Goater wrote: > > > > > On 12/21/22 13:22, Guenter Roeck wrote: > > > > > > Generated from hardware using the following command and > > > > > > then padding > > > > > > with 0xff to fill out a power-of-2: > > > > > > xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp > > > > > > > > > > > > Cc: Michael Walle > > > > > > Cc: Tudor Ambarus > > > > > > Signed-off-by: Guenter Roeck > > > > > > > > > > Reviewed-by: Cédric Le Goater > > > > > > > > If SFDP is a standard, couldn't we have an function to generate > > > > it from > > > > the flash parameters? > > > > > > > > > > No, it's not practical as you have to specify all the flash parameters > > > at flash declaration. > > > > Indeed and the definition of flash models in QEMU is far to cover all > > the SFDP > > features. The known_devices array of m25p80 would be huge ! However, we > > could > > generate some of the SFDP tables if no raw data is provided. It could be > > useful > > for testing drivers. > > I don't think adding (incomplete) SFDP tables makes sense for any real > devices. E.g. sometimes our linux driver will look at specific bits in > SFDP to figure out what particular flash device is attached. For example > when there are different flashes with the same jedec id. > > But since the last released kernel, we support a generic SFDP driver, which > is used when there is no matching driver for the flash's jedec id. > Theoretically, you can build your own flash device (with a unique id) and > generate the sfdp tables for that one. > How about older kernels versions ? Would those still support such (virtual ?) flash devices ? Thanks, Guenter
[PATCH] m25p80: Add the is25wp256 SFPD table
Generated from hardware using the following command and then padding
with 0xff to fill out a power-of-2:
xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
Cc: Michael Walle
Cc: Tudor Ambarus
Signed-off-by: Guenter Roeck
---
hw/block/m25p80.c | 3 ++-
hw/block/m25p80_sfdp.c | 40
hw/block/m25p80_sfdp.h | 2 ++
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 02adc87527..802d2eb021 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -221,7 +221,8 @@ static const FlashPartInfo known_devices[] = {
{ INFO("is25wp032", 0x9d7016, 0, 64 << 10, 64, ER_4K) },
{ INFO("is25wp064", 0x9d7017, 0, 64 << 10, 128, ER_4K) },
{ INFO("is25wp128", 0x9d7018, 0, 64 << 10, 256, ER_4K) },
-{ INFO("is25wp256", 0x9d7019, 0, 64 << 10, 512, ER_4K) },
+{ INFO("is25wp256", 0x9d7019, 0, 64 << 10, 512, ER_4K),
+ .sfdp_read = m25p80_sfdp_is25wp256 },
/* Macronix */
{ INFO("mx25l2005a", 0xc22012, 0, 64 << 10, 4, ER_4K) },
diff --git a/hw/block/m25p80_sfdp.c b/hw/block/m25p80_sfdp.c
index 77615fa29e..b33811a4f5 100644
--- a/hw/block/m25p80_sfdp.c
+++ b/hw/block/m25p80_sfdp.c
@@ -330,3 +330,43 @@ static const uint8_t sfdp_w25q01jvq[] = {
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
};
define_sfdp_read(w25q01jvq);
+
+/*
+ * Integrated Silicon Solution (ISSI)
+ */
+
+static const uint8_t sfdp_is25wp256[] = {
+0x53, 0x46, 0x44, 0x50, 0x06, 0x01, 0x01, 0xff,
+0x00, 0x06, 0x01, 0x10, 0x30, 0x00, 0x00, 0xff,
+0x9d, 0x05, 0x01, 0x03, 0x80, 0x00, 0x00, 0x02,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xe5, 0x20, 0xf9, 0xff, 0xff, 0xff, 0xff, 0x0f,
+0x44, 0xeb, 0x08, 0x6b, 0x08, 0x3b, 0x80, 0xbb,
+0xfe, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0xff,
+0xff, 0xff, 0x44, 0xeb, 0x0c, 0x20, 0x0f, 0x52,
+0x10, 0xd8, 0x00, 0xff, 0x23, 0x4a, 0xc9, 0x00,
+0x82, 0xd8, 0x11, 0xce, 0xcc, 0xcd, 0x68, 0x46,
+0x7a, 0x75, 0x7a, 0x75, 0xf7, 0xae, 0xd5, 0x5c,
+0x4a, 0x42, 0x2c, 0xff, 0xf0, 0x30, 0xfa, 0xa9,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0x50, 0x19, 0x50, 0x16, 0x9f, 0xf9, 0xc0, 0x64,
+0x8f, 0xef, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+};
+define_sfdp_read(is25wp256);
diff --git a/hw/block/m25p80_sfdp.h b/hw/block/m25p80_sfdp.h
index df7adfb5ce..011a880f66 100644
--- a/hw/block/m25p80_sfdp.h
+++ b/hw/block/m25p80_sfdp.h
@@ -26,4 +26,6 @@ uint8_t m25p80_sfdp_w25q512jv(uint32_t addr);
uint8_t m25p80_sfdp_w25q01jvq(uint32_t addr);
+uint8_t m25p80_sfdp_is25wp256(uint32_t addr);
+
#endif
--
2.36.2
Re: [PATCH v2] target/sh4: Fix TB_FLAG_UNALIGN
On Mon, Dec 12, 2022 at 08:30:42AM -0600, Richard Henderson wrote:
> On 12/11/22 19:13, Guenter Roeck wrote:
> > On Sat, Dec 10, 2022 at 07:27:46AM -0800, Guenter Roeck wrote:
> > > Hi,
> > >
> > > On Thu, Sep 01, 2022 at 11:15:09AM +0100, Richard Henderson wrote:
> > > > The value previously chosen overlaps GUSA_MASK.
> > > >
> > > > Rename all DELAY_SLOT_* and GUSA_* defines to emphasize
> > > > that they are included in TB_FLAGs. Add aliases for the
> > > > FPSCR and SR bits that are included in TB_FLAGS, so that
> > > > we don't accidentally reassign those bits.
> > > >
> > > > Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus")
> > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856
> > > > Signed-off-by: Richard Henderson
> > >
> > > I noticed that my sh4 emulations crash randomly with qemu v7.2-rc4.
> > > This happens with all Linux kernel versions. Testing shows that this
> > > patch is responsible. Reverting it fixes the problem.
> > >
> >
> > The patch below fixes the problem for me.
>
> Thanks for the investigation.
>
>
> > +++ b/target/sh4/cpu.c
> > @@ -47,7 +47,7 @@ static void superh_cpu_synchronize_from_tb(CPUState *cs,
> > SuperHCPU *cpu = SUPERH_CPU(cs);
> > cpu->env.pc = tb_pc(tb);
> > -cpu->env.flags = tb->flags;
> > +cpu->env.flags = tb->flags & TB_FLAG_ENVFLAGS_MASK;
>
> Only this hunk should be necessary.
>
Confirmed.
Do you plan to send a formal patch, or do you want me to do it ?
Thanks,
Guenter
Re: [PATCH v2] target/sh4: Fix TB_FLAG_UNALIGN
On 12/12/22 06:30, Richard Henderson wrote:
On 12/11/22 19:13, Guenter Roeck wrote:
On Sat, Dec 10, 2022 at 07:27:46AM -0800, Guenter Roeck wrote:
Hi,
On Thu, Sep 01, 2022 at 11:15:09AM +0100, Richard Henderson wrote:
The value previously chosen overlaps GUSA_MASK.
Rename all DELAY_SLOT_* and GUSA_* defines to emphasize
that they are included in TB_FLAGs. Add aliases for the
FPSCR and SR bits that are included in TB_FLAGS, so that
we don't accidentally reassign those bits.
Fixes: 4da06fb3062 ("target/sh4: Implement prctl_unalign_sigbus")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/856
Signed-off-by: Richard Henderson
I noticed that my sh4 emulations crash randomly with qemu v7.2-rc4.
This happens with all Linux kernel versions. Testing shows that this
patch is responsible. Reverting it fixes the problem.
The patch below fixes the problem for me.
Thanks for the investigation.
+++ b/target/sh4/cpu.c
@@ -47,7 +47,7 @@ static void superh_cpu_synchronize_from_tb(CPUState *cs,
SuperHCPU *cpu = SUPERH_CPU(cs);
cpu->env.pc = tb_pc(tb);
- cpu->env.flags = tb->flags;
+ cpu->env.flags = tb->flags & TB_FLAG_ENVFLAGS_MASK;
Only this hunk should be necessary.
I'll give it a try.
Thanks,
Guenter
Re: [PATCH v3 1/2] hw/nvme: Implement shadow doorbell buffer support
On 12/12/22 05:45, Klaus Jensen wrote: On Dec 12 05:39, Guenter Roeck wrote: On 12/12/22 01:58, Klaus Jensen wrote: On Dec 8 12:39, Guenter Roeck wrote: On Thu, Dec 08, 2022 at 12:13:55PM -0800, Guenter Roeck wrote: On Thu, Dec 08, 2022 at 10:47:42AM -0800, Guenter Roeck wrote: A cq head doorbell mmio is skipped... And it is not the fault of the kernel. The kernel is in it's good right to skip the mmio since the cq eventidx is not properly updated. Adding that and it boots properly on riscv. But I'm perplexed as to why this didnt show up on our regularly tested platforms. Gonna try to get this in for 7.2! I see another problem with sparc64. [5.261508] could not locate request for tag 0x0 [5.261711] nvme nvme0: invalid id 0 completed on queue 1 That is seen repeatedly until the request times out. I'll test with your patch to see if it resolves this problem as well, and will bisect otherwise. The second problem is unrelated to the doorbell problem. It is first seen in qemu v7.1. I'll try to bisect. Unfortunately, the problem observed with sparc64 also bisects to this patch. Making things worse, "hw/nvme: fix missing cq eventidx update" does not fix it (which is why I initially thought it was unrelated). I used the following qemu command line. qemu-system-sparc64 -M sun4v -cpu "TI UltraSparc IIi" -m 512 -snapshot \ -device nvme,serial=foo,drive=d0,bus=pciB \ -drive file=rootfs.ext2,if=none,format=raw,id=d0 \ -kernel arch/sparc/boot/image -no-reboot \ -append "root=/dev/nvme0n1 console=ttyS0" \ -nographic -monitor none Hi Guenter, Thank you very much for the detailed reports and I apologize for the fallout of this. I think this is related to missing endian conversions when handling the shadow doorbells. I'm not sure if there is a kernel issue here as well, because as far as I can tell, the shadow doorbells are updated like so: old_value = *dbbuf_db; *dbbuf_db = value; (where `value` is the new head/tail value depending on if this is an sq or cq). Keith, is the kernel doing something magically here I am not aware of, or isn't this missing some cpu_to_le32() / le32_to_cpu() calls as well? Wouldn't that mean that nvme doorbell support would be broken in Linux on all big endian systems ? Maybe it is, but it seems a bit unlikely. No, not broken in general - only for shadow doorbells. On regular MMIO, the linux helpers takes care of endianness (and so does the MMIO callbacks in QEMU). However, for shadow doorbells, the doorbell value is written to host memory - and Linux (and QEMU) does not handle that correctly wrt. endianness. I got it all working with the series I just posted for QEMU (v4) and fixing endianness conversion for the above in the kernel (patch pending for linux-nvme). Hmm, interesting. I guess I'll wait for the Linux patch to be posted. That makes me wonder, though: Are the Linux changes really necessary ? If this never worked, would it be possible to adjust the qemu code in a way that it just works with the existing Linux code ? Alternatively, would it be possible to add a runtime flag to qemu that would let me disable shadow doorbell support ? I am testing all Linux kernel branches, and without such a flag I'd have to carry a patch just disabling it in qemu until all kernel branches are fixed (if that ever happens). Thanks, Guenter
