From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 8/8] libxl, hvmloader: Don't relocate memory for MMIO hole Date: Fri, 21 Jun 2013 09:31:55 +0100 Message-ID: <51C40F7B.8010803@eu.citrix.com> References: <1371746007-19073-1-git-send-email-george.dunlap@eu.citrix.com> <1371746007-19073-9-git-send-email-george.dunlap@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Keir Fraser , Ian Campbell , Hanweidong , xen-devel@lists.xen.org, Stefano Stabellini , Ian Jackson List-Id: xen-devel@lists.xenproject.org On 20/06/13 18:38, Stefano Stabellini wrote: > On Thu, 20 Jun 2013, George Dunlap wrote: >> At the moment, qemu-xen can't handle memory being relocated by >> hvmloader. This may happen if a device with a large enough memory >> region is passed through to the guest. At the moment, if this >> happens, then at some point in the future qemu will crash and the >> domain will hang. (qemu-traditional is fine.) >> >> It's too late in the release to do a proper fix, so we try to do >> damage control. >> >> hvmloader already has mechanisms to relocate memory to 64-bit space >> if it can't make a big enough MMIO hole. By default this is 2GiB; if >> we just refuse to make the hole bigger if it will overlap with guest >> memory, then the relocation will happen by default. >> >> v3: >> - Fix polarity of comparison >> - Move diagnostic messages to another patch >> - Tested with xen platform pci device hacked to have different BAR sizes >> {256MiB, 1GiB} x {qemu-xen, qemu-traditional} x various memory >> configurations >> - Add comment explaining why we default to "allow" >> - Remove cast to bool >> v2: >> - style fixes >> - fix and expand comment on the MMIO hole loop >> - use "%d" rather than "%s" -> (...)?"1":"0" >> - use bool instead of uint8_t >> - Move 64-bit bar relocate detection to another patch >> - Add more diagnostic messages >> >> Signed-off-by: George Dunlap >> CC: Ian Campbell >> CC: Ian Jackson >> CC: Stefano Stabellini >> CC: Hanweidong >> CC: Keir Fraser >> CC: Keir Fraser >> --- >> tools/firmware/hvmloader/pci.c | 49 +++++++++++++++++++++++++++++-- >> tools/libxl/libxl_dm.c | 6 ++++ >> xen/include/public/hvm/hvm_xs_strings.h | 1 + >> 3 files changed, 54 insertions(+), 2 deletions(-) >> >> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c >> index 3108c8a..2364177 100644 >> --- a/tools/firmware/hvmloader/pci.c >> +++ b/tools/firmware/hvmloader/pci.c >> @@ -27,6 +27,8 @@ >> >> #include >> #include >> +#include >> +#include >> >> unsigned long pci_mem_start = PCI_MEM_START; >> unsigned long pci_mem_end = PCI_MEM_END; >> @@ -57,6 +59,32 @@ void pci_setup(void) >> } *bars = (struct bars *)scratch_start; >> unsigned int i, nr_bars = 0; >> >> + const char *s; >> + /* >> + * Do we allow hvmloader to relocate guest memory in order to >> + * increase the size of the lowmem MMIO hole? Defaulting to 1 >> + * here will mean that non-libxl toolstacks (including xend and >> + * home-grown ones) will experience this series as "no change". >> + * It does mean that those using qemu-xen will still experience >> + * the bug (described below); but it also means that those using >> + * qemu-traditional will *not* experience any change; and it also >> + * means that there is a work-around for those using qemu-xen, >> + * namely switching to qemu-traditional. >> + * >> + * If we defaulted to 0, and failing to resize the hole caused any >> + * problems with qemu-traditional, then there is no work-around. >> + * >> + * Since xend can't talk to qemu-traditional, I think this is the >> + * option that will have the least impact. >> + */ >> + bool allow_memory_relocate = 1; >> + >> + s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL); >> + if ( s ) >> + allow_memory_relocate = strtoll(s, NULL, 0); > Considering that strtoll retuns a long long, are we sure that this > allocation does what we want for all the possible long long values > that can be returned? > > For example, if strtoll returns -1, do we want allow_memory_relocate to > be set to true? The only valid values here are "0" and "1"; everything else is undefined. Look, the bike shed is already painted, the brushes have been washed and put away. Leave it be. :-) > > > >> + printf("Relocating guest memory for lowmem MMIO space %s\n", >> + allow_memory_relocate?"enabled":"disabled"); >> + >> /* Program PCI-ISA bridge with appropriate link routes. */ >> isa_irq = 0; >> for ( link = 0; link < 4; link++ ) >> @@ -208,8 +236,25 @@ void pci_setup(void) >> pci_writew(devfn, PCI_COMMAND, cmd); >> } >> >> - while ( (mmio_total > (pci_mem_end - pci_mem_start)) && >> - ((pci_mem_start << 1) != 0) ) >> + /* >> + * At the moment qemu-xen can't deal with relocated memory regions. >> + * It's too close to the release to make a proper fix; for now, >> + * only allow the MMIO hole to grow large enough to move guest memory >> + * if we're running qemu-traditional. Items that don't fit will be >> + * relocated into the 64-bit address space. >> + * >> + * This loop now does the following: >> + * - If allow_memory_relocate, increase the MMIO hole until it's >> + * big enough, or until it's 2GiB >> + * - If !allow_memory_relocate, increase the MMIO hole until it's >> + * big enough, or until it's 2GiB, or until it overlaps guest >> + * memory >> + */ >> + while ( (mmio_total > (pci_mem_end - pci_mem_start)) >> + && ((pci_mem_start << 1) != 0) >> + && (allow_memory_relocate >> + || (((pci_mem_start << 1) >> PAGE_SHIFT) >> + >= hvm_info->low_mem_pgend)) ) >> pci_mem_start <<= 1; >> >> if ( mmio_total > (pci_mem_end - pci_mem_start) ) { >> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c >> index ac1f90e..342d2ce 100644 >> --- a/tools/libxl/libxl_dm.c >> +++ b/tools/libxl/libxl_dm.c >> @@ -1154,6 +1154,12 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss) >> libxl__xs_write(gc, XBT_NULL, >> libxl__sprintf(gc, "%s/hvmloader/bios", path), >> "%s", libxl_bios_type_to_string(b_info->u.hvm.bios)); >> + /* Disable relocating memory to make the MMIO hole larger >> + * unless we're running qemu-traditional */ >> + libxl__xs_write(gc, XBT_NULL, >> + libxl__sprintf(gc, "%s/hvmloader/allow-memory-relocate", path), >> + "%d", >> + b_info->device_model_version==LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL); > line length Blame the person who made a *47-character* identifier. Seriously, would you rather have the "==" on a separate line or something? That seems really ridiculous. I'll break the sprintf though. -George