While playing with this, I've noticed a number of issues, some actual bugs, some merely cosmetic (at least at this point in time. This is the collection of adjustments made, with bug fixes first. 1: honor multiple per-device reserved memory regions 2: establish per-device reserved memory policy earlier 3: make "rdm=" parsing comply with documentation 4: pass correct "hotplug" argument to libxl__device_pci_setdefault() 5: align reserved device memory boundary for HAP guests Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:44:11PM +0100, Jan Beulich wrote: > While playing with this, I've noticed a number of issues, > some actual bugs, some merely cosmetic (at least at this point > in time. This is the collection of adjustments made, with bug > fixes first. I will leave committing this series to you because your patches don't seem to work with git-am. In any case, please allow some time for Anthony and Ian to comment on this series. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
While in "host" strategy all regions get processed, of the per-device
ones only the first entry has been consumed so far.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -471,8 +471,7 @@ int libxl__domain_device_construct_rdm(l
/* Query RDM entries per-device */
for (i = 0; i < d_config->num_pcidevs; i++) {
- unsigned int nr_entries;
- bool new = true;
+ unsigned int n, nr_entries;
seg = d_config->pcidevs[i].domain;
bus = d_config->pcidevs[i].bus;
@@ -489,36 +488,41 @@ int libxl__domain_device_construct_rdm(l
assert(xrdm);
- /*
- * Need to check whether this entry is already saved in the array.
- * This could come from two cases:
- *
- * - user may configure to get all RDMs in this platform, which
- * is already queried before this point
- * - or two assigned devices may share one RDM entry
- *
- * Different policies may be configured on the same RDM due to
- * above two cases. But we don't allow to assign such a group
- * devies right now so it doesn't come true in our case.
- */
- for (j = 0; j < d_config->num_rdms; j++) {
- if (d_config->rdms[j].start == pfn_to_paddr(xrdm[0].start_pfn))
- {
- /*
- * So the per-device policy always override the global
- * policy in this case.
- */
- d_config->rdms[j].policy = d_config->pcidevs[i].rdm_policy;
- new = false;
- break;
+ for (n = 0; n < nr_entries; ++n) {
+ bool new = true;
+
+ /*
+ * Need to check whether this entry is already saved in the
+ * array. This could come from two cases:
+ *
+ * - user may configure to get all RDMs in this platform,
+ * which is already queried before this point
+ * - or two assigned devices may share one RDM entry
+ *
+ * Different policies may be configured on the same RDM due to
+ * above two cases. But we don't allow to assign such a group
+ * of devices right now so it doesn't come true in our case.
+ */
+ for (j = 0; j < d_config->num_rdms; j++) {
+ if (d_config->rdms[j].start
+ == pfn_to_paddr(xrdm[n].start_pfn))
+ {
+ /*
+ * So the per-device policy always override the
+ * global policy in this case.
+ */
+ d_config->rdms[j].policy
+ = d_config->pcidevs[i].rdm_policy;
+ new = false;
+ break;
+ }
}
- }
- if (new) {
- add_rdm_entry(gc, d_config,
- pfn_to_paddr(xrdm[0].start_pfn),
- pfn_to_paddr(xrdm[0].nr_pages),
- d_config->pcidevs[i].rdm_policy);
+ if (new)
+ add_rdm_entry(gc, d_config,
+ pfn_to_paddr(xrdm[n].start_pfn),
+ pfn_to_paddr(xrdm[n].nr_pages),
+ d_config->pcidevs[i].rdm_policy);
}
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:46:17PM +0100, Jan Beulich wrote: > While in "host" strategy all regions get processed, of the per-device > ones only the first entry has been consumed so far. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Wei Liu <wl@xen.org> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Reserved device memory region processing as well as E820 table creation
happen earlier than the adding of (PCI) devices, yet they consume the
policy (e.g. to decide whether to add entries to the E820 table). But so
far it was only at the stage of PCI device addition that the final
policy was established (i.e. if not explicitly specified by the guest
config file).
Note that I couldn't find the domain ID to be available in
libxl__domain_device_construct_rdm(), but observing that
libxl__device_pci_setdefault() also doesn't use it, for the time being
DOMID_INVALID gets passed. An obvious alternative would be to drop the
unused parameter/argument, yet at that time the question would be
whether to also drop other unused ones.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -488,6 +488,11 @@ int libxl__domain_device_construct_rdm(l
assert(xrdm);
+ rc = libxl__device_pci_setdefault(gc, DOMID_INVALID,
+ &d_config->pcidevs[i], false);
+ if (rc)
+ goto out;
+
for (n = 0; n < nr_entries; ++n) {
bool new = true;
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1709,6 +1709,8 @@ _hidden void libxl__device_pci_add(libxl
libxl__ao_device *aodev);
_hidden void libxl__device_pci_destroy_all(libxl__egc *egc, uint32_t domid,
libxl__multidev *);
+_hidden int libxl__device_pci_setdefault(libxl__gc *gc, uint32_t domid,
+ libxl_device_pci *pci, bool hotplug);
_hidden bool libxl__is_igd_vga_passthru(libxl__gc *gc,
const libxl_domain_config *d_config);
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1483,8 +1483,8 @@ static int libxl__device_pci_reset(libxl
return -1;
}
-static int libxl__device_pci_setdefault(libxl__gc *gc, uint32_t domid,
- libxl_device_pci *pci, bool hotplug)
+int libxl__device_pci_setdefault(libxl__gc *gc, uint32_t domid,
+ libxl_device_pci *pci, bool hotplug)
{
/* We'd like to force reserve rdm specific to a device by default.*/
if (pci->rdm_policy == LIBXL_RDM_RESERVE_POLICY_INVALID)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:46:44PM +0100, Jan Beulich wrote: > Reserved device memory region processing as well as E820 table creation > happen earlier than the adding of (PCI) devices, yet they consume the > policy (e.g. to decide whether to add entries to the E820 table). But so > far it was only at the stage of PCI device addition that the final > policy was established (i.e. if not explicitly specified by the guest > config file). > > Note that I couldn't find the domain ID to be available in > libxl__domain_device_construct_rdm(), but observing that > libxl__device_pci_setdefault() also doesn't use it, for the time being > DOMID_INVALID gets passed. An obvious alternative would be to drop the > unused parameter/argument, yet at that time the question would be > whether to also drop other unused ones. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Wei Liu <wl@xen.org> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Documentation says "<RDM_RESERVATION_STRING> is a comma separated list
of <KEY=VALUE> settings, from the following list". There's no mention
of a specific order, yet so far the parsing logic did accept only
strategy, then policy (and neither of the two omitted). Make "state"
move
- back to STATE_TYPE when finding a comma after having parsed the
<VALUE> part of a setting,
- to STATE_TERMINAL otherwise.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxlu_pci.c
+++ b/tools/libxl/libxlu_pci.c
@@ -194,9 +194,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl
switch(state) {
case STATE_TYPE:
if (*ptr == '=') {
- state = STATE_RDM_STRATEGY;
*ptr = '\0';
- if (strcmp(tok, "strategy")) {
+ if (!strcmp(tok, "strategy")) {
+ state = STATE_RDM_STRATEGY;
+ } else if (!strcmp(tok, "policy")) {
+ state = STATE_RESERVE_POLICY;
+ } else {
XLU__PCI_ERR(cfg, "Unknown RDM state option: %s", tok);
goto parse_error;
}
@@ -205,7 +208,7 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl
break;
case STATE_RDM_STRATEGY:
if (*ptr == '\0' || *ptr == ',') {
- state = STATE_RESERVE_POLICY;
+ state = *ptr == ',' ? STATE_TYPE : STATE_TERMINAL;
*ptr = '\0';
if (!strcmp(tok, "host")) {
rdm->strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST;
@@ -217,19 +220,8 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl
}
break;
case STATE_RESERVE_POLICY:
- if (*ptr == '=') {
- state = STATE_OPTIONS_V;
- *ptr = '\0';
- if (strcmp(tok, "policy")) {
- XLU__PCI_ERR(cfg, "Unknown RDM property value: %s", tok);
- goto parse_error;
- }
- tok = ptr + 1;
- }
- break;
- case STATE_OPTIONS_V:
if (*ptr == ',' || *ptr == '\0') {
- state = STATE_TERMINAL;
+ state = *ptr == ',' ? STATE_TYPE : STATE_TERMINAL;
*ptr = '\0';
if (!strcmp(tok, "strict")) {
rdm->policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:47:04PM +0100, Jan Beulich wrote: > Documentation says "<RDM_RESERVATION_STRING> is a comma separated list > of <KEY=VALUE> settings, from the following list". There's no mention > of a specific order, yet so far the parsing logic did accept only > strategy, then policy (and neither of the two omitted). Make "state" > move > - back to STATE_TYPE when finding a comma after having parsed the > <VALUE> part of a setting, > - to STATE_TERMINAL otherwise. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Wei Liu <wl@xen.org> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Uniformly passing "false" can't be right, but has been benign because of
the function not using this parameter.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1567,7 +1567,7 @@ void libxl__device_pci_add(libxl__egc *e
}
}
- rc = libxl__device_pci_setdefault(gc, domid, pcidev, false);
+ rc = libxl__device_pci_setdefault(gc, domid, pcidev, !starting);
if (rc) goto out;
if (pcidev->seize && !pciback_dev_is_assigned(gc, pcidev)) {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:47:27PM +0100, Jan Beulich wrote: > Uniformly passing "false" can't be right, but has been benign because of > the function not using this parameter. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Wei Liu <wl@xen.org> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
As the code comment says, this will allow use of a 2Mb super page
mapping at the end of "low" memory.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -563,6 +563,13 @@ int libxl__domain_device_construct_rdm(l
/* Just check if RDM > our memory boundary. */
if (rdm_start > rdm_mem_boundary) {
/*
+ * For HAP guests round down to a 2Mb boundary to allow use
+ * of large page mappings.
+ */
+ if (libxl_defbool_val(d_config->c_info.hap)
+ && rdm_start > 0x200000)
+ rdm_start &= ~0x1fffff;
+ /*
* We will move downwards lowmem_end so we have to expand
* highmem_end.
*/
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Tue, Feb 18, 2020 at 04:47:49PM +0100, Jan Beulich wrote:
> As the code comment says, this will allow use of a 2Mb super page
> mapping at the end of "low" memory.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -563,6 +563,13 @@ int libxl__domain_device_construct_rdm(l
> /* Just check if RDM > our memory boundary. */
> if (rdm_start > rdm_mem_boundary) {
> /*
> + * For HAP guests round down to a 2Mb boundary to allow use
> + * of large page mappings.
> + */
> + if (libxl_defbool_val(d_config->c_info.hap)
> + && rdm_start > 0x200000)
Please use MB(2) here.
With this fixed:
Acked-by: Wei Liu <wl@xen.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.02.2020 12:43, Wei Liu wrote:
> On Tue, Feb 18, 2020 at 04:47:49PM +0100, Jan Beulich wrote:
>> As the code comment says, this will allow use of a 2Mb super page
>> mapping at the end of "low" memory.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -563,6 +563,13 @@ int libxl__domain_device_construct_rdm(l
>> /* Just check if RDM > our memory boundary. */
>> if (rdm_start > rdm_mem_boundary) {
>> /*
>> + * For HAP guests round down to a 2Mb boundary to allow use
>> + * of large page mappings.
>> + */
>> + if (libxl_defbool_val(d_config->c_info.hap)
>> + && rdm_start > 0x200000)
>
> Please use MB(2) here.
Will do, but then what about the ~0x1fffff on the next line? Should
this become ~(MB(2) - 1)?
> With this fixed:
>
> Acked-by: Wei Liu <wl@xen.org>
Thanks.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Thu, Feb 20, 2020 at 12:45:54PM +0100, Jan Beulich wrote:
> On 20.02.2020 12:43, Wei Liu wrote:
> > On Tue, Feb 18, 2020 at 04:47:49PM +0100, Jan Beulich wrote:
> >> As the code comment says, this will allow use of a 2Mb super page
> >> mapping at the end of "low" memory.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/tools/libxl/libxl_dm.c
> >> +++ b/tools/libxl/libxl_dm.c
> >> @@ -563,6 +563,13 @@ int libxl__domain_device_construct_rdm(l
> >> /* Just check if RDM > our memory boundary. */
> >> if (rdm_start > rdm_mem_boundary) {
> >> /*
> >> + * For HAP guests round down to a 2Mb boundary to allow use
> >> + * of large page mappings.
> >> + */
> >> + if (libxl_defbool_val(d_config->c_info.hap)
> >> + && rdm_start > 0x200000)
> >
> > Please use MB(2) here.
>
> Will do, but then what about the ~0x1fffff on the next line? Should
> this become ~(MB(2) - 1)?
Yes that would be good too.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.