Hi Philippe,
On Tue, Dec 10, 2019 at 9:59 AM Philippe Mathieu-Daudé <phi...@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
