From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v5 1/1] Add mmio_hole_size Date: Wed, 01 Oct 2014 12:33:31 -0400 Message-ID: <542C2CDB.4090307@terremark.com> References: <1410452423-11345-1-git-send-email-dslutz@verizon.com> <1410452423-11345-2-git-send-email-dslutz@verizon.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: George Dunlap , Don Slutz , Ian Campbell Cc: Boris Ostrovsky , Stefano Stabellini , Ian Jackson , Jan Beulich , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 09/30/14 09:15, George Dunlap wrote: > On Thu, Sep 11, 2014 at 5:20 PM, 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. >> >> Signed-off-by: Don Slutz > FYI you'll have to rebase this since it doesn't apply cleanly anymore. Or, I have rebased. > Also, I've been meaning to look at this for months; sorry it's taken so long. > > Overall looks fine, just a few comments: > >> @@ -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. > But if this needs to be rushed to get in, this is fine with me. :) >> if ( mmio_total > (pci_mem_end - pci_mem_start) ) >> { >> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c >> index c81a25b..94da7fa 100644 >> --- a/tools/libxc/xc_hvm_build_x86.c >> +++ b/tools/libxc/xc_hvm_build_x86.c >> @@ -628,14 +628,40 @@ int xc_hvm_build_target_mem(xc_interface *xch, >> int target, >> const char *image_name) >> { >> + return xc_hvm_build_target_mem_with_hole(xch, domid, memsize, target, >> + image_name, 0); >> +} >> + >> +int xc_hvm_build_with_hole(xc_interface *xch, uint32_t domid, >> + struct xc_hvm_build_args *args, >> + uint64_t mmio_hole_size) >> +{ >> + 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) >> + { >> + args->mmio_size = mmio_hole_size; >> + } >> + } >> + return xc_hvm_build(xch, domid, args); >> +} >> + >> +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) >> +{ >> struct xc_hvm_build_args args = {}; >> >> memset(&args, 0, sizeof(struct xc_hvm_build_args)); >> args.mem_size = (uint64_t)memsize << 20; >> args.mem_target = (uint64_t)target << 20; >> args.image_file_name = image_name; >> - >> - return xc_hvm_build(xch, domid, &args); >> + return xc_hvm_build_with_hole(xch, domid, &args, mmio_hole_size); >> } >> >> /* >> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h >> index 40bbac8..d35f38d 100644 >> --- 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... > > libxc isn't a stable interface -- can't we just have the callers set > mmio_size in hvm_args and have them call xc_hvm_build()? I am looking into this. I have was not sure that I could change or drop xc_hvm_build_target_mem(). >> @@ -696,10 +698,28 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, >> /* Switching here to the machine "pc" which does not add >> * the xen-platform device instead of the default "xenfv" machine. >> */ >> - flexarray_append(dm_args, "pc,accel=xen"); >> + machinearg = libxl__sprintf(gc, "pc,accel=xen"); >> } else { >> - flexarray_append(dm_args, "xenfv"); >> + machinearg = libxl__sprintf(gc, "xenfv"); >> } >> + if (b_info->u.hvm.mmio_hole_size) { >> + uint64_t max_ram_below_4g = (1ULL << 32) - >> + b_info->u.hvm.mmio_hole_size; >> + >> + if (max_ram_below_4g > HVM_BELOW_4G_MMIO_START) { >> + LIBXL__LOG(ctx, LIBXL__LOG_WARNING, >> + "max-ram-below-4g=%"PRIu64 >> + " (mmio_hole_size=%"PRIu64 >> + ") too big > %u ignored.\n", >> + max_ram_below_4g, >> + b_info->u.hvm.mmio_hole_size, >> + HVM_BELOW_4G_MMIO_START); >> + } else { >> + machinearg = libxl__sprintf(gc, "%s,max-ram-below-4g=%"PRIu64, >> + machinearg, max_ram_below_4g); >> + } >> + } > What happens if the version of qemu that's installed doesn't support > max-ram-below-4g? QEMU will report an error and abort. Guest will not start. -Don Slutz > -George