From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH for 4.5 v6 1/1] Add mmio_hole_size Date: Wed, 08 Oct 2014 09:41:36 -0400 Message-ID: <54353F10.2040801@terremark.com> References: <1412344118-20449-1-git-send-email-dslutz@verizon.com> <1412344118-20449-2-git-send-email-dslutz@verizon.com> <20141007140813.GE2604@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141007140813.GE2604@laptop.dumpdata.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: Ian Campbell , Stefano Stabellini , Ian Jackson , Don Slutz , xen-devel@lists.xen.org, Jan Beulich , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 10/07/14 10:08, Konrad Rzeszutek Wilk wrote: > On Fri, Oct 03, 2014 at 09:48:38AM -0400, Don Slutz wrote: >> If you add enough PCI devices then all mmio may not fit below 4G >> which may not be the layout the user wanted. This allows you to >> increase the below 4G address space that PCI devices can use and >> therefore in more cases not have any mmio that is above 4G. >> >> There are real PCI cards that do not support mmio over 4G, so if you >> want to emulate them precisely, you may also need to increase the >> space below 4G for them. There are drivers for these cards that also >> do not work if they have their mmio space mapped above 4G. >> >> This allows growing the MMIO hole to the size needed. >> >> This may help with using pci passthru and HVM. > Ha! It definitly helps in some cases with GPUs. >> Signed-off-by: Don Slutz >> --- >> v6: >> I think we should get rid of xc_hvm_build_with_hole() entirely. >> Done. >> Add text describing restrictions on hole size >> Done. >> "<=", to be consistent with the check in >> libxl__domain_create_info_setdefault () >> Done >> >> v5: >> Add LIBXL_HAVE_BUILDINFO_HVM_MMIO_HOLE_SIZE. >> >> v4: >> s/pci_hole_min_size/mmio_hole_size/ >> s/HVM_BELOW_4G_RAM_END/HVM_BELOW_4G_MMIO_START/ >> Add ignore to hvmloader message >> Adjust commit message >> Dropped min; stopped mmio_hole_size changing in hvmloader >> >> >> docs/man/xl.cfg.pod.5 | 16 +++++++++ >> tools/firmware/hvmloader/config.h | 1 - >> tools/firmware/hvmloader/pci.c | 71 +++++++++++++++++++++++++++------------ >> tools/libxl/libxl.h | 5 +++ >> tools/libxl/libxl_create.c | 29 ++++++++++++---- >> tools/libxl/libxl_dm.c | 24 +++++++++++-- >> tools/libxl/libxl_dom.c | 8 +++++ >> tools/libxl/libxl_types.idl | 1 + >> tools/libxl/xl_cmdimpl.c | 12 +++++++ >> 9 files changed, 136 insertions(+), 31 deletions(-) >> >> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 >> index 8bba21c..0a870d2 100644 >> --- a/docs/man/xl.cfg.pod.5 >> +++ b/docs/man/xl.cfg.pod.5 >> @@ -1111,6 +1111,22 @@ wallclock (i.e., real) time. >> >> =back >> >> +=head3 Memory layout >> + >> +=over 4 >> + >> +=item B >> + >> +Specifies the size the MMIO hole below 4GiB will be. Only valid for >> +device_model_version = "qemu-xen". Note: this requires QEMU 2.1.0 >> +or later. > This patch depends on 'max-ram-below-4g' being in QEMU upstream. > Has that been codified/Acked by the maintainers there? > (It would be rather cumbersome if this changed). Yes, it is part of QEMU 2.1 which has been released. > Is there a link to the latest patch-set? I currently do not have an external git server. > > Naturally to use this with Xen 4.5 one would have to replace > the qemu-xen that we ship with it, with an upstream one. Nope. The QEMU that is part of Xen also has this. Was done in: Message-ID: References: <1406554299-25744-1-git-send-email-dslutz@verizon.com> > > I believe Fedora is an distro that does that (as in > it builds using the same QEMU that KVM and Xen are using). > Are there any other ones? Not sure. > > I am conflicted about this as: > - Internally (Oracel) we have an usage for this and at some > point developed something quite similar, so I am partially > biased to this. > > - This by itself won't work with the version of QEMU-xen that > is going to be shipped in Xen 4.5. Which means it is a bit > of an dead code - but then there are some patches that we > put in Xen 4.5 that were cleanups to help with further work. > And there are also pieces of code in the hypervisor > which don't have corresponding code in the toolstack. This is wrong. The QEMU-xen that is going to be shipped with Xen 4.5 supports this. > - The 'max-ram-below-4g' not being codified makes me a bit > uneasy - as we would have to alter this when your patches > make it in QEMU v2.1 (or later). I did not understand this. > On the patch itself: > - It is isolated. It won't be run by the majority of users - hence > any bugs that it might cause are in the common code. There > does not seem much.. > > - It needs an Ack or Reviewed by the toolstack maintainers > and also from George (who touched the hvmloader last time). > > >> + >> +Cannot be smaller than 256MiB. Cannot be larger than 4GiB. >> + ... >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 8b82584..3737148 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -23,6 +23,7 @@ >> #include >> #include >> #include >> +#include >> >> int libxl__domain_create_info_setdefault(libxl__gc *gc, >> libxl_domain_create_info *c_info) >> @@ -428,13 +429,26 @@ int libxl__domain_build(libxl__gc *gc, >> vments[4] = "start_time"; >> vments[5] = libxl__sprintf(gc, "%lu.%02d", start_time.tv_sec,(int)start_time.tv_usec/10000); >> >> - localents = libxl__calloc(gc, 7, sizeof(char *)); >> - localents[0] = "platform/acpi"; >> - localents[1] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0"; >> - localents[2] = "platform/acpi_s3"; >> - localents[3] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0"; >> - localents[4] = "platform/acpi_s4"; >> - localents[5] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0"; >> + localents = libxl__calloc(gc, 9, sizeof(char *)); >> + i = 0; >> + localents[i++] = "platform/acpi"; >> + localents[i++] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0"; >> + localents[i++] = "platform/acpi_s3"; >> + localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0"; >> + localents[i++] = "platform/acpi_s4"; >> + localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s4) ? "1" : "0"; >> + if (info->u.hvm.mmio_hole_size) { >> + uint64_t max_ram_below_4g = >> + (1ULL << 32) - info->u.hvm.mmio_hole_size; >> + >> + if (max_ram_below_4g <= HVM_BELOW_4G_MMIO_START) { >> + localents[i++] = "platform/mmio_hole_size"; >> + localents[i++] = >> + libxl__sprintf(gc, "%"PRIu64, >> + info->u.hvm.mmio_hole_size); >> + } >> + } >> + assert(i < 9); > Why? Please include an comment explaining why? Ok. Does: /* Check for heap corruption, localents array size is 9 */ help? >> >> break; >> case LIBXL_DOMAIN_TYPE_PV: >> @@ -460,6 +474,7 @@ int libxl__domain_build(libxl__gc *gc, >> vments[i++] = "image/cmdline"; >> vments[i++] = (char *) state->pv_cmdline; >> } >> + assert(i < 11); > Why? Please include an comment explaining why. Ditto: /* Check for heap corruption, vments array size is 11 */ >> >> break; >> default: >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c ... >> index c734f79..0a70da6 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -34,6 +34,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "libxl.h" >> #include "libxl_utils.h" >> @@ -1111,6 +1112,17 @@ static void parse_config_data(const char *config_source, >> exit(-ERROR_FAIL); >> } >> >> + if (!xlu_cfg_get_long(config, "mmio_hole_size", &l, 0)) { >> + b_info->u.hvm.mmio_hole_size = (uint64_t) l; >> + if (b_info->u.hvm.mmio_hole_size < HVM_BELOW_4G_MMIO_LENGTH || > Oh boy, this check for 256MB is surely obfuscated by the macros. > > Could you include a comment saying that HVM_BELOW_4G_MMIO_LENGTH resolves > to 256MB please? Ok. -Don Slutz >> + b_info->u.hvm.mmio_hole_size > HVM_BELOW_4G_MMIO_START) { >> + fprintf(stderr, "ERROR: invalid value 0x%"PRIx64" (%"PRIu64 >> + ") for \"mmio_hole_size\"\n", >> + b_info->u.hvm.mmio_hole_size, >> + b_info->u.hvm.mmio_hole_size); >> + exit (1); >> + } >> + } >> if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) { >> const char *s = libxl_timer_mode_to_string(l); >> fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer is deprecated. " >> -- >> 1.8.4 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel