From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.4 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C148EC2D0A8 for ; Mon, 28 Sep 2020 10:09:57 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5DA0C22204 for ; Mon, 28 Sep 2020 10:09:57 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=suse.com header.i=@suse.com header.b="mxNz/wTb" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5DA0C22204 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kMq63-0007n7-Ma; Mon, 28 Sep 2020 10:09:27 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kMq63-0007n2-75 for xen-devel@lists.xenproject.org; Mon, 28 Sep 2020 10:09:27 +0000 X-Inumbo-ID: f8d0a3d7-9692-4a39-97d1-26f6832820ee Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id f8d0a3d7-9692-4a39-97d1-26f6832820ee; Mon, 28 Sep 2020 10:09:26 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1601287765; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UfjCCj3FA9GdPhjgM84xr3tmT4HNQKMl9TzwgTYaZZA=; b=mxNz/wTbIAUNLeOt4zpiWCPNW4MRUU8HelgRQKaxJIKvGdZkA+ajNGfqmQaSD98A2spAQr EJVdWOsdtPiHVbxi3bBOEycKajtVHH395oYnScA5fM5XpUMpGbOZkO9B+E4dwmNg7l047w j+gUVAn9SpVnKTgwDkEsa13zvV58LCM= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 5B78CB21B; Mon, 28 Sep 2020 10:09:25 +0000 (UTC) Subject: Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory() To: Julien Grall Cc: xen-devel@lists.xenproject.org, alex.bennee@linaro.org, masami.hiramatsu@linaro.org, ehem+xen@m5p.com, bertrand.marquis@arm.com, andre.przywara@arm.com, Julien Grall , Stefano Stabellini , Volodymyr Babchuk , Andrew Cooper , George Dunlap , Ian Jackson , Wei Liu , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= References: <20200926205542.9261-1-julien@xen.org> <20200926205542.9261-2-julien@xen.org> From: Jan Beulich Message-ID: Date: Mon, 28 Sep 2020 12:09:24 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On 28.09.2020 11:58, Julien Grall wrote: > On 28/09/2020 09:18, Jan Beulich wrote: >> On 26.09.2020 22:55, Julien Grall wrote: >>> --- a/xen/arch/x86/acpi/lib.c >>> +++ b/xen/arch/x86/acpi/lib.c >>> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size) >>> if ((phys + size) <= (1 * 1024 * 1024)) >>> return __va(phys); >>> >>> + /* No arch specific implementation after early boot */ >>> + if (system_state >= SYS_STATE_boot) >>> + return NULL; >> >> Considering the code in context above, the comment isn't entirely >> correct. > > How about "No arch specific implementation after early boot but for the > first 1MB"? That or simply "No further ...". >>> +{ >>> + unsigned long vaddr = (unsigned long)ptr; >>> + >>> + if (vaddr >= DIRECTMAP_VIRT_START && >>> + vaddr < DIRECTMAP_VIRT_END) { >>> + ASSERT(!((__pa(ptr) + size - 1) >> 20)); >>> + return true; >>> + } >>> + >>> + return (vaddr >= __fix_to_virt(FIX_ACPI_END)) && >>> + (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE)); >> >> Indentation is slightly wrong here. > > This is Linux coding style and therefore is using hard tab. Care to > explain the problem? The two opening parentheses should align with one another, shouldn't they? >>> + ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1, >>> + ACPI_MAP_MEM_ATTR, VMAP_DEFAULT); >>> + >>> + return !ptr ? NULL : (ptr + offs); >> >> Slightly odd that you don't let the success case go first, > > I don't really see the problem. Are you nitpicking? > >> the more that it's minimally shorter: >> >> return ptr ? ptr + offs : NULL; Well, I said "slightly odd" as sort of a replacement of "Nit:". But I really think it would be more logical the other way around, not so much for how it looks like, but to aid the not uncommon compiler heuristics of assuming the "true" path is the common one. Jan