From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Date: Fri, 10 Jul 2015 11:19:55 +0800 Message-ID: <559F39DB.8030106@intel.com> References: <1436420047-25356-1-git-send-email-tiejun.chen@intel.com> <1436420047-25356-12-git-send-email-tiejun.chen@intel.com> <21918.47614.887667.234848@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21918.47614.887667.234848@mariner.uk.xensource.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: Ian Jackson Cc: Stefano Stabellini , Wei Liu , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org > I have found a few things in this patch which I would like to see > improved. See below. > > Given how late I am with this review, I do not feel that I should be > nacking it at this time. You have a tools ack from Wei, so my > comments are not a blocker for this series. > > But if you need to respin, please take these comments into account, > and consider which are feasible to fix in the time available. If you > are respinning this series targeting Xen 4.7 or later, please address > all of the points I make below. Thanks for your comments and looks I should address them now. > > Thanks. > > >> +int libxl__domain_device_construct_rdm(libxl__gc *gc, >> + libxl_domain_config *d_config, >> + uint64_t rdm_mem_boundary, >> + struct xc_hvm_build_args *args) > ... >> + uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull\ > <<32); > ... >> + if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_\ > pcidevs) > > There are quite a few of these long lines, which should be wrapped. > See tools/libxl/CODING_STYLE. Sorry I can't found any case to what you're talking. So are you saying this? if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_pcidevs) Or @@ -143,6 +143,15 @@ static bool overlaps_rdm(uint64_t start, uint64_t memsize, } /* + * Check whether any rdm should be exposed.. + * Returns true if needs, else returns false. + */ +static bool exposes_rdm(uint32_t strategy, int num_pcidevs) +{ + return strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !num_pcidevs; +} + +/* * Check reported RDM regions and handle potential gfn conflicts according * to user preferred policy. * @@ -182,7 +191,7 @@ int libxl__domain_device_construct_rdm(libxl__gc *gc, uint64_t highmem_end = args->highmem_end ? args->highmem_end : (1ull<<32); /* Might not expose rdm. */ - if (strategy == LIBXL_RDM_RESERVE_STRATEGY_IGNORE && !d_config->num_pcidevs) + if (exposes_rdm(strategy, d_config->num_pcidevs)) return 0; /* Query all RDM entries in this platform */ > >> + d_config->num_rdms = nr_entries; >> + d_config->rdms = libxl__realloc(NOGC, d_config->rdms, >> + d_config->num_rdms * sizeof(libxl_device_rdm)); > > This code is remarkably similar to a function later on which adds an > rdm. Please can you factor it out. Do you mean I should merge them as one as possible? But seems not be possible because we have seveal combinations of these two conditions, strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST and one or pci devices are also passes through. #1. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST but without any devices So it appropriately needs this libxl__realloc() here. The second libxl__realloc() doesn't take any effect. #2. strategy = LIBXL_RDM_RESERVE_STRATEGY_HOST with one or more devices Actually we don't need the second libxl__realloc(). This is same as #1. #3. strategy != LIBXL_RDM_RESERVE_STRATEGY_HOST also with one or more devices So we just need the second libxl__realloc() later. The fist libxl__realloc() doesn't be called at all. Especially, its very possible we're going to handle less RDMs, compared to LIBXL_RDM_RESERVE_STRATEGY_HOST. > >> + } else >> + d_config->num_rdms = 0; > > Please can you put { } around the else block too. I don't think this > mixed style is good. Fixed. > >> + for (j = 0; j < d_config->num_rdms; j++) { >> + if (d_config->rdms[j].start == >> + (uint64_t)xrdm[0].start_pfn << XC_PAGE_SHIFT) > > This construct > (uint64_t)some_pfn << XC_PAGE_SHIFT > appears an awful lot. > > I would prefer it if it were done in an inline function (or maybe a > macro). Like this? #define AAAA(x) ((uint64_t)x << XC_PAGE_SHIFT) Sorry I can't figure out a good name here :) Any suggestions? > > >> + libxl_domain_build_info *const info = &d_config->b_info; >> + /* >> + * Currently we fix this as 2G to guarantte how to handle > ^^^^^^^^^ > > Should read "guarantee". Fixed. > >> + ret = libxl__domain_device_construct_rdm(gc, d_config, >> + rdm_mem_boundary, >> + &args); >> + if (ret) { >> + LOG(ERROR, "checking reserved device memory failed"); >> + goto out; >> + } > > `rc' should be used here rather than `ret'. (It is unfortunate that > this function has poor style already, but it would be best not to make > it worse.) > I can do this but according to tools/libxl/CODING_STYLE, looks I should first post a separate patch to fix this code style issue, and then rebase my patch, right? Thanks TIejun