From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [v10][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Date: Mon, 20 Jul 2015 14:32:27 +0100 Message-ID: <21932.63595.566823.211293@mariner.uk.xensource.com> References: <1437373023-14884-1-git-send-email-tiejun.chen@intel.com> <1437373023-14884-12-git-send-email-tiejun.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1437373023-14884-12-git-send-email-tiejun.chen@intel.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: Tiejun Chen Cc: Stefano Stabellini , Wei Liu , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org Tiejun Chen writes ("[v10][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM"): > While building a VM, HVM domain builder provides struct hvm_info_table{} > to help hvmloader. Currently it includes two fields to construct guest > e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should > check them to fix any conflict with RDM. ... Thanks. I think this patch is nearly there. > +static int > +libxl__xc_device_get_rdm(libxl__gc *gc, ... > + *xrdm = libxl__malloc(gc, > + *nr_entries * sizeof(xen_reserved_device_memory_t)); Sorry for not spotting this before, but this should use GCNEW_ARRAY. > +} > + > +#define pfn_to_paddr(x) ((uint64_t)x << XC_PAGE_SHIFT) > +static void Missing blank line after the #define - although I think this #define could profitably be moved to libxl_internal.h. If you do keep it here please move it further up the file, to at least before any function or struct definition. Also the #define is missing safety ( ) around x. > +set_rdm_entries(libxl__gc *gc, libxl_domain_config *d_config, > + uint64_t rdm_start, uint64_t rdm_size, int rdm_policy, > + unsigned int nr_entries) > +{ > + assert(nr_entries); > + > + d_config->num_rdms = nr_entries; > + d_config->rdms = libxl__realloc(NOGC, d_config->rdms, > + d_config->num_rdms * sizeof(libxl_device_rdm)); > + > + d_config->rdms[d_config->num_rdms - 1].start = rdm_start; > + d_config->rdms[d_config->num_rdms - 1].size = rdm_size; > + d_config->rdms[d_config->num_rdms - 1].policy = rdm_policy; > +} Thanks for breaking this out. I think though that the division of labour between this function and the call site is confusing. Can I suggest a function void add_rdm_entry(libxl__gc *gc, libxl_domain_config *d_config, uint64_t rdm_start, uint64_t rdm_size, int rdm_policy) which assumes that d_config->num_rdms is set correctly, and increments it ? (Please put the increment at the end so that the assignments are to ->rdms[d_config->num_rdms], or perhaps make a convenience alias.) > +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) > +{ ... > + /* Query all RDM entries in this platform */ > + if (strategy == LIBXL_RDM_RESERVE_STRATEGY_HOST) { ... > + } else { > + d_config->num_rdms = 0; > + } Does this not override the domain configuration's num_rdms ? I don't think that is correct. If the domain configuration has rdms and num_rdms already set, then the strategy should presumably be ignored. (Passing the same domain configuration struct to libxl_domain_create_new, after destroying the domain, ought to work, even after the first call has modified it.) Can you please also wrap at least the already-word-wrapped comments to 70 (or maybe 75) columns ? What you have lookes like this when I quote it for review: > + * Next step is to check and avoid potential conflict between RDM entri\ es > + * and guest RAM. To avoid intrusive impact to existing memory layout > + * {lowmem, mmio, highmem} which is passed around various function bloc\ ks, > + * below conflicts are not handled which are rare and handling them wou\ ld Thanks, Ian.