From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7D2F1803A5 for ; Mon, 13 Mar 2017 15:12:11 -0700 (PDT) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 06B57C05AA4F; Mon, 13 Mar 2017 22:12:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-193.phx2.redhat.com [10.3.116.193]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2DMCAxs010036; Mon, 13 Mar 2017 18:12:10 -0400 To: Jordan Justen , "Kinney, Michael D" References: <20170223014814.10937-1-lersek@redhat.com> <20170223014814.10937-12-lersek@redhat.com> <148914088572.26490.12215430240628550434@jljusten-skl> <55e1eade-1d06-4b04-3e07-5ceef61a5602@redhat.com> <148944077646.15816.5488638936147140185@jljusten-skl> Cc: edk2-devel-01 From: Laszlo Ersek Message-ID: Date: Mon, 13 Mar 2017 23:12:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <148944077646.15816.5488638936147140185@jljusten-skl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 13 Mar 2017 22:12:12 +0000 (UTC) Subject: Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Mar 2017 22:12:11 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/13/17 22:32, Jordan Justen wrote: > On 2017-03-10 11:36:36, Laszlo Ersek wrote: >> On 03/10/17 11:14, Jordan Justen wrote: >>> On 2017-02-22 17:48:13, Laszlo Ersek wrote: >>>> + // >>>> + ScratchBuffer->Features = mSmiFeatures; >>>> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem, >>> >>> I noticed a fair amount of function calls where the first args are >>> appearing on the first line, and then the rest appear on subsequent >>> lines indented two spaces. >> >> Yes, that's intentional. The coding style nominally requires each >> argument on a separate line (if they don't all fit on a single >> line), but that wastes a huge amount of vertical space (which is bad >> because less code fits in a screenful). So I frequently follow a >> semi-compressed style where I place the arguments continuously, but >> wherever I have to break the line (because of the 80 char limit) I >> stick with the indentation required by the coding style. Also, when >> I apply this style, I don't break the final closing paren off to a >> separate line (because this style is primarily contiguous). >> >> I mostly employ this style when the arguments are otherwise simple >> (i.e., when the "one arg per line" style does not improve clarity, >> just wastes a lot of space). >> >> This is not new, I've been coding like this in edk2 virtually >> forever. >> > > I understand that I've given an r-b in the past for code like this. I > have noticed it previously, but in some cases it doesn't look as > strange. For example, with the DEBUG macro, the error level seems less > important to move to the next line. I guess it is probably bad to make > an exception there. > > I do think that lining up the arguments should be prioritized over > saving vertical whitespace. I think we just have to accept that EDK II > is not efficient in terms of vertical space. In fact, EDK II is just > plain verbose in general. :) > > At least we don't follow the kernel's strict 8 char indent and 80 > columns limit. (Although, there are some good arguments in favor of > this in terms of forcing code to be broken down into smaller > functions.) > > I would like to know if Mike has any opinion on whether this is a bug > in the coding style spec to not address this more definitively. "CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find: (1) 5.2.2.4 Subsequent lines of multi-line function calls should line up one or two tab-stops from the beginning of the function name Use either one or two tab stops to ensure that each parameter is indented at least two spaces after the function name. Either of the below examples is acceptable: Status = gBS->AllocatePool ( EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE), &PrivateData ); Status = gBS->AllocatePool ( EfiBootServicesData, sizeof (DRIVER_NAME_INSTANCE), &PrivateData ); My notes on this: - my code complies with the indentation requirement - the passage is silent on whether each argument should be broken off to a new line. The examples are called "acceptable", not "required". I guess this is what your question to Mike is about. - I don't see what the difference is between the two examples. To me they look identical. - In the examples, the closing parens line up with "AllocatePool", not with "EfiBootServicesData". I think I have never ever seen this style in edk2. (2) 6.5.2.4 Comments are allowed on the parameters of a function call. These comments provide supplemental information about the parameters for that specific function call. The information in parameter comments should not repeat the information in the descriptive text for the @param entries in the special documentation block describing the function. Function call parameter comments are never Doxygen comments. Status = TestString ( String, // Comment for first parameter Index + 3, // Comment for second parameter &Value // Comment for third parameter ); My notes on this: - the closing paren's indentation is inconsistent with the examples given under 5.2.2.4 (but consistent with edk2 practice). If the agreement is to break every single argument in a multi-line function call (or function-like macro call) off to a separate line, then please let us file a BZ for that (EDK2/Documentation I guess?), and then I'll comply with that *new* requirement in my patches that I post *after* the BZ is filed. I see a number of open items for FDF/INF/etc specs: https://bugzilla.tianocore.org/buglist.cgi?component=Documentation&product=EDK2&query_format=advanced so I think the TianoCore bugzilla is the right place to track coding style spec reports as well. Thanks Laszlo > > -Jordan > >>> >>>> + sizeof ScratchBuffer->Features); >>>> + if (RETURN_ERROR (Status)) { >>>> + goto FatalError; >>>> + } >>>> + >>> >>> I can't say that the coding style prohibits this, but it does seem odd >>> looking. I would expect to see: >>> >>> [1] >>> >>> Status = QemuFwCfgS3WriteBytes ( >>> mRequestedFeaturesItem, >>> sizeof ScratchBuffer->Features >>> ); >>> >>> The coding style examples look like this: >>> >>> Status = QemuFwCfgS3WriteBytes ( >>> mRequestedFeaturesItem, >>> sizeof ScratchBuffer->Features >>> ); >>> >>> But this looks less like what I'm used to actually seeing in EDK II. >>> (Look at DXE Core, for example. It looks like [1].) >>> >>> I personally think this is okay to save a line, but it doesn't seem to >>> follow the coding style doc: >>> >>> Status = QemuFwCfgS3WriteBytes ( >>> mRequestedFeaturesItem, >>> sizeof ScratchBuffer->Features); >>> >>> Now, we all know EDK II has a style of it's own, but outide EDK II, I >>> might expect something like: >>> >>> Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem, >>> sizeof ScratchBuffer->Features); >>> >>> Which once again shows arguements on subsequent lines are lined up. >>> Based on that, I think [1] is the best style for EDK II code. >> >> Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments. >> >> However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation. >> >> Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver: >> >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) // >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virtio-blk header in first desc >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) // >> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, >> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_DESC_F_NEXT, &Indices); >> >> >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) // >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRING_DESC_F_WRITE is interpreted from the host's point of view. >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) // >> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), >> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Indices); >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) } >> >> >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) // >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host status in last (second or third) desc >> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) // >> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, >> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_DESC_F_WRITE, &Indices); >> >> >> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp, >> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) &PhysicalBlockExp); >> >> >> More recent code from e.g. "SmmAccessPei.c": >> >> >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return SmramAccessGetCapabilities (This->LockState, This->OpenState, >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) SmramMapSize, SmramMap); >> >> >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only " >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId, >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q35_MCH_DEVICE_ID)); >> >> >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) // >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the zero graphics memory sizes configured above, set the >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-related stolen memory bases to the same as TOLUD. >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) // >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM), >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRamMb << MCH_GBSM_MB_SHIFT); >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM), >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRamMb << MCH_BGSM_MB_SHIFT); >> >> >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState, >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &SmramMapSize, SmramMap); >> >> >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState], >> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof SmramMap[DescIdxSmmS3ResumeState]); >> >> >> Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c": >> >> >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->WritePointers = AllocatePool (WritePointerCount * >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) sizeof *Context->WritePointers); >> >> >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n", >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue, >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3Context->Used)); >> >> >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, >> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) NULL /* Registration */, (VOID **)&S3SaveState); >> >> I've been entirely consistent about this. >> >> Thanks >> Laszlo >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >