Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine

["Philippe Mathieu-Daudé" <philmd@redhat.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?