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 16:58:52 -0400 Message-ID: <5435A58C.1070904@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> <54353F10.2040801@terremark.com> <20141008144921.GB18573@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: <20141008144921.GB18573@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 , Don Slutz Cc: Ian Campbell , Stefano Stabellini , Ian Jackson , xen-devel@lists.xen.org, Jan Beulich , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org On 10/08/14 10:49, Konrad Rzeszutek Wilk wrote: > On Wed, Oct 08, 2014 at 09:41:36AM -0400, Don Slutz wrote: >> 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. > I think you might just want to ditch that information as > .. Ok, I can drop the Note: >> >>> 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. > .. you say that the version of QEMU-xen that is going to be with > Xen 4.5 will have 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. > You can ignore that sentence. Somehow I was thinking your patches > for QEMU were not in the QEMU code base - as I read your 'QEMU 2.1.0 > or later' as - 'later' == 'not in tree yet.'. > >>> 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 */ > I was thinking more of - why even the need for them? It is pretty > evident what the value would be - so you could just ditch them. > Unless Ian or Wei think it should be in there. If I do not hear anything, I will drop them. -Don Slutz > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel