From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web12.3783.1574687598979234114 for ; Mon, 25 Nov 2019 05:13:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=l//t2MSE; spf=pass (domain: linaro.org, ip: 209.85.221.65, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f65.google.com with SMTP id z10so17886804wrs.12 for ; Mon, 25 Nov 2019 05:13:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LVzgIXbHylufhp0sitvtb86kdEKtKYL9Us8gJGHu57k=; b=l//t2MSEr6FwvlL48f0g/3xWT6xiklTW1vGEc7bFxgxt6ctmRQmfM/qI0kNoMBuN3N Hl+UpK8QXVfdDe48gkF/ldimBURuuDr29Yi866fFKBBihZkHOmwlUSlsF/cjL0e10zKI 8qf4fJtEH/yEsThql2+VcDMUFewDWCE6uztZStxwphiirbQAOtY5Y7TSHGU+1SLTSyab FBIC2T5tMLPOyD5wfwRGxWUhTcPgJVcqDGeFPjvqjCwc9+8cPraOz2Cvozop4wIwidK2 g4bc7GDae1uA8WkYUvad2i6s4RVLsb3VoRHWSfdle5BtUwywW5B08g34pbvgjLHFUvDy 1R7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LVzgIXbHylufhp0sitvtb86kdEKtKYL9Us8gJGHu57k=; b=SaxjpvIlXu5Nisf5Kot1XCvlfG5MiqcpVvCD1XQKGQrp5rWjuuBZpG047v8/rVaY34 0wReWezLt81e8CpuZ8fQ4SV8X0tttM9bMkKZzwsdZ4h5gqq7xVAEejNbwNqI/gV0VQys nmDBqYMuFTGM3W1nbPnyJkBmRm0x3Oa/lOYMdRL6SjLHCFCk8PGaqN1o2EuSomCyzdIv 9WiW7tKlBzvmP/w1Q3uXgIYvmCBW+WKsF04y9BhLuYSbz2Rkin81P7Q+s2UlbnZQ/sOe WwShaGT1yuSsrADXAfikoh/IkhCNXx5Ds4PEoSDK/Q9msGD4p8qhOKHwdpWZDIvVOdJA OT3A== X-Gm-Message-State: APjAAAW8Ry1uePGz6ziXvEnyu7aITllrPNxqMZ4u1I0J/qn2Mkja5a1t LfAz0NDDKWvC6KCUhBIJ4ZoOh69fND3/eicJs8J+xw== X-Google-Smtp-Source: APXvYqwPs+VN04p3Vahw4gvkFQbjjToy+EsOMEuKw0dM98MTcNZceTO8D5XdSXwWPcA/PSSaBdNqcwmaBUPhxF57U3k= X-Received: by 2002:adf:e6d0:: with SMTP id y16mr8459276wrm.32.1574687597398; Mon, 25 Nov 2019 05:13:17 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: <20191125125828.GQ7359@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Mon, 25 Nov 2019 14:13:19 +0100 Message-ID: Subject: Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits To: Leif Lindholm Cc: edk2-devel-groups-io Content-Type: text/plain; charset="UTF-8" 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. So that leaves printing to the console using boot services, which is not something we should be doing from a DXE_DRIVER type module. 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.