We cannot entirely eliminate the manual boot script building in this
driver, as it also programs lower-level chipset registers (SMI_EN,
GEN_PMCON_1) at S3 resume, not just registers exposed via fw_cfg.
We can nonetheless replace the manually built opcodes for the latter class
of registers with QemuFwCfgS3Lib function calls. We preserve the ordering
between the two sets of registers (low-level chipset first, fw_cfg
second).
This patch demonstrates that manual handling of S3SaveState protocol
installation can be combined with QemuFwCfgS3Lib, even without upsetting
the original order between boot script fragments. An S3SaveState notify
function running at TPL_CALLBACK can safely queue another S3SaveState
notify function at TPL_CALLBACK with QemuFwCfgS3TransferOwnership().
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 +++++++-------------
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 5 +-
3 files changed, 77 insertions(+), 157 deletions(-)
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
index 9d5f1dbcb57e..3f3a5d3ea9b1 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
@@ -37,13 +37,10 @@ NegotiateSmiFeatures (
/**
Append a boot script fragment that will re-select the previously negotiated
SMI features during S3 resume.
-
- @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to
- the S3 boot script with.
**/
VOID
SaveSmiFeatures (
- IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState
+ VOID
);
#endif
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index bd257f15d955..ecdab2fd9a86 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -18,6 +18,7 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include "SmiFeatures.h"
@@ -29,29 +30,26 @@
//
// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
-// for the S3 boot script fragment to write to and read from. The buffer
-// captures a combined fw_cfg item selection + write command using the DMA
-// access method. Note that we don't trust the runtime OS to preserve the
-// contents of the buffer, the boot script will first rewrite it.
+// for the S3 boot script fragment to write to and read from.
//
#pragma pack (1)
-typedef struct {
- FW_CFG_DMA_ACCESS Access;
- UINT64 Features;
+typedef union {
+ UINT64 Features;
+ UINT8 FeaturesOk;
} SCRATCH_BUFFER;
#pragma pack ()
//
// These carry the selector keys of the "etc/smi/requested-features" and
// "etc/smi/features-ok" fw_cfg files from NegotiateSmiFeatures() to
-// SaveSmiFeatures().
+// AppendFwCfgBootScript().
//
STATIC FIRMWARE_CONFIG_ITEM mRequestedFeaturesItem;
STATIC FIRMWARE_CONFIG_ITEM mFeaturesOkItem;
//
// Carries the negotiated SMI features from NegotiateSmiFeatures() to
-// SaveSmiFeatures().
+// AppendFwCfgBootScript().
//
STATIC UINT64 mSmiFeatures;
@@ -168,157 +166,81 @@ FatalError:
}
/**
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION provided to QemuFwCfgS3Lib.
+**/
+STATIC
+VOID
+EFIAPI
+AppendFwCfgBootScript (
+ IN OUT VOID *Context, OPTIONAL
+ IN OUT VOID *ExternalScratchBuffer
+ )
+{
+ SCRATCH_BUFFER *ScratchBuffer;
+ RETURN_STATUS Status;
+
+ ScratchBuffer = ExternalScratchBuffer;
+
+ //
+ // Write the negotiated feature bitmap into "etc/smi/requested-features".
+ //
+ ScratchBuffer->Features = mSmiFeatures;
+ Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
+ sizeof ScratchBuffer->Features);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ //
+ // Read back "etc/smi/features-ok". This invokes the feature validation &
+ // lockdown. (The validation succeeded at first boot.)
+ //
+ Status = QemuFwCfgS3ReadBytes (mFeaturesOkItem,
+ sizeof ScratchBuffer->FeaturesOk);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ //
+ // If "etc/smi/features-ok" read as 1, we're good. Otherwise, hang the S3
+ // resume process.
+ //
+ Status = QemuFwCfgS3CheckValue (&ScratchBuffer->FeaturesOk,
+ sizeof ScratchBuffer->FeaturesOk, MAX_UINT8, 1);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation boot script saved\n",
+ __FUNCTION__));
+ return;
+
+FatalError:
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+}
+
+
+/**
Append a boot script fragment that will re-select the previously negotiated
SMI features during S3 resume.
-
- @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to
- the S3 boot script with.
**/
VOID
SaveSmiFeatures (
- IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState
+ VOID
)
{
- SCRATCH_BUFFER *ScratchBuffer;
- EFI_STATUS Status;
- UINT64 AccessAddress;
- UINT32 ControlPollData;
- UINT32 ControlPollMask;
- UINT16 FeaturesOkItemAsUint16;
- UINT8 FeaturesOkData;
- UINT8 FeaturesOkMask;
+ RETURN_STATUS Status;
- ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
- if (ScratchBuffer == NULL) {
- DEBUG ((DEBUG_ERROR, "%a: scratch buffer allocation failed\n",
- __FUNCTION__));
- goto FatalError;
- }
-
- //
- // Populate the scratch buffer with a select + write fw_cfg DMA command that
- // will write the negotiated feature bitmap into
- // "etc/smi/requested-features".
- //
- ScratchBuffer->Access.Control = SwapBytes32 (
- (UINT32)mRequestedFeaturesItem << 16 |
- FW_CFG_DMA_CTL_SELECT |
- FW_CFG_DMA_CTL_WRITE
- );
- ScratchBuffer->Access.Length = SwapBytes32 (
- (UINT32)sizeof ScratchBuffer->Features);
- ScratchBuffer->Access.Address = SwapBytes64 (
- (UINTN)&ScratchBuffer->Features);
- ScratchBuffer->Features = mSmiFeatures;
-
- //
- // Copy the scratch buffer into the boot script. When replayed, this
- // EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE will restore the current contents of the
- // scratch buffer, in-place.
//
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)ScratchBuffer, // Address
- sizeof *ScratchBuffer, // Count
- (VOID*)ScratchBuffer // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Append an opcode that will write the address of the scratch buffer to the
- // fw_cfg DMA address register, which consists of two 32-bit IO ports. The
- // second (highest address, least significant) write will start the transfer.
+ // We are already running at TPL_CALLBACK, on the stack of
+ // OnS3SaveStateInstalled(). But that's okay, we can easily queue more
+ // notification functions while executing a notification function.
//
- AccessAddress = SwapBytes64 ((UINTN)&ScratchBuffer->Access);
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
- (UINTN)2, // Count
- &AccessAddress // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
+ Status = QemuFwCfgS3TransferOwnership (AppendFwCfgBootScript, NULL,
+ sizeof (SCRATCH_BUFFER));
+ if (RETURN_ERROR (Status)) {
+ ASSERT (FALSE);
+ CpuDeadLoop ();
}
-
- //
- // The EFI_BOOT_SCRIPT_MEM_POLL_OPCODE will wait until the Control word reads
- // as zero (transfer complete). As timeout we use MAX_UINT64 * 100ns, which
- // is approximately 58494 years.
- //
- ControlPollData = 0;
- ControlPollMask = MAX_UINT32;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)(UINTN)&ScratchBuffer->Access.Control, // Address
- &ControlPollData, // Data
- &ControlPollMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Select the "etc/smi/features-ok" fw_cfg file, which invokes the feature
- // validation & lockdown. (The validation succeeded at first boot.)
- //
- FeaturesOkItemAsUint16 = (UINT16)mFeaturesOkItem;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint16, // Width
- (UINT64)FW_CFG_IO_SELECTOR, // Address
- (UINTN)1, // Count
- &FeaturesOkItemAsUint16 // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Read the contents (one byte) of "etc/smi/features-ok". If the value is
- // one, we're good. Otherwise, continue reading the data port: QEMU returns 0
- // past the end of the fw_cfg item, so this will hang the resume process,
- // which matches our intent.
- //
- FeaturesOkData = 1;
- FeaturesOkMask = MAX_UINT8;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)FW_CFG_IO_DATA, // Address
- &FeaturesOkData, // Data
- &FeaturesOkMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_POLL_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- DEBUG ((DEBUG_VERBOSE, "%a: ScratchBuffer@%p\n", __FUNCTION__,
- (VOID *)ScratchBuffer));
- return;
-
-FatalError:
- ASSERT (FALSE);
- CpuDeadLoop ();
}
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index bb79fce0855b..e1cd3d02ac36 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -378,14 +378,15 @@ OnS3SaveStateInstalled (
CpuDeadLoop ();
}
+ DEBUG ((DEBUG_VERBOSE, "%a: chipset boot script saved\n", __FUNCTION__));
+
//
// Append a boot script fragment that re-selects the negotiated SMI features.
//
if (mSmiFeatureNegotiation) {
- SaveSmiFeatures (S3SaveState);
+ SaveSmiFeatures ();
}
- DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__));
gBS->CloseEvent (Event);
mS3SaveStateInstalled = NULL;
}
--
2.9.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
> + 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.
-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
>
>> + 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
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.
-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
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-03-13 15:12:08, Laszlo Ersek wrote:
>
> "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.
>
Right. It seems like this is a case where the text perhaps could be
more clear. Just looking at the precedence in the tree, it seems
likely that we might want to say that if the call requires multiple
lines, then the first parameter should be indented on the next line.
> - I don't see what the difference is between the two examples. To me
> they look identical.
I think this example formatting got messed up. I saw a previous
version that varied very subtlely.
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
I always thought the rule was 2 spaces indenting beyond the function
name.
These example make me think that the intension is that your tab key
might only want to align to columns that are even numbered, so you
might have the result be that the indent from the function name would
be 3 spaces.
I don't know... I prefer the rule that you indent 2 spaces from the
function name. :)
> - In the examples, the closing parens line up with "AllocatePool", not
> with "EfiBootServicesData". I think I have never ever seen this style
> in edk2.
>
Yeah. Me either. This is confusing.
-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, I agree that there are some inconsistencies in the edk2 C source files. For this specific example, I prefer what Jordan recommended: > > Status = QemuFwCfgS3WriteBytes ( > > mRequestedFeaturesItem, > > sizeof ScratchBuffer->Features > > ); When multi-line: * One argument per line * Indent each argument 2 spaces from start of function name * Close parenthesis aligned with start of arguments I am actively working on adding the EDK II C Coding standard and other EDK II docs to gitbooks. That will be a good place to discuss and clarify or correct the EDK II C coding standard. Given inconsistencies in some of the edk2 C source files and some clarification needed in EDK II C Coding Standard, I would not require any changes here. I would prefer everyone follow this specific recommendation if possible. Hopefully we can expand the PatchCheck tool over time to help check code against the EDK II C Coding Standard. Mike > -----Original Message----- > From: Justen, Jordan L > Sent: Monday, March 13, 2017 3:44 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com> > Cc: edk2-devel-01 <edk2-devel@lists.01.org> > Subject: Re: [edk2] [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with > QemuFwCfgS3Lib > > On 2017-03-13 15:12:08, Laszlo Ersek wrote: > > > > "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. > > > > Right. It seems like this is a case where the text perhaps could be > more clear. Just looking at the precedence in the tree, it seems > likely that we might want to say that if the call requires multiple > lines, then the first parameter should be indented on the next line. > > > - I don't see what the difference is between the two examples. To me > > they look identical. > > I think this example formatting got messed up. I saw a previous > version that varied very subtlely. > > Status = gBS->AllocatePool ( > EfiBootServicesData, > sizeof (DRIVER_NAME_INSTANCE), > &PrivateData > ); > > Status = gBS->AllocatePool ( > EfiBootServicesData, > sizeof (DRIVER_NAME_INSTANCE), > &PrivateData > ); > > I always thought the rule was 2 spaces indenting beyond the function > name. > > These example make me think that the intension is that your tab key > might only want to align to columns that are even numbered, so you > might have the result be that the indent from the function name would > be 3 spaces. > > I don't know... I prefer the rule that you indent 2 spaces from the > function name. :) > > > - In the examples, the closing parens line up with "AllocatePool", not > > with "EfiBootServicesData". I think I have never ever seen this style > > in edk2. > > > > Yeah. Me either. This is confusing. > > -Jordan _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/14/17 02:58, Kinney, Michael D wrote: > Laszlo, > > I agree that there are some inconsistencies in the edk2 C source files. > > For this specific example, I prefer what Jordan recommended: > >>> Status = QemuFwCfgS3WriteBytes ( >>> mRequestedFeaturesItem, >>> sizeof ScratchBuffer->Features >>> ); > > When multi-line: > * One argument per line > * Indent each argument 2 spaces from start of function name > * Close parenthesis aligned with start of arguments > > I am actively working on adding the EDK II C Coding standard > and other EDK II docs to gitbooks. That will be a good place > to discuss and clarify or correct the EDK II C coding standard. > > Given inconsistencies in some of the edk2 C source files and > some clarification needed in EDK II C Coding Standard, I would > not require any changes here. If I understand correctly, that means v2 of this set should be acceptable. If that's right, can you please R-b this set, Jordan? > I would prefer everyone follow > this specific recommendation if possible. I can do that, in the future, if we codify the preference in some written form, even before the CCS is updated and/or imported to gitbooks. Should we file a TianoCore BZ for it? I can do that, if you prefer. Thanks! Laszlo > Hopefully we can expand the PatchCheck tool over time to help > check code against the EDK II C Coding Standard. > > Mike > > >> -----Original Message----- >> From: Justen, Jordan L >> Sent: Monday, March 13, 2017 3:44 PM >> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com> >> Cc: edk2-devel-01 <edk2-devel@lists.01.org> >> Subject: Re: [edk2] [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with >> QemuFwCfgS3Lib >> >> On 2017-03-13 15:12:08, Laszlo Ersek wrote: >>> >>> "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. >>> >> >> Right. It seems like this is a case where the text perhaps could be >> more clear. Just looking at the precedence in the tree, it seems >> likely that we might want to say that if the call requires multiple >> lines, then the first parameter should be indented on the next line. >> >>> - I don't see what the difference is between the two examples. To me >>> they look identical. >> >> I think this example formatting got messed up. I saw a previous >> version that varied very subtlely. >> >> Status = gBS->AllocatePool ( >> EfiBootServicesData, >> sizeof (DRIVER_NAME_INSTANCE), >> &PrivateData >> ); >> >> Status = gBS->AllocatePool ( >> EfiBootServicesData, >> sizeof (DRIVER_NAME_INSTANCE), >> &PrivateData >> ); >> >> I always thought the rule was 2 spaces indenting beyond the function >> name. >> >> These example make me think that the intension is that your tab key >> might only want to align to columns that are even numbered, so you >> might have the result be that the indent from the function name would >> be 3 spaces. >> >> I don't know... I prefer the rule that you indent 2 spaces from the >> function name. :) >> >>> - In the examples, the closing parens line up with "AllocatePool", not >>> with "EfiBootServicesData". I think I have never ever seen this style >>> in edk2. >>> >> >> Yeah. Me either. This is confusing. >> >> -Jordan _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/14/17 14:48, Laszlo Ersek wrote: > On 03/14/17 02:58, Kinney, Michael D wrote: >> Laszlo, >> >> I agree that there are some inconsistencies in the edk2 C source files. >> >> For this specific example, I prefer what Jordan recommended: >> >>>> Status = QemuFwCfgS3WriteBytes ( >>>> mRequestedFeaturesItem, >>>> sizeof ScratchBuffer->Features >>>> ); >> >> When multi-line: >> * One argument per line >> * Indent each argument 2 spaces from start of function name >> * Close parenthesis aligned with start of arguments >> >> I am actively working on adding the EDK II C Coding standard >> and other EDK II docs to gitbooks. That will be a good place >> to discuss and clarify or correct the EDK II C coding standard. >> >> Given inconsistencies in some of the edk2 C source files and >> some clarification needed in EDK II C Coding Standard, I would >> not require any changes here. > > If I understand correctly, that means v2 of this set should be acceptable. > > If that's right, can you please R-b this set, Jordan? > >> I would prefer everyone follow >> this specific recommendation if possible. > > I can do that, in the future, if we codify the preference in some > written form, even before the CCS is updated and/or imported to > gitbooks. Should we file a TianoCore BZ for it? I can do that, if you > prefer. I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=425>. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.