From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v5 1/1] Add mmio_hole_size Date: Tue, 7 Oct 2014 12:25:19 +0100 Message-ID: <5433CD9F.1020308@eu.citrix.com> References: <1410452423-11345-1-git-send-email-dslutz@verizon.com> <1410452423-11345-2-git-send-email-dslutz@verizon.com> <542C2CDB.4090307@terremark.com> <542D1C96020000780003BCE5@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <542D1C96020000780003BCE5@mail.emea.novell.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: Jan Beulich , Don Slutz Cc: Boris Ostrovsky , "xen-devel@lists.xen.org" , Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 10/02/2014 08:36 AM, Jan Beulich wrote: >>>> On 01.10.14 at 18:33, wrote: >> On 09/30/14 09:15, George Dunlap wrote: >>> On Thu, Sep 11, 2014 at 5:20 PM, Don Slutz wrote: >>>> @@ -237,26 +243,49 @@ void pci_setup(void) >>>> pci_writew(devfn, PCI_COMMAND, cmd); >>>> } >>>> >>>> - /* >>>> - * 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_hole_size ) >>>> + { >>>> + uint64_t max_ram_below_4g = (1ULL << 32) - mmio_hole_size; >>>> + >>>> + if ( max_ram_below_4g > HVM_BELOW_4G_MMIO_START ) >>>> + { >>>> + printf("max_ram_below_4g=0x"PRIllx >>>> + " too big for mmio_hole_size=0x"PRIllx >>>> + " has been ignored.\n", >>>> + PRIllx_arg(max_ram_below_4g), >>>> + PRIllx_arg(mmio_hole_size)); >>>> + } >>>> + else >>>> + { >>>> + pci_mem_start = max_ram_below_4g; >>>> + printf("pci_mem_start=0x%lx (was 0x%x) for mmio_hole_size=%lu\n", >>>> + pci_mem_start, HVM_BELOW_4G_MMIO_START, >>>> + (long)mmio_hole_size); >>>> + } >>>> + } >>>> + else >>>> + { >>>> + /* >>>> + * 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; >>>> + } >>> I don't think these need to be disjoint. There's no reason you >>> couldn't set the size for the default, and then allow the code to make >>> it bigger for guests which allow that. >> The support for changing mmio_hole_size is still "missing" from QEMU. >> So this code only works for qemu-traditional. I think Jan said >> back on v1 or v2 (sorry, e-mail issues) that since this is a config, >> disable the auto changing code. > Because it didn't seem like you would want to properly take care > of both cases together (iirc the fact that the configured hole size > could be other than a power of 2 introduced a conflict with the > current resizing logic). I.e. doing one or the other is a suitable > first step imo, but with room for improvement. Right -- I hadn't thought about the power of 2 thing for the current resizing logic. That will take some actual fixing, so it's probably best if we put that off until later. > >>>> --- a/tools/libxc/xenguest.h >>>> +++ b/tools/libxc/xenguest.h >>>> @@ -244,12 +244,23 @@ struct xc_hvm_build_args { >>>> int xc_hvm_build(xc_interface *xch, uint32_t domid, >>>> struct xc_hvm_build_args *hvm_args); >>>> >>>> +int xc_hvm_build_with_hole(xc_interface *xch, uint32_t domid, >>>> + struct xc_hvm_build_args *args, >>>> + uint64_t mmio_hole_size); >>>> + >>>> int xc_hvm_build_target_mem(xc_interface *xch, >>>> uint32_t domid, >>>> int memsize, >>>> int target, >>>> const char *image_name); >>>> >>>> +int xc_hvm_build_target_mem_with_hole(xc_interface *xch, >>>> + uint32_t domid, >>>> + int memsize, >>>> + int target, >>>> + const char *image_name, >>>> + uint64_t mmio_hole_size); >>> Why on earth do we need all of these extra functions? Particularly >>> ones like xc_hvm_build_target_mem_with_hole(), which isn't even called >>> by anyone, AFAICT? >> int xc_hvm_build_target_mem(xc_interface *xch, >> uint32_t domid, >> int memsize, >> int target, >> const char *image_name) >> { >> return xc_hvm_build_target_mem_with_hole(xch, domid, memsize, target, >> image_name, 0); >> } >> >> So it is called... > You're kidding, aren't you? If this is the only caller, we can very well > do without the ..._with_hole() one. Indeed, I didn't even get what Don was saying at first. When I looked, xc_hvm_build* was called exactly twice: xc_hvm_build() was called in libxl, and xc_hvm_build_target_mem() was called by xend. I think originally those functions were there for xapi compatibility (I was working on a XenServer feature when I wrote these; and this was before libxl was really a thing). But now that we have libxl, breaking libxc compatibility is a feature to push people to use the libxl interfaces instead. :-) -George