Re: [Qemu-devel] [PATCH] hw/arm: Add SBSA machine type

[Andrew Jones]

On Thu, Jun 28, 2018 at 04:11:56PM +0800, Hongbo Zhang wrote:
> On 27 June 2018 at 22:56, Igor Mammedov <address@hidden> wrote:
> > On Wed, 27 Jun 2018 18:13:08 +0800
> > Hongbo Zhang <address@hidden> wrote:
> >
> >> This patch introduces a new Arm machine type 'SBSA' with features:
> >>  - Based on legacy 'virt' machine type.
> >>  - Newly designed memory map.
> >>  - EL2 and EL3 are enabled by default.
> >>  - AHCI controller attached to system bus, and then CDROM and hard disc
> >>    can be added to it.
> >>  - EHCI controller attached to system bus, with USB mouse and key board
> >>    installed by default.
> >>  - E1000 ethernet card on PCIE bus.
> >>  - VGA display adaptor on PCIE bus.
> >>  - Default CPU type cortex-a57, 4 cores, and 1G bytes memory.
> >>  - No virtio functions enabled, since this is to emulate real hardware.
> > I'd drop all default (convenience) devices unless they are specified in 
> > SBSA,
> > and make management explicitly provide needed devices on CLI.
> >
> Thanks for review.
> 
> I mentioned these default values because they are different from the
> 'virt' machine from which this sbsa machine derives. (sbsa has uart
> too, it is same with 'virt' PL011).
> Qemu implementation should has such default values, if the sbsa
> machine does not offer them, they fallback to the parent machine's.

QEMU's default devices is one of the problems with QEMU. Any serious
use of QEMU must be done with '-nodefaults' and then explicit devices.
To avoid requiring long command lines QEMU supports '-readconfig'. We
already have two mach-virt configs that can be used as a basis for a
new SBSA config. See docs/config/mach-virt-graphical.cfg and
docs/config/mach-virt-serial.cfg.

> The parent 'virt' machine's default cpu type is cortext-a15, that is
> not proper for a armv8 server, and its default memory is 128M, that is
> not enough to load uefi, even 512M cannot either, so I set the sbsa
> defualt minimal memory to 1G.

config files support setting a memory size, but unfortunately not the
CPU type. I'm not sure why it doesn't. Maybe readconfig can be taught
to do so?

> 
> The core numbers, is a new default. Server is smp in common sense,
> single core isn't like a server, so some even want to set to max
> capability as default, but this isn't good either I think, because
> gicv3 memory space in Qemu can even support more than 200 cores, that
> is too much.

Core numbers can be managed with config
[smp-opts]
  cpus = "4"

> (gicv2 currently support 8 cores, but there are issues to boot smp for
> both gicv2 and gicv3 now, it should be another thread)
> 
> >>
> >> This is the prototype, more features can be added in futrue.
> >>
> >> Purpose of this is to have a standard QEMU platform for Arm firmware
> >> developement etc. where the legacy machines cannot meets requirements.
> >>
> >> Arm Trusted Firmware and UEFI porting to this are done seperately.
> >>
> >> Signed-off-by: Hongbo Zhang <address@hidden>
> >> ---
> > [...]
> >
> >> @@ -94,6 +98,8 @@
> >>
> >>  #define PLATFORM_BUS_NUM_IRQS 64
> >>
> >> +#define SATA_NUM_PORTS 6
> >> +
> >>  /* RAM limit in GB. Since VIRT_MEM starts at the 1GB mark, this means
> >>   * RAM can go up to the 256GB mark, leaving 256GB of the physical
> >>   * address space unallocated and free for future use between 256G and 
> >> 512G.
> >> @@ -168,6 +174,47 @@ static const int a15irqmap[] = {
> >>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
> >>  };
> >>
> >> +static const MemMapEntry sbsa_memmap[] = {
> >> +    /* Space up to 0x8000000 is reserved for a boot ROM */
> >> +    [VIRT_FLASH] =              {          0, 0x08000000 },
> >> +    [VIRT_CPUPERIPHS] =         { 0x08000000, 0x00020000 },
> >> +    /* GIC distributor and CPU interfaces sit inside the CPU peripheral 
> >> space */
> >> +    [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
> >> +    [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
> >> +    [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> >> +    /* The space in between here is reserved for GICv3 CPU/vCPU/HYP */
> >> +    [VIRT_GIC_ITS] =            { 0x08080000, 0x00020000 },
> >> +    /* This redistributor space allows up to 2*64kB*123 CPUs */
> >> +    [VIRT_GIC_REDIST] =         { 0x080A0000, 0x00F60000 },
> >> +    [VIRT_UART] =               { 0x09000000, 0x00001000 },
> >> +    [VIRT_RTC] =                { 0x09010000, 0x00001000 },
> >> +    [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
> >> +    [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> >> +    [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> >> +    [VIRT_AHCI] =               { 0x09050000, 0x00010000 },
> >> +    [VIRT_EHCI] =               { 0x09060000, 0x00010000 },
> >> +    [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> >> +    [VIRT_SECURE_MEM] =         { 0x0e000000, 0x01000000 },
> >> +    [VIRT_PCIE_MMIO] =          { 0x10000000, 0x7fff0000 },
> >> +    [VIRT_PCIE_PIO] =           { 0x8fff0000, 0x00010000 },
> >> +    [VIRT_PCIE_ECAM] =          { 0x90000000, 0x10000000 },
> >> +    /* Second PCIe window, 508GB wide at the 4GB boundary */
> >> +    [VIRT_PCIE_MMIO_HIGH] =     { 0x100000000ULL, 0x7F00000000ULL },
> >> +    [VIRT_MEM] =                { 0x8000000000ULL, RAMLIMIT_BYTES },
> > does spec require support for 32-bit guest or is it only 64bit,
> > if the later I'd put it somewhere high where we can increase region
> > to terrabytes.
> >
> Spec only requires it to be compliant with armv8.
> Designed purpose of this is to run 64bit guests, because in practice
> an arm server is usually 64bit.

I agree with Igor that if we're going to add a new machine type that is
only for 64bit, then we shouldn't base our memory map on mach-virt's,
but rather create a new one taking advantage of a much larger address
space. Firmware would need to start taking the base of RAM from the
DTB, and the address of the DTB from x0, but it probably should anyway.

> 
> > another idea that was floating around (considering we don't care about 
> > legacy)
> > is to use flexible base address and tell firmware[*] via register where 
> > it's.
> > Then it would be able to support 32 guest with small amount of RAM
> > and 64 bit guests with huge amount of RAM using a single memory range.
> > (to keep memory management simple). It's also future friendly, as in case
> > if we need to move it we could do easily by changing base address for new 
> > machine
> > type only and firmware would automatically pick it up from register
> > (without all compat nightmare we have with 2 regions in pc/q35 machines).
> >
> > [*] When I've talked with Laszlo about it he said it was not easy to do in 
> > uefi
> > but still possible.
> Sounds not easy.
> I think the first step is to get this merged, and then we can add more
> features if necessary.
> I've already seen some points in trusted firmware and uefi need to be fixed.

It sounds to me like the current mach-virt machine type, with an SBSA
readconfig file would be sufficient for now, and that we should only
merge a new machine type when firmware supports a dynamic RAM base,
allowing the new machine type to have quite a different memory map.

> 
> >
> > same applies to GIG regions/mmio/ecam where we are twisting our hands when 
> > trying to
> > increase number of things.
> >
> > [...]
> >
>

Thanks,
drew