diff mbox series

[kvm-unit-tests,v2,3/7] lib/asm: Add definitions of memory areas

Message ID 20201002154420.292134-4-imbrenda@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series Rewrite the allocators | expand

Commit Message

Claudio Imbrenda Oct. 2, 2020, 3:44 p.m. UTC 
Add definitions and boundaries of memory areas for some architectures.
This is needed by the next patch.

Most architectures only get one generic memory area, wherease x86 and
s390x get some more attention:

x86 gets
* lowest area (24-bit addresses)
* low area (32-bit addresses)
* the rest

s390x gets
* low area (31-bit addresses)
* the rest

Notice that the number indicates the order in which the areas are
scanned when more than one area is indicated. The default order tries
to get allocations from higher address ranges before trying lower ones.
This tries to keep the precious lower addresses as free as possible.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/asm-generic/memory_areas.h | 11 +++++++++++
 lib/arm/asm/memory_areas.h     | 11 +++++++++++
 lib/arm64/asm/memory_areas.h   | 11 +++++++++++
 lib/powerpc/asm/memory_areas.h | 11 +++++++++++
 lib/ppc64/asm/memory_areas.h   | 11 +++++++++++
 lib/s390x/asm/memory_areas.h   | 17 +++++++++++++++++
 lib/x86/asm/memory_areas.h     | 22 ++++++++++++++++++++++
 7 files changed, 94 insertions(+)
 create mode 100644 lib/asm-generic/memory_areas.h
 create mode 100644 lib/arm/asm/memory_areas.h
 create mode 100644 lib/arm64/asm/memory_areas.h
 create mode 100644 lib/powerpc/asm/memory_areas.h
 create mode 100644 lib/ppc64/asm/memory_areas.h
 create mode 100644 lib/s390x/asm/memory_areas.h
 create mode 100644 lib/x86/asm/memory_areas.h

Comments

Andrew Jones Oct. 3, 2020, 9:23 a.m. UTC | #1 
On Fri, Oct 02, 2020 at 05:44:16PM +0200, Claudio Imbrenda wrote:
> Add definitions and boundaries of memory areas for some architectures.
> This is needed by the next patch.
> 
> Most architectures only get one generic memory area, wherease x86 and
> s390x get some more attention:
> 
> x86 gets
> * lowest area (24-bit addresses)
> * low area (32-bit addresses)
> * the rest
> 
> s390x gets
> * low area (31-bit addresses)
> * the rest
> 
> Notice that the number indicates the order in which the areas are
> scanned when more than one area is indicated. The default order tries
> to get allocations from higher address ranges before trying lower ones.
> This tries to keep the precious lower addresses as free as possible.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  lib/asm-generic/memory_areas.h | 11 +++++++++++
>  lib/arm/asm/memory_areas.h     | 11 +++++++++++
>  lib/arm64/asm/memory_areas.h   | 11 +++++++++++
>  lib/powerpc/asm/memory_areas.h | 11 +++++++++++
>  lib/ppc64/asm/memory_areas.h   | 11 +++++++++++
>  lib/s390x/asm/memory_areas.h   | 17 +++++++++++++++++
>  lib/x86/asm/memory_areas.h     | 22 ++++++++++++++++++++++
>  7 files changed, 94 insertions(+)
>  create mode 100644 lib/asm-generic/memory_areas.h
>  create mode 100644 lib/arm/asm/memory_areas.h
>  create mode 100644 lib/arm64/asm/memory_areas.h
>  create mode 100644 lib/powerpc/asm/memory_areas.h
>  create mode 100644 lib/ppc64/asm/memory_areas.h
>  create mode 100644 lib/s390x/asm/memory_areas.h
>  create mode 100644 lib/x86/asm/memory_areas.h
> 
> diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
> new file mode 100644
> index 0000000..927baa7
> --- /dev/null
> +++ b/lib/asm-generic/memory_areas.h
> @@ -0,0 +1,11 @@
> +#ifndef MEMORY_AREAS_H

_ASM_GENERIC_MEMORY_AREAS_H_

> +#define MEMORY_AREAS_H
> +
> +#define AREA_NORMAL_PFN 0
> +#define AREA_NORMAL_NUMBER 0
> +#define AREA_NORMAL 1
> +
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff

Do we really need both a "type number", like AREA_NORMAL, and a
"number number" (AREA_NORMAL_NUMBER)? Why not just search in the order
of the type numbers? Or in the reverse order, if that's better? Also,
would an enum be more appropriate for the type numbers?

> +
> +#endif
> diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> new file mode 100644
> index 0000000..927baa7
> --- /dev/null
> +++ b/lib/arm/asm/memory_areas.h
> @@ -0,0 +1,11 @@
> +#ifndef MEMORY_AREAS_H

_ASMARM_MEMORY_AREAS_H_

We certainly don't want the same define as the generic file, as it's
possible an arch will want to simply extend the generic code, e.g.

 #ifndef _ASMARM_MEMORY_AREAS_H_
 #define _ASMARM_MEMORY_AREAS_H_
 #include #include <asm-generic/memory_areas.h>

 /* ARM memory area stuff here */

 #endif

This comment applies to the rest of memory_areas.h files. Look at
other lib/$ARCH/asm/*.h files to get the define prefix.

> +#define MEMORY_AREAS_H
> +
> +#define AREA_NORMAL_PFN 0
> +#define AREA_NORMAL_NUMBER 0
> +#define AREA_NORMAL 1
> +
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff
> +
> +#endif
[...]
> diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
> new file mode 100644
> index 0000000..4856a27
> --- /dev/null
> +++ b/lib/s390x/asm/memory_areas.h
> @@ -0,0 +1,17 @@
> +#ifndef MEMORY_AREAS_H
> +#define MEMORY_AREAS_H
> +
> +#define AREA_NORMAL_PFN BIT(31-12)

BIT(31 - PAGE_SHIFT)

> +#define AREA_NORMAL_NUMBER 0
> +#define AREA_NORMAL 1
> +
> +#define AREA_LOW_PFN 0
> +#define AREA_LOW_NUMBER 1
> +#define AREA_LOW 2
> +
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff
> +
> +#define AREA_DMA31 AREA_LOW

I don't work on s390x, but I'd rather not add too many aliases. I think
a single name with the min and max address bits embedded in it is
probably best. Maybe something like AREA_0_31 and AREA_31_52, or
whatever.

> +
> +#endif
> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> new file mode 100644
> index 0000000..d704df3
> --- /dev/null
> +++ b/lib/x86/asm/memory_areas.h
> @@ -0,0 +1,22 @@
> +#ifndef MEMORY_AREAS_H
> +#define MEMORY_AREAS_H
> +
> +#define AREA_NORMAL_PFN BIT(32-12)
> +#define AREA_NORMAL_NUMBER 0
> +#define AREA_NORMAL 1
> +
> +#define AREA_LOW_PFN BIT(24-12)
> +#define AREA_LOW_NUMBER 1
> +#define AREA_LOW 2
> +
> +#define AREA_LOWEST_PFN 0
> +#define AREA_LOWEST_NUMBER 2
> +#define AREA_LOWEST 4
> +
> +#define AREA_DMA24 AREA_LOWEST
> +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)

Aha, now I finally see that there's a type number and a number number
because the type number is a bit, presumably for some flag field that's
coming in a later patch. I'll have to look at the other patches to see
how that's used, but at the moment I feel like there should be another
way to represent memory areas without requiring a handful of defines
and aliases for each one.

Thanks,
drew

> +
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff
> +
> +#endif
> -- 
> 2.26.2
>
Claudio Imbrenda Oct. 5, 2020, 7:10 a.m. UTC | #2 
On Sat, 3 Oct 2020 11:23:27 +0200
Andrew Jones <drjones@redhat.com> wrote:

[...]

> > diff --git a/lib/asm-generic/memory_areas.h
> > b/lib/asm-generic/memory_areas.h new file mode 100644
> > index 0000000..927baa7
> > --- /dev/null
> > +++ b/lib/asm-generic/memory_areas.h
> > @@ -0,0 +1,11 @@
> > +#ifndef MEMORY_AREAS_H  
> 
> _ASM_GENERIC_MEMORY_AREAS_H_
> 
> > +#define MEMORY_AREAS_H
> > +
> > +#define AREA_NORMAL_PFN 0
> > +#define AREA_NORMAL_NUMBER 0
> > +#define AREA_NORMAL 1
> > +
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff  
> 
> Do we really need both a "type number", like AREA_NORMAL, and a
> "number number" (AREA_NORMAL_NUMBER)? Why not just search in the order
> of the type numbers? Or in the reverse order, if that's better? Also,
> would an enum be more appropriate for the type numbers?

you understood the reason later on, but also consider that enums cannot
be extended and they need to be arch dependent, and not always have all
the elements.
 
> > +
> > +#endif
> > diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> > new file mode 100644
> > index 0000000..927baa7
> > --- /dev/null
> > +++ b/lib/arm/asm/memory_areas.h
> > @@ -0,0 +1,11 @@
> > +#ifndef MEMORY_AREAS_H  
> 
> _ASMARM_MEMORY_AREAS_H_
> 
> We certainly don't want the same define as the generic file, as it's
> possible an arch will want to simply extend the generic code, e.g.
> 
>  #ifndef _ASMARM_MEMORY_AREAS_H_
>  #define _ASMARM_MEMORY_AREAS_H_
>  #include #include <asm-generic/memory_areas.h>

I see now, I guess if an arch is fine with the generic version it can
include it instead of redefining it.

I'll fix the defines and the names

>  /* ARM memory area stuff here */
> 
>  #endif
> 
> This comment applies to the rest of memory_areas.h files. Look at
> other lib/$ARCH/asm/*.h files to get the define prefix.
> 
> > +#define MEMORY_AREAS_H
> > +
> > +#define AREA_NORMAL_PFN 0
> > +#define AREA_NORMAL_NUMBER 0
> > +#define AREA_NORMAL 1
> > +
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff
> > +
> > +#endif  
> [...]
> > diff --git a/lib/s390x/asm/memory_areas.h
> > b/lib/s390x/asm/memory_areas.h new file mode 100644
> > index 0000000..4856a27
> > --- /dev/null
> > +++ b/lib/s390x/asm/memory_areas.h
> > @@ -0,0 +1,17 @@
> > +#ifndef MEMORY_AREAS_H
> > +#define MEMORY_AREAS_H
> > +
> > +#define AREA_NORMAL_PFN BIT(31-12)  
> 
> BIT(31 - PAGE_SHIFT)
> 
> > +#define AREA_NORMAL_NUMBER 0
> > +#define AREA_NORMAL 1
> > +
> > +#define AREA_LOW_PFN 0
> > +#define AREA_LOW_NUMBER 1
> > +#define AREA_LOW 2
> > +
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff
> > +
> > +#define AREA_DMA31 AREA_LOW  
> 
> I don't work on s390x, but I'd rather not add too many aliases. I
> think a single name with the min and max address bits embedded in it
> is probably best. Maybe something like AREA_0_31 and AREA_31_52, or
> whatever.

the aliases are arch-specific and are just to make the life easier, you
could just always use the generic macros.

the generic macros, by the way, need to be generic because they are
used in common code, and there we can't have arch-specific names

> > +
> > +#endif
> > diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> > new file mode 100644
> > index 0000000..d704df3
> > --- /dev/null
> > +++ b/lib/x86/asm/memory_areas.h
> > @@ -0,0 +1,22 @@
> > +#ifndef MEMORY_AREAS_H
> > +#define MEMORY_AREAS_H
> > +
> > +#define AREA_NORMAL_PFN BIT(32-12)
> > +#define AREA_NORMAL_NUMBER 0
> > +#define AREA_NORMAL 1
> > +
> > +#define AREA_LOW_PFN BIT(24-12)
> > +#define AREA_LOW_NUMBER 1
> > +#define AREA_LOW 2
> > +
> > +#define AREA_LOWEST_PFN 0
> > +#define AREA_LOWEST_NUMBER 2
> > +#define AREA_LOWEST 4
> > +
> > +#define AREA_DMA24 AREA_LOWEST
> > +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)  
> 
> Aha, now I finally see that there's a type number and a number number
> because the type number is a bit, presumably for some flag field
> that's coming in a later patch. I'll have to look at the other

I will fix the patch description to make it clear

> patches to see how that's used, but at the moment I feel like there
> should be another way to represent memory areas without requiring a
> handful of defines and aliases for each one.

I think this is the most straightforward way, even though it might not
necessarily look very clean, but... suggestions welcome :)

> Thanks,
> drew
> 
> > +
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff
> > +
> > +#endif
> > -- 
> > 2.26.2
> >   
>
Paolo Bonzini Nov. 6, 2020, 11:34 a.m. UTC | #3 
On 02/10/20 17:44, Claudio Imbrenda wrote:
> x86 gets
> * lowest area (24-bit addresses)
> * low area (32-bit addresses)
> * the rest

x86 if anything could use a 36-bit area; the 24-bit one is out of scope 
for what kvm-unit-tests does.

So something like this:

diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
index d704df3..952f5bd 100644
--- a/lib/x86/asm/memory_areas.h
+++ b/lib/x86/asm/memory_areas.h
@@ -1,20 +1,19 @@
  #ifndef MEMORY_AREAS_H
  #define MEMORY_AREAS_H

-#define AREA_NORMAL_PFN BIT(32-12)
+#define AREA_NORMAL_PFN BIT(36-12)
  #define AREA_NORMAL_NUMBER 0
  #define AREA_NORMAL 1

-#define AREA_LOW_PFN BIT(24-12)
-#define AREA_LOW_NUMBER 1
-#define AREA_LOW 2
+#define AREA_PAE_HIGH_PFN BIT(32-12)
+#define AREA_PAE_HIGH_NUMBER 1
+#define AREA_PAE_HIGH 2

-#define AREA_LOWEST_PFN 0
-#define AREA_LOWEST_NUMBER 2
-#define AREA_LOWEST 4
+#define AREA_LOW_PFN 0
+#define AREA_LOW_NUMBER 2
+#define AREA_LOW 4

-#define AREA_DMA24 AREA_LOWEST
-#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
+#define AREA_PAE (AREA_PAE | AREA_LOW)

  #define AREA_ANY -1
  #define AREA_ANY_NUMBER 0xff

Paolo
Claudio Imbrenda Nov. 6, 2020, 12:58 p.m. UTC | #4 
On Fri, 6 Nov 2020 12:34:10 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 02/10/20 17:44, Claudio Imbrenda wrote:
> > x86 gets
> > * lowest area (24-bit addresses)
> > * low area (32-bit addresses)
> > * the rest  
> 
> x86 if anything could use a 36-bit area; the 24-bit one is out of
> scope for what kvm-unit-tests does.

sure... I went with what I remembered about the x86 architecture, but
I'm not an expert

my patch was meant to be some "sensible defaults" that people with
more knowledge should override anyway :)

> So something like this:
> 
> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> index d704df3..952f5bd 100644
> --- a/lib/x86/asm/memory_areas.h
> +++ b/lib/x86/asm/memory_areas.h
> @@ -1,20 +1,19 @@
>   #ifndef MEMORY_AREAS_H
>   #define MEMORY_AREAS_H
> 
> -#define AREA_NORMAL_PFN BIT(32-12)
> +#define AREA_NORMAL_PFN BIT(36-12)
>   #define AREA_NORMAL_NUMBER 0
>   #define AREA_NORMAL 1
> 
> -#define AREA_LOW_PFN BIT(24-12)
> -#define AREA_LOW_NUMBER 1
> -#define AREA_LOW 2
> +#define AREA_PAE_HIGH_PFN BIT(32-12)
> +#define AREA_PAE_HIGH_NUMBER 1
> +#define AREA_PAE_HIGH 2
> 
> -#define AREA_LOWEST_PFN 0
> -#define AREA_LOWEST_NUMBER 2
> -#define AREA_LOWEST 4
> +#define AREA_LOW_PFN 0
> +#define AREA_LOW_NUMBER 2
> +#define AREA_LOW 4
> 
> -#define AREA_DMA24 AREA_LOWEST
> -#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
> +#define AREA_PAE (AREA_PAE | AREA_LOW)
> 
>   #define AREA_ANY -1
>   #define AREA_ANY_NUMBER 0xff
> 
> Paolo
>
Paolo Bonzini Nov. 6, 2020, 1:04 p.m. UTC | #5 
On 06/11/20 13:58, Claudio Imbrenda wrote:
>> x86 if anything could use a 36-bit area; the 24-bit one is out of
>> scope for what kvm-unit-tests does.
> sure... I went with what I remembered about the x86 architecture, but
> I'm not an expert
> 
> my patch was meant to be some "sensible defaults" that people with
> more knowledge should override anyway:)

Yep, got it.  Though, "why are 24-bit addresses special" (for ISA DMA 
that is only used by floppies and SoundBlaster cards) is actually way 
more for experts than "why are 36-bit addresses special" at this point 
in time!

Paolo
diff mbox series

Patch

diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
new file mode 100644
index 0000000..927baa7
--- /dev/null
+++ b/lib/asm-generic/memory_areas.h
@@ -0,0 +1,11 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN 0
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif
diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
new file mode 100644
index 0000000..927baa7
--- /dev/null
+++ b/lib/arm/asm/memory_areas.h
@@ -0,0 +1,11 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN 0
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif
diff --git a/lib/arm64/asm/memory_areas.h b/lib/arm64/asm/memory_areas.h
new file mode 100644
index 0000000..927baa7
--- /dev/null
+++ b/lib/arm64/asm/memory_areas.h
@@ -0,0 +1,11 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN 0
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif
diff --git a/lib/powerpc/asm/memory_areas.h b/lib/powerpc/asm/memory_areas.h
new file mode 100644
index 0000000..927baa7
--- /dev/null
+++ b/lib/powerpc/asm/memory_areas.h
@@ -0,0 +1,11 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN 0
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif
diff --git a/lib/ppc64/asm/memory_areas.h b/lib/ppc64/asm/memory_areas.h
new file mode 100644
index 0000000..927baa7
--- /dev/null
+++ b/lib/ppc64/asm/memory_areas.h
@@ -0,0 +1,11 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN 0
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif
diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
new file mode 100644
index 0000000..4856a27
--- /dev/null
+++ b/lib/s390x/asm/memory_areas.h
@@ -0,0 +1,17 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN BIT(31-12)
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_LOW_PFN 0
+#define AREA_LOW_NUMBER 1
+#define AREA_LOW 2
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#define AREA_DMA31 AREA_LOW
+
+#endif
diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
new file mode 100644
index 0000000..d704df3
--- /dev/null
+++ b/lib/x86/asm/memory_areas.h
@@ -0,0 +1,22 @@ 
+#ifndef MEMORY_AREAS_H
+#define MEMORY_AREAS_H
+
+#define AREA_NORMAL_PFN BIT(32-12)
+#define AREA_NORMAL_NUMBER 0
+#define AREA_NORMAL 1
+
+#define AREA_LOW_PFN BIT(24-12)
+#define AREA_LOW_NUMBER 1
+#define AREA_LOW 2
+
+#define AREA_LOWEST_PFN 0
+#define AREA_LOWEST_NUMBER 2
+#define AREA_LOWEST 4
+
+#define AREA_DMA24 AREA_LOWEST
+#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
+
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
+#endif