Re: [Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page
>>> On 17.08.18 at 18:37, wrote:
> On Fri, Aug 17, 2018 at 04:31:21AM -0600, Jan Beulich wrote:
>> >>> On 17.08.18 at 12:17, wrote:
>> On 14.08.18 at 15:43, wrote:
>> >> +switch ( e820.map[i].type )
>> >> +{
>> >> +case E820_RAM:
>> >> +return RAM_TYPE_CONVENTIONAL;
>> >> +
>> >> +case E820_RESERVED:
>> >> +return RAM_TYPE_RESERVED;
>> >> +
>> >> +case E820_UNUSABLE:
>> >> +return RAM_TYPE_UNUSABLE;
>> >> +
>> >> +case E820_ACPI:
>> >> +case E820_NVS:
>> >> +return RAM_TYPE_ACPI;
>> >> +
>> >> +default:
>> >> +ASSERT_UNREACHABLE();
>> >> +return -1;
>> >> +}
>> >> +}
>> >> +
>> >> +return -1;
>> >> +}
>> >
>> > One more case to consider: What about a page part of which is
>> > a given type, and the other part of which is simply missing from
>> > the E820 table? I'm uncertain whether in that case it might be a
>> > good idea in general to report it as having the given type; for the
>> > specific purpose you want the function for, that would imo be
>> > quite helpful.
>>
>> Considering RAM_TYPE_* are bit masks - perhaps the function
>> should OR together all types found for the requested page, and
>> let the caller go from there? And to account for my earlier remark,
>> add a separate RAM_TYPE_UNKNOWN (and never have the function
>> return -1 or some such; its return type then would better be
>> unsigned int)?
>
> I can do something along this lines, but then the functionality of the
> existing inclusive option is likely to change on boxes having memory
> types not aligned to a page boundary.
That's an expected consequence, yes, and given this is a workaround
option anyway I'm afraid we'll have to live with it being unclear whether
this would be a change to the better or to the worse.
> Also, we should likely discuss what to do with a page that for example
> has both unusable and reserved regions. I would map it in the
> inclusive case because it has a reserved region, but would like to
> hear your opinion.
Not page-aligned unusable regions are pretty suspicious imo. Along
the lines of the above response, I think I'd be fine going either of the
two possible routes - we simply can't tell ahead of time which one
might be better.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page
On Fri, Aug 17, 2018 at 04:31:21AM -0600, Jan Beulich wrote:
> >>> On 17.08.18 at 12:17, wrote:
> On 14.08.18 at 15:43, wrote:
> >> +switch ( e820.map[i].type )
> >> +{
> >> +case E820_RAM:
> >> +return RAM_TYPE_CONVENTIONAL;
> >> +
> >> +case E820_RESERVED:
> >> +return RAM_TYPE_RESERVED;
> >> +
> >> +case E820_UNUSABLE:
> >> +return RAM_TYPE_UNUSABLE;
> >> +
> >> +case E820_ACPI:
> >> +case E820_NVS:
> >> +return RAM_TYPE_ACPI;
> >> +
> >> +default:
> >> +ASSERT_UNREACHABLE();
> >> +return -1;
> >> +}
> >> +}
> >> +
> >> +return -1;
> >> +}
> >
> > One more case to consider: What about a page part of which is
> > a given type, and the other part of which is simply missing from
> > the E820 table? I'm uncertain whether in that case it might be a
> > good idea in general to report it as having the given type; for the
> > specific purpose you want the function for, that would imo be
> > quite helpful.
>
> Considering RAM_TYPE_* are bit masks - perhaps the function
> should OR together all types found for the requested page, and
> let the caller go from there? And to account for my earlier remark,
> add a separate RAM_TYPE_UNKNOWN (and never have the function
> return -1 or some such; its return type then would better be
> unsigned int)?
I can do something along this lines, but then the functionality of the
existing inclusive option is likely to change on boxes having memory
types not aligned to a page boundary.
Also, we should likely discuss what to do with a page that for example
has both unusable and reserved regions. I would map it in the
inclusive case because it has a reserved region, but would like to
hear your opinion.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page
>>> On 17.08.18 at 12:17, wrote:
On 14.08.18 at 15:43, wrote:
>> +switch ( e820.map[i].type )
>> +{
>> +case E820_RAM:
>> +return RAM_TYPE_CONVENTIONAL;
>> +
>> +case E820_RESERVED:
>> +return RAM_TYPE_RESERVED;
>> +
>> +case E820_UNUSABLE:
>> +return RAM_TYPE_UNUSABLE;
>> +
>> +case E820_ACPI:
>> +case E820_NVS:
>> +return RAM_TYPE_ACPI;
>> +
>> +default:
>> +ASSERT_UNREACHABLE();
>> +return -1;
>> +}
>> +}
>> +
>> +return -1;
>> +}
>
> One more case to consider: What about a page part of which is
> a given type, and the other part of which is simply missing from
> the E820 table? I'm uncertain whether in that case it might be a
> good idea in general to report it as having the given type; for the
> specific purpose you want the function for, that would imo be
> quite helpful.
Considering RAM_TYPE_* are bit masks - perhaps the function
should OR together all types found for the requested page, and
let the caller go from there? And to account for my earlier remark,
add a separate RAM_TYPE_UNKNOWN (and never have the function
return -1 or some such; its return type then would better be
unsigned int)?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page
>>> On 14.08.18 at 15:43, wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1181,6 +1181,12 @@ int page_is_ram_type(unsigned long mfn, unsigned long
> mem_type)
> return 0;
> }
>
> +int page_get_type(unsigned long mfn)
> +{
> +ASSERT_UNREACHABLE();
> +return -1;
> +}
With the assertion, why is the function needed in the first place?
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -430,6 +430,40 @@ int page_is_ram_type(unsigned long mfn, unsigned long
> mem_type)
> return 0;
> }
>
> +int page_get_type(unsigned long mfn)
Considering we already have get_page_type(), this function name
needs to change. At the very least page_get_ram_type(), looking
at the set of values it may return.
> +{
> +uint64_t maddr = pfn_to_paddr(mfn);
> +unsigned int i;
> +
> +for ( i = 0; i < e820.nr_map; i++ )
> +{
> +/* Test the range. */
> +if ( (e820.map[i].addr <= maddr) &&
> + ((e820.map[i].addr + e820.map[i].size) >= (maddr + PAGE_SIZE)) )
Would you mind inverting the condition and using continue, both
to reduce indentation of the switch and to make it very clear that
no braces are needed / wanted?
> +switch ( e820.map[i].type )
> +{
> +case E820_RAM:
> +return RAM_TYPE_CONVENTIONAL;
> +
> +case E820_RESERVED:
> +return RAM_TYPE_RESERVED;
> +
> +case E820_UNUSABLE:
> +return RAM_TYPE_UNUSABLE;
> +
> +case E820_ACPI:
> +case E820_NVS:
> +return RAM_TYPE_ACPI;
> +
> +default:
> +ASSERT_UNREACHABLE();
> +return -1;
> +}
> +}
> +
> +return -1;
> +}
One more case to consider: What about a page part of which is
a given type, and the other part of which is simply missing from
the E820 table? I'm uncertain whether in that case it might be a
good idea in general to report it as having the given type; for the
specific purpose you want the function for, that would imo be
quite helpful.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v5 4/8] mm: introduce a helper to get the memory type of a page
The type is only returned if the full page has the same type. This
function is unimplemented for ARM.
Signed-off-by: Roger Pau Monné
---
Cc: Stefano Stabellini
Cc: Julien Grall
Cc: Andrew Cooper
Cc: George Dunlap
Cc: Ian Jackson
Cc: Jan Beulich
Cc: Konrad Rzeszutek Wilk
Cc: Tim Deegan
Cc: Wei Liu
---
xen/arch/arm/mm.c| 6 ++
xen/arch/x86/mm.c| 34 ++
xen/include/xen/mm.h | 2 ++
3 files changed, 42 insertions(+)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index d234c46e41..f734b9287a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1181,6 +1181,12 @@ int page_is_ram_type(unsigned long mfn, unsigned long
mem_type)
return 0;
}
+int page_get_type(unsigned long mfn)
+{
+ASSERT_UNREACHABLE();
+return -1;
+}
+
unsigned long domain_get_maximum_gpfn(struct domain *d)
{
return gfn_x(d->arch.p2m.max_mapped_gfn);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a1a1f5f3c3..cb4b68b2a9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -430,6 +430,40 @@ int page_is_ram_type(unsigned long mfn, unsigned long
mem_type)
return 0;
}
+int page_get_type(unsigned long mfn)
+{
+uint64_t maddr = pfn_to_paddr(mfn);
+unsigned int i;
+
+for ( i = 0; i < e820.nr_map; i++ )
+{
+/* Test the range. */
+if ( (e820.map[i].addr <= maddr) &&
+ ((e820.map[i].addr + e820.map[i].size) >= (maddr + PAGE_SIZE)) )
+switch ( e820.map[i].type )
+{
+case E820_RAM:
+return RAM_TYPE_CONVENTIONAL;
+
+case E820_RESERVED:
+return RAM_TYPE_RESERVED;
+
+case E820_UNUSABLE:
+return RAM_TYPE_UNUSABLE;
+
+case E820_ACPI:
+case E820_NVS:
+return RAM_TYPE_ACPI;
+
+default:
+ASSERT_UNREACHABLE();
+return -1;
+}
+}
+
+return -1;
+}
+
unsigned long domain_get_maximum_gpfn(struct domain *d)
{
if ( is_hvm_domain(d) )
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 24654e8e22..9584fe3a77 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -597,6 +597,8 @@ int __must_check donate_page(struct domain *d, struct
page_info *page,
#define RAM_TYPE_ACPI 0x0008
/* TRUE if the whole page at @mfn is of the requested RAM type(s) above. */
int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
+/* Returns the page type if the whole page is of the same type. */
+int page_get_type(unsigned long mfn);
/* Prepare/destroy a ring for a dom0 helper. Helper with talk
* with Xen on behalf of this domain. */
--
2.18.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
