Re: [Qemu-devel] [PATCH v7 2/2] hw/arm: Add arm SBSA reference machine, devices part

[Radoslaw Biernacki]

On Wed, 8 May 2019 at 13:30, Hongbo Zhang <address@hidden> wrote:

> On Tue, 30 Apr 2019 at 22:17, Peter Maydell <address@hidden>
> wrote:
> >
> > On Thu, 18 Apr 2019 at 05:05, Hongbo Zhang <address@hidden>
> wrote:
> > >
> > > Following the previous patch, this patch adds peripheral devices to the
> > > newly introduced SBSA-ref machine.
> > >
> > > Signed-off-by: Hongbo Zhang <address@hidden>
> > > ---
> > >  hw/arm/sbsa-ref.c | 451
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 451 insertions(+)
> >
> > Some fairly minor comments on this one.
> >
> > > +static void create_flash(const SBSAMachineState *vms,
> > > +                         MemoryRegion *sysmem,
> > > +                         MemoryRegion *secure_sysmem)
> > > +{
> > > +    /*
> > > +     * Create one secure and nonsecure flash devices to fill
> SBSA_FLASH
> > > +     * space in the memmap, file passed via -bios goes in the first
> one.
> > > +     */
> > > +    hwaddr flashsize = vms->memmap[SBSA_FLASH].size / 2;
> > > +    hwaddr flashbase = vms->memmap[SBSA_FLASH].base;
> > > +
> > > +    create_one_flash("sbsa-ref.flash0", flashbase, flashsize,
> > > +                     bios_name, secure_sysmem);
> > > +    create_one_flash("sbsa-ref.flash1", flashbase + flashsize,
> flashsize,
> > > +                     NULL, sysmem);
> > > +}
> >
> > I think Markus might have an opinion on the best way to create
> > flash devices on a new board model. Is "just create two flash
> > devices the way the virt board does" the right thing?
> >
> For the firmware part, we are using two flashes, one secure and
> another non-secure.
>
> > > +static void create_ahci(const SBSAMachineState *vms, qemu_irq *pic)
> > > +{
> > > +    hwaddr base = vms->memmap[SBSA_AHCI].base;
> > > +    int irq = vms->irqmap[SBSA_AHCI];
> > > +    DeviceState *dev;
> > > +    DriveInfo *hd[NUM_SATA_PORTS];
> > > +    SysbusAHCIState *sysahci;
> > > +    AHCIState *ahci;
> > > +    int i;
> > > +
> > > +    dev = qdev_create(NULL, "sysbus-ahci");
> > > +    qdev_prop_set_uint32(dev, "num-ports", NUM_SATA_PORTS);
> > > +    qdev_init_nofail(dev);
> > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> > > +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]);
> > > +
> > > +    sysahci = SYSBUS_AHCI(dev);
> > > +    ahci = &sysahci->ahci;
> > > +    ide_drive_get(hd, ARRAY_SIZE(hd));
> > > +    for (i = 0; i < ahci->ports; i++) {
> > > +        if (hd[i] == NULL) {
> > > +            continue;
> > > +        }
> > > +        ide_create_drive(&ahci->dev[i].port, 0, hd[i]);
> > > +    }
> > > +}
> > > +
> > > +static void create_ehci(const SBSAMachineState *vms, qemu_irq *pic)
> > > +{
> > > +    hwaddr base = vms->memmap[SBSA_EHCI].base;
> > > +    int irq = vms->irqmap[SBSA_EHCI];
> > > +    USBBus *usb_bus;
> > > +
> > > +    sysbus_create_simple("platform-ehci-usb", base, pic[irq]);
> > > +
> > > +    usb_bus = usb_bus_find(-1);
> > > +    usb_create_simple(usb_bus, "usb-kbd");
> > > +    usb_create_simple(usb_bus, "usb-mouse");
> >
> > I don't think we should automatically create the usb keyboard
> > and mouse devices. The user can do it on the command line if they
> > want them.
> >
> OK.
>

Actually I need to rise an objection to this one.
As we trying to make SBSA machine as close as possible to real machine, we
should have keyboard and mouse.
Those have the same requirement as for VGA. It's just an expected piece of
HW when you for e.g. installing a server.
We also do a lot of FW work so it is expected to have keyboard (and even
mouse) in UEFI.


>
> > >  static void sbsa_ref_init(MachineState *machine)
> > >  {
> > >      SBSAMachineState *vms = SBSA_MACHINE(machine);
> > > @@ -125,6 +552,7 @@ static void sbsa_ref_init(MachineState *machine)
> > >      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> > >      const CPUArchIdList *possible_cpus;
> > >      int n, sbsa_max_cpus;
> > > +    qemu_irq pic[NUM_IRQS];
> > >
> > >      if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a57"))) {
> > >          error_report("sbsa-ref: CPU type other than the built-in "
> > > @@ -209,11 +637,34 @@ static void sbsa_ref_init(MachineState *machine)
> > >                                           machine->ram_size);
> > >      memory_region_add_subregion(sysmem, vms->memmap[SBSA_MEM].base,
> ram);
> > >
> > > +    create_fdt(vms);
> > > +
> > > +    create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
> > > +
> > > +    create_secure_ram(vms, secure_sysmem);
> > > +
> > > +    create_gic(vms, pic);
> > > +
> > > +    create_uart(vms, pic, SBSA_UART, sysmem, serial_hd(0));
> > > +    create_uart(vms, pic, SBSA_SECURE_UART, secure_sysmem,
> serial_hd(1));
> > > +    create_uart(vms, pic, SBSA_SECURE_UART_MM, secure_sysmem,
> serial_hd(2));
> >
> > What's the third UART for (ie what is the name intended to mean)?
> > Should we have more than one non-secure UART?
> >
> Yes, this is called " Standalone Management Mode", I will add comment
> for it, this is needed by server RAS feature.
>
> > thanks
> > -- PMM
>