[kvmtool,v2,20/22] arm: Reorganise and document memory map

Message ID	20210225005915.26423-21-andre.przywara@arm.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@kernel.org> From: Andre Przywara <andre.przywara@arm.com> To: Will Deacon <will@kernel.org>, Julien Thierry <julien.thierry.kdev@gmail.com> Cc: Alexandru Elisei <alexandru.elisei@arm.com>, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, Marc Zyngier <maz@kernel.org>, Sami Mujawar <sami.mujawar@arm.com> Subject: [PATCH kvmtool v2 20/22] arm: Reorganise and document memory map Date: Thu, 25 Feb 2021 00:59:13 +0000 Message-Id: <20210225005915.26423-21-andre.przywara@arm.com> In-Reply-To: <20210225005915.26423-1-andre.przywara@arm.com> References: <20210225005915.26423-1-andre.przywara@arm.com> Precedence: bulk
Series	Unify I/O port and MMIO trap handling \| expand [kvmtool,v2,00/22] Unify I/O port and MMIO trap handling [kvmtool,v2,01/22] ioport: Remove ioport__setup_arch() [kvmtool,v2,02/22] hw/serial: Use device abstraction for FDT generator function [kvmtool,v2,03/22] ioport: Retire .generate_fdt_node functionality [kvmtool,v2,04/22] mmio: Extend handling to include ioport emulation [kvmtool,v2,05/22] hw/i8042: Clean up data types [kvmtool,v2,06/22] hw/i8042: Refactor trap handler [kvmtool,v2,07/22] hw/i8042: Switch to new trap handlers [kvmtool,v2,08/22] x86/ioport: Refactor trap handlers [kvmtool,v2,09/22] x86/ioport: Switch to new trap handlers [kvmtool,v2,10/22] hw/rtc: Refactor trap handlers [kvmtool,v2,11/22] hw/rtc: Switch to new trap handler [kvmtool,v2,12/22] hw/vesa: Switch trap handling to use MMIO handler [kvmtool,v2,13/22] hw/serial: Refactor trap handler [kvmtool,v2,14/22] hw/serial: Switch to new trap handlers [kvmtool,v2,15/22] vfio: Refactor ioport trap handler [kvmtool,v2,16/22] vfio: Switch to new ioport trap handlers [kvmtool,v2,17/22] virtio: Switch trap handling to use MMIO handler [kvmtool,v2,18/22] pci: Switch trap handling to use MMIO handler [kvmtool,v2,19/22] Remove ioport specific routines [kvmtool,v2,20/22] arm: Reorganise and document memory map [kvmtool,v2,21/22] hw/serial: ARM/arm64: Use MMIO at higher addresses [kvmtool,v2,22/22] hw/rtc: ARM/arm64: Use MMIO at higher addresses

Message ID

20210225005915.26423-21-andre.przywara@arm.com (mailing list archive)

State

New, archived

Headers

From: Andre Przywara <andre.przywara@arm.com>
To: Will Deacon <will@kernel.org>,
        Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>, kvm@vger.kernel.org,
        kvmarm@lists.cs.columbia.edu, Marc Zyngier <maz@kernel.org>,
        Sami Mujawar <sami.mujawar@arm.com>
Subject: [PATCH kvmtool v2 20/22] arm: Reorganise and document memory map
Date: Thu, 25 Feb 2021 00:59:13 +0000
Message-Id: <20210225005915.26423-21-andre.przywara@arm.com>
In-Reply-To: <20210225005915.26423-1-andre.przywara@arm.com>
References: <20210225005915.26423-1-andre.przywara@arm.com>
Precedence: bulk

Series

Unify I/O port and MMIO trap handling | expand

Commit Message

Andre Przywara Feb. 25, 2021, 12:59 a.m. UTC

The hardcoded memory map we expose to a guest is currently described
using a series of partially interconnected preprocessor constants,
which is hard to read and follow.

In preparation for moving the UART and RTC to some different MMIO
region, document the current map with some ASCII art, and clean up the
definition of the sections.

No functional change.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/include/arm-common/kvm-arch.h | 41 ++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Alexandru Elisei March 9, 2021, 3:46 p.m. UTC | #1

Hi Andre,

This is a really good idea, thank you for implementing it!

Some comments below.

On 2/25/21 12:59 AM, Andre Przywara wrote:
> The hardcoded memory map we expose to a guest is currently described
> using a series of partially interconnected preprocessor constants,
> which is hard to read and follow.
>
> In preparation for moving the UART and RTC to some different MMIO
> region, document the current map with some ASCII art, and clean up the
> definition of the sections.
>
> No functional change.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/include/arm-common/kvm-arch.h | 41 ++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index d84e50cd..b12255b0 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -7,14 +7,33 @@
>  
>  #include "arm-common/gic.h"
>  
> +/*
> + * The memory map used for ARM guests (not to scale):
> + *
> + * 0      64K  16M     32M     48M            1GB       2GB
> + * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
> + * | (PCI) |////| int.  |       |        |     |         |
> + * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
> + * | ports |////| UART, |       |  MMIO  |     |  (AXI)  |
> + * |       |////| RTC   |       |        |     |         |
> + * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
> + */

Nitpick: I searched the PCI Local Bus Specification revision 3.0 (which kvmtool
currently implements) for the term I/O ports, and found one mention in a schematic
for an add-in card. The I/O region is called in the spec I/O Space.

I don't know what "int." means in the region for the UART and RTC.

The comment says that the art is not to scale, so I don't think there's any need
for the "..." between the corners of the regions. To my eyes, it makes the ASCII
art look crooked.

The next patches add the UART and RTC outside the first 64K, I think the region
should be documented in the patches where the changes are made, not here. Another
alternative would be to move this patch to the end of the series instead of
incrementally changing the memory ASCII art (which I imagine is time consuming).

Otherwise, the numbers look OK.

> +
>  #define ARM_IOPORT_AREA		_AC(0x0000000000000000, UL)
> -#define ARM_FLASH_AREA		_AC(0x0000000002000000, UL)
> -#define ARM_MMIO_AREA		_AC(0x0000000003000000, UL)
> +#define ARM_MMIO_AREA		_AC(0x0000000001000000, UL)

The patch says it is *documenting* the memory layout, but here it is *changing*
the layout. Other than that, I like the shuffling of definitions so the kvmtool
global defines are closer to the arch values.

Thanks,

Alex

>  #define ARM_AXI_AREA		_AC(0x0000000040000000, UL)
>  #define ARM_MEMORY_AREA		_AC(0x0000000080000000, UL)
>  
> -#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
> -#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
> +#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
> +#define ARM_IOPORT_SIZE		(1U << 16)
> +
> +
> +#define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
> +#define KVM_FLASH_MAX_SIZE	0x1000000
> +
> +#define KVM_VIRTIO_MMIO_AREA	(KVM_FLASH_MMIO_BASE + KVM_FLASH_MAX_SIZE)
> +#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - \
> +				(KVM_VIRTIO_MMIO_AREA + ARM_GIC_SIZE))
>  
>  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
>  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
> @@ -22,19 +41,17 @@
>  #define ARM_GIC_DIST_SIZE	0x10000
>  #define ARM_GIC_CPUI_SIZE	0x20000
>  
> -#define KVM_FLASH_MMIO_BASE	ARM_FLASH_AREA
> -#define KVM_FLASH_MAX_SIZE	(ARM_MMIO_AREA - ARM_FLASH_AREA)
>  
> -#define ARM_IOPORT_SIZE		(1U << 16)
> -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
> +#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
>  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
> +#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
>  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
>  
> -#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
> -#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
> -#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
> -#define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
> +
> +#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
> +#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
> +
>  
>  #define KVM_IOEVENTFD_HAS_PIO	0
>

Andre Przywara March 15, 2021, 12:27 p.m. UTC | #2

On Tue, 9 Mar 2021 15:46:29 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi,

> Hi Andre,
> 
> This is a really good idea, thank you for implementing it!
> 
> Some comments below.
> 
> On 2/25/21 12:59 AM, Andre Przywara wrote:
> > The hardcoded memory map we expose to a guest is currently described
> > using a series of partially interconnected preprocessor constants,
> > which is hard to read and follow.
> >
> > In preparation for moving the UART and RTC to some different MMIO
> > region, document the current map with some ASCII art, and clean up the
> > definition of the sections.
> >
> > No functional change.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arm/include/arm-common/kvm-arch.h | 41 ++++++++++++++++++++++---------
> >  1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> > index d84e50cd..b12255b0 100644
> > --- a/arm/include/arm-common/kvm-arch.h
> > +++ b/arm/include/arm-common/kvm-arch.h
> > @@ -7,14 +7,33 @@
> >  
> >  #include "arm-common/gic.h"
> >  
> > +/*
> > + * The memory map used for ARM guests (not to scale):
> > + *
> > + * 0      64K  16M     32M     48M            1GB       2GB
> > + * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
> > + * | (PCI) |////| int.  |       |        |     |         |
> > + * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
> > + * | ports |////| UART, |       |  MMIO  |     |  (AXI)  |
> > + * |       |////| RTC   |       |        |     |         |
> > + * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
> > + */  
> 
> Nitpick: I searched the PCI Local Bus Specification revision 3.0 (which kvmtool
> currently implements) for the term I/O ports, and found one mention in a schematic
> for an add-in card. The I/O region is called in the spec I/O Space.

Right, will change that.
 
> I don't know what "int." means in the region for the UART and RTC.

It was meant to mean "internal", but this is really nonsensical, so I
will replace it with "plat MMIO".
 
> The comment says that the art is not to scale, so I don't think there's any need
> for the "..." between the corners of the regions. To my eyes, it makes the ASCII
> art look crooked.

fixed.
 
> The next patches add the UART and RTC outside the first 64K, I think the region
> should be documented in the patches where the changes are made, not here. Another
> alternative would be to move this patch to the end of the series instead of
> incrementally changing the memory ASCII art (which I imagine is time consuming).

You passed the scrutiny test ;-)
I noticed this last minute, but didn't feel like changing it then.
Have done it now.

> 
> Otherwise, the numbers look OK.
> 
> > +
> >  #define ARM_IOPORT_AREA		_AC(0x0000000000000000, UL)
> > -#define ARM_FLASH_AREA		_AC(0x0000000002000000, UL)
> > -#define ARM_MMIO_AREA		_AC(0x0000000003000000, UL)
> > +#define ARM_MMIO_AREA		_AC(0x0000000001000000, UL)  
> 
> The patch says it is *documenting* the memory layout, but here it is *changing*
> the layout. Other than that, I like the shuffling of definitions so the kvmtool
> global defines are closer to the arch values.

I amended the commit message to mention that I change the value of
ARM_MMIO_AREA, but that stays internal to that file and doesn't affect
the other definitions.
In fact I imported this header into a C file and printed all
externally used names, before and after: it didn't show any changes.

Cheers,
Andre

> >  #define ARM_AXI_AREA		_AC(0x0000000040000000, UL)
> >  #define ARM_MEMORY_AREA		_AC(0x0000000080000000, UL)
> >  
> > -#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
> > -#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
> > +#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
> > +#define ARM_IOPORT_SIZE		(1U << 16)
> > +
> > +
> > +#define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
> > +#define KVM_FLASH_MAX_SIZE	0x1000000
> > +
> > +#define KVM_VIRTIO_MMIO_AREA	(KVM_FLASH_MMIO_BASE + KVM_FLASH_MAX_SIZE)
> > +#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - \
> > +				(KVM_VIRTIO_MMIO_AREA + ARM_GIC_SIZE))
> >  
> >  #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
> >  #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
> > @@ -22,19 +41,17 @@
> >  #define ARM_GIC_DIST_SIZE	0x10000
> >  #define ARM_GIC_CPUI_SIZE	0x20000
> >  
> > -#define KVM_FLASH_MMIO_BASE	ARM_FLASH_AREA
> > -#define KVM_FLASH_MAX_SIZE	(ARM_MMIO_AREA - ARM_FLASH_AREA)
> >  
> > -#define ARM_IOPORT_SIZE		(1U << 16)
> > -#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
> > +#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
> >  #define ARM_PCI_CFG_SIZE	(1ULL << 24)
> > +#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
> >  #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
> >  				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> >  
> > -#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
> > -#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
> > -#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
> > -#define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
> > +
> > +#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
> > +#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
> > +
> >  
> >  #define KVM_IOEVENTFD_HAS_PIO	0
> >

diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index d84e50cd..b12255b0 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -7,14 +7,33 @@ 
 
 #include "arm-common/gic.h"
 
+/*
+ * The memory map used for ARM guests (not to scale):
+ *
+ * 0      64K  16M     32M     48M            1GB       2GB
+ * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
+ * | (PCI) |////| int.  |       |        |     |         |
+ * |  I/O  |////| MMIO: | Flash | virtio | GIC |   PCI   |  DRAM
+ * | ports |////| UART, |       |  MMIO  |     |  (AXI)  |
+ * |       |////| RTC   |       |        |     |         |
+ * +-------+-..-+-------+-------+--....--+-----+--.....--+---......
+ */
+
 #define ARM_IOPORT_AREA		_AC(0x0000000000000000, UL)
-#define ARM_FLASH_AREA		_AC(0x0000000002000000, UL)
-#define ARM_MMIO_AREA		_AC(0x0000000003000000, UL)
+#define ARM_MMIO_AREA		_AC(0x0000000001000000, UL)
 #define ARM_AXI_AREA		_AC(0x0000000040000000, UL)
 #define ARM_MEMORY_AREA		_AC(0x0000000080000000, UL)
 
-#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
-#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
+#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
+#define ARM_IOPORT_SIZE		(1U << 16)
+
+
+#define KVM_FLASH_MMIO_BASE	(ARM_MMIO_AREA + 0x1000000)
+#define KVM_FLASH_MAX_SIZE	0x1000000
+
+#define KVM_VIRTIO_MMIO_AREA	(KVM_FLASH_MMIO_BASE + KVM_FLASH_MAX_SIZE)
+#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - \
+				(KVM_VIRTIO_MMIO_AREA + ARM_GIC_SIZE))
 
 #define ARM_GIC_DIST_BASE	(ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
 #define ARM_GIC_CPUI_BASE	(ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
@@ -22,19 +41,17 @@ 
 #define ARM_GIC_DIST_SIZE	0x10000
 #define ARM_GIC_CPUI_SIZE	0x20000
 
-#define KVM_FLASH_MMIO_BASE	ARM_FLASH_AREA
-#define KVM_FLASH_MAX_SIZE	(ARM_MMIO_AREA - ARM_FLASH_AREA)
 
-#define ARM_IOPORT_SIZE		(1U << 16)
-#define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
+#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
 #define ARM_PCI_CFG_SIZE	(1ULL << 24)
+#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
 #define ARM_PCI_MMIO_SIZE	(ARM_MEMORY_AREA - \
 				(ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
 
-#define KVM_IOPORT_AREA		ARM_IOPORT_AREA
-#define KVM_PCI_CFG_AREA	ARM_AXI_AREA
-#define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
-#define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
+
+#define ARM_LOMAP_MAX_MEMORY	((1ULL << 32) - ARM_MEMORY_AREA)
+#define ARM_HIMAP_MAX_MEMORY	((1ULL << 40) - ARM_MEMORY_AREA)
+
 
 #define KVM_IOEVENTFD_HAS_PIO	0

[kvmtool,v2,20/22] arm: Reorganise and document memory map

Commit Message

Comments

Patch