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

[Niek Linnenbank <nieklinnenbank@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7127 bytes --]

Hi Philippe,

On Tue, Dec 10, 2019 at 9:59 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> 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);
>
>
Now it makes much more sense to me, thanks a lot for explaining this!

Allright, I'll use this approach to finish the work for removing
mc->ignore_memory_transaction_failures.

Regards,
Niek


> (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?
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 9041 bytes --]