Re: [Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM
On Wed, Jun 03, 2015 at 10:25:47AM +0800, Chen, Tiejun wrote:
[...]
+static struct xen_reserved_device_memory
+*xc_device_get_rdm(libxl__gc *gc,
+ uint32_t flag,
+ uint16_t seg,
+ uint8_t bus,
+ uint8_t devfn,
+ unsigned int *nr_entries)
+{
+struct xen_reserved_device_memory *xrdm = NULL;
+int rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+ xrdm, nr_entries);
+
Please separate declaration and function call. Also change xrdm to NULL
Are you saying this?
struct xen_reserved_device_memory *xrdm = NULL;
int rc;
rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
xrdm, nr_entries);
Yes, splitting rc = to a separate line. The other thing is:
rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
NULL, nr_entries);
^ here
It's mostly cosmetic. IMHO it is clearer than passing xrdm which is
always NULL in that function call.
in that function call.
Sorry, what do you mean by this point? Or you let me to change xrdm to NULL
inside xc_reserved_device_memory_map()?
+assert( rc = 0 );
+/* 0 means we have no any rdm entry. */
+if ( !rc )
+goto out;
Also set *nr_entries = 0; otherwise you can't distinguish error vs 0
entries.
*nr_entries is always updated by xc_reserved_device_memory_map() above.
Actually no. If xc_hypercall_bounce_pre fails in the function,
nr_entries is untouched.
+
+if ( errno == ENOBUFS )
+{
+if ( (xrdm = malloc(*nr_entries *
+sizeof(xen_reserved_device_memory_t))) == NULL
)
[...]
+return -1;
Please return libxl error code.
ERROR_FAIL?
Yes, that's fine.
[...]
+args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH;
+mmio_start = (1ull 32) - args.mmio_size;
+
+if (args.lowmem_size mmio_start)
+args.lowmem_size = mmio_start;
+
+/*
+ * We'd like to set a memory boundary to determine if we need to check
+ * any overlap with reserved device memory.
+ */
+rdm_mem_boundary = 0x8000;
+if (info-rdm_mem_boundary_memkb)
I'm going to update this chunk of codes as follows:
#1. @@ -858,6 +858,12 @@ const char *libxl_defbool_to_string(libxl_defbool
b);
#define LIBXL_TIMER_MODE_DEFAULT -1
#define LIBXL_MEMKB_DEFAULT ~0ULL
+/*
+ * We'd like to set a memory boundary to determine if we need to check
+ * any overlap with reserved device memory.
+ */
+#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
+
#define LIBXL_MS_VM_GENID_LEN 16
typedef struct {
uint8_t bytes[LIBXL_MS_VM_GENID_LEN];
I think you mean info-rdm_mem_boundary_memkb != LIBXL_MEMKB_DEFAULT?
#2.
@@ -109,6 +109,10 @@ void libxl__rdm_setdefault(libxl__gc *gc,
libxl_domain_build_info *b_info)
{
if (b_info-rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID)
b_info-rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
+
+if (b_info-rdm_mem_boundary_memkb == LIBXL_MEMKB_DEFAULT)
+b_info-rdm_mem_boundary_memkb =
+LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
This looks plausible.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM
On 2015/6/7 19:20, Wei Liu wrote:
On Wed, Jun 03, 2015 at 10:25:47AM +0800, Chen, Tiejun wrote:
[...]
+static struct xen_reserved_device_memory
+*xc_device_get_rdm(libxl__gc *gc,
+ uint32_t flag,
+ uint16_t seg,
+ uint8_t bus,
+ uint8_t devfn,
+ unsigned int *nr_entries)
+{
+struct xen_reserved_device_memory *xrdm = NULL;
+int rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
+ xrdm, nr_entries);
+
Please separate declaration and function call. Also change xrdm to NULL
Are you saying this?
struct xen_reserved_device_memory *xrdm = NULL;
int rc;
rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
xrdm, nr_entries);
Yes, splitting rc = to a separate line. The other thing is:
rc = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn,
NULL, nr_entries);
^ here
It's mostly cosmetic. IMHO it is clearer than passing xrdm which is
always NULL in that function call.
Right.
in that function call.
Sorry, what do you mean by this point? Or you let me to change xrdm to NULL
inside xc_reserved_device_memory_map()?
+assert( rc = 0 );
+/* 0 means we have no any rdm entry. */
+if ( !rc )
+goto out;
Also set *nr_entries = 0; otherwise you can't distinguish error vs 0
entries.
*nr_entries is always updated by xc_reserved_device_memory_map() above.
Actually no. If xc_hypercall_bounce_pre fails in the function,
nr_entries is untouched.
Sure.
+
+if ( errno == ENOBUFS )
+{
+if ( (xrdm = malloc(*nr_entries *
+sizeof(xen_reserved_device_memory_t))) == NULL )
[...]
+return -1;
Please return libxl error code.
ERROR_FAIL?
Yes, that's fine.
Thanks
Tiejun
[...]
+args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH;
+mmio_start = (1ull 32) - args.mmio_size;
+
+if (args.lowmem_size mmio_start)
+args.lowmem_size = mmio_start;
+
+/*
+ * We'd like to set a memory boundary to determine if we need to check
+ * any overlap with reserved device memory.
+ */
+rdm_mem_boundary = 0x8000;
+if (info-rdm_mem_boundary_memkb)
I'm going to update this chunk of codes as follows:
#1. @@ -858,6 +858,12 @@ const char *libxl_defbool_to_string(libxl_defbool
b);
#define LIBXL_TIMER_MODE_DEFAULT -1
#define LIBXL_MEMKB_DEFAULT ~0ULL
+/*
+ * We'd like to set a memory boundary to determine if we need to check
+ * any overlap with reserved device memory.
+ */
+#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
+
#define LIBXL_MS_VM_GENID_LEN 16
typedef struct {
uint8_t bytes[LIBXL_MS_VM_GENID_LEN];
I think you mean info-rdm_mem_boundary_memkb != LIBXL_MEMKB_DEFAULT?
#2.
@@ -109,6 +109,10 @@ void libxl__rdm_setdefault(libxl__gc *gc,
libxl_domain_build_info *b_info)
{
if (b_info-rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID)
b_info-rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
+
+if (b_info-rdm_mem_boundary_memkb == LIBXL_MEMKB_DEFAULT)
+b_info-rdm_mem_boundary_memkb =
+LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
This looks plausible.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM
On 2015/6/3 0:29, Wei Liu wrote:
On Fri, May 22, 2015 at 05:35:04PM +0800, Tiejun Chen wrote:
While building a VM, HVM domain builder provides struct hvm_info_table{}
to help hvmloader. Currently it includes two fields to construct guest
e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should
check them to fix any conflict with RAM.
RMRR can reside in address space beyond 4G theoretically, but we never
see this in real world. So in order to avoid breaking highmem layout
we don't solve highmem conflict. Note this means highmem rmrr could still
be supported if no conflict.
But in the case of lowmem, RMRR probably scatter the whole RAM space.
Especially multiple RMRR entries would worsen this to lead a complicated
memory layout. And then its hard to extend hvm_info_table{} to work
hvmloader out. So here we're trying to figure out a simple solution to
avoid breaking existing layout. So when a conflict occurs,
#1. Above a predefined boundary (default 2G)
- move lowmem_end below reserved region to solve conflict;
#2. Below a predefined boundary (default 2G)
- Check strict/relaxed policy.
strict policy leads to fail libxl. Note when both policies
are specified on a given region, 'strict' is always preferred.
relaxed policy issue a warning message and also mask this entry
INVALID
to indicate we shouldn't expose this entry to hvmloader.
Note this predefined boundary can be changes with the parameter
rdm_mem_boundary in .cfg file.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
It would be better you write down what you changed in this version after
--- marker.
What we normally do is
libxl: implement FOO
FOO is needed because ...
Signed-off-by: Wei Liu wei.l...@citrix.com
---
changes in vN:
* bar - baz
* more comments
---
The stuff between two --- will be automatically discarded when
committing.
I knew about this rule.
Actually I already mentioned this change in patch #00,
v2:
* Instead of that fixed predefined rdm memory boundary, we'd like to
introduce a parameter, rdm_mem_boundary, to set this threshold value.
...
So I didn't explain this again separately so sorry for this inconvenience.
docs/man/xl.cfg.pod.5 | 21
tools/libxc/include/xenguest.h | 1 +
tools/libxc/xc_hvm_build_x86.c | 25 ++--
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_dm.c | 253 +
tools/libxl/libxl_dom.c| 27 -
tools/libxl/libxl_internal.h | 11 +-
tools/libxl/libxl_types.idl| 8 ++
tools/libxl/xl_cmdimpl.c | 3 +
9 files changed, 337 insertions(+), 14 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 12c34c4..80e3930 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -764,6 +764,27 @@ is default.
Note this would override global Brdm option.
+=item Brdm_mem_boundary=MBYTES
+
+Number of megabytes to set a boundary for checking rdm conflict.
+
+When RDM conflicts with RAM, RDM probably scatter the whole RAM space.
+Especially multiple RMRR entries would worsen this to lead a complicated
+memory layout. So here we're trying to figure out a simple solution to
+avoid breaking existing layout. So when a conflict occurs,
+
+#1. Above a predefined boundary
+- move lowmem_end below reserved region to solve conflict;
+
+#2. Below a predefined boundary
+- Check strict/relaxed policy.
+strict policy leads to fail libxl. Note when both policies
+are specified on a given region, 'strict' is always preferred.
+relaxed policy issue a warning message and also mask this entry
INVALID
+to indicate we shouldn't expose this entry to hvmloader.
+
+Her the default is 2G.
Typo her.
s/her/here
I get the idea. I will leave grammar / syntax check to native speakers.
Sure :)
+
=back
=back
diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 7581263..4cb7e9f 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -234,6 +234,7 @@ struct xc_hvm_firmware_module {
};
struct xc_hvm_build_args {
+uint64_t lowmem_size;/* All low memory size in bytes. */
You might find this value unnecessary with my patch to consolidate
memory layout generation in libxl?
I also noticed this from your patch. And also I replied you online, I
would rebase my patches once yours is acked. So at this point, yes, this
should be gone when you introduce lowmem_end.
uint64_t mem_size; /* Memory size in bytes. */
uint64_t mem_target; /* Memory target in bytes. */
uint64_t mmio_size; /* Size of the MMIO hole in bytes. */
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index e45ae4a..9a1567a 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -21,6 +21,7 @@
Re: [Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM
On Fri, May 22, 2015 at 05:35:04PM +0800, Tiejun Chen wrote:
While building a VM, HVM domain builder provides struct hvm_info_table{}
to help hvmloader. Currently it includes two fields to construct guest
e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should
check them to fix any conflict with RAM.
RMRR can reside in address space beyond 4G theoretically, but we never
see this in real world. So in order to avoid breaking highmem layout
we don't solve highmem conflict. Note this means highmem rmrr could still
be supported if no conflict.
But in the case of lowmem, RMRR probably scatter the whole RAM space.
Especially multiple RMRR entries would worsen this to lead a complicated
memory layout. And then its hard to extend hvm_info_table{} to work
hvmloader out. So here we're trying to figure out a simple solution to
avoid breaking existing layout. So when a conflict occurs,
#1. Above a predefined boundary (default 2G)
- move lowmem_end below reserved region to solve conflict;
#2. Below a predefined boundary (default 2G)
- Check strict/relaxed policy.
strict policy leads to fail libxl. Note when both policies
are specified on a given region, 'strict' is always preferred.
relaxed policy issue a warning message and also mask this entry
INVALID
to indicate we shouldn't expose this entry to hvmloader.
Note this predefined boundary can be changes with the parameter
rdm_mem_boundary in .cfg file.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
It would be better you write down what you changed in this version after
--- marker.
What we normally do is
libxl: implement FOO
FOO is needed because ...
Signed-off-by: Wei Liu wei.l...@citrix.com
---
changes in vN:
* bar - baz
* more comments
---
The stuff between two --- will be automatically discarded when
committing.
docs/man/xl.cfg.pod.5 | 21
tools/libxc/include/xenguest.h | 1 +
tools/libxc/xc_hvm_build_x86.c | 25 ++--
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_dm.c | 253
+
tools/libxl/libxl_dom.c| 27 -
tools/libxl/libxl_internal.h | 11 +-
tools/libxl/libxl_types.idl| 8 ++
tools/libxl/xl_cmdimpl.c | 3 +
9 files changed, 337 insertions(+), 14 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 12c34c4..80e3930 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -764,6 +764,27 @@ is default.
Note this would override global Brdm option.
+=item Brdm_mem_boundary=MBYTES
+
+Number of megabytes to set a boundary for checking rdm conflict.
+
+When RDM conflicts with RAM, RDM probably scatter the whole RAM space.
+Especially multiple RMRR entries would worsen this to lead a complicated
+memory layout. So here we're trying to figure out a simple solution to
+avoid breaking existing layout. So when a conflict occurs,
+
+#1. Above a predefined boundary
+- move lowmem_end below reserved region to solve conflict;
+
+#2. Below a predefined boundary
+- Check strict/relaxed policy.
+strict policy leads to fail libxl. Note when both policies
+are specified on a given region, 'strict' is always preferred.
+relaxed policy issue a warning message and also mask this entry
INVALID
+to indicate we shouldn't expose this entry to hvmloader.
+
+Her the default is 2G.
Typo her.
I get the idea. I will leave grammar / syntax check to native speakers.
+
=back
=back
diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 7581263..4cb7e9f 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -234,6 +234,7 @@ struct xc_hvm_firmware_module {
};
struct xc_hvm_build_args {
+uint64_t lowmem_size;/* All low memory size in bytes. */
You might find this value unnecessary with my patch to consolidate
memory layout generation in libxl?
uint64_t mem_size; /* Memory size in bytes. */
uint64_t mem_target; /* Memory target in bytes. */
uint64_t mmio_size; /* Size of the MMIO hole in bytes. */
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index e45ae4a..9a1567a 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -21,6 +21,7 @@
#include stdlib.h
#include unistd.h
#include zlib.h
+#include assert.h
#include xg_private.h
#include xc_private.h
@@ -98,11 +99,8 @@ static void build_hvm_info(void *hvm_info_page, uint64_t
mem_size,
uint8_t sum;
int i;
-if ( lowmem_end mmio_start )
-{
-highmem_end = (1ull32) + (lowmem_end - mmio_start);
-lowmem_end = mmio_start;
-}
+if ( args-mem_size lowmem_end )
+highmem_end = (1ull32) + (args-mem_size - lowmem_end);
[Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM
While building a VM, HVM domain builder provides struct hvm_info_table{}
to help hvmloader. Currently it includes two fields to construct guest
e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should
check them to fix any conflict with RAM.
RMRR can reside in address space beyond 4G theoretically, but we never
see this in real world. So in order to avoid breaking highmem layout
we don't solve highmem conflict. Note this means highmem rmrr could still
be supported if no conflict.
But in the case of lowmem, RMRR probably scatter the whole RAM space.
Especially multiple RMRR entries would worsen this to lead a complicated
memory layout. And then its hard to extend hvm_info_table{} to work
hvmloader out. So here we're trying to figure out a simple solution to
avoid breaking existing layout. So when a conflict occurs,
#1. Above a predefined boundary (default 2G)
- move lowmem_end below reserved region to solve conflict;
#2. Below a predefined boundary (default 2G)
- Check strict/relaxed policy.
strict policy leads to fail libxl. Note when both policies
are specified on a given region, 'strict' is always preferred.
relaxed policy issue a warning message and also mask this entry
INVALID
to indicate we shouldn't expose this entry to hvmloader.
Note this predefined boundary can be changes with the parameter
rdm_mem_boundary in .cfg file.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
docs/man/xl.cfg.pod.5 | 21
tools/libxc/include/xenguest.h | 1 +
tools/libxc/xc_hvm_build_x86.c | 25 ++--
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_dm.c | 253 +
tools/libxl/libxl_dom.c| 27 -
tools/libxl/libxl_internal.h | 11 +-
tools/libxl/libxl_types.idl| 8 ++
tools/libxl/xl_cmdimpl.c | 3 +
9 files changed, 337 insertions(+), 14 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 12c34c4..80e3930 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -764,6 +764,27 @@ is default.
Note this would override global Brdm option.
+=item Brdm_mem_boundary=MBYTES
+
+Number of megabytes to set a boundary for checking rdm conflict.
+
+When RDM conflicts with RAM, RDM probably scatter the whole RAM space.
+Especially multiple RMRR entries would worsen this to lead a complicated
+memory layout. So here we're trying to figure out a simple solution to
+avoid breaking existing layout. So when a conflict occurs,
+
+#1. Above a predefined boundary
+- move lowmem_end below reserved region to solve conflict;
+
+#2. Below a predefined boundary
+- Check strict/relaxed policy.
+strict policy leads to fail libxl. Note when both policies
+are specified on a given region, 'strict' is always preferred.
+relaxed policy issue a warning message and also mask this entry
INVALID
+to indicate we shouldn't expose this entry to hvmloader.
+
+Her the default is 2G.
+
=back
=back
diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 7581263..4cb7e9f 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -234,6 +234,7 @@ struct xc_hvm_firmware_module {
};
struct xc_hvm_build_args {
+uint64_t lowmem_size;/* All low memory size in bytes. */
uint64_t mem_size; /* Memory size in bytes. */
uint64_t mem_target; /* Memory target in bytes. */
uint64_t mmio_size; /* Size of the MMIO hole in bytes. */
diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index e45ae4a..9a1567a 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -21,6 +21,7 @@
#include stdlib.h
#include unistd.h
#include zlib.h
+#include assert.h
#include xg_private.h
#include xc_private.h
@@ -98,11 +99,8 @@ static void build_hvm_info(void *hvm_info_page, uint64_t
mem_size,
uint8_t sum;
int i;
-if ( lowmem_end mmio_start )
-{
-highmem_end = (1ull32) + (lowmem_end - mmio_start);
-lowmem_end = mmio_start;
-}
+if ( args-mem_size lowmem_end )
+highmem_end = (1ull32) + (args-mem_size - lowmem_end);
memset(hvm_info_page, 0, PAGE_SIZE);
@@ -279,7 +277,7 @@ static int setup_guest(xc_interface *xch,
elf_parse_binary(elf);
v_start = 0;
-v_end = args-mem_size;
+v_end = args-lowmem_size;
if ( nr_pages target_pages )
memflags |= XENMEMF_populate_on_demand;
@@ -344,8 +342,14 @@ static int setup_guest(xc_interface *xch,
for ( i = 0; i nr_pages; i++ )
page_array[i] = i;
-for ( i = mmio_start PAGE_SHIFT; i nr_pages; i++ )
-page_array[i] += mmio_size PAGE_SHIFT;
+/*
+ * Actually v_end is args-lowmem_size, and we already adjusted
+ * this below mmio_start when we check rdm previously, so here
+ * this
