From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web10.4913.1574692729008437009 for ; Mon, 25 Nov 2019 06:38:49 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=OFjPzqoN; spf=pass (domain: linaro.org, ip: 209.85.128.65, mailfrom: leif.lindholm@linaro.org) Received: by mail-wm1-f65.google.com with SMTP id g206so15425574wme.1 for ; Mon, 25 Nov 2019 06:38:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=b4Qo5TDaQbPrWwMgGY3AhpiktuQmFCBnYG2aGGJGHqo=; b=OFjPzqoN2glyanChMTL0bZUjaAA/9i8KSaCZCYqUqI3Ix0h8zYb7VG5zBj++nHIYQO vpDmd4HI3+xR1A5Z13LVT7abVXZJwTEZzRKMjzP8ZnlH2gLWIFw+AiVESlO+OrKn6/mv zpg5/up/ISo1pPxXgyzGgXYLjuRGTYy/XZLZmCnZ+Q/ku6FcxX7vBvs6QiMQWdKug+Xr jgR3WIj2AO7tp5GrmWhVMxEyRX/UWOGyazhbMSI/v9+fwwT9BC2MfvScAaO6kSjH0g6K GeUjFRcclF7V21mURi4hfnKAY7OlIRXbvYve9OEpvBfBLrK7cOLJlid3T0S5HnZkHh1r trGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=b4Qo5TDaQbPrWwMgGY3AhpiktuQmFCBnYG2aGGJGHqo=; b=bPNp2X3EqTYkMglajEFEZ9qChEwXLriY5VVr7hM5hTm3wnpHbvqsftBWMMEv+hdNgp iANKwrhzXWQpMwaRpou7qFeHT2uBOBQrqkr5QxLELYsj6N7/TGjCN5yvHn8OwAWMuU5a MybVycc4Yrm4ffo7fv2Um9CyjUH7UPBIo69pJa5ouE54LEVtF+E492ToCEIrTqWFYe/n VgwTxqn0b/eJrb2wVKl/ZnW5q2TDPIkhnYjxK1fDTkwh2esJGJpOYMzsHFpUaIUYbXFL aogm3uL0OZI2H+oA/3gV5xOiwKECrHsDyidfoUSU4ePqgi623XhhB0mxyZY1HIlaX3aS MEBg== X-Gm-Message-State: APjAAAXrUceH8pNJ+EZC4Aw1gr7Icu7TW9eoZq2iISQpys2uy1n4UDJQ rmCy0ZQTpRqizRVvXYRu5beUag== X-Google-Smtp-Source: APXvYqzRiYIZYnQC1rINnEiqAcmoYGX5fpyqcdZmb6TuB0O1q2Bfv4aFq/puts/rI5Zy8pAUcxkE/g== X-Received: by 2002:a1c:1b8d:: with SMTP id b135mr12044087wmb.55.1574692727535; Mon, 25 Nov 2019 06:38:47 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v20sm8956151wmj.32.2019.11.25.06.38.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2019 06:38:46 -0800 (PST) Date: Mon, 25 Nov 2019 14:38:45 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: edk2-devel-groups-io Subject: Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Message-ID: <20191125143845.GR7359@bivouac.eciton.net> References: <20191121083227.2850-1-ard.biesheuvel@linaro.org> <20191121083227.2850-2-ard.biesheuvel@linaro.org> <20191125124606.GO7359@bivouac.eciton.net> <20191125125828.GQ7359@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Nov 25, 2019 at 14:13:19 +0100, Ard Biesheuvel wrote: > On Mon, 25 Nov 2019 at 13:58, Leif Lindholm wrote: > > > > On Mon, Nov 25, 2019 at 13:52:24 +0100, Ard Biesheuvel wrote: > > > > > - if (MemoryType == EfiBootServicesData) { > > > > > - Allocation = AllocateAlignedPages (Pages, Alignment); > > > > > - } else if (MemoryType == EfiRuntimeServicesData) { > > > > > - Allocation = AllocateAlignedRuntimePages (Pages, Alignment); > > > > > + if (MemoryType == EfiBootServicesData || > > > > > + MemoryType == EfiRuntimeServicesData) { > > > > > + Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment); > > > > > } else { > > > > > return EFI_INVALID_PARAMETER; > > > > > } > > > > > @@ -479,6 +596,15 @@ NonCoherentDmaLibConstructor ( > > > > > { > > > > > InitializeListHead (&UncachedAllocationList); > > > > > > > > > > + // > > > > > + // Ensure that the combination of DMA addressing offset and limit produces > > > > > + // a sane value. > > > > > + // > > > > > + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset)); > > > > > > > > Is this worth turning into a hard conditional and error return rather > > > > than an assert? The following statement will end up wrapping downwards > > > > if this condition is not true. > > > > > > > > > Constructor return values are only used in ASSERT_EFI_ERROR() calls in > > > the ProcessLibraryConstructorList() routine that gets generated by the > > > build tools (in AutoGen.c), so returning an error here would > > > essentially be dead code, given that we would only make it to this > > > point on builds that are known to ignore it. > > > > Hmm, ack. > > Print error message? > > How? Using DEBUG()? That would be dead code as well, since it is only > active in DEBUG builds, which means the ASSERT() would fire first and > prevent the DEBUG() from ever being reached. Clearly not. > So that leaves printing to the console using boot services, which is > not something we should be doing from a DXE_DRIVER type module. Yeah, maybe that's too urgh. > Given that these are both platform specific values defined at platform > creation time, I think policing them only in a DEBUG build is quite > appropriate, tbh. I dunno, it just feels to me like it's following the pattern of the ArmPlatformGetVirtualMemoryMap() implementations (which do the same). This is a hard error detectable at compile time. My preferred way to handle it would really be a compilation error, but that brings us back to last week's discussion on runtime vs. preprocessor... / Leif