Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Do you mean I should merge them as one as possible? "Factor it out" means to break out into a separate function (or maybe a macro or something, but in this case a function is appropriate). So in this case take the two sets of similar code, combine them into a function with appropriate arguments, and then call that function in both places. Finding multiple occurrences of very similar code is usually a sign that refactoring is needed. Thanks for you explanation. But seems not be possible because we have seveal combinations of these two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or pci devices are also passes through. [snip] Sorry I can't figure out a good name here :) Any suggestions? The hypervisor seems to call this `pfn_to_paddr'. Okay. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Chen, Tiejun writes ("Re: [v7][PATCH 11/16] tools/libxl: detect and avoid
conflicts with RDM"):
> > There are quite a few of these long lines, which should be wrapped.
> > See tools/libxl/CODING_STYLE.
>
> Sorry I can't found any case to what you're talking.
>
> So are you saying this?
>
> if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE &&
> !d_config->num_pcidevs)
> Or
Yes, I meant `linewrapped', not to make a wrapper function. Sorry for
not being clear.
> >> +d_config->num_rdms = nr_entries;
> >> +d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
> >> +d_config->num_rdms * sizeof(libxl_device_rdm));
> >
> > This code is remarkably similar to a function later on which adds an
> > rdm. Please can you factor it out.
>
> Do you mean I should merge them as one as possible?
"Factor it out" means to break out into a separate function (or maybe
a macro or something, but in this case a function is appropriate). So
in this case take the two sets of similar code, combine them into a
function with appropriate arguments, and then call that function in
both places.
Finding multiple occurrences of very similar code is usually a sign
that refactoring is needed.
> But seems not be possible because we have seveal combinations of these
> two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or
> pci devices are also passes through.
I'm not saying you need to merge the two conditions, which are indeed
different, but: the work of reallocing the array and filling in the
new entry could be lifted into a function which would be called in
both places.
> >> +for (j = 0; j < d_config->num_rdms; j++) {
> >> +if (d_config->rdms[j].start ==
> >> + (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT)
> >
> > This construct
> > (uint64_t)some_pfn << XC_PAGE_SHIFT
> > appears an awful lot.
> >
> > I would prefer it if it were done in an inline function (or maybe a
> > macro).
>
> Like this?
>
> #define (x) ((uint64_t)x << XC_PAGE_SHIFT)
Something like that, although inline functions are normally better if
a macro is not required. And in this case it isn't, so it should be a
function I think.
> Sorry I can't figure out a good name here :) Any suggestions?
The hypervisor seems to call this `pfn_to_paddr'.
> >> +ret = libxl__domain_device_construct_rdm(gc, d_config,
> >> + rdm_mem_boundary,
> >> + &args);
> >> +if (ret) {
> >> +LOG(ERROR, "checking reserved device memory failed");
> >> +goto out;
> >> +}
> >
> > `rc' should be used here rather than `ret'. (It is unfortunate that
> > this function has poor style already, but it would be best not to make
> > it worse.)
>
> I can do this but according to tools/libxl/CODING_STYLE, looks I should
> first post a separate patch to fix this code style issue, and then
> rebase my patch, right?
You are introducing a new use of `ret' rather than `rc'. AFAICT the
function already has a mixture, and there is no problem with just
using `rc' here. So I think you do not need to fix the rest of the
function: simply using `ret' rather than `rc' in your added lines
would result in an arrangement which would be correct, and at least as
conformant to the style guide as before.
(Of course if you want to fix the rest of the function then that would
be very welcome, and then you should do it as a separate patch.
However, at this stage before the codefreeze you probably prefer to
avoid taking on anything which is not completely critical.)
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
I have found a few things in this patch which I would like to see
improved. See below.
Given how late I am with this review, I do not feel that I should be
nacking it at this time. You have a tools ack from Wei, so my
comments are not a blocker for this series.
But if you need to respin, please take these comments into account,
and consider which are feasible to fix in the time available. If you
are respinning this series targeting Xen 4.7 or later, please address
all of the points I make below.
Thanks for your comments and looks I should address them now.
Thanks.
+int libxl__domain_device_construct_rdm(libxl__gc *gc,
+ libxl_domain_config *d_config,
+ uint64_t rdm_mem_boundary,
+ struct xc_hvm_build_args *args)
...
+uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull\
<<32);
...
+if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_\
pcidevs)
There are quite a few of these long lines, which should be wrapped.
See tools/libxl/CODING_STYLE.
Sorry I can't found any case to what you're talking.
So are you saying this?
if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE &&
!d_config->num_pcidevs)
Or
@@ -143,6 +143,15 @@ static bool overlaps_rdm(uint64_t start, uint64_t
memsize,
}
/*
+ * Check whether any rdm should be exposed..
+ * Returns true if needs, else returns false.
+ */
+static bool exposes_rdm(uint32_t strategy, int num_pcidevs)
+{
+return strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !num_pcidevs;
+}
+
+/*
* Check reported RDM regions and handle potential gfn conflicts according
* to user preferred policy.
*
@@ -182,7 +191,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc,
uint64_t highmem_end = args->highmem_end ? args->highmem_end :
(1ull<<32);
/* Might not expose rdm. */
-if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE &&
!d_config->num_pcidevs)
+if (exposes_rdm(strategy, d_config->num_pcidevs))
return 0;
/* Query all RDM entries in this platform */
+d_config->num_rdms = nr_entries;
+d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
+d_config->num_rdms * sizeof(libxl_device_rdm));
This code is remarkably similar to a function later on which adds an
rdm. Please can you factor it out.
Do you mean I should merge them as one as possible?
But seems not be possible because we have seveal combinations of these
two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or
pci devices are also passes through.
#1. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST but without any devices
So it appropriately needs this libxl__realloc() here. The second
libxl__realloc() doesn't take any effect.
#2. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST with one or more devices
Actually we don't need the second libxl__realloc(). This is same as #1.
#3. strategy != LIBXL_RDM_RESERVE_STRATEGY_HOST also with one or more
devices
So we just need the second libxl__realloc() later. The fist
libxl__realloc() doesn't be called at all. Especially, its very possible
we're going to handle less RDMs, compared to
LIBXL_RDM_RESERVE_STRATEGY_HOST.
+} else
+d_config->num_rdms = 0;
Please can you put { } around the else block too. I don't think this
mixed style is good.
Fixed.
+for (j = 0; j < d_config->num_rdms; j++) {
+if (d_config->rdms[j].start ==
+ (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT)
This construct
(uint64_t)some_pfn << XC_PAGE_SHIFT
appears an awful lot.
I would prefer it if it were done in an inline function (or maybe a
macro).
Like this?
#define (x) ((uint64_t)x << XC_PAGE_SHIFT)
Sorry I can't figure out a good name here :) Any suggestions?
+libxl_domain_build_info *const info = &d_config->b_info;
+/*
+ * Currently we fix this as 2G to guarantte how to handle
^
Should read "guarantee".
Fixed.
+ret = libxl__domain_device_construct_rdm(gc, d_config,
+ rdm_mem_boundary,
+ &args);
+if (ret) {
+LOG(ERROR, "checking reserved device memory failed");
+goto out;
+}
`rc' should be used here rather than `ret'. (It is unfortunate that
this function has poor style already, but it would be best not to make
it worse.)
I can do this but according to tools/libxl/CODING_STYLE, looks I should
first post a separate patch to fix this code style issue, and then
rebase my patch, right?
Thanks
TIejun
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Tiejun Chen writes ("[v7][PATCH 11/16] 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 RDM.
...
> CC: Ian Jackson
> CC: Stefano Stabellini
> CC: Ian Campbell
> CC: Wei Liu
> Acked-by: Wei Liu
> Signed-off-by: Tiejun Chen
> Reviewed-by: Kevin Tian
I have found a few things in this patch which I would like to see
improved. See below.
Given how late I am with this review, I do not feel that I should be
nacking it at this time. You have a tools ack from Wei, so my
comments are not a blocker for this series.
But if you need to respin, please take these comments into account,
and consider which are feasible to fix in the time available. If you
are respinning this series targeting Xen 4.7 or later, please address
all of the points I make below.
Thanks.
> +int libxl__domain_device_construct_rdm(libxl__gc *gc,
> + libxl_domain_config *d_config,
> + uint64_t rdm_mem_boundary,
> + struct xc_hvm_build_args *args)
...
> +uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull\
<<32);
...
> +if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_\
pcidevs)
There are quite a few of these long lines, which should be wrapped.
See tools/libxl/CODING_STYLE.
> +d_config->num_rdms = nr_entries;
> +d_config->rdms = libxl__realloc(NOGC, d_config->rdms,
> +d_config->num_rdms * sizeof(libxl_device_rdm));
This code is remarkably similar to a function later on which adds an
rdm. Please can you factor it out.
> +} else
> +d_config->num_rdms = 0;
Please can you put { } around the else block too. I don't think this
mixed style is good.
> +for (j = 0; j < d_config->num_rdms; j++) {
> +if (d_config->rdms[j].start ==
> + (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT)
This construct
(uint64_t)some_pfn << XC_PAGE_SHIFT
appears an awful lot.
I would prefer it if it were done in an inline function (or maybe a
macro).
> +libxl_domain_build_info *const info = &d_config->b_info;
> +/*
> + * Currently we fix this as 2G to guarantte how to handle
^
Should read "guarantee".
> +ret = libxl__domain_device_construct_rdm(gc, d_config,
> + rdm_mem_boundary,
> + &args);
> +if (ret) {
> +LOG(ERROR, "checking reserved device memory failed");
> +goto out;
> +}
`rc' should be used here rather than `ret'. (It is unfortunate that
this function has poor style already, but it would be best not to make
it worse.)
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
CC: Ian Jackson CC: Stefano Stabellini CC: Ian Campbell CC: Wei Liu Acked-by: Wei Liu Signed-off-by: Tiejun Chen Reviewed-by: Kevin Tian Typo here "kevint" Fixed. No need to resend just for this though. I think committer can handle this for you. If you happen to resend because of changes in other patches, please correct this. Sure. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On Thu, Jul 09, 2015 at 01:34:02PM +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 RDM.
>
> 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 (2G)
> - move lowmem_end below reserved region to solve conflict;
>
> #2. Below a predefined boundary (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 later we need to provide a parameter to set that predefined boundary
> dynamically.
>
> CC: Ian Jackson
> CC: Stefano Stabellini
> CC: Ian Campbell
> CC: Wei Liu
> Acked-by: Wei Liu
> Signed-off-by: Tiejun Chen
> Reviewed-by: Kevin Tian
Typo here "kevint"
No need to resend just for this though. I think committer can handle
this for you.
If you happen to resend because of changes in other patches, please
correct this.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [v7][PATCH 11/16] 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 RDM.
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 (2G)
- move lowmem_end below reserved region to solve conflict;
#2. Below a predefined boundary (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 later we need to provide a parameter to set that predefined boundary
dynamically.
CC: Ian Jackson
CC: Stefano Stabellini
CC: Ian Campbell
CC: Wei Liu
Acked-by: Wei Liu
Signed-off-by: Tiejun Chen
Reviewed-by: Kevin Tian
---
v7:
* Just sync with the fallout of renaming parameters from patch #10.
v6:
* fix some code stypes
* Refine libxl__xc_device_get_rdm()
v5:
* A little change to make sure the per-device policy always override the global
policy and correct its associated code comments.
* Fix one typo in the patch head description
* Rename xc_device_get_rdm() with libxl__xc_device_get_rdm(), and then replace
malloc() with libxl__malloc(), and finally cleanup this fallout.
* libxl__xc_device_get_rdm() should return proper libxl error code, ERROR_FAIL.
Then instead, the allocated RDM entries would be returned with an out
parameter.
v4:
* Consistent to use term "RDM".
* Unconditionally set *nr_entries to 0
* Grab to all sutffs to provide a parameter to set our predefined boundary
dynamically to as a separated patch later
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_dm.c | 264 +++
tools/libxl/libxl_dom.c | 17 ++-
tools/libxl/libxl_internal.h | 11 +-
tools/libxl/libxl_types.idl | 7 ++
5 files changed, 298 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f75d4f1..c8a32d5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -459,7 +459,7 @@ int libxl__domain_build(libxl__gc *gc,
switch (info->type) {
case LIBXL_DOMAIN_TYPE_HVM:
-ret = libxl__build_hvm(gc, domid, info, state);
+ret = libxl__build_hvm(gc, domid, d_config, state);
if (ret)
goto out;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 317a8eb..54b67ee 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -90,6 +90,270 @@ const char *libxl__domain_device_model(libxl__gc *gc,
return dm;
}
+static int
+libxl__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)
+{
+int rc = 0, r;
+
+/*
+ * We really can't presume how many entries we can get in advance.
+ */
+*nr_entries = 0;
+r = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
+ NULL, nr_entries);
+assert(r <= 0);
+/* "0" means we have no any rdm entry. */
+if (!r) goto out;
+
+if (errno != ENOBUFS) {
+rc = ERROR_FAIL;
+goto out;
+}
+
+*xrdm = libxl__malloc(gc,
+ *nr_entries * sizeof(xen_reserved_device_memory_t));
+r = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
+ *xrdm, nr_entries);
+if (r)
+rc = ERROR_FAIL;
+
+ out:
+if (rc) {
+*nr_entries = 0;
+*xrdm = NULL;
+LOG(ERROR, "Could not get reserved device memory maps.\n");
+}
+return rc;
+}
+
+/*
+ * Check whether there exists rdm hole in the specified memory range.
+ * Returns true if exists, else returns false.
+ */
+static bool overlaps_rdm(uint64_t start, uint64_t memsize,
+ uint64_t rdm_start, uint64_t rdm_size)
+{
+return (start + memsize > rdm_start) && (start < rdm_start + rdm_size);
+}
+
+/*
+ * Check reported RDM regions and handle potential gfn conflicts
