From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM Date: Mon, 08 Jun 2015 10:16:45 +0800 Message-ID: <5574FB0D.8050003@intel.com> References: <1432287314-4388-1-git-send-email-tiejun.chen@intel.com> <1432287314-4388-5-git-send-email-tiejun.chen@intel.com> <20150602162935.GU19403@zion.uk.xensource.com> <556E65AB.8070709@intel.com> <20150607112026.GP29102@zion.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: <20150607112026.GP29102@zion.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: Wei Liu Cc: kevin.tian@intel.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, tim@xen.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, JBeulich@suse.com, yang.z.zhang@intel.com, Ian.Jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 2015/6/7 19:20, Wei Liu wrote: > On Wed, Jun 03, 2015 at 10:25:47AM +0800, Chen, Tiejun wrote: > [...] >>>> +static struct xen_reserved_device_memory >>>> +*xc_device_get_rdm(libxl__gc *gc, >>>> + uint32_t flag, >>>> + uint16_t seg, >>>> + uint8_t bus, >>>> + uint8_t devfn, >>>> + unsigned int *nr_entries) >>>> +{ >>>> + struct xen_reserved_device_memory *xrdm = NULL; >>>> + int rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn, >>>> + xrdm, nr_entries); >>>> + >>> >>> Please separate declaration and function call. Also change xrdm to NULL >> >> Are you saying this? >> >> struct xen_reserved_device_memory *xrdm = NULL; >> int rc; >> >> rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn, >> xrdm, nr_entries); > > Yes, splitting "rc = " to a separate line. The other thing is: > > rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn, > NULL, nr_entries); > ^ here > > It's mostly cosmetic. IMHO it is clearer than passing xrdm which is > always NULL in that function call. Right. > >> >>> in that function call. >> >> Sorry, what do you mean by this point? Or you let me to change xrdm to NULL >> inside xc_reserved_device_memory_map()? >> >>> >>>> + assert( rc <= 0 ); >>>> + /* "0" means we have no any rdm entry. */ >>>> + if ( !rc ) >>>> + goto out; >>> >>> Also set *nr_entries = 0; otherwise you can't distinguish error vs 0 >>> entries. >> >> *nr_entries is always updated by xc_reserved_device_memory_map() above. >> > > Actually no. If xc_hypercall_bounce_pre fails in the function, > nr_entries is untouched. Sure. > >>> >>>> + >>>> + if ( errno == ENOBUFS ) >>>> + { >>>> + if ( (xrdm = malloc(*nr_entries * >>>> + sizeof(xen_reserved_device_memory_t))) == NULL ) > [...] >>>> + return -1; >>> >>> Please return libxl error code. >> >> ERROR_FAIL? >> > > Yes, that's fine. Thanks Tiejun > > [...] >>>> + args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH; >>>> + mmio_start = (1ull << 32) - args.mmio_size; >>>> + >>>> + if (args.lowmem_size > mmio_start) >>>> + args.lowmem_size = mmio_start; >>>> + >>>> + /* >>>> + * We'd like to set a memory boundary to determine if we need to check >>>> + * any overlap with reserved device memory. >>>> + */ >>>> + rdm_mem_boundary = 0x80000000; >>>> + if (info->rdm_mem_boundary_memkb) >>> >> >> I'm going to update this chunk of codes as follows: >> >> #1. @@ -858,6 +858,12 @@ const char *libxl_defbool_to_string(libxl_defbool >> b); >> #define LIBXL_TIMER_MODE_DEFAULT -1 >> #define LIBXL_MEMKB_DEFAULT ~0ULL >> >> +/* >> + * We'd like to set a memory boundary to determine if we need to check >> + * any overlap with reserved device memory. >> + */ >> +#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024) >> + >> #define LIBXL_MS_VM_GENID_LEN 16 >> typedef struct { >> uint8_t bytes[LIBXL_MS_VM_GENID_LEN]; >> >>> I think you mean info->rdm_mem_boundary_memkb != LIBXL_MEMKB_DEFAULT? >>> >> >> #2. >> >> @@ -109,6 +109,10 @@ void libxl__rdm_setdefault(libxl__gc *gc, >> libxl_domain_build_info *b_info) >> { >> if (b_info->rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID) >> b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED; >> + >> + if (b_info->rdm_mem_boundary_memkb == LIBXL_MEMKB_DEFAULT) >> + b_info->rdm_mem_boundary_memkb = >> + LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT; > > This looks plausible. > > Wei. >