Re: [Qemu-devel] [PATCH 15/15] ppc: Add aCube Sam460ex board

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Aug 24, 2017 at 11:37:09PM +0200, BALATON Zoltan wrote:
> On Thu, 24 Aug 2017, David Gibson wrote:
> > On Wed, Aug 23, 2017 at 01:12:06PM +0200, BALATON Zoltan wrote:
> > > On Wed, 23 Aug 2017, David Gibson wrote:
> > > > On Sun, Aug 20, 2017 at 07:23:05PM +0200, BALATON Zoltan wrote:
> > > > > Add emulation of aCube Sam460ex board based on AMCC 460EX embedded SoC.
> > > > > This is not a full implementation yet with a lot of components still
> > > > > missing but enough to start a Linux kernel and the U-Boot firmware.
> > > > > 
> > > > > Signed-off-by: François Revol <revol@free.fr>
> > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > 
> > > > As usual, only fairly superficial review here.
> > > > 
> > > > > ---
> > > > >  default-configs/ppcemb-softmmu.mak |   3 +
> > > > >  hw/ppc/Makefile.objs               |   2 +
> > > > >  hw/ppc/sam460ex.c                  | 611 +++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 616 insertions(+)
> > > > >  create mode 100644 hw/ppc/sam460ex.c
> > > > > 
> > > > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > > > index 635923a..90b42f0 100644
> > > > > --- a/default-configs/ppcemb-softmmu.mak
> > > > > +++ b/default-configs/ppcemb-softmmu.mak
> > > [...]
> > > > > +/*****************************************************************************/
> > > > > +/* SPD eeprom content from mips_malta.c */
> > > > 
> > > > What's the connection with mips_malta?
> > > 
> > > The board's firmware wants to see SPD EEPROMs of the connected memory while
> > > initialising the memory controller. This is why we need to implement SDRAM
> > > controller, I2C and SPD EEPROMs. MIPS malta board had already SPD EEPROM
> > > implementation so this is based on that. The comment just indicates where
> > > this code comes from.
> > 
> > Ok.
> > 
> > [snip]
> > > > > +        env->nip = bi->entry;
> > > > > +
> > > > > +        /* Create a mapping for the kernel.  */
> > > > > +        mmubooke_create_initial_mapping(env, 0, 0);
> > > > > +        env->gpr[6] = tswap32(EPAPR_MAGIC);
> > > > 
> > > > I'm pretty sure the tswap can't be right here.  env->gpr is in host
> > > > native order and I'd expect the constant to be as well.
> > > 
> > > I know nothing about this, maybe Francois remembers why it's there. But
> > > booting linux with -kernel works so it's probably either correct or does not
> > > matter.
> > 
> > Have you attempted it on both BE and LE hosts though?
> 
> No, I don't have a BE host, only tried on LE. I'm guessing this may come
> from hw/ppc/virtex_ml507.c where EPAPR_MAGIC is also swapped.

Sounds like virtex is wrong too - bet they haven't tried on a BE host either.

> The only other
> place this magic number appears is in e500 where it's not swapped though so
> I don't really know what should be correct here. In u-boot sources
> (arch/powerpc/lib/bootm.c) it seems to use this magic if CONFIG_OF_LIBFDT is
> defined which seems to be set for this board so that means we likely need
> this (but maybe not for the legacy uImages I've tried). Since CPU is big
> endian and constant is defined on the host this probably should be
> cpu_to_be32(EPAPR_MAGIC). Does that sound better?

No.  It's going into a register, not memory, and the registers are in
host-native order, since they're being manipulated as whole values,
not byte arrays (the appropriate swaps for the guest endianness happen
when emulating load and store instructions).  Remember an integer is
just an integer - it's only when you encode it into a byte array that
it acquires an endianness.

Although it sounded from another part of this thread as if this might
be a non-epapr boot anyway, making the whole thing moot.

> [...]
> > > > > +    /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
> > > > > +    memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram", 256 << 10,
> > > > > +                           &error_abort);
> > > > > +    memory_region_add_subregion(address_space_mem, 0x400000000LL, l2cache_ram);
> > > > > +
> > > > > +    /* USB */
> > > > > +    sysbus_create_simple(TYPE_PPC4xx_EHCI, 0x4bffd0400, uic[2][29]);
> > > > > +    dev = qdev_create(NULL, "sysbus-ohci");
> > > > > +    qdev_prop_set_string(dev, "masterbus", "usb-bus.0");
> > > > > +    qdev_prop_set_uint32(dev, "num-ports", 6);
> > > > > +    qdev_init_nofail(dev);
> > > > > +    sbdev = SYS_BUS_DEVICE(dev);
> > > > > +    sysbus_mmio_map(sbdev, 0, 0x4bffd0000);
> > > > > +    sysbus_connect_irq(sbdev, 0, uic[2][30]);
> > > > > +    usb_create_simple(usb_bus_find(-1), "usb-kbd");
> > > > > +    usb_create_simple(usb_bus_find(-1), "usb-mouse");
> > > > > +
> > > > > +    /* PCI bus */
> > > > > +    ppc460ex_pcie_init(env);
> > > > > +    /*XXX: FIXME: is this correct? */
> > > > > +    dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
> > > > > +                                uic[1][0], uic[1][20], uic[1][21], uic[1][22],
> > > > > +                                NULL);
> > > > > +    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
> > > > > +    if (!pci_bus) {
> > > > > +        fprintf(stderr, "couldn't create PCI controller!\n");
> > > > > +        exit(1);
> > > > > +    }
> > > > > +    memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(),
> > > > > +                             0, 0x10000);
> > > > > +    memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);
> > > > 
> > > > Does it really make sense to just embed the ISA IO space here, rather
> > > > than actually instanting a PCI<->ISA bridge?
> > > 
> > > I'm not sure if this is correct but I don't know how it's handled on real
> > > hardware. The board does not have ISA and I don't think it has a bridge but
> > > the IO space appears at this location according to the datasheet (In System
> > > Memory Address Map it's listed as PCI I/O 0xc08000000-0xc0800ffff) and
> > > clients expect PCI card's io registers to be accessible here. If anyone
> > > knows how it's done on real hardware and if there's a better way to model
> > > this please let me know.
> > 
> > Ah, ok.  I think the confusion here is that you can have PCI I/O space
> > without ISA or a system IO space.  In fact that's pretty standard on
> > things without a CPU level IO space (which means just about everything
> > except x86).
> > 
> > But in that case I'd expect the PCI host bridge to map its IO memory
> > regions directly into address_space_memory rather than involving the
> > global address_space_io (what get_system_io()) returns.  The only
> > reason I can see that you'd need to involve get_system_io() is if you
> > have devices registering themselves directly into the global IO space,
> > which should only happen for legacy ISA devices, which it sounds like
> > you don't have.
> > 
> > Possibly this is an error in the PCI bridge implementation that your
> > code here is essentially a workaround for, though.
> 
> So in my understanding, there's a system_io space created automatically
> which appears in the memory tree anyway but would otherwise be unused and
> this was just reused here for the pci io space so it does not need another
> memory region for this. If it's not acceptable this way (although
> ppc440_bamboo.c and ppc4xx_pci.c also does it the exact same way) an
> alternative may be to change it to add another mem region for io to
> ppc440_pcix.c (although it already has iomem for pci.reg so this might be
> more confusing than using system_io here) but I think pcix device can't map
> this to address_space_memory itself because this device could appear in
> different SoCs where the memory areas might be at different addresses so
> this new region should then be registered as a sysbus mmio space and be
> mapped from the board code with sysbus_mmio_map()? I find that much more
> confusing than how it's done now which is also more consistent with existing
> code modelling similar devices.

Ugh, I see.  I hadn't realized bamboo etc. were doing this.  It's not
a good idea - most obviously it breaks down if you have multiple
independent PCI host bridges, which have independent IO spaces.

I'm a bit torn whether to let this in as is because precedent and hope
to get them all fixed at some future point, or to insist that you
change it to stop the bad idea spreading.

The right way is, indeed, to create a new memory region for the
bridge's IO space.  A SysBusDevice can have multiple MMIO regions, so
you can use one for the bridge's memory space, another for its IO
space (and still more for config space and/or bridge internal
registers if you need them).

> > > > > +    /* PCI devices */
> > > > > +    pci_create_simple(pci_bus, PCI_DEVFN(6, 0), "sm501");
> > > > > +    /* SoC has a single SATA port but we don't emulate that yet
> > > > > +     * However, firmware and usual clients have driver for SiI311x
> > > > > +     * so add one for convenience by default */
> > > > > +    pci_create_simple(pci_bus, -1, "sii3112");
> > > > 
> > > > You should probably not create this when started with -nodefaults.
> > > 
> > > We don't emulate the on-board SATA port of the SoC and without this there's
> > > no other way to connect disks (maybe over USB, but firmware has a bug which
> > > prevents that too even on real hardware AFAIK, I've backported a fix which
> > > made booting from USB work but that broke keyboard) so while this is an
> > > external card it's pretty much unusable without this so it's added by
> > > default.
> > 
> > Yes, but you're not just adding it by default, you're adding it
> > *always*.  -nodefaults should turn that off (and the user will need to
> > manually instantiate it - or another disk controller, I guess).
> 
> OK I got it now. I still don't see when -nodefaults could be useful to
> cripple the board and make it easier to create non-working configurations
> but I can easily add this conditional around this line and hope users stay
> away from this switch and won't complain when it does not boot when they use
> it. (Well, it does not really boot most of the time without this switch
> either so I don't think it matters much at the moment :-) )

The convention exists because for non-trivial cases the next layer up
- whether it's a person or a management layer like livirt - often
needs more explicit and complete control of the device in the system.
Even if that's at the cost of having to explicitly add essential
devices.  It's still pretty imperfect, there's still a distinction
between "default" and "always present" devices which can be far from
obvious.

Nonetheless, I think the sil device lies firmly enough on the
"shouldn't be always present" side of the fuzzy line to exclude it
with -nodefaults.

Note that this can also be useful for experiments - e.g. just looking
at firmware behaviour with no disk at all, adding some entirely
different block interface with a specialized kernel to handle it.  Or
even booting into a kernel running purely from a ramfs with no disk.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]