Re: [edk2-devel] ExtScsiPassThru support for Logical SCSI devices
> -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Saturday, July 13, 2019 6:11 AM > To: siadriravikumaru...@gmail.com > Cc: devel@edk2.groups.io; Wu, Hao A; Cetola, Stephano > Subject: Re: [edk2-devel] ExtScsiPassThru support for Logical SCSI devices > > On 06/10/19 14:32, Ravi Kumar Siadri wrote: > > Hi all, > > > > I have couple of queries regarding the > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL attribute support in > > EFI_EXT_SCSI_PASS_THRU_PROTOCOL. > > > > 1. > > The Attributes field of the EFI_EXT_SCSI_PASS_THRU_PROTOCOL interface > > tells if the interface is for physical SCSI devices or logical SCSI > > devices. Drivers for non-RAID SCSI controllers will set both the > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL, and the > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL bits. > > > > If the Drivers is for non-RAID SCSI controllers why to set the > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL bit. Will there be any > case > > that non-RAID SCSI controllers have Logical devices connected to it ?? > > > > 2. > > Drivers for RAID controllers that allow access to the physical devices > > and logical devices will produce two EFI_EXT_SCSI_PASS_THRU_PROTOCOL > > interfaces: One with the just the > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL bit set and another > with > > just the EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL bit set. One > > interface can be used to access the physical devices attached to the > > RAID controller, and the other can be used to access the logical > > devices attached to the RAID controller for its current configuration. > > > > Any background why EFI_EXT_SCSI_PASS_THRU_PROTOCOL needed 2 > instances > > ? why can't we install attributes > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL and > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL on same Instance ?? > How > > User locates the Instance with > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL bit set and Instance with > > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL bit set. ?? > > Please consult the UEFI Driver Writer's Guide on these questions. (What > I have on my disk now is Version 1.01, for UEFI 2.3.1, released > 03/08/2012.) Thanks Laszlo, The information mentioned above is really helpful. As a supplementary, the UEFI Driver Writer's Guide is also available online at: https://edk2-docs.gitbooks.io/edk-ii-uefi-driver-writer-s-guide/ Best Regards, Hao Wu > > Excerpts: > > > 6.3 Hybrid drivers > > > > [...] > > > > A driver for a multi-channel RAID SCSI host controller is a hybrid > > driver. It produces the Extended SCSI Pass Thru Protocol (with the > > logical bit on) on the controller handle and creates child handles > > with Extended SCSI Pass Thru Protocol for each physical channel (with > > the logical bit off). > > > 20.1.3 SCSI Adapters with RAID > > > > A SCSI host controller driver may also support SCSI adapters with RAID > > capability. The following figure shows an example implementation with > > two physical SCSI channels and one logical channel. The two physical > > channels are implemented on the SCSI adapter. The RAID component then > > configures these two channels to produce a logical SCSI channel. The > > two physical channels each have Extended SCSI Pass Thru installed, but > > these are not be used except for diagnostic use. For the logical > > channel, the SCSI host controller driver produces another Extended > > SCSI Pass Thru Protocol (with physical bit turned off) instance based > > on the RAID configuration. Requests sent to the Extended SCSI Pass > > Thru protocol for the logical channel are processed by the SCSI host > > controller drivers and converted into requests on the physical SCSI > > channels. The platform firmware must only enumerate and boot from SCSI > > targets present on the logical SCSI channel. > > > > The SCSI adapter hardware may not be able to expose the physical SCSI > > channel(s) to upper-level software when implementing RAID. If the > > physical SCSI channel cannot be exposed to upper software, then the > > SCSI host controller driver is only required to produce a single > > logical channel for the RAID. > > > > Although the basic theory is the same as the one on a physical > > channel, it is different from a manufacturing and diagnostic > > perspective. If the physical SCSI channels are exposed, any SCSI > > command, including diagnostic ones, can be sent to an individual > > channel, which is very helpful on manufacturing lines. Furthermore, > > the diagnostic command can be sent simultaneously to all physical > > channels using the non-blocking mode that is supported by Extended > > SCSI Pass Thru Protocol. The diagnostic process may considerably > > benefit from the performance gain. In summary, it is suggested to > > expose physical SCSI channel whenever possible. > > > 20.1.4.2 Start() > > > > [...] > > > > The Start() function tells the SCSI host controller driver to start > > managing the SCSI host controller. In this function, a
Re: [edk2-devel] [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
Reviewed-by: Ray Ni
> -Original Message-
> From: Zeng, Star
> Sent: Friday, July 12, 2019 6:53 PM
> To: Dong, Eric ; devel@edk2.groups.io
> Cc: Ni, Ray ; Laszlo Ersek ; Kumar,
> Chandana C ; Zeng, Star
>
> Subject: RE: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> gBS service.
>
> Some minor comments inline.
>
> > -Original Message-
> > From: Dong, Eric
> > Sent: Friday, July 12, 2019 9:53 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray ; Laszlo Ersek ;
> > Kumar, Chandana C ; Zeng, Star
> >
> > Subject: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> > gBS service.
>
> "gBS service" is not match with the assertion information, gBS is the concept
> in DXE phase.
> So here, please "PEI service" to be accurate.
>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
> >
> > AP calls CollectProcessorData() to collect processor info.
> > CollectProcessorData function finally calls PcdGetSize function to get
> > DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
>
> Same comments with above.
>
> > which caused below assert info:
> > Processor Info: Package: 1, MaxCore : 4, MaxThread: 1
> > Package: 0, Valid Core : 4
> > ASSERT [CpuFeaturesPei] c:\projects\jsl\jsl_v1193\Edk2\MdePkg\Library
> > \PeiServicesTablePointerLibIdt\PeiServicesTablePointer.c(48):
> > PeiServices != ((void *) 0)
> >
> > This change uses saved global pcd size instead of calls PcdGetSize to
> > fix this issue.
> >
> > Cc: Ray Ni
> > Cc: Laszlo Ersek
> > Cc: Chandana Kumar
> > Cc: Star Zeng
> > Signed-off-by: Eric Dong
> > ---
> > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 13
> > - .../RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c |
> > 5 +
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index aff7ad600c..87bfc64250 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -246,19 +246,20 @@ CpuInitDataInitialize (
> >
> >@param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> >@param[in] OrFeatureBitMask The feature bit mask to do OR
> operation
> > + @param[in] BitMaskSize The CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > SupportedMaskOr (
> >IN UINT8 *SupportedFeatureMask,
> > - IN UINT8 *OrFeatureBitMask
> > + IN UINT8 *OrFeatureBitMask,
> > + IN UINT32 BitMaskSize
> >)
> > {
> >UINTN Index;
> > - UINTN BitMaskSize;
> >UINT8 *Data1;
> >UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >Data1 = SupportedFeatureMask;
> >Data2 = OrFeatureBitMask;
> >for (Index = 0; Index < BitMaskSize; Index++) { @@ -384,12 +385,14
> > @@ CollectProcessorData (
> >//
> >SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > -CpuFeature->FeatureMask
> > +CpuFeature->FeatureMask,
> > +CpuFeaturesData->BitMaskSize
> > );
> > } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo,
> > CpuFeature->ConfigData)) {
> >SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > -CpuFeature->FeatureMask
> > +CpuFeature->FeatureMask,
> > +CpuFeaturesData->BitMaskSize
> > );
> > }
> > Entry = Entry->ForwardLink;
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index fa0f0b41e2..36aabd7267 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > @@ -658,6 +658,11 @@ RegisterCpuFeatureWorker (
> > InitializeListHead (>FeatureList);
> > InitializeSpinLock (>CpuFlags.MemoryMappedLock);
> > InitializeSpinLock (>CpuFlags.ConsoleLogLock);
> > +//
> > +// Driver has assumption that these three PCD should has same
> > + buffer
> > size.
>
> It is library, not driver. So how about "The code has assumption that these
> three PCDs should have same buffer size."?
> The proposed sentence also uses 'PCDs' and 'have'.
>
>
> With the comments handled, Reviewed-by: Star Zeng
>
> Thanks,
> Star
>
> > +//
> > +ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesCapability));
> > +ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesSupport));
> > CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
>
Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
1. FeatureMaskSize can be BitMaskSize. It clearly tells the caller that this
function will assume bit size of SupportedFeatureMask is the same as that of
PcdCpuFeaturesCapability.
SetCapabilityPcd (
IN UINT8 *SupportedFeatureMask,
IN UINTN FeatureMaskSize
2. IsBitMaskMatchCheck () returns TRUE when the bits in FeatureMask and
DependentBitMask overlap. We cannot change its behavior using CompareMem.
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Dong,
> Eric
> Sent: Friday, July 12, 2019 9:53 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray ; Laszlo Ersek ; Kumar,
> Chandana C ; Zeng, Star
>
> Subject: [edk2-devel] [Patch 2/2]
> UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
>
> Function in this library may be used by APs. Assert will be trig if AP uses
> dynamic pcd.
> This patch enhance the current code, remove the unnecessary usage of
> dynamic PCD. This change try to avoid report this issue again later.
>
> Cc: Ray Ni
> Cc: Laszlo Ersek
> Cc: Chandana Kumar
> Cc: Star Zeng
> Signed-off-by: Eric Dong
> ---
> .../CpuFeaturesInitialize.c | 64 +-
> .../RegisterCpuFeatures.h | 10 +-
> .../RegisterCpuFeaturesLib.c | 114 ++
> 3 files changed, 77 insertions(+), 111 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 87bfc64250..16b99c0c27 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -21,16 +21,12 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR",
> L"MMIO", L"CACHE", L"SEMAP", L"INVA VOID SetCapabilityPcd (
>IN UINT8 *SupportedFeatureMask,
> - IN UINT32 FeatureMaskSize
> + IN UINTN FeatureMaskSize
>)
> {
>EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
> - ASSERT (FeatureMaskSize == BitMaskSize);
> -
> - Status = PcdSetPtrS (PcdCpuFeaturesCapability, ,
> SupportedFeatureMask);
> + Status = PcdSetPtrS (PcdCpuFeaturesCapability, ,
> + SupportedFeatureMask);
>ASSERT_EFI_ERROR (Status);
> }
>
> @@ -38,16 +34,16 @@ SetCapabilityPcd (
>Worker function to save PcdCpuFeaturesSetting.
>
>@param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> + @param[in] FeatureMaskSize CPU feature bits mask buffer size.
> **/
> VOID
> SetSettingPcd (
> - IN UINT8 *SupportedFeatureMask
> + IN UINT8 *SupportedFeatureMask,
> + IN UINTN BitMaskSize
>)
> {
>EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Status = PcdSetPtrS (PcdCpuFeaturesSetting, ,
> SupportedFeatureMask);
>ASSERT_EFI_ERROR (Status);
> }
> @@ -272,19 +268,20 @@ SupportedMaskOr (
>
>@param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
>@param[in] AndFeatureBitMask The feature bit mask to do AND
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> SupportedMaskAnd (
>IN UINT8 *SupportedFeatureMask,
> - IN CONST UINT8 *AndFeatureBitMask
> + IN CONST UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
>)
> {
>UINTN Index;
> - UINTN BitMaskSize;
>UINT8 *Data1;
>CONST UINT8*Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Data1 = SupportedFeatureMask;
>Data2 = AndFeatureBitMask;
>for (Index = 0; Index < BitMaskSize; Index++) { @@ -297,19 +294,19 @@
> SupportedMaskAnd (
>
>@param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
>@param[in] AndFeatureBitMask The feature bit mask to do XOR
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> **/
> VOID
> SupportedMaskCleanBit (
>IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *AndFeatureBitMask
> + IN UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
>)
> {
>UINTN Index;
> - UINTN BitMaskSize;
>UINT8 *Data1;
>UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Data1 = SupportedFeatureMask;
>Data2 = AndFeatureBitMask;
>for (Index = 0; Index < BitMaskSize; Index++) { @@ -323,6 +320,7 @@
> SupportedMaskCleanBit (
>
>@param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
>
Re: [edk2-devel] [PATCH] MdeModulePkg: Fix various typos
Hello Coeur,
Thanks for the effort.
For other packages, I saw you always use one patch to address all the
typos you found in that package.
As you mentioned, the typos number in MdeModulePkg is rather large so you
decide to split them into multiple patches. So could you help to do it in
finer granularity and use 1 patch to address the typos in 1 module/library?
If the number of patches is huge (maybe over 20), you may push them on a
branch within your fork of the edk2 repo and add the link of such branch
for review.
Thanks in advance.
Best Regards,
Hao Wu
From: Antoine Coeur [mailto:co...@gmx.fr]
Sent: Saturday, July 13, 2019 1:31 PM
To: devel@edk2.groups.io
Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star
Subject: Fw: [PATCH] MdeModulePkg: Fix various typos
CC the maintainers.
This is only a small subset of all the spelling/typo issues from MdeModulePkg.
But I don't want to overload you with too much to review, so I'll only post one
MdeModulePkg batch at a time.
Coeur
Gesendet: Samstag, 13. Juli 2019 um 12:57 Uhr
Von: "Antoine Cœur"
An: devel@edk2.groups.io
Cc: "Antoine Cœur"
Betreff: [PATCH] MdeModulePkg: Fix various typos
Fix various typos in MdeModulePkg.
Signed-off-by: Coeur
---
.../Application/CapsuleApp/CapsuleApp.c | 2 +-
.../Application/CapsuleApp/CapsuleDump.c | 6 ++---
.../PlatformVarCleanupLib/PlatVarCleanupLib.c | 10 +++
MdeModulePkg/MdeModulePkg.dec | 26 +--
MdeModulePkg/MdeModulePkg.uni | 8 +++---
.../Universal/DevicePathDxe/DevicePathDxe.uni | 4 +--
.../Disk/RamDiskDxe/RamDiskProtocol.c | 2 +-
.../EbcDxe/EbcDebugger/EdbCmdSymbol.c | 10 +++
.../PlatDriOverrideDxe.uni | 10 +++
9 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/MdeModulePkg/Application/CapsuleApp/CapsuleApp.c
b/MdeModulePkg/Application/CapsuleApp/CapsuleApp.c
index 3439ce5feb..4034714773 100644
--- a/MdeModulePkg/Application/CapsuleApp/CapsuleApp.c
+++ b/MdeModulePkg/Application/CapsuleApp/CapsuleApp.c
@@ -494,7 +494,7 @@ BuildGatherList (
}
//
- // Record descirptor header
+ // Record descriptor header
//
if (Index == 0) {
BlockDescriptorsHeader = BlockDescriptors1;
diff --git a/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
b/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
index 58a93568d0..d623d7c809 100644
--- a/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
+++ b/MdeModulePkg/Application/CapsuleApp/CapsuleDump.c
@@ -38,7 +38,7 @@ DumpUxCapsule (
{
EFI_DISPLAY_CAPSULE *DisplayCapsule;
DisplayCapsule = (EFI_DISPLAY_CAPSULE *)CapsuleHeader;
- Print(L"[UxCapusule]\n");
+ Print(L"[UxCapsule]\n");
Print(L"CapsuleHeader:\n");
Print(L" CapsuleGuid - %g\n", >CapsuleHeader.CapsuleGuid);
Print(L" HeaderSize - 0x%x\n", DisplayCapsule->CapsuleHeader.HeaderSize);
@@ -199,7 +199,7 @@ DumpCapsule (
DumpFmpCapsule(CapsuleHeader);
}
if (IsNestedFmpCapsule(CapsuleHeader)) {
- Print(L"[NestedCapusule]\n");
+ Print(L"[NestedCapsule]\n");
Print(L"CapsuleHeader:\n");
Print(L" CapsuleGuid - %g\n", >CapsuleGuid);
Print(L" HeaderSize - 0x%x\n", CapsuleHeader->HeaderSize);
@@ -793,7 +793,7 @@ DumpCapsuleFromDisk (
goto Done;
}
- Print(L"The infomation of the capsules:\n");
+ Print(L"The information of the capsules:\n");
for (Index = 0; Index < FileCount; Index++) {
FileHandle = NULL;
diff --git a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
index 968c044a31..d9daf7f86a 100644
--- a/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
+++ b/MdeModulePkg/Library/PlatformVarCleanupLib/PlatVarCleanupLib.c
@@ -260,12 +260,12 @@ CreateUserVariableNode (
ASSERT (UserVariableNameNode->PromptString != NULL);
UnicodeSPrint (UserVariableNameNode->PromptString, StringSize, L" %s",
UserVariableNameNode->Name);
//
- // (33 chars of "Attribtues = 0x and DataSize = 0x" + 1 terminator + (sizeof
(UINT32) + sizeof (UINTN)) * 2) * sizeof (CHAR16).
+ // (33 chars of "Attributes = 0x and DataSize = 0x" + 1 terminator + (sizeof
(UINT32) + sizeof (UINTN)) * 2) * sizeof (CHAR16).
//
StringSize = (33 + 1 + (sizeof (UINT32) + sizeof (UINTN)) * 2) * sizeof
(CHAR16);
UserVariableNameNode->HelpString = AllocatePool (StringSize);
ASSERT (UserVariableNameNode->HelpString != NULL);
- UnicodeSPrint (UserVariableNameNode->HelpString, StringSize, L"Attribtues =
0x%08x and DataSize = 0x%x", UserVariableNameNode->Attributes,
UserVariableNameNode->DataSize);
+ UnicodeSPrint (UserVariableNameNode->HelpString, StringSize, L"Attributes =
0x%08x and DataSize = 0x%x", UserVariableNameNode->Attributes,
UserVariableNameNode->DataSize);
UserVariableNameNode->Deleted = FALSE;
InsertTailList (>NameLink, >Link);
Index++;
@@ -332,7 +332,7 @@ DestroyUserVariableNode (
it's caller's responsibility to free the memory after using it.
@retval EFI_SUCCESS Create time based payload successfully.
- @retval EFI_OUT_OF_RESOURCES There are not enough memory resourses to create
time based payload.
+ @retval EFI_OUT_OF_RESOURCES There are
