Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Chen, Tiejun writes (Re: [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM): We just need to sync two patches: Thanks. Note I just compiled with these changes since right now I can't access any machine to test. I intend to produce a git branch RSN. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
OOPS! Please refer to this version: (One miss changing flag to flags in patch #11 although we can compile successfully.) #1. To patch #8 diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 2991333..9c5ef8b 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1316,7 +1316,7 @@ int xc_get_machine_memory_map(xc_interface *xch, uint32_t max_entries); int xc_reserved_device_memory_map(xc_interface *xch, - uint32_t flag, + uint32_t flags, uint16_t seg, uint8_t bus, uint8_t devfn, diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c index 298b3b5..1b074b7 100644 --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -686,7 +686,7 @@ int xc_domain_set_memory_map(xc_interface *xch, } int xc_reserved_device_memory_map(xc_interface *xch, - uint32_t flag, + uint32_t flags, uint16_t seg, uint8_t bus, uint8_t devfn, @@ -695,11 +695,11 @@ int xc_reserved_device_memory_map(xc_interface *xch, { int rc; struct xen_reserved_device_memory_map xrdmmap = { -.flag = flag, -.seg = seg, -.bus = bus, -.devfn = devfn, -.nr_entries = *max_entries +.flags = flags, +.nr_entries = *max_entries, +.dev.pci.seg = seg, +.dev.pci.bus = bus, +.dev.pci.devfn = devfn, }; DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct xen_reserved_device_memory) * #2. To patch #11 diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 29476fc..8d103c3 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -94,7 +94,7 @@ const char *libxl__domain_device_model(libxl__gc *gc, static int libxl__xc_device_get_rdm(libxl__gc *gc, - uint32_t flag, + uint32_t flags, uint16_t seg, uint8_t bus, uint8_t devfn, @@ -107,7 +107,7 @@ libxl__xc_device_get_rdm(libxl__gc *gc, * 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, +r = xc_reserved_device_memory_map(CTX-xch, flags, seg, bus, devfn, NULL, nr_entries); assert(r = 0); /* 0 means we have no any rdm entry. */ @@ -119,7 +119,7 @@ libxl__xc_device_get_rdm(libxl__gc *gc, } GCNEW_ARRAY(*xrdm, *nr_entries); -r = xc_reserved_device_memory_map(CTX-xch, flag, seg, bus, devfn, +r = xc_reserved_device_memory_map(CTX-xch, flags, seg, bus, devfn, *xrdm, nr_entries); if (r) rc = ERROR_FAIL; @@ -212,7 +212,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc, unsigned int nr_entries; /* Collect all rdm info if exist. */ -rc = libxl__xc_device_get_rdm(gc, PCI_DEV_RDM_ALL, +rc = libxl__xc_device_get_rdm(gc, XENMEM_RDM_ALL, 0, 0, 0, nr_entries, xrdm); if (rc) goto out; @@ -240,7 +240,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc, devfn = PCI_DEVFN(d_config-pcidevs[i].dev, d_config-pcidevs[i].func); nr_entries = 0; -rc = libxl__xc_device_get_rdm(gc, ~PCI_DEV_RDM_ALL, +rc = libxl__xc_device_get_rdm(gc, 0, seg, bus, devfn, nr_entries, xrdm); if (rc) goto out; Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
I intend to produce a git branch RSN. On the staging? Tomorrow I can pull to check/test this. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 22.07.15 at 12:04, ian.jack...@eu.citrix.com wrote: Tiejun Chen writes ([v11][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. Acked-by: Ian Jackson ian.jack...@eu.citrix.com Did you intentionally not move this forward to the v13 you just sent? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 22.07.15 at 03:30, tiejun.c...@intel.com wrote: CC: Ian Jackson ian.jack...@eu.citrix.com CC: Stefano Stabellini stefano.stabell...@eu.citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Wei Liu wei.l...@citrix.com Acked-by: Wei Liu wei.l...@citrix.com Signed-off-by: Tiejun Chen tiejun.c...@intel.com Reviewed-by: Kevin Tian kevin.t...@intel.com --- v11: * Use GCNEW_ARRAY to replace libxl__malloc() * #define pfn_to_paddrk is missing safety () around x, and move this into libxl_internal.h * Rename set_rdm_entries() to add_rdm_entry() and put the increment at the end so that the assignments are to -rdms[d_config-num_rdms]. * Simply make it so that if there are any rdms specified in the domain config, they are used instead of the automatically gathered information (from strategy and devices). So just return if d_config-rmds is valid. * Shorten some code comments. I think it is not the first time that we're pointing out to you that when you make not just cosmetic changes, review and ack tags should be dropped. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Tiejun Chen writes ([v11][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. Acked-by: Ian Jackson ian.jack...@eu.citrix.com ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 22.07.15 at 10:52, tiejun.c...@intel.com wrote: On 2015/7/22 16:28, Jan Beulich wrote: On 22.07.15 at 03:30, tiejun.c...@intel.com wrote: CC: Ian Jackson ian.jack...@eu.citrix.com CC: Stefano Stabellini stefano.stabell...@eu.citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Wei Liu wei.l...@citrix.com Acked-by: Wei Liu wei.l...@citrix.com Signed-off-by: Tiejun Chen tiejun.c...@intel.com Reviewed-by: Kevin Tian kevin.t...@intel.com --- v11: * Use GCNEW_ARRAY to replace libxl__malloc() * #define pfn_to_paddrk is missing safety () around x, and move this into libxl_internal.h * Rename set_rdm_entries() to add_rdm_entry() and put the increment at the end so that the assignments are to -rdms[d_config-num_rdms]. * Simply make it so that if there are any rdms specified in the domain config, they are used instead of the automatically gathered information (from strategy and devices). So just return if d_config-rmds is valid. * Shorten some code comments. I think it is not the first time that we're pointing out to you that when you make not just cosmetic changes, review and ack tags should be dropped. I don't recall this sort of requirement was mentioned. Instead, this is new to me. So where can I found this warning you said previously? Even if I misremember and didn't send such a request your way, if you read xen-devel you'd have seen this quite a number of times. Furthermore, you ask me to drop Reviewed-by/Acked-by in this revision, what's next? Just to this example, No.1 revision: Acked-by: Wei Liu wei.l...@citrix.com Reviewed-by: Kevin Tian kevin.t...@intel.com No.2 revision: I addressed some comments raised by Jackson. But you mean Reviewed-by/Acked-by should be dropped. Not unilaterally - it depends on the nature of the changes and the area of code the tag was offered for (I'd tend to say that reviews normally cover the whole patch, but acks may have limited meaning and hence may be retained in a wider set of cases). No.3 revision: I assume Jackson Ack or Review to this so I should leave one line like this, Reviewed-by: Ian Jackson ian.jack...@eu.citrix.com without two previous Acked-by/Reviewed-by, right? So looks like the latter always override the former, right? And I also can't understand why we should drop Reviewed-by/Acked-by from other guys. And, all new comments I addressed don't conflict with our previous revision so why? In the case at hand you even had to adjust the algorithm (whether and how to honor incoming data). I.e. the question isn't whether there's a conflict, but whether there's any adjustment that may, from an abstract pov, result in the person having offered such a tag to now possibly have a different opinion. You have to face it - (s)he may be of the opinion that the change was wrong (reviewers make mistakes just like contributors do) or the suggestion wrongly carried out. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On Wed, Jul 22, 2015 at 9:52 AM, Chen, Tiejun tiejun.c...@intel.com wrote: And I also can't understand why we should drop Reviewed-by/Acked-by from other guys. And, all new comments I addressed don't conflict with our previous revision so why? You must remove Reviewed-by / Acked-by *for a given patch* *when you change that patch in a significant way*. Acked-by: John Smith is saying that John Smith is OK with the patch the way that it is. But just because John Smith was OK with the patch before you made changes doesn't mean he's OK with the patch after you made changes. Very small changes -- like shifting whitespace or fixing typos -- are usually OK to keep the Ack. But if you make anything more substantial -- even if you just reword a comment significantly -- then you have to drop the Acked-by or Reviewed-by, and get them again. (Usually if the changes really are minor then it's fairly quick to get them again.) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Tiejun Chen writes ([v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM): Acked-by: Wei Liu wei.l...@citrix.com I have dropped Wei's ack on this from my git branch, as it was clearly inappropriate to retain it. (v11 was acked by me, so there is no need for a re-review by Wei unless he feels it would be useful.) Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 2015/7/22 21:24, Ian Jackson wrote: Tiejun Chen writes ([v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM): Acked-by: Wei Liu wei.l...@citrix.com I have dropped Wei's ack on this from my git branch, as it was clearly inappropriate to retain it. (v11 was acked by me, so there is no need for a re-review by Wei unless he feels it would be useful.) Ian, Sounds you start to merge them into your tree? But now Jan is trying to update patch #1 as you see. I think something needs to be synced on tool sides. Although that is not finished, at least three changes exist: #1. PCI_DEV_RDM_ALL - XENMEM_RDM_ALL #2. new public memop interface structure #2.1 +union { +struct physdev_pci_device pci; +} dev; #2.2 flag - flags Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 2015/7/22 16:28, Jan Beulich wrote: On 22.07.15 at 03:30, tiejun.c...@intel.com wrote: CC: Ian Jackson ian.jack...@eu.citrix.com CC: Stefano Stabellini stefano.stabell...@eu.citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Wei Liu wei.l...@citrix.com Acked-by: Wei Liu wei.l...@citrix.com Signed-off-by: Tiejun Chen tiejun.c...@intel.com Reviewed-by: Kevin Tian kevin.t...@intel.com --- v11: * Use GCNEW_ARRAY to replace libxl__malloc() * #define pfn_to_paddrk is missing safety () around x, and move this into libxl_internal.h * Rename set_rdm_entries() to add_rdm_entry() and put the increment at the end so that the assignments are to -rdms[d_config-num_rdms]. * Simply make it so that if there are any rdms specified in the domain config, they are used instead of the automatically gathered information (from strategy and devices). So just return if d_config-rmds is valid. * Shorten some code comments. I think it is not the first time that we're pointing out to you that when you make not just cosmetic changes, review and ack tags should be dropped. I don't recall this sort of requirement was mentioned. Instead, this is new to me. So where can I found this warning you said previously? Furthermore, you ask me to drop Reviewed-by/Acked-by in this revision, what's next? Just to this example, No.1 revision: Acked-by: Wei Liu wei.l...@citrix.com Reviewed-by: Kevin Tian kevin.t...@intel.com No.2 revision: I addressed some comments raised by Jackson. But you mean Reviewed-by/Acked-by should be dropped. No.3 revision: I assume Jackson Ack or Review to this so I should leave one line like this, Reviewed-by: Ian Jackson ian.jack...@eu.citrix.com without two previous Acked-by/Reviewed-by, right? So looks like the latter always override the former, right? And I also can't understand why we should drop Reviewed-by/Acked-by from other guys. And, all new comments I addressed don't conflict with our previous revision so why? Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
On 2015/7/22 22:04, Ian Jackson wrote: Chen, Tiejun writes (Re: [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM): Sounds you start to merge them into your tree? But now Jan is trying to update patch #1 as you see. I think something needs to be synced on tool sides. Although that is not finished, at least three changes exist: Thanks. I am negotiating with Jan et al on IRC. We just need to sync two patches: #1. To patch #8: diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 2991333..9c5ef8b 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1316,7 +1316,7 @@ int xc_get_machine_memory_map(xc_interface *xch, uint32_t max_entries); int xc_reserved_device_memory_map(xc_interface *xch, - uint32_t flag, + uint32_t flags, uint16_t seg, uint8_t bus, uint8_t devfn, diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c index 298b3b5..1b074b7 100644 --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -686,7 +686,7 @@ int xc_domain_set_memory_map(xc_interface *xch, } int xc_reserved_device_memory_map(xc_interface *xch, - uint32_t flag, + uint32_t flags, uint16_t seg, uint8_t bus, uint8_t devfn, @@ -695,11 +695,11 @@ int xc_reserved_device_memory_map(xc_interface *xch, { int rc; struct xen_reserved_device_memory_map xrdmmap = { -.flag = flag, -.seg = seg, -.bus = bus, -.devfn = devfn, -.nr_entries = *max_entries +.flags = flags, +.nr_entries = *max_entries, +.dev.pci.seg = seg, +.dev.pci.bus = bus, +.dev.pci.devfn = devfn, }; DECLARE_HYPERCALL_BOUNCE(entries, sizeof(struct xen_reserved_device_memory) * #2. To patch #11: diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 29476fc..40b2bba 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -212,7 +212,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc, unsigned int nr_entries; /* Collect all rdm info if exist. */ -rc = libxl__xc_device_get_rdm(gc, PCI_DEV_RDM_ALL, +rc = libxl__xc_device_get_rdm(gc, XENMEM_RDM_ALL, 0, 0, 0, nr_entries, xrdm); if (rc) goto out; @@ -240,7 +240,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc, devfn = PCI_DEVFN(d_config-pcidevs[i].dev, d_config-pcidevs[i].func); nr_entries = 0; -rc = libxl__xc_device_get_rdm(gc, ~PCI_DEV_RDM_ALL, +rc = libxl__xc_device_get_rdm(gc, 0, seg, bus, devfn, nr_entries, xrdm); if (rc) goto out; Note I just compiled with these changes since right now I can't access any machine to test. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM
Chen, Tiejun writes (Re: [v11][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM): Sounds you start to merge them into your tree? But now Jan is trying to update patch #1 as you see. I think something needs to be synced on tool sides. Although that is not finished, at least three changes exist: Thanks. I am negotiating with Jan et al on IRC. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [v11][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 ian.jack...@eu.citrix.com CC: Stefano Stabellini stefano.stabell...@eu.citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Wei Liu wei.l...@citrix.com Acked-by: Wei Liu wei.l...@citrix.com Signed-off-by: Tiejun Chen tiejun.c...@intel.com Reviewed-by: Kevin Tian kevin.t...@intel.com --- v11: * Use GCNEW_ARRAY to replace libxl__malloc() * #define pfn_to_paddrk is missing safety () around x, and move this into libxl_internal.h * Rename set_rdm_entries() to add_rdm_entry() and put the increment at the end so that the assignments are to -rdms[d_config-num_rdms]. * Simply make it so that if there are any rdms specified in the domain config, they are used instead of the automatically gathered information (from strategy and devices). So just return if d_config-rmds is valid. * Shorten some code comments. v9 ~ v10: * Nothing is changed. v8: * Introduce pfn_to_paddr(x) - ((uint64_t)x XC_PAGE_SHIFT) and set_rdm_entries() to factor out current codes. 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 | 274 +++ tools/libxl/libxl_dom.c | 17 ++- tools/libxl/libxl_internal.h | 14 ++- tools/libxl/libxl_types.idl | 7 ++ 5 files changed, 311 insertions(+), 3 deletions(-) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 7c884c4..5b57062 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -407,7 +407,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 634b8d2..29476fc 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -92,6 +92,280 @@ 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; +} + +