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

From: Claudio Imbrenda <imbrenda@linux.ibm.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, frankja@linux.ibm.com,
	david@redhat.com, thuth@redhat.com, cohuck@redhat.com,
	lvivier@redhat.com
Subject: Re: [kvm-unit-tests PATCH v2 3/7] lib/asm: Add definitions of memory areas
Date: Mon, 5 Oct 2020 09:10:13 +0200	[thread overview]
Message-ID: <20201005091013.2e0b98a9@ibm-vm> (raw)
In-Reply-To: <20201003092327.5xl7nzx27o35jqwf@kamzik.brq.redhat.com>

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