Re: [edk2-devel] [PATCH v1 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and XD check only on BSP

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

> > I originally thought CheckFeatureSupported() when running in parallel when
> SMM base relocation is done in PEI
> > might corrupt the global variables.
> 
> That was / is my concern as well. That's why I asked Jiaxin if
> CheckFeatureSupported() was executed concurrently by multiple processors.
> 

Yes, Laszlo, CheckFeatureSupported is executed in parallel by multiple processors.


> > But then I realized the function only perform variable modification from
> TRUE to FALSE.
> > So even the code runs in parallel, it should be safe.
> 
> I too noticed that those transitions are all TRUE->FALSE, and (IIRC) all
> the variables are just BOOLEANs (so 1 byte).
> 
> Still, I don't like that (to be clear, I don't even like the pre-patch
> state):
> 
> - The global variables are not even marked volatile.
> - We don't use any locking, like interlocked exchange.
> - We do other things than just "mBoolean = FALSE": we patch instructions
> in the SMI assembly code, too.
> 
> All in all -- as long as this function can indeed run on multiple
> processors at the same time --, I think it would be much *cleaner* to:
> 
> - split the BSP-only functionality off of CheckFeatureSupported() --
> more importantly, off of SmmInitHandler. All that feature testing could
> be done in "single-threaded" code, could it not?
> 

I doubt the change to split the functionality check off of the SmmInitHandler. SmmInitHandler is called in SMI. I'm are not 100% sure the CET & XD & BTS features are same in non-smm & smm environment. If we move out of the smm environment, it might be have potential issue. Stay in SMI is more safe change.


> - if we need to check some feature on each processor in separation, and
> then build a big conjunction of those results in the end, then I find it
> better if the implementation actually follows this logic. Let the
> processors store their results in separate BOOLEAN elements of an array,
> and let the BSP go over the array once the APs have settled.
> 

I also thought of that way to handle the BTS feature since it's the only feature need check on each processor. That is the good way to handle the global variable might be corrupted by the multiple modification. but as Ray explained, the function only perform variable modification from TRUE to FALSE. So even the code runs in parallel, it's safe. That's the reason why I keep the original logic. 

> Minimally, at least document why the current lock-less but concurrent
> approach is safe!
> 

I agree. We can document in code comment. I will refine the patch to include this. 

Thanks,
Jiaxin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115076): https://edk2.groups.io/g/devel/message/115076
Mute This Topic: https://groups.io/mt/104094806/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-