QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Niek Linnenbank <nieklinnenbank@gmail.com>
Cc: Beniamino Galvani <b.galvani@gmail.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine
Date: Tue, 10 Dec 2019 09:59:29 +0100
Message-ID: <f5983dbb-28f6-206f-a180-83633a049325@redhat.com> (raw)
In-Reply-To: <CAPan3WrN28W-h9RYA88LbA8eWP6Me9VcNisnZhwNgC2WOgVLxg@mail.gmail.com>

On 12/6/19 11:15 PM, Niek Linnenbank wrote:
[...]
>      >      > +static void orangepi_machine_init(MachineClass *mc)
>      >      > +{
>      >      > +    mc->desc = "Orange Pi PC";
>      >      > +    mc->init = orangepi_init;
>      >      > +    mc->units_per_default_bus = 1;
>      >      > +    mc->min_cpus = AW_H3_NUM_CPUS;
>      >      > +    mc->max_cpus = AW_H3_NUM_CPUS;
>      >      > +    mc->default_cpus = AW_H3_NUM_CPUS;
>      >
>      >              mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
>      >
>      >      > +    mc->ignore_memory_transaction_failures = true;
>      >
>      >     You should not use this flag in new design. See the
>     documentation in
>      >     include/hw/boards.h:
>      >
>      >        * @ignore_memory_transaction_failures:
>      >        *    [...] New board models
>      >        *    should instead use "unimplemented-device" for all memory
>      >     ranges where
>      >        *    the guest will attempt to probe for a device that
>     QEMU doesn't
>      >        *    implement and a stub device is required.
>      >
>      >     You already use the "unimplemented-device".
>      >
>      > Thanks, I'm working on this now. I think that at least I'll need
>     to add
>      > all of the devices mentioned in the 4.1 Memory Mapping chapter of
>     the
>      > datasheet
>      > as an unimplemented device. Previously I only added some that I
>     thought
>      > were relevant.
>      >
>      > I added all the missing devices as unimplemented and removed the
>      > ignore_memory_transaction_failures flag
> 
>     I was going to say, "instead of adding *all* the devices regions you
>     can
>     add the likely bus decoding regions", probably:
> 
>     0x01c0.0000   128KiB   AMBA AXI
>     0x01c2.0000   64KiB    AMBA APB
> 
>     But too late.
> 
> 
> Hehe its okey, I can change it to whichever is preferable: the minimum set
> with unimplemented device entries to get a working linux kernel / u-boot or
> just cover the full memory space from the datasheet. My initial thought 
> was that if
> we only provide the minimum set, and the linux kernel later adds a new 
> driver for a device
> which is not marked unimplemented, it will trigger the data abort and 
> potentially resulting in a non-booting kernel.
> 
> But I'm not sure what is normally done here. I do see other board files 
> using the create_unimplemented_device() function,
> but I dont know if they are covering the whole memory space or not.

If they don't cover all memory regions, the guest code can trigger a 
data abort indeed. Since we don't know the memory layout of old board, 
they are still supported with ignore_memory_transaction_failures=true, 
so guest still run unaffected.
We expect new boards to implement the minimum layout.
As long as your guest is happy and usable, UNIMP devices are fine, 
either as big region or individual device (this requires someone with 
access to the datasheet to verify). If you can add a UNIMP for each 
device - which is what you did - it is even better because the the unimp 
access log will be more useful, having finer granularity.

 > I added all the missing devices as unimplemented and removed the
 > ignore_memory_transaction_failures flag

IOW, you already did the best you could do :)

>      > from the machine. Now it seems Linux gets a data abort while
>     probing the
>      > uart1 serial device at 0x01c28400,
> 
>     Did you add the UART1 as UNIMP or 16550?
> 
> 
> I discovered what goes wrong here. See this kernel oops message:
> 
> [    1.084985] [f08600f8] *pgd=6f00a811, *pte=01c28653, *ppte=01c28453
> [    1.085564] Internal error: : 8 [#1] SMP ARM
> [    1.085698] Modules linked in:
> [    1.085940] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.4.0-11747-g2f13437b8917 #4
> [    1.085968] Hardware name: Allwinner sun8i Family
> [    1.086447] PC is at dw8250_setup_port+0x10/0x10c
> [    1.086478] LR is at dw8250_probe+0x500/0x56c
> 
> It tries to access the UART0 at base address 0x01c28400, which I did 
> provide. The strange
> thing is that is accesses offset 0xf8, thus address 0x01c284f8. The 
> datasheet does not mention this register
> but if we provide the 1KiB (0x400) I/O space it should at least read as 
> zero and writes ignored. Unfortunately the emulated
> serial driver only maps a small portion until 0x1f:
> 
> (qemu) info mtree
> ...
>      0000000001c28000-0000000001c2801f (prio 0, i/o): serial
>      0000000001c28400-0000000001c2841f (prio 0, i/o): serial
>      0000000001c28800-0000000001c2881f (prio 0, i/o): serial
> 
> 
> Apparently, the register that the mainline linux kernel is using is 
> DesignWare specific:
> 
> drivers/tty/serial/8250/8250_dwlib.c:13:
> 
> /* Offsets for the DesignWare specific registers */
> #define DW_UART_DLF<--->0xc0 /* Divisor Latch Fraction Register */
> #define DW_UART_CPR<--->0xf4 /* Component Parameter Register */
> #define DW_UART_UCV<--->0xf8 /* UART Component Version */
> 
> I tried to find a way to increase the memory mapped size of the serial 
> device I created with serial_mm_init(),
> but I don't think its possible with that interface.
> 
> I did manage to get it working by overlaying the UART0 with another 
> unimplemented device
> that does have an I/O size of 0x400, but I guess that is probably not 
> the solution we are looking for?

IMHO this is the correct solution :)

Memory regions have priority. By default a device has priority 0, and 
UNIMP device has priority -1000.

So you can use:

    serial_mm_init(get_system_memory(), AW_H3_UART0_REG_BASE, 2,
                   s->irq[AW_H3_GIC_SPI_UART0], 115200,
                   serial_hd(0), DEVICE_NATIVE_ENDIAN);
    create_unimplemented_device("DesignWare-uart",\
                                AW_H3_UART0_REG_BASE,
                                0x400);

(Or cleaner, add a create_designware_uart(...) function that does both).

(qemu) info mtree
...
    0000000001c28000-0000000001c2801f (prio 0, i/o): serial
    0000000001c28000-0000000001c283ff (prio -1000, i/o): DesignWare-uart

You could create an UNIMP region of 0x400 - 0x20 = 0x3e0, but that would 
appear this is a different device, so I don't recommend that.

 > I wonder, did any of the other SoC / boards have this problem when
 > removing mc->ignore_memory_transaction_failures?



  reply index

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 21:09 [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Niek Linnenbank
2019-12-02 21:09 ` [PATCH 01/10] hw: arm: add Allwinner H3 System-on-Chip Niek Linnenbank
2019-12-04 16:53   ` Philippe Mathieu-Daudé
2019-12-04 20:44     ` Niek Linnenbank
2019-12-10  9:02   ` Philippe Mathieu-Daudé
2019-12-10 19:17     ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine Niek Linnenbank
2019-12-03  9:17   ` Philippe Mathieu-Daudé
2019-12-03 19:33     ` Niek Linnenbank
2019-12-04  9:03       ` Philippe Mathieu-Daudé
2019-12-04 19:50         ` Niek Linnenbank
2019-12-05 22:15     ` Niek Linnenbank
2019-12-06  5:41       ` Philippe Mathieu-Daudé
2019-12-06 22:15         ` Niek Linnenbank
2019-12-10  8:59           ` Philippe Mathieu-Daudé [this message]
2019-12-10 19:14             ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 03/10] arm: allwinner-h3: add Clock Control Unit Niek Linnenbank
2019-12-13  0:03   ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 04/10] arm: allwinner-h3: add USB host controller Niek Linnenbank
2019-12-04 16:11   ` Aleksandar Markovic
2019-12-04 20:20     ` Niek Linnenbank
2019-12-10  7:56   ` Philippe Mathieu-Daudé
2019-12-10  8:29     ` Gerd Hoffmann
2019-12-10 19:11       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 05/10] arm: allwinner-h3: add System Control module Niek Linnenbank
2019-12-13  0:09   ` Philippe Mathieu-Daudé
2019-12-15 23:27     ` Niek Linnenbank
2019-12-16  0:17       ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on() Niek Linnenbank
2019-12-06 14:24   ` Peter Maydell
2019-12-06 20:01     ` Niek Linnenbank
2019-12-13 20:52       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 07/10] arm: allwinner-h3: add CPU Configuration module Niek Linnenbank
2019-12-02 21:09 ` [PATCH 08/10] arm: allwinner-h3: add Security Identifier device Niek Linnenbank
2019-12-06 14:27   ` Peter Maydell
2019-12-06 16:35     ` Philippe Mathieu-Daudé
2019-12-06 20:20       ` Niek Linnenbank
2019-12-02 21:09 ` [PATCH 09/10] arm: allwinner-h3: add SD/MMC host controller Niek Linnenbank
2019-12-11 22:34   ` Niek Linnenbank
2019-12-12 23:56     ` Philippe Mathieu-Daudé
2019-12-13 21:00       ` Niek Linnenbank
2019-12-14 13:59         ` Philippe Mathieu-Daudé
2019-12-14 20:32           ` Niek Linnenbank
2019-12-15 23:07       ` Niek Linnenbank
2019-12-16  0:14         ` Philippe Mathieu-Daudé
2019-12-16 19:46           ` Niek Linnenbank
2019-12-16 21:28             ` Philippe Mathieu-Daudé
2019-12-02 21:09 ` [PATCH 10/10] arm: allwinner-h3: add EMAC ethernet device Niek Linnenbank
2019-12-03  9:33   ` KONRAD Frederic
2019-12-03 19:41     ` Niek Linnenbank
2019-12-04 15:14     ` Philippe Mathieu-Daudé
2019-12-04 15:22       ` KONRAD Frederic
2019-12-03  8:47 ` [PATCH 00/10] Add Allwinner H3 SoC and Orange Pi PC Machine Philippe Mathieu-Daudé
2019-12-03 19:25   ` Niek Linnenbank
2019-12-10  8:40     ` Philippe Mathieu-Daudé
2019-12-09 21:37   ` Niek Linnenbank
2019-12-10  8:26     ` Philippe Mathieu-Daudé
2019-12-10 20:12       ` Niek Linnenbank
2019-12-12 23:07         ` Niek Linnenbank
2019-12-12 23:25           ` Philippe Mathieu-Daudé
2019-12-13 20:45             ` Niek Linnenbank
2019-12-03  9:02 ` Philippe Mathieu-Daudé
2019-12-03 19:32   ` Niek Linnenbank
2019-12-06 14:16     ` Peter Maydell
2019-12-09 22:24       ` Aleksandar Markovic
2019-12-10 10:34 ` KONRAD Frederic
2019-12-10 19:55   ` Niek Linnenbank

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f5983dbb-28f6-206f-a180-83633a049325@redhat.com \
    --to=philmd@redhat.com \
    --cc=b.galvani@gmail.com \
    --cc=nieklinnenbank@gmail.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-arm@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git