diff mbox series

[v5,1/2] hw/arm: Add arm SBSA reference machine, skeleton part

Message ID 1544173675-14217-2-git-send-email-hongbo.zhang@linaro.org
State New
Headers show
Series Add arm SBSA reference machine | expand

Commit Message

Hongbo Zhang Dec. 7, 2018, 9:07 a.m. UTC
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.
 - Re-designed memory map.
 - CPU type cortex-a57.
 - EL2 and EL3 are enabled.
 - GIC version 3.
 - System bus AHCI controller.
 - System bus XHCI controller(TBD).
 - 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>

---
 hw/arm/Makefile.objs  |   2 +-
 hw/arm/sbsa-ref.c     | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |   1 +
 3 files changed, 279 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/sbsa-ref.c

-- 
2.7.4

Comments

Peter Maydell Jan. 22, 2019, 11:41 a.m. UTC | #1
On Fri, 7 Dec 2018 at 09:08, 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.

>  - Re-designed memory map.

>  - CPU type cortex-a57.

>  - EL2 and EL3 are enabled.

>  - GIC version 3.

>  - System bus AHCI controller.

>  - System bus XHCI controller(TBD).

>  - 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.


Firstly, apologies for this having sat on my to-review queue for
so long...

> Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>

> ---

>  hw/arm/Makefile.objs  |   2 +-

>  hw/arm/sbsa-ref.c     | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++

>  include/hw/arm/virt.h |   1 +

>  3 files changed, 279 insertions(+), 1 deletion(-)

>  create mode 100644 hw/arm/sbsa-ref.c

>

> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs

> index d51fcec..a8895eb 100644

> --- a/hw/arm/Makefile.objs

> +++ b/hw/arm/Makefile.objs

> @@ -1,4 +1,4 @@

> -obj-y += boot.o virt.o sysbus-fdt.o

> +obj-y += boot.o virt.o sbsa-ref.o sysbus-fdt.o

>  obj-$(CONFIG_ACPI) += virt-acpi-build.o

>  obj-$(CONFIG_DIGIC) += digic_boards.o

>  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o

> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c

> new file mode 100644

> index 0000000..1d6252b

> --- /dev/null

> +++ b/hw/arm/sbsa-ref.c

> @@ -0,0 +1,277 @@

> +/*

> + * ARM SBSA Reference Platform emulation

> + *

> + * Copyright (c) 2018 Linaro Limited

> + * Written by Hongbo Zhang <hongbo.zhang@linaro.org>

> + *

> + * Based on hw/arm/virt.c

> + *

> + * This program is free software; you can redistribute it and/or modify it

> + * under the terms and conditions of the GNU General Public License,

> + * version 2 or later, as published by the Free Software Foundation.

> + *

> + * This program is distributed in the hope it will be useful, but WITHOUT

> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or

> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for

> + * more details.

> + *

> + * You should have received a copy of the GNU General Public License along with

> + * this program.  If not, see <http://www.gnu.org/licenses/>.

> + */

> +

> +#include "qemu/osdep.h"

> +#include "qapi/error.h"

> +#include "hw/arm/arm.h"

> +#include "hw/arm/virt.h"


This isn't the virt board, so I think it would be better if it did
not include the virt.h header or use structures/classes that are
private to the virt board. It should be its own thing.

> +#include "hw/devices.h"

> +#include "net/net.h"

> +#include "sysemu/device_tree.h"

> +#include "sysemu/numa.h"

> +#include "sysemu/sysemu.h"

> +#include "hw/loader.h"

> +#include "exec/address-spaces.h"

> +#include "qemu/error-report.h"

> +#include "hw/pci-host/gpex.h"

> +#include "hw/arm/sysbus-fdt.h"

> +#include "hw/arm/fdt.h"

> +#include "hw/intc/arm_gic.h"

> +#include "hw/intc/arm_gicv3_common.h"

> +#include "kvm_arm.h"

> +#include "hw/ide/internal.h"

> +#include "hw/ide/ahci_internal.h"

> +#include "qemu/units.h"

> +

> +#define NUM_IRQS 256

> +

> +#define RAMLIMIT_GB 8192

> +#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)

> +

> +static const MemMapEntry sbsa_ref_memmap[] = {

> +    /* 512M boot ROM */

> +    [VIRT_FLASH] =              {          0, 0x20000000 },

> +    /* 512M secure memery */


"memory"

> +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> +    /* GIC distributor and CPU interface expansion spaces reserved */

> +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },


If they're just reserved you don't really need to list them here,
as they're covered by the VIRT_CPUPERIPHS space anyway. (You
don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

> +    /* 64M redistributor space allows up to 512 CPUs */

> +    [VIRT_GIC_REDIST] =         { 0x40080000, 0x04000000 },

> +    /* Space here reserved for redistributor and vCPU/HYP expansion */

> +    [VIRT_UART] =               { 0x60000000, 0x00001000 },

> +    [VIRT_RTC] =                { 0x60010000, 0x00001000 },

> +    [VIRT_GPIO] =               { 0x60020000, 0x00001000 },

> +    [VIRT_SECURE_UART] =        { 0x60030000, 0x00001000 },

> +    [VIRT_SMMU] =               { 0x60040000, 0x00020000 },

> +    /* Space here reserved for more SMMUs */

> +    [VIRT_AHCI] =               { 0x60100000, 0x00010000 },

> +    /* Space here reserved for other devices */

> +    [VIRT_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },

> +    /* 256M PCIE ECAM space */

> +    [VIRT_PCIE_ECAM] =          { 0x80000000, 0x10000000 },


Comment says 256M but the size field says it's larger...

> +    /* ~1TB for PCIE MMIO (4GB to 1024GB boundary) */

> +    [VIRT_PCIE_MMIO] =          { 0x100000000ULL, 0xFF00000000ULL },

> +    [VIRT_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },

> +};

> +

> +static const int sbsa_ref_irqmap[] = {

> +    [VIRT_UART] = 1,

> +    [VIRT_RTC] = 2,

> +    [VIRT_PCIE] = 3, /* ... to 6 */

> +    [VIRT_GPIO] = 7,

> +    [VIRT_SECURE_UART] = 8,

> +    [VIRT_AHCI] = 9,

> +};

> +

> +static void sbsa_ref_init(MachineState *machine)

> +{

> +    VirtMachineState *vms = VIRT_MACHINE(machine);


As noted above, I think it would be better to be your own
subclass of MachineState, rather than being a subclass of
the virt board. Is there anything that becomes particularly
awkward if you do it that way?

thanks
-- PMM
Hongbo Zhang Jan. 28, 2019, 10:16 a.m. UTC | #2
On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Fri, 7 Dec 2018 at 09:08, 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.

> >  - Re-designed memory map.

> >  - CPU type cortex-a57.

> >  - EL2 and EL3 are enabled.

> >  - GIC version 3.

> >  - System bus AHCI controller.

> >  - System bus XHCI controller(TBD).

> >  - 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.

>

> Firstly, apologies for this having sat on my to-review queue for

> so long...

>

Understand, it is OK, thanks for review.

> > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>

> > ---

> >  hw/arm/Makefile.objs  |   2 +-

> >  hw/arm/sbsa-ref.c     | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++

> >  include/hw/arm/virt.h |   1 +

> >  3 files changed, 279 insertions(+), 1 deletion(-)

> >  create mode 100644 hw/arm/sbsa-ref.c

> >

> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs

> > index d51fcec..a8895eb 100644

> > --- a/hw/arm/Makefile.objs

> > +++ b/hw/arm/Makefile.objs

> > @@ -1,4 +1,4 @@

> > -obj-y += boot.o virt.o sysbus-fdt.o

> > +obj-y += boot.o virt.o sbsa-ref.o sysbus-fdt.o

> >  obj-$(CONFIG_ACPI) += virt-acpi-build.o

> >  obj-$(CONFIG_DIGIC) += digic_boards.o

> >  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o

> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c

> > new file mode 100644

> > index 0000000..1d6252b

> > --- /dev/null

> > +++ b/hw/arm/sbsa-ref.c

> > @@ -0,0 +1,277 @@

> > +/*

> > + * ARM SBSA Reference Platform emulation

> > + *

> > + * Copyright (c) 2018 Linaro Limited

> > + * Written by Hongbo Zhang <hongbo.zhang@linaro.org>

> > + *

> > + * Based on hw/arm/virt.c

> > + *

> > + * This program is free software; you can redistribute it and/or modify it

> > + * under the terms and conditions of the GNU General Public License,

> > + * version 2 or later, as published by the Free Software Foundation.

> > + *

> > + * This program is distributed in the hope it will be useful, but WITHOUT

> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or

> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for

> > + * more details.

> > + *

> > + * You should have received a copy of the GNU General Public License along with

> > + * this program.  If not, see <http://www.gnu.org/licenses/>.

> > + */

> > +

> > +#include "qemu/osdep.h"

> > +#include "qapi/error.h"

> > +#include "hw/arm/arm.h"

> > +#include "hw/arm/virt.h"

>

> This isn't the virt board, so I think it would be better if it did

> not include the virt.h header or use structures/classes that are

> private to the virt board. It should be its own thing.

>

> > +#include "hw/devices.h"

> > +#include "net/net.h"

> > +#include "sysemu/device_tree.h"

> > +#include "sysemu/numa.h"

> > +#include "sysemu/sysemu.h"

> > +#include "hw/loader.h"

> > +#include "exec/address-spaces.h"

> > +#include "qemu/error-report.h"

> > +#include "hw/pci-host/gpex.h"

> > +#include "hw/arm/sysbus-fdt.h"

> > +#include "hw/arm/fdt.h"

> > +#include "hw/intc/arm_gic.h"

> > +#include "hw/intc/arm_gicv3_common.h"

> > +#include "kvm_arm.h"

> > +#include "hw/ide/internal.h"

> > +#include "hw/ide/ahci_internal.h"

> > +#include "qemu/units.h"

> > +

> > +#define NUM_IRQS 256

> > +

> > +#define RAMLIMIT_GB 8192

> > +#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)

> > +

> > +static const MemMapEntry sbsa_ref_memmap[] = {

> > +    /* 512M boot ROM */

> > +    [VIRT_FLASH] =              {          0, 0x20000000 },

> > +    /* 512M secure memery */

>

> "memory"

>

Oops :(

> > +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> > +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> > +    /* GIC distributor and CPU interface expansion spaces reserved */

> > +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> > +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },

>

> If they're just reserved you don't really need to list them here,

> as they're covered by the VIRT_CPUPERIPHS space anyway. (You

> don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

>

Yes, this can be removed.

> > +    /* 64M redistributor space allows up to 512 CPUs */

> > +    [VIRT_GIC_REDIST] =         { 0x40080000, 0x04000000 },

> > +    /* Space here reserved for redistributor and vCPU/HYP expansion */

> > +    [VIRT_UART] =               { 0x60000000, 0x00001000 },

> > +    [VIRT_RTC] =                { 0x60010000, 0x00001000 },

> > +    [VIRT_GPIO] =               { 0x60020000, 0x00001000 },

> > +    [VIRT_SECURE_UART] =        { 0x60030000, 0x00001000 },

> > +    [VIRT_SMMU] =               { 0x60040000, 0x00020000 },

> > +    /* Space here reserved for more SMMUs */

> > +    [VIRT_AHCI] =               { 0x60100000, 0x00010000 },

> > +    /* Space here reserved for other devices */

> > +    [VIRT_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },

> > +    /* 256M PCIE ECAM space */

> > +    [VIRT_PCIE_ECAM] =          { 0x80000000, 0x10000000 },

>

> Comment says 256M but the size field says it's larger...

>

I calculated, 256M should be 0x10000000, 7 zeros.

> > +    /* ~1TB for PCIE MMIO (4GB to 1024GB boundary) */

> > +    [VIRT_PCIE_MMIO] =          { 0x100000000ULL, 0xFF00000000ULL },

> > +    [VIRT_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },

> > +};

> > +

> > +static const int sbsa_ref_irqmap[] = {

> > +    [VIRT_UART] = 1,

> > +    [VIRT_RTC] = 2,

> > +    [VIRT_PCIE] = 3, /* ... to 6 */

> > +    [VIRT_GPIO] = 7,

> > +    [VIRT_SECURE_UART] = 8,

> > +    [VIRT_AHCI] = 9,

> > +};

> > +

> > +static void sbsa_ref_init(MachineState *machine)

> > +{

> > +    VirtMachineState *vms = VIRT_MACHINE(machine);

>

> As noted above, I think it would be better to be your own

> subclass of MachineState, rather than being a subclass of

> the virt board. Is there anything that becomes particularly

> awkward if you do it that way?

>

Just wanted to save code lines since this board derives from virt.
It is fine to have its own header file, will do that.

> thanks

> -- PMM
Peter Maydell Jan. 28, 2019, 10:42 a.m. UTC | #3
On Mon, 28 Jan 2019 at 10:16, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>

> On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:

> >

> > On Fri, 7 Dec 2018 at 09:08, 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.

> > >  - Re-designed memory map.

> > >  - CPU type cortex-a57.

> > >  - EL2 and EL3 are enabled.

> > >  - GIC version 3.

> > >  - System bus AHCI controller.

> > >  - System bus XHCI controller(TBD).

> > >  - 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.

> >

> > Firstly, apologies for this having sat on my to-review queue for

> > so long...

> >

> Understand, it is OK, thanks for review.

>

> > > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>

> > > ---

> > >  hw/arm/Makefile.objs  |   2 +-

> > >  hw/arm/sbsa-ref.c     | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++

> > >  include/hw/arm/virt.h |   1 +

> > >  3 files changed, 279 insertions(+), 1 deletion(-)

> > >  create mode 100644 hw/arm/sbsa-ref.c

> > >

> > > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs

> > > index d51fcec..a8895eb 100644

> > > --- a/hw/arm/Makefile.objs

> > > +++ b/hw/arm/Makefile.objs

> > > @@ -1,4 +1,4 @@

> > > -obj-y += boot.o virt.o sysbus-fdt.o

> > > +obj-y += boot.o virt.o sbsa-ref.o sysbus-fdt.o

> > >  obj-$(CONFIG_ACPI) += virt-acpi-build.o

> > >  obj-$(CONFIG_DIGIC) += digic_boards.o

> > >  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o

> > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c

> > > new file mode 100644

> > > index 0000000..1d6252b

> > > --- /dev/null

> > > +++ b/hw/arm/sbsa-ref.c

> > > @@ -0,0 +1,277 @@

> > > +/*

> > > + * ARM SBSA Reference Platform emulation

> > > + *

> > > + * Copyright (c) 2018 Linaro Limited

> > > + * Written by Hongbo Zhang <hongbo.zhang@linaro.org>

> > > + *

> > > + * Based on hw/arm/virt.c

> > > + *

> > > + * This program is free software; you can redistribute it and/or modify it

> > > + * under the terms and conditions of the GNU General Public License,

> > > + * version 2 or later, as published by the Free Software Foundation.

> > > + *

> > > + * This program is distributed in the hope it will be useful, but WITHOUT

> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or

> > > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for

> > > + * more details.

> > > + *

> > > + * You should have received a copy of the GNU General Public License along with

> > > + * this program.  If not, see <http://www.gnu.org/licenses/>.

> > > + */

> > > +

> > > +#include "qemu/osdep.h"

> > > +#include "qapi/error.h"

> > > +#include "hw/arm/arm.h"

> > > +#include "hw/arm/virt.h"

> >

> > This isn't the virt board, so I think it would be better if it did

> > not include the virt.h header or use structures/classes that are

> > private to the virt board. It should be its own thing.

> >

> > > +#include "hw/devices.h"

> > > +#include "net/net.h"

> > > +#include "sysemu/device_tree.h"

> > > +#include "sysemu/numa.h"

> > > +#include "sysemu/sysemu.h"

> > > +#include "hw/loader.h"

> > > +#include "exec/address-spaces.h"

> > > +#include "qemu/error-report.h"

> > > +#include "hw/pci-host/gpex.h"

> > > +#include "hw/arm/sysbus-fdt.h"

> > > +#include "hw/arm/fdt.h"

> > > +#include "hw/intc/arm_gic.h"

> > > +#include "hw/intc/arm_gicv3_common.h"

> > > +#include "kvm_arm.h"

> > > +#include "hw/ide/internal.h"

> > > +#include "hw/ide/ahci_internal.h"

> > > +#include "qemu/units.h"

> > > +

> > > +#define NUM_IRQS 256

> > > +

> > > +#define RAMLIMIT_GB 8192

> > > +#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)

> > > +

> > > +static const MemMapEntry sbsa_ref_memmap[] = {

> > > +    /* 512M boot ROM */

> > > +    [VIRT_FLASH] =              {          0, 0x20000000 },

> > > +    /* 512M secure memery */

> >

> > "memory"

> >

> Oops :(

>

> > > +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> > > +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> > > +    /* GIC distributor and CPU interface expansion spaces reserved */

> > > +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> > > +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },

> >

> > If they're just reserved you don't really need to list them here,

> > as they're covered by the VIRT_CPUPERIPHS space anyway. (You

> > don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

> >

> Yes, this can be removed.

>

> > > +    /* 64M redistributor space allows up to 512 CPUs */

> > > +    [VIRT_GIC_REDIST] =         { 0x40080000, 0x04000000 },

> > > +    /* Space here reserved for redistributor and vCPU/HYP expansion */

> > > +    [VIRT_UART] =               { 0x60000000, 0x00001000 },

> > > +    [VIRT_RTC] =                { 0x60010000, 0x00001000 },

> > > +    [VIRT_GPIO] =               { 0x60020000, 0x00001000 },

> > > +    [VIRT_SECURE_UART] =        { 0x60030000, 0x00001000 },

> > > +    [VIRT_SMMU] =               { 0x60040000, 0x00020000 },

> > > +    /* Space here reserved for more SMMUs */

> > > +    [VIRT_AHCI] =               { 0x60100000, 0x00010000 },

> > > +    /* Space here reserved for other devices */

> > > +    [VIRT_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },

> > > +    /* 256M PCIE ECAM space */

> > > +    [VIRT_PCIE_ECAM] =          { 0x80000000, 0x10000000 },

> >

> > Comment says 256M but the size field says it's larger...

> >

> I calculated, 256M should be 0x10000000, 7 zeros.

>

> > > +    /* ~1TB for PCIE MMIO (4GB to 1024GB boundary) */

> > > +    [VIRT_PCIE_MMIO] =          { 0x100000000ULL, 0xFF00000000ULL },

> > > +    [VIRT_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },

> > > +};

> > > +

> > > +static const int sbsa_ref_irqmap[] = {

> > > +    [VIRT_UART] = 1,

> > > +    [VIRT_RTC] = 2,

> > > +    [VIRT_PCIE] = 3, /* ... to 6 */

> > > +    [VIRT_GPIO] = 7,

> > > +    [VIRT_SECURE_UART] = 8,

> > > +    [VIRT_AHCI] = 9,

> > > +};

> > > +

> > > +static void sbsa_ref_init(MachineState *machine)

> > > +{

> > > +    VirtMachineState *vms = VIRT_MACHINE(machine);

> >

> > As noted above, I think it would be better to be your own

> > subclass of MachineState, rather than being a subclass of

> > the virt board. Is there anything that becomes particularly

> > awkward if you do it that way?

> >

> Just wanted to save code lines since this board derives from virt.

> It is fine to have its own header file, will do that.

>

> > thanks

> > -- PMM




-- 
12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8
Peter Maydell Jan. 28, 2019, 10:43 a.m. UTC | #4
On Mon, 28 Jan 2019 at 10:16, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>

> On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:

> > Comment says 256M but the size field says it's larger...

> >

> I calculated, 256M should be 0x10000000, 7 zeros.


Yes, you're right, this was my mistake.

PS: sorry about the other blank mail, the gmail interface seems
to have changed somehow and is making it easier to accidentally
send a no-new-content email :-(

thanks
-- PMM
Hongbo Zhang Jan. 30, 2019, 8:59 a.m. UTC | #5
On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Fri, 7 Dec 2018 at 09:08, 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.

> >  - Re-designed memory map.

> >  - CPU type cortex-a57.

> >  - EL2 and EL3 are enabled.

> >  - GIC version 3.

> >  - System bus AHCI controller.

> >  - System bus XHCI controller(TBD).

> >  - 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.

>

> Firstly, apologies for this having sat on my to-review queue for

> so long...

>

> > Signed-off-by: Hongbo Zhang <hongbo.zhang@linaro.org>

> > ---

> >  hw/arm/Makefile.objs  |   2 +-

> >  hw/arm/sbsa-ref.c     | 277 ++++++++++++++++++++++++++++++++++++++++++++++++++

> >  include/hw/arm/virt.h |   1 +

> >  3 files changed, 279 insertions(+), 1 deletion(-)

> >  create mode 100644 hw/arm/sbsa-ref.c

> >

> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs

> > index d51fcec..a8895eb 100644

> > --- a/hw/arm/Makefile.objs

> > +++ b/hw/arm/Makefile.objs

> > @@ -1,4 +1,4 @@

> > -obj-y += boot.o virt.o sysbus-fdt.o

> > +obj-y += boot.o virt.o sbsa-ref.o sysbus-fdt.o

> >  obj-$(CONFIG_ACPI) += virt-acpi-build.o

> >  obj-$(CONFIG_DIGIC) += digic_boards.o

> >  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o

> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c

> > new file mode 100644

> > index 0000000..1d6252b

> > --- /dev/null

> > +++ b/hw/arm/sbsa-ref.c

> > @@ -0,0 +1,277 @@

> > +/*

> > + * ARM SBSA Reference Platform emulation

> > + *

> > + * Copyright (c) 2018 Linaro Limited

> > + * Written by Hongbo Zhang <hongbo.zhang@linaro.org>

> > + *

> > + * Based on hw/arm/virt.c

> > + *

> > + * This program is free software; you can redistribute it and/or modify it

> > + * under the terms and conditions of the GNU General Public License,

> > + * version 2 or later, as published by the Free Software Foundation.

> > + *

> > + * This program is distributed in the hope it will be useful, but WITHOUT

> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or

> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for

> > + * more details.

> > + *

> > + * You should have received a copy of the GNU General Public License along with

> > + * this program.  If not, see <http://www.gnu.org/licenses/>.

> > + */

> > +

> > +#include "qemu/osdep.h"

> > +#include "qapi/error.h"

> > +#include "hw/arm/arm.h"

> > +#include "hw/arm/virt.h"

>

> This isn't the virt board, so I think it would be better if it did

> not include the virt.h header or use structures/classes that are

> private to the virt board. It should be its own thing.

>

> > +#include "hw/devices.h"

> > +#include "net/net.h"

> > +#include "sysemu/device_tree.h"

> > +#include "sysemu/numa.h"

> > +#include "sysemu/sysemu.h"

> > +#include "hw/loader.h"

> > +#include "exec/address-spaces.h"

> > +#include "qemu/error-report.h"

> > +#include "hw/pci-host/gpex.h"

> > +#include "hw/arm/sysbus-fdt.h"

> > +#include "hw/arm/fdt.h"

> > +#include "hw/intc/arm_gic.h"

> > +#include "hw/intc/arm_gicv3_common.h"

> > +#include "kvm_arm.h"

> > +#include "hw/ide/internal.h"

> > +#include "hw/ide/ahci_internal.h"

> > +#include "qemu/units.h"

> > +

> > +#define NUM_IRQS 256

> > +

> > +#define RAMLIMIT_GB 8192

> > +#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)

> > +

> > +static const MemMapEntry sbsa_ref_memmap[] = {

> > +    /* 512M boot ROM */

> > +    [VIRT_FLASH] =              {          0, 0x20000000 },

> > +    /* 512M secure memery */

>

> "memory"

>

> > +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> > +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> > +    /* GIC distributor and CPU interface expansion spaces reserved */

> > +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> > +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },

>

> If they're just reserved you don't really need to list them here,

> as they're covered by the VIRT_CPUPERIPHS space anyway. (You

> don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

>

We need more consideration here.
Why CPUPERIPHS is used to cover DIST and CPU interface? is this from
some old platform? I don't see the term in GICv3 memory map in the
user manual, so do we still need it for this Arm64 GICv3?
If we only list CPUPERIPHS but without DIST, I am afraid firmware or
driver developer still looking for the term of DIST for base address.

For GICv3, what we can have are DIST, CPU, REDIST, VCPU and HYP.
CPU, VCPU and HYP are not simulated for GICv3 (curious it still works
without CPU interface emulated), so we only have DIST and REDIST.
Can we only list the DIST and REDIST without CPUPERIPHS?

> > +    /* 64M redistributor space allows up to 512 CPUs */

> > +    [VIRT_GIC_REDIST] =         { 0x40080000, 0x04000000 },

> > +    /* Space here reserved for redistributor and vCPU/HYP expansion */

> > +    [VIRT_UART] =               { 0x60000000, 0x00001000 },

> > +    [VIRT_RTC] =                { 0x60010000, 0x00001000 },

> > +    [VIRT_GPIO] =               { 0x60020000, 0x00001000 },

> > +    [VIRT_SECURE_UART] =        { 0x60030000, 0x00001000 },

> > +    [VIRT_SMMU] =               { 0x60040000, 0x00020000 },

> > +    /* Space here reserved for more SMMUs */

> > +    [VIRT_AHCI] =               { 0x60100000, 0x00010000 },

> > +    /* Space here reserved for other devices */

> > +    [VIRT_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },

> > +    /* 256M PCIE ECAM space */

> > +    [VIRT_PCIE_ECAM] =          { 0x80000000, 0x10000000 },

>

> Comment says 256M but the size field says it's larger...

>

> > +    /* ~1TB for PCIE MMIO (4GB to 1024GB boundary) */

> > +    [VIRT_PCIE_MMIO] =          { 0x100000000ULL, 0xFF00000000ULL },

> > +    [VIRT_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },

> > +};

> > +

> > +static const int sbsa_ref_irqmap[] = {

> > +    [VIRT_UART] = 1,

> > +    [VIRT_RTC] = 2,

> > +    [VIRT_PCIE] = 3, /* ... to 6 */

> > +    [VIRT_GPIO] = 7,

> > +    [VIRT_SECURE_UART] = 8,

> > +    [VIRT_AHCI] = 9,

> > +};

> > +

> > +static void sbsa_ref_init(MachineState *machine)

> > +{

> > +    VirtMachineState *vms = VIRT_MACHINE(machine);

>

> As noted above, I think it would be better to be your own

> subclass of MachineState, rather than being a subclass of

> the virt board. Is there anything that becomes particularly

> awkward if you do it that way?

>

> thanks

> -- PMM
Peter Maydell Jan. 31, 2019, 12:01 p.m. UTC | #6
On Wed, 30 Jan 2019 at 08:59, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:
>

> On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:

> >

> > On Fri, 7 Dec 2018 at 09:08, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

> > > +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> > > +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> > > +    /* GIC distributor and CPU interface expansion spaces reserved */

> > > +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> > > +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },

> >

> > If they're just reserved you don't really need to list them here,

> > as they're covered by the VIRT_CPUPERIPHS space anyway. (You

> > don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

> >

> We need more consideration here.


Yeah, this is a little more complex than I thought.

> Why CPUPERIPHS is used to cover DIST and CPU interface? is this from

> some old platform? I don't see the term in GICv3 memory map in the

> user manual, so do we still need it for this Arm64 GICv3?

> If we only list CPUPERIPHS but without DIST, I am afraid firmware or

> driver developer still looking for the term of DIST for base address.


OK, so what CPUPERIPHS does (in the virt board) is set the
CPU property which defines the reset value of the CBAR_EL1 register.
(The size specified isn't used.)

The set of memory mapped things in this area should (in theory)
be the same as what the hardware CPU puts there, because guest
code can reasonably look at CBAR and expect to find there the
devices that the real CPU puts there. Unfortunately on the virt
board we don't get this right (and in any case we support multiple
CPU types which don't necessarily have the same set of devices).
Things work in practice because Linux and OVMF both look at
the device tree rather than assuming they can find things
via CBAR_EL1. This is awkward to fix in the virt board without
breaking compatibility, but we can get it right for this new board.

> For GICv3, what we can have are DIST, CPU, REDIST, VCPU and HYP.

> CPU, VCPU and HYP are not simulated for GICv3 (curious it still works

> without CPU interface emulated), so we only have DIST and REDIST.

> Can we only list the DIST and REDIST without CPUPERIPHS?


QEMU's GICv3 implementation only supports the system register
interface, not the memory-mapped interface. This is why we don't
have a memory mapped thing to put into VIRT_GIC_HYP/VIRT_GIC_CPU/
VIRT_GIC_VCPU.

For the Cortex-A53/A57/A72, the only things in the CBAR area are
these memory mapped interfaces that we don't implement:
  CBAR+0x00000 : cpu interface
  CBAR+0x10000 : virt interface control
  CBAR+0x20000 : vcpu interface
  CBAR+0x2F000 : alias of vcpu interface
(other parts of the CBAR+0x00000 to CBAR+0x3ffff reserved)

So for the SBSA reference board, I think the right thing is to
leave a gap in the memory map of 0x40000 in size for CPUPERIPHS,
with a comment that this is reserved CPU peripheral space
because we don't implement the GICv3 memory-mapped CPU interface.
Put GIC_DIST somewhere else (it is a GICv3 device register
bank, not part of the CPU's peripherals), and don't define
GIC_CPU at all (since you don't use it).

thanks
-- PMM
Hongbo Zhang Feb. 1, 2019, 9:29 a.m. UTC | #7
On Thu, 31 Jan 2019 at 20:02, Peter Maydell <peter.maydell@linaro.org> wrote:
>

> On Wed, 30 Jan 2019 at 08:59, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

> >

> > On Tue, 22 Jan 2019 at 19:42, Peter Maydell <peter.maydell@linaro.org> wrote:

> > >

> > > On Fri, 7 Dec 2018 at 09:08, Hongbo Zhang <hongbo.zhang@linaro.org> wrote:

> > > > +    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },

> > > > +    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },

> > > > +    /* GIC distributor and CPU interface expansion spaces reserved */

> > > > +    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },

> > > > +    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },

> > >

> > > If they're just reserved you don't really need to list them here,

> > > as they're covered by the VIRT_CPUPERIPHS space anyway. (You

> > > don't list the VIRT_GIC_HYP registers or VIRT_GIC_VCPU.)

> > >

> > We need more consideration here.

>

> Yeah, this is a little more complex than I thought.

>

> > Why CPUPERIPHS is used to cover DIST and CPU interface? is this from

> > some old platform? I don't see the term in GICv3 memory map in the

> > user manual, so do we still need it for this Arm64 GICv3?

> > If we only list CPUPERIPHS but without DIST, I am afraid firmware or

> > driver developer still looking for the term of DIST for base address.

>

> OK, so what CPUPERIPHS does (in the virt board) is set the

> CPU property which defines the reset value of the CBAR_EL1 register.

> (The size specified isn't used.)

>

> The set of memory mapped things in this area should (in theory)

> be the same as what the hardware CPU puts there, because guest

> code can reasonably look at CBAR and expect to find there the

> devices that the real CPU puts there. Unfortunately on the virt

> board we don't get this right (and in any case we support multiple

> CPU types which don't necessarily have the same set of devices).

> Things work in practice because Linux and OVMF both look at

> the device tree rather than assuming they can find things

> via CBAR_EL1. This is awkward to fix in the virt board without

> breaking compatibility, but we can get it right for this new board.

>

> > For GICv3, what we can have are DIST, CPU, REDIST, VCPU and HYP.

> > CPU, VCPU and HYP are not simulated for GICv3 (curious it still works

> > without CPU interface emulated), so we only have DIST and REDIST.

> > Can we only list the DIST and REDIST without CPUPERIPHS?

>

> QEMU's GICv3 implementation only supports the system register

> interface, not the memory-mapped interface. This is why we don't

> have a memory mapped thing to put into VIRT_GIC_HYP/VIRT_GIC_CPU/

> VIRT_GIC_VCPU.

>

> For the Cortex-A53/A57/A72, the only things in the CBAR area are

> these memory mapped interfaces that we don't implement:

>   CBAR+0x00000 : cpu interface

>   CBAR+0x10000 : virt interface control

>   CBAR+0x20000 : vcpu interface

>   CBAR+0x2F000 : alias of vcpu interface

> (other parts of the CBAR+0x00000 to CBAR+0x3ffff reserved)

>

> So for the SBSA reference board, I think the right thing is to

> leave a gap in the memory map of 0x40000 in size for CPUPERIPHS,

> with a comment that this is reserved CPU peripheral space

> because we don't implement the GICv3 memory-mapped CPU interface.

> Put GIC_DIST somewhere else (it is a GICv3 device register

> bank, not part of the CPU's peripherals), and don't define

> GIC_CPU at all (since you don't use it).

>

Yes, should like this, thanks.

> thanks

> -- PMM
diff mbox series

Patch

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index d51fcec..a8895eb 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,4 +1,4 @@ 
-obj-y += boot.o virt.o sysbus-fdt.o
+obj-y += boot.o virt.o sbsa-ref.o sysbus-fdt.o
 obj-$(CONFIG_ACPI) += virt-acpi-build.o
 obj-$(CONFIG_DIGIC) += digic_boards.o
 obj-$(CONFIG_EXYNOS4) += exynos4_boards.o
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
new file mode 100644
index 0000000..1d6252b
--- /dev/null
+++ b/hw/arm/sbsa-ref.c
@@ -0,0 +1,277 @@ 
+/*
+ * ARM SBSA Reference Platform emulation
+ *
+ * Copyright (c) 2018 Linaro Limited
+ * Written by Hongbo Zhang <hongbo.zhang@linaro.org>
+ *
+ * Based on hw/arm/virt.c
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/arm/arm.h"
+#include "hw/arm/virt.h"
+#include "hw/devices.h"
+#include "net/net.h"
+#include "sysemu/device_tree.h"
+#include "sysemu/numa.h"
+#include "sysemu/sysemu.h"
+#include "hw/loader.h"
+#include "exec/address-spaces.h"
+#include "qemu/error-report.h"
+#include "hw/pci-host/gpex.h"
+#include "hw/arm/sysbus-fdt.h"
+#include "hw/arm/fdt.h"
+#include "hw/intc/arm_gic.h"
+#include "hw/intc/arm_gicv3_common.h"
+#include "kvm_arm.h"
+#include "hw/ide/internal.h"
+#include "hw/ide/ahci_internal.h"
+#include "qemu/units.h"
+
+#define NUM_IRQS 256
+
+#define RAMLIMIT_GB 8192
+#define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
+
+static const MemMapEntry sbsa_ref_memmap[] = {
+    /* 512M boot ROM */
+    [VIRT_FLASH] =              {          0, 0x20000000 },
+    /* 512M secure memery */
+    [VIRT_SECURE_MEM] =         { 0x20000000, 0x20000000 },
+    [VIRT_CPUPERIPHS] =         { 0x40000000, 0x00080000 },
+    /* GIC distributor and CPU interface expansion spaces reserved */
+    [VIRT_GIC_DIST] =           { 0x40000000, 0x00010000 },
+    [VIRT_GIC_CPU] =            { 0x40040000, 0x00010000 },
+    /* 64M redistributor space allows up to 512 CPUs */
+    [VIRT_GIC_REDIST] =         { 0x40080000, 0x04000000 },
+    /* Space here reserved for redistributor and vCPU/HYP expansion */
+    [VIRT_UART] =               { 0x60000000, 0x00001000 },
+    [VIRT_RTC] =                { 0x60010000, 0x00001000 },
+    [VIRT_GPIO] =               { 0x60020000, 0x00001000 },
+    [VIRT_SECURE_UART] =        { 0x60030000, 0x00001000 },
+    [VIRT_SMMU] =               { 0x60040000, 0x00020000 },
+    /* Space here reserved for more SMMUs */
+    [VIRT_AHCI] =               { 0x60100000, 0x00010000 },
+    /* Space here reserved for other devices */
+    [VIRT_PCIE_PIO] =           { 0x7fff0000, 0x00010000 },
+    /* 256M PCIE ECAM space */
+    [VIRT_PCIE_ECAM] =          { 0x80000000, 0x10000000 },
+    /* ~1TB for PCIE MMIO (4GB to 1024GB boundary) */
+    [VIRT_PCIE_MMIO] =          { 0x100000000ULL, 0xFF00000000ULL },
+    [VIRT_MEM] =                { 0x10000000000ULL, RAMLIMIT_BYTES },
+};
+
+static const int sbsa_ref_irqmap[] = {
+    [VIRT_UART] = 1,
+    [VIRT_RTC] = 2,
+    [VIRT_PCIE] = 3, /* ... to 6 */
+    [VIRT_GPIO] = 7,
+    [VIRT_SECURE_UART] = 8,
+    [VIRT_AHCI] = 9,
+};
+
+static void sbsa_ref_init(MachineState *machine)
+{
+    VirtMachineState *vms = VIRT_MACHINE(machine);
+    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");
+        exit(1);
+    }
+
+    if (machine->kernel_filename && firmware_loaded) {
+        error_report("sbsa-ref: No fw_cfg device on this machine, "
+                     "so -kernel option is not supported when firmware loaded, "
+                     "please load OS from hard disk instead");
+        exit(1);
+    }
+
+    /* This machine has EL3 enabled, external firmware should supply PSCI
+     * implementation, so the QEMU's internal PSCI is disabled.
+     */
+    vms->psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
+
+    sbsa_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / GICV3_REDIST_SIZE;
+
+    if (max_cpus > sbsa_max_cpus) {
+        error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
+                     "supported by machine 'sbsa-ref' (%d)",
+                     max_cpus, sbsa_max_cpus);
+        exit(1);
+    }
+
+    vms->smp_cpus = smp_cpus;
+
+    if (machine->ram_size > vms->memmap[VIRT_MEM].size) {
+        error_report("sbsa-ref: cannot model more than %dGB RAM", RAMLIMIT_GB);
+        exit(1);
+    }
+
+    secure_sysmem = g_new(MemoryRegion, 1);
+    memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
+                       UINT64_MAX);
+    memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
+
+    possible_cpus = mc->possible_cpu_arch_ids(machine);
+    for (n = 0; n < possible_cpus->len; n++) {
+        Object *cpuobj;
+        CPUState *cs;
+
+        if (n >= smp_cpus) {
+            break;
+        }
+
+        cpuobj = object_new(possible_cpus->cpus[n].type);
+        object_property_set_int(cpuobj, possible_cpus->cpus[n].arch_id,
+                                "mp-affinity", NULL);
+
+        cs = CPU(cpuobj);
+        cs->cpu_index = n;
+
+        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
+                          &error_fatal);
+
+        if (object_property_find(cpuobj, "reset-cbar", NULL)) {
+            object_property_set_int(cpuobj, vms->memmap[VIRT_CPUPERIPHS].base,
+                                    "reset-cbar", &error_abort);
+        }
+
+        object_property_set_link(cpuobj, OBJECT(sysmem), "memory",
+                                 &error_abort);
+
+        object_property_set_link(cpuobj, OBJECT(secure_sysmem),
+                                 "secure-memory", &error_abort);
+
+        object_property_set_bool(cpuobj, true, "realized", &error_fatal);
+        object_unref(cpuobj);
+    }
+
+    memory_region_allocate_system_memory(ram, NULL, "sbsa-ref.ram",
+                                         machine->ram_size);
+    memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base, ram);
+
+    vms->bootinfo.ram_size = machine->ram_size;
+    vms->bootinfo.kernel_filename = machine->kernel_filename;
+    vms->bootinfo.nb_cpus = smp_cpus;
+    vms->bootinfo.board_id = -1;
+    vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
+    vms->bootinfo.firmware_loaded = firmware_loaded;
+    arm_load_kernel(ARM_CPU(first_cpu), &vms->bootinfo);
+}
+
+static uint64_t sbsa_ref_cpu_mp_affinity(VirtMachineState *vms, int idx)
+{
+    uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+
+    if (!vmc->disallow_affinity_adjustment) {
+        clustersz = GICV3_TARGETLIST_BITS;
+    }
+    return arm_cpu_mp_affinity(idx, clustersz);
+}
+
+static const CPUArchIdList *sbsa_ref_possible_cpu_arch_ids(MachineState *ms)
+{
+    VirtMachineState *vms = VIRT_MACHINE(ms);
+    int n;
+
+    if (ms->possible_cpus) {
+        assert(ms->possible_cpus->len == max_cpus);
+        return ms->possible_cpus;
+    }
+
+    ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
+                                  sizeof(CPUArchId) * max_cpus);
+    ms->possible_cpus->len = max_cpus;
+    for (n = 0; n < ms->possible_cpus->len; n++) {
+        ms->possible_cpus->cpus[n].type = ms->cpu_type;
+        ms->possible_cpus->cpus[n].arch_id =
+            sbsa_ref_cpu_mp_affinity(vms, n);
+        ms->possible_cpus->cpus[n].props.has_thread_id = true;
+        ms->possible_cpus->cpus[n].props.thread_id = n;
+    }
+    return ms->possible_cpus;
+}
+
+static CpuInstanceProperties
+sbsa_ref_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(ms);
+
+    assert(cpu_index < possible_cpus->len);
+    return possible_cpus->cpus[cpu_index].props;
+}
+
+static int64_t
+sbsa_ref_get_default_cpu_node_id(const MachineState *ms, int idx)
+{
+    return idx % nb_numa_nodes;
+}
+
+static void sbsa_ref_instance_init(Object *obj)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->memmap = sbsa_ref_memmap;
+    vms->irqmap = sbsa_ref_irqmap;
+}
+
+static void sbsa_ref_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->init = sbsa_ref_init;
+    mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a57");
+    mc->max_cpus = 512;
+    mc->pci_allow_0_address = true;
+    mc->minimum_page_bits = 12;
+    mc->block_default_type = IF_IDE;
+    mc->no_cdrom = 1;
+    mc->default_ram_size = 1 * GiB;
+    mc->default_cpus = 4;
+    mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids;
+    mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props;
+    mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id;
+}
+
+static const TypeInfo sbsa_ref_info = {
+    .name          = MACHINE_TYPE_NAME("sbsa-ref"),
+    .parent        = TYPE_VIRT_MACHINE,
+    .instance_init = sbsa_ref_instance_init,
+    .class_init    = sbsa_ref_class_init,
+};
+
+static void sbsa_ref_machine_init(void)
+{
+    type_register_static(&sbsa_ref_info);
+}
+
+type_init(sbsa_ref_machine_init);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 9a870cc..c73c46b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -78,6 +78,7 @@  enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_AHCI,
 };
 
 typedef enum VirtIOMMUType {