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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 8 May 2019 19:22:53 +0800
Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

> On Tue, 30 Apr 2019 at 22:04, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Thu, 18 Apr 2019 at 05:05, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
> > >
> > > For the Aarch64, there is one machine 'virt', it is primarily meant to
> > > run on KVM and execute virtualization workloads, but we need an
> > > environment as faithful as possible to physical hardware, for supporting
> > > firmware and OS development for pysical Aarch64 machines.
> > >
> > > This patch introduces new machine type 'sbsa-ref' with main features:
> > >  - Based on 'virt' machine type.
> > >  - A new memory map.
> > >  - CPU type cortex-a57.
> > >  - EL2 and EL3 are enabled.
> > >  - GIC version 3.
> > >  - System bus AHCI controller.
> > >  - System bus EHCI controller.
> > >  - CDROM and hard disc on AHCI bus.
> > >  - E1000E ethernet card on PCIE bus.
> > >  - VGA display adaptor on PCIE bus.
> > >  - No virtio deivces.
> > >  - No fw_cfg device.
> > >  - No ACPI table supplied.
> > >  - Only minimal device tree nodes.
> > >
> > > Arm Trusted Firmware and UEFI porting to this are done accordingly, and
> > > it should supply ACPI tables to load OS, the minimal device tree nodes
> > > supplied from this platform are only to pass the dynamic info reflecting
> > > command line input to firmware, not for loading OS.
> > >
> > > To make the review easier, this task is split into two patches, the
> > > fundamental sceleton part and the peripheral devices part, this patch is
> > > the first part.
> > >
> > > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>
> >
> > Hi. This patch looks good to me. I have a couple of very minor
> > comments below.
> >
> > The only other thing I'm not sure about is whether the recent work
> > (both in master and in pending patchset) to add support for memory
> > hotplug and nvdimms to the 'virt' board is applicable here. I've
> > cc'd Igor and Eric to ask their opinion on that question.
> >
> My opinnion is, if we don't have conclusion before I send out next
> iteration, we can just abandon it at this time, currently from my side
> I don't see any reqirement for such features, what's more even if in
> the future we need it, we can still add it by seperate patch, maybe we
> probably have other features to be added in future too.

It should be possible to add nvdimm and memory hotplug later.
(I don't see big issues here, as long as it would be possible
to carve out continuous range in address space for it.

Considering ABI to guest, one could reuse QEMU's notification/enumeration
code for that or implement their own.

PS:
However there will be the same issues that we've had with Seabios when
ACPI tables were generated there (i.e. cease-less invention of interfaces
to to pass information from QEMU to firmware). But if I recall correctly,
it seems that it's intentional design decision for this board,
where user supplies/builds in firmware a board description (acpi/dt/...)
manually.

 
> > > +static const MemMapEntry sbsa_ref_memmap[] = {
> > > +    /* 512M boot ROM */
> > > +    [SBSA_FLASH] =              {          0, 0x20000000 },
> > > +    /* 512M secure memory */
> > > +    [SBSA_SECURE_MEM] =         { 0x20000000, 0x20000000 },
> > > +    /* Space reserved for CPU peripheral devices */
> > > +    [SBSA_CPUPERIPHS] =         { 0x40000000, 0x00040000 },
> > > +    [SBSA_GIC_DIST] =           { 0x40060000, 0x00010000 },
> > > +    [SBSA_GIC_REDIST] =         { 0x40080000, 0x04000000 },
> > > +    [SBSA_UART] =               { 0x60000000, 0x00001000 },
> > > +    [SBSA_RTC] =                { 0x60010000, 0x00001000 },
> > > +    [SBSA_GPIO] =               { 0x60020000, 0x00001000 },
> > > +    [SBSA_SECURE_UART] =        { 0x60030000, 0x00001000 },
> > > +    [SBSA_SECURE_UART_MM] =     { 0x60040000, 0x00001000 },
> > > +    [SBSA_SMMU] =               { 0x60050000, 0x00020000 },
> > > +    /* Space here reserved for more SMMUs */
> > > +    [SBSA_AHCI] =               { 0x60100000, 0x00010000 },
> > > +    [SBSA_EHCI] =               { 0x60110000, 0x00010000 },
> > > +    /* Space here reserved for other devices */
> > > +    [SBSA_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
> > > +    /* 32-bit address PCIE MMIO space */
> > > +    [SBSA_PCIE_MMIO] =          { 0x80000000, 0x70000000 },
> > > +    /* 256M PCIE ECAM space */
> > > +    [SBSA_PCIE_ECAM] =          { 0xf0000000, 0x10000000 },
> > > +    /* ~1TB PCIE MMIO space (4GB to 1024GB boundary) */
> > > +    [SBSA_PCIE_MMIO_HIGH] =     { 0x100000000ULL, 0xFF00000000ULL },
> > > +    [SBSA_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },
> > > +};
> > > +
> > > +static const int sbsa_ref_irqmap[] = {
> > > +    [SBSA_UART] = 1,
> > > +    [SBSA_RTC] = 2,
> > > +    [SBSA_PCIE] = 3, /* ... to 6 */
> > > +    [SBSA_GPIO] = 7,
> > > +    [SBSA_SECURE_UART] = 8,
> > > +    [SBSA_SECURE_UART_MM] = 9,
> > > +    [SBSA_AHCI] = 10,
> > > +    [SBSA_EHCI] = 11,
> > > +};
> >
> > Since we only have one memory map and one irqmap, I think that
> > rather than setting up vms->memmap[x] and vms->irqmap[x] and then
> > always using those, we should just refer directly to the globals.
> > The indirection in virt is originally because I was thinking we
> > might want to have more than one layout (and because the code
> > derives from the vexpress boards, which really do have two
> > different layouts depending on the version), and then it turned
> > out to be useful that we could pass the VirtMachineState* to
> > the ACPI table generation code and let it get at the addresses
> > and IRQ numbers that way. But neither of those applies here, I think.
> >
> Yes, good.
> 
> > > +
> > > +static void sbsa_ref_init(MachineState *machine)
> > > +{
> > > +    SBSAMachineState *vms = SBSA_MACHINE(machine);
> >
> > This is a very trivial nitpick, but I think we should call
> > this variable 'sms', not 'vms', since we changed the name of
> > the struct type. (Same applies in some other functions later.)
> >
> Good catch, this is not 'trivial' I think, they should be changed obviously.
> 
> > > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    MemoryRegion *sysmem = get_system_memory();
> > > +    MemoryRegion *secure_sysmem = NULL;
> > > +    MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > +    bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> > > +    const CPUArchIdList *possible_cpus;
> > > +    int n, sbsa_max_cpus;
> > > +
> > > +    if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a57"))) {
> > > +        error_report("sbsa-ref: CPU type other than the built-in "
> > > +                     "cortex-a57 not supported");
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (kvm_enabled()) {
> > > +        error_report("sbsa-ref: KVM is not supported at this machine");
> >
> > "for this machine".
> >
> > > +        exit(1);
> > > +    }
> >
> > thanks
> > -- PMM