From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.34178.1590758109940028588 for ; Fri, 29 May 2020 06:15:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=hT6GxMVN; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id s8so3493068wrt.9 for ; Fri, 29 May 2020 06:15:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=/t343kGH/ADZkt0zPi7HgL5IgaqLJUGfvn2937NCu9s=; b=hT6GxMVNa7Ud8nf55udFYC7d2qlDLm58u9XmCjR/njxPl+QuCvhhR7/wQdEZ7kyn1n ABU2bhLvPH8Ga0qfS/PWH2eWa2ubb1vb/+ko0nD9TNQJ5yvfmtvrj5fI5idrvUsAJSbE KKpxrlfe6Krg18JzFXX0jP4d7m+x3df+ltZtdH25m6jQeQDLhtT4htWt3tMOr0eaXsUq Q9p33DX1u/oWaUlxMCskREGuAXUyQTUpuvHXTR3r2RdXxJl+xyflmA0bIoFsV38IPTub OlhEqlDr8Qg4Zo6sz3oALAsu/ZzfHqCJcoCsYsDJa0WlgESJTqeF1egpgrbwhN9n5YaC 7F1A== 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=/t343kGH/ADZkt0zPi7HgL5IgaqLJUGfvn2937NCu9s=; b=SHU6kVXKHoaDW9uRC+bCeMHtuGGJcQzg6Z6xbwPslcqvDfjGW9UqqhckApifPcyb58 7yLnMY0BcpdZzQ4uHRYYYr+G3gGzSkGFfngnTkxdaKexrDUAt1VRLwt/aLci+Ea3tZ45 2YnPlRLD/q6Px5XKpxwi+WIy0JNEO55LQaIYgFRIzbMRpKHv8PnMg7hOVYAvGLRtivNc x2cthGovM3OmCXT+sOxOAsDrgT/5seV2b/SHW4TuS6NZepEYc5+czgHswO57g16dRNfw uzjWLS6gVHmuQDqSFLhRKszxsvbb/ZVeIGe/cJZYrZE06gg+umun0lMaqjlqBV1uyrp4 K7ZQ== X-Gm-Message-State: AOAM533MUKg7FR1QJ+d8zd9JViavOeYftu2dAPAgJMy70YbI4YQRqAny 24gPAMkABIQ5dCHQmV1zRjpe0g== X-Google-Smtp-Source: ABdhPJxfhU2fS8Gpi389SxuOg5J5Vy3xTtXUrGHxN7kUucFUJI9Bq2pORsCQUM/SoZHkiihNrNebyw== X-Received: by 2002:adf:e604:: with SMTP id p4mr3387244wrm.212.1590758108446; Fri, 29 May 2020 06:15:08 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id l19sm10562352wmj.14.2020.05.29.06.15.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 May 2020 06:15:07 -0700 (PDT) Date: Fri, 29 May 2020 14:15:05 +0100 From: "Leif Lindholm" To: Daniel Schaefer Cc: devel@edk2.groups.io, Abner Chang , Gilbert Chen , Michael D Kinney Subject: Re: [edk2-devel] [PATCH v2 3/3] ProcessorPkg/Library: Add RiscVEdk2SbiLib Message-ID: <20200529131505.GM1923@vanye> References: <20200515133937.29909-1-daniel.schaefer@hpe.com> <20200515133937.29909-4-daniel.schaefer@hpe.com> <20200520182729.GN1923@vanye> <492edaee-f3a1-9d36-0dc0-c70564912d2c@hpe.com> MIME-Version: 1.0 In-Reply-To: <492edaee-f3a1-9d36-0dc0-c70564912d2c@hpe.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, May 29, 2020 at 14:43:43 +0200, Daniel Schaefer wrote: > Hi Leif, > > thanks for this super careful review! > Comments and one question inline. > > - Daniel > > On 5/20/20 8:27 PM, Leif Lindholm wrote: > > On Fri, May 15, 2020 at 15:39:37 +0200, Daniel Schaefer wrote: > > > Library provides interfaces to invoke SBI extensions. > > > > > > Signed-off-by: Daniel Schaefer > > > > > > Cc: Abner Chang > > > Cc: Gilbert Chen > > > Cc: Michael D Kinney > > > Cc: Leif Lindholm > > > --- > > > Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.inf | 28 + > > > Silicon/RISC-V/ProcessorPkg/Include/IndustryStandard/RiscVOpensbi.h | 43 +- > > > Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h | 631 ++++++++++++++++ > > > Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c | 789 ++++++++++++++++++++ > > > 4 files changed, 1466 insertions(+), 25 deletions(-) > > > > > > diff --git a/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > > new file mode 100644 > > > index 000000000000..cf77814e3bbc > > > --- /dev/null > > > +++ b/Silicon/RISC-V/ProcessorPkg/Include/Library/RiscVEdk2SbiLib.h > > > > > + long error; ///< SBI status code > > > + long value; ///< Value returned > > > > This looks like a maintenance hazard (means different things to GCC > > and VS for example). Can we use UINT32? > > I'll use UINTN because it's bigger than 32bits on RISCV64 Oh, then definitely that, yes. > > > > > +}; > > > + > > > +#define SbiCall0(ext_id, func_id) \ > > > + SbiCall(ext_id, func_id, 0, 0, 0, 0, 0, 0) > > > +#define SbiCall1(ext_id, func_id, arg0) \ > > > + SbiCall(ext_id, func_id, arg0, 0, 0, 0, 0, 0) > > > +#define SbiCall2(ext_id, func_id, arg0, arg1) \ > > > + SbiCall(ext_id, func_id, arg0, arg1, 0, 0, 0, 0) > > > +#define SbiCall3(ext_id, func_id, arg0, arg1, arg2) \ > > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, 0, 0, 0) > > > +#define SbiCall4(ext_id, func_id, arg0, arg1, arg2, arg3) \ > > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, 0, 0) > > > +#define SbiCall5(ext_id, func_id, arg0, arg1, arg2, arg3, arg4) \ > > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, 0) > > > +#define SbiCall6(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, arg5) \ > > > + SbiCall(ext_id, func_id, arg0, arg1, arg2, arg3, arg4, arg5) > > > > Ugh. This looks way too much like pre-EFIAPI x86 code. > > > > Is this a pattern used across multiple codebases? > > If not, could we make this a simple argc/argv instead and do the > > unpacking inside SbiCall()? Hmm, maybe that would make the call sites > > even worse. > > > > If we need to keep these, coding style says all macros should use > > UPPERCASE_AND_UNDERSCORES. > > > > Yeah, I think argc/argv is going to make the callsites worse. What about > vararg, like I used in SbiVendorCall in > Silicon/RISC-V/ProcessorPkg/Library/RiscVEdk2SbiLib/RiscVEdk2SbiLib.c > ? Or does that have a performance impact? Maybe an architecture specific > one? It *might* have a performance impact, but I'd be astonished if it is noticeable. Yes please, if you don't mind, I think that would really improve the readability. > > > +/** > > > + Get the CPU's vendor ID > > > + > > > + Reads the mvendorid CSR. > > > > What is an MvendorId? MachineVendorId? > > Even if an official register name, I would prefer function and > > arguments to be given proper descriptive CamelCase names. > > Yes, it's the official register name. Alright, will change it. If it *is* the official register name, adding it to the glossary section is also OK. > > > +**/ > > > +VOID > > > +EFIAPI > > > +SbiGetMarchId ( > > > + OUT UINTN *MarchId > > > + ); > > > + > > > +/** > > > + Get the CPU's implementation ID > > > + > > > + Reads the mimpid CSR. > > > + > > > + @param[out] MimpId The CPU's implementation ID. > > > > Above "Impl" is used for "Impelentation". *Strictly* speaking, "Impl" > > doesn't fall in the pretty small group of abbreviations permitted > > without a glossary section in the source file, but it's clear enough > > to me I'll let it slip. > > The same cannot be said for "Imp". > > MachineImplId? > > Sounds good. Should it be added to the permitted abbreviations? > If I spell it out fully here, it becomes quite long. I'm OK with Impl. We should consider adding it to the list. > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +SbiHartGetStatus ( > > > + IN UINTN HartId, > > > + OUT UINTN *HartStatus > > > + ); > > > + > > > +/// > > > +/// Timer extension > > > +/// > > > + > > > +/** > > > + Clear pending timer interrupt bit and set timer for next event after StimeValue. > > > > What does the S stand for in Stime? > > That's what they call it in the spec: stime_value. > I guess it stands for supervisor. Should we change it to just `Time`? That would be a valid thing to do. Another would be to add it to the glossary. > > > + if (!ret.error) { > > > + ScratchSpace = (struct sbi_scratch *) ret.value; > > > + SbiPlatform = (struct sbi_platform *) sbi_platform_ptr(ScratchSpace); > > > + SbiPlatform->firmware_context = (long unsigned int) FirmwareContext; > > > > UINT64? > > UINTN for compatibility with 32bits. Of course. Thanks! / Leif