Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-14 Thread Paolo Bonzini 
On 13/10/2015 15:26, Laszlo Ersek wrote:
>>//
>> +  // The write to the control register is synchronous and only affects the
>> +  // current CPU, so bring in the APs first.  The SMI handler expects that
>> +  // all APs will rendez-vous within one PcdCpuSmmApSyncTimeout (though it
>> +  // helpfully tries sending SMI IPIs to the missing processors if there are
>> +  // any).
>> +  //
>> +  SendSmiIpiAllExcludingSelf ();
>> +
>> +  //

Nevermind, this patch is unfortunately broken.

SendSmiIpiAllExcludingSelf does not work after ExitBootServices, because
it hardcodes the physical address of the APIC.  While that could be
fixed in LocalApicLib, it turns out that using SmmSyncModeRelaxedAp is a
trivial addition to my series that introduces a SmmCpuFeaturesLib
implementation specific to OvmfPkg.  Therefore, I'm withdrawing this
patch and submitting another in that thread.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-14 Thread Laszlo Ersek 
On 10/14/15 13:37, Paolo Bonzini wrote:
> On 13/10/2015 15:26, Laszlo Ersek wrote:
>>>//
>>> +  // The write to the control register is synchronous and only affects the
>>> +  // current CPU, so bring in the APs first.  The SMI handler expects that
>>> +  // all APs will rendez-vous within one PcdCpuSmmApSyncTimeout (though it
>>> +  // helpfully tries sending SMI IPIs to the missing processors if there 
>>> are
>>> +  // any).
>>> +  //
>>> +  SendSmiIpiAllExcludingSelf ();
>>> +
>>> +  //
> 
> Nevermind, this patch is unfortunately broken.
> 
> SendSmiIpiAllExcludingSelf does not work after ExitBootServices, because
> it hardcodes the physical address of the APIC.  While that could be
> fixed in LocalApicLib, it turns out that using SmmSyncModeRelaxedAp is a
> trivial addition to my series that introduces a SmmCpuFeaturesLib
> implementation specific to OvmfPkg.  Therefore, I'm withdrawing this
> patch and submitting another in that thread.

Thank you.

If you create the SmmCpuFeaturesLib instance as a copy, initially,
please make sure that you use, as basis, the CRLF-converted patches of
Mike that I pushed last night. So that the copy come from a
CRLF-terminated source, and your further customizations be similar.

Thank you!
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Paolo Bonzini 


On 09/10/2015 23:58, Laszlo Ersek wrote:
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c 
> b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> new file mode 100644
> index 000..e895fd6
> --- /dev/null
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> @@ -0,0 +1,365 @@
> +/** @file
> +
> +  A DXE_RUNTIME_DRIVER providing synchronous SMI activations via the
> +  EFI_SMM_CONTROL2_PROTOCOL.
> +
> +  We expect the PEI phase to have covered the following:
> +  - ensure that the underlying QEMU machine type be Q35
> +(responsible: OvmfPkg/SmmAccess/SmmAccessPei.inf)
> +  - ensure that the ACPI PM IO space be configured
> +(responsible: OvmfPkg/PlatformPei/PlatformPei.inf)
> +
> +  Our own entry point is responsible for confirming the SMI feature and for
> +  configuring it.
> +
> +  Copyright (C) 2013, 2015, Red Hat, Inc.
> +  Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution. The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> WITHOUT
> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +//
> +// The absolute IO port address of the SMI Control and Enable Register. It is
> +// only used to carry information from the entry point function to the
> +// S3SaveState protocol installation callback, strictly before the runtime
> +// phase.
> +//
> +STATIC UINTN mSmiEnable;
> +
> +//
> +// Event signaled when an S3SaveState protocol interface is installed.
> +//
> +STATIC EFI_EVENT mS3SaveStateInstalled;
> +
> +/**
> +  Invokes SMI activation from either the preboot or runtime environment.
> +
> +  This function generates an SMI.
> +
> +  @param[in] ThisThe EFI_SMM_CONTROL2_PROTOCOL instance.
> +  @param[in,out] CommandPort The value written to the command port.
> +  @param[in,out] DataPortThe value written to the data port.
> +  @param[in] PeriodicOptional mechanism to engender a 
> periodic
> + stream.
> +  @param[in] ActivationInterval  Optional parameter to repeat at this
> + period one time or, if the Periodic
> + Boolean is set, periodically.
> +
> +  @retval EFI_SUCCESSThe SMI/PMI has been engendered.
> +  @retval EFI_DEVICE_ERROR   The timing is unsupported.
> +  @retval EFI_INVALID_PARAMETER  The activation period is unsupported.
> +  @retval EFI_INVALID_PARAMETER  The last periodic activation has not been
> + cleared.
> +  @retval EFI_NOT_STARTEDThe SMM base service has not been 
> initialized.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +SmmControl2DxeTrigger (
> +  IN CONST EFI_SMM_CONTROL2_PROTOCOL  *This,
> +  IN OUT UINT8*CommandPort   OPTIONAL,
> +  IN OUT UINT8*DataPort  OPTIONAL,
> +  IN BOOLEAN  Periodic   OPTIONAL,
> +  IN UINTNActivationInterval OPTIONAL
> +  )
> +{
> +  //
> +  // No support for queued or periodic activation.
> +  //
> +  if (Periodic || ActivationInterval > 0) {
> +return EFI_DEVICE_ERROR;
> +  }
> +
> +  //
> +  // The so-called "Advanced Power Management Status Port Register" is in 
> fact
> +  // a generic data passing register, between the caller and the SMI
> +  // dispatcher. The ICH9 spec calls it "scratchpad register" --  calling it
> +  // "status" elsewhere seems quite the misnomer. Status registers usually
> +  // report about hardware status, while this register is fully governed by
> +  // software.
> +  //
> +  // Write to the status register first, as this won't trigger the SMI just
> +  // yet. Then write to the control register.
> +  //
> +  IoWrite8 (ICH9_APM_STS, DataPort== NULL ? 0 : *DataPort);
> +  IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
> +  return EFI_SUCCESS;
> +}

I tested multiprocessor SMM with your branch, and we're really close.  KVM
doesn't handle SMIs correctly (ironically I fixed the same bug in QEMU with
commit a9bad65, "target-i386: wake up processors that receive an SMI",
2015-05-19).  TCG boots... but it takes about 5 minutes.

This is because the SMI handler, and in particular SmmWaitForApArrival,
expects that SmmControl2DxeTrigger triggers an SMI IPI on all processors
rather than just the BSP.  QEMU's port 0xb2 only affects the current CPU,
thus all SMM invocations loop for a second (the default value of
PcdCpuSmmApSyncTimeout) before SmmWaitForApArrival sends another SMI IPI
to the

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Paolo Bonzini 


On 13/10/2015 15:26, Laszlo Ersek wrote:
> On the other hand, I don't know if Quark is a UP vs. MP system to begin
> with.

Quark is UP only.  It's so UP that it triggers an erratum if you use
lock-prefixed instructions. :)

> Either the EFI_SMM_CONTROL2_PROTOCOL.Trigger()
> spec is incomplete in PI, or QEMU's emulation of APM_CNT is not
> appropriate.

That's possible.  However, it's too late to change QEMU.

[I always feel bad when snipping a long reply to 3 lines...]

Paolo

> Either way, I'm okay with this fix, but I'd like to
> understand the expectations.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Laszlo Ersek 
On 10/13/15 15:38, Paolo Bonzini wrote:
> 
> 
> On 13/10/2015 15:26, Laszlo Ersek wrote:
>> On the other hand, I don't know if Quark is a UP vs. MP system to begin
>> with.
> 
> Quark is UP only.  It's so UP that it triggers an erratum if you use
> lock-prefixed instructions. :)
> 
>> Either the EFI_SMM_CONTROL2_PROTOCOL.Trigger()
>> spec is incomplete in PI, or QEMU's emulation of APM_CNT is not
>> appropriate.
> 
> That's possible.  However, it's too late to change QEMU.
> 
> [I always feel bad when snipping a long reply to 3 lines...]

No problem. :)

I'll squash your fix into the next version, with many thanks. :) It's
our platform after all, and we can do in OvmfPkg whatever needs to be
done on it. :)

Cheers!
Laszlo

> 
> Paolo
> 
>> Either way, I'm okay with this fix, but I'd like to
>> understand the expectations.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Paolo Bonzini 


On 13/10/2015 15:43, Laszlo Ersek wrote:
> I'll squash your fix into the next version, with many thanks. :) It's
> our platform after all, and we can do in OvmfPkg whatever needs to be
> done on it. :)

Indeed.  Though, since the OVMF SmmCpuFeaturesLib will not require
configuring the MTRRs on every SMI, we can also look into using
SmmCpuSyncModeRelaxedAp and removing the IPI.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Paolo Bonzini 


On 13/10/2015 18:35, Brian J. Johnson wrote:
> 
> Traditionally, SMI handling has been global.  If the h/w didn't
> broadcast the SMI to all CPUs, the SMI handler did so itself.  The BSP
> would wait for all APs to "check in" to SMM, then it would do whatever
> work the SMI required, and signal the APs to resume.  That ensured that
> the OS wasn't active on the machine while the BSP was handling the SMI,
> which was required for certain uses of SMI.
> 
> However, this (obviously) doesn't scale well, so Intel has been moving
> towards signaling SMI to only a single processor, and avoiding the
> machine-wide rendezvous when it isn't necessary.

Yup, this is the RelaxedAp sync mode.  We can (and should) adopt it later.

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Paolo Bonzini 


On 13/10/2015 18:49, Laszlo Ersek wrote:
>> > 
>> > However, this (obviously) doesn't scale well, so Intel has been moving
>> > towards signaling SMI to only a single processor, and avoiding the
>> > machine-wide rendezvous when it isn't necessary.  BIOS implementations
>> > may be lagging behind.
> But... when is it necessary? Paolo implied it might not be necessary for
> us because MTRR changes are not relevant on our virtual platform --
> otherwise all CPUs would have to agree on the MTRR settings --, but
> isn't there a security aspect to it as well?

MTRR changes are only needed on processors without SMRRs (which is 32
bits processors according to the default SmmCpuFeaturesLib).

We don't have SMRRs, but we also are immune from caching issues because
all of our memory mapping is done in the hypervisor and the processor
sees it.  On real hardware, it's done in the MCH (northbridge).

In any case, it's all customizable through SmmCpuFeaturesLib.
SmmCpuFeaturesLib and EFI_SMM_CONTROL2_PROTOCOL must be somehow in sync,
which perhaps is why the UEFI spec doesn't go into details.

PcdCpuSmmSyncMode is also not part of the UEFI spec, I guess?

Paolo

> All UefiCpuPkg/UefiCpuPkg.dec says is:
> 
> ## Indicates the CPU synchronization method used when processing an SMI.
> #   0x00  - Traditional CPU synchronization method.
> #   0x01  - Relaxed CPU synchronization method.
> # @Prompt SMM CPU Synchronization Method.
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x6014
> 
> Uhm. Thanks?...

I like it when the answer is in the code! :)

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Laszlo Ersek 
On 10/13/15 18:53, Paolo Bonzini wrote:
> 
> 
> On 13/10/2015 18:49, Laszlo Ersek wrote:

 However, this (obviously) doesn't scale well, so Intel has been moving
 towards signaling SMI to only a single processor, and avoiding the
 machine-wide rendezvous when it isn't necessary.  BIOS implementations
 may be lagging behind.
>> But... when is it necessary? Paolo implied it might not be necessary for
>> us because MTRR changes are not relevant on our virtual platform --
>> otherwise all CPUs would have to agree on the MTRR settings --, but
>> isn't there a security aspect to it as well?
> 
> MTRR changes are only needed on processors without SMRRs (which is 32
> bits processors according to the default SmmCpuFeaturesLib).
> 
> We don't have SMRRs, but we also are immune from caching issues because
> all of our memory mapping is done in the hypervisor and the processor
> sees it.  On real hardware, it's done in the MCH (northbridge).
> 
> In any case, it's all customizable through SmmCpuFeaturesLib.
> SmmCpuFeaturesLib and EFI_SMM_CONTROL2_PROTOCOL must be somehow in sync,
> which perhaps is why the UEFI spec doesn't go into details.

EFI_SMM_CONTROL2_PROTOCOL is from the PI spec, not the UEFI spec.

SmmCpuFeaturesLib is an edk2 artifact, not governed by PI.

(But, of course, it might be sensible to require that *in edk2* the two
be in sync!)

> 
> PcdCpuSmmSyncMode is also not part of the UEFI spec, I guess?

PCDs are also edk2 artifacts. In DXE they (the dynamic ones) are
implemented with a protocol "of course", but that protocol is not
specified in any industry standard.

Thanks!
Laszlo

> 
> Paolo
> 
>> All UefiCpuPkg/UefiCpuPkg.dec says is:
>>
>> ## Indicates the CPU synchronization method used when processing an SMI.
>> #   0x00  - Traditional CPU synchronization method.
>> #   0x01  - Relaxed CPU synchronization method.
>> # @Prompt SMM CPU Synchronization Method.
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x6014
>>
>> Uhm. Thanks?...
> 
> I like it when the answer is in the code! :)
> 
> Paolo
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Tim Lewis 
Laszlo, 

The PI specification describes DynamicEx PCDs. The PCD protocol is in PI: 
volume 3, chapter 8.

All other PCDs are EDK2 artifacts, although some bits have drifted into the 
packaging specification. 

Tim

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Tuesday, October 13, 2015 10:03 AM
To: Paolo Bonzini <pbonz...@redhat.com>; Brian J. Johnson <bjohn...@sgi.com>; 
edk2-de...@ml01.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Jiewen Yao 
<jiewen@intel.com>
Subject: Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement 
EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

On 10/13/15 18:53, Paolo Bonzini wrote:
> 
> 
> On 13/10/2015 18:49, Laszlo Ersek wrote:
>>>>
>>>> However, this (obviously) doesn't scale well, so Intel has been 
>>>> moving towards signaling SMI to only a single processor, and 
>>>> avoiding the machine-wide rendezvous when it isn't necessary.  BIOS 
>>>> implementations may be lagging behind.
>> But... when is it necessary? Paolo implied it might not be necessary 
>> for us because MTRR changes are not relevant on our virtual platform 
>> -- otherwise all CPUs would have to agree on the MTRR settings --, 
>> but isn't there a security aspect to it as well?
> 
> MTRR changes are only needed on processors without SMRRs (which is 32 
> bits processors according to the default SmmCpuFeaturesLib).
> 
> We don't have SMRRs, but we also are immune from caching issues 
> because all of our memory mapping is done in the hypervisor and the 
> processor sees it.  On real hardware, it's done in the MCH (northbridge).
> 
> In any case, it's all customizable through SmmCpuFeaturesLib.
> SmmCpuFeaturesLib and EFI_SMM_CONTROL2_PROTOCOL must be somehow in 
> sync, which perhaps is why the UEFI spec doesn't go into details.

EFI_SMM_CONTROL2_PROTOCOL is from the PI spec, not the UEFI spec.

SmmCpuFeaturesLib is an edk2 artifact, not governed by PI.

(But, of course, it might be sensible to require that *in edk2* the two be in 
sync!)

> 
> PcdCpuSmmSyncMode is also not part of the UEFI spec, I guess?

PCDs are also edk2 artifacts. In DXE they (the dynamic ones) are implemented 
with a protocol "of course", but that protocol is not specified in any industry 
standard.

Thanks!
Laszlo

> 
> Paolo
> 
>> All UefiCpuPkg/UefiCpuPkg.dec says is:
>>
>> ## Indicates the CPU synchronization method used when processing an SMI.
>> #   0x00  - Traditional CPU synchronization method.
>> #   0x01  - Relaxed CPU synchronization method.
>> # @Prompt SMM CPU Synchronization Method.
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x6014
>>
>> Uhm. Thanks?...
> 
> I like it when the answer is in the code! :)
> 
> Paolo
> 

___
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

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Laszlo Ersek 
On 10/13/15 18:35, Brian J. Johnson wrote:
> On 10/13/2015 08:26 AM, Laszlo Ersek wrote:
>>
>> First of all, if the edk2 reference code (in the SMM core and in
>> PiSmmCpuDxeSmm) depends on such behavior justifiedly, then I think we
>> have a bug in the PI specification. Namely, version 1.4 thereof does not
>> seem to require that EFI_SMM_CONTROL2_PROTOCOL.Trigger() raise an SMI on
>> *all* processors. (See volume 4, section 5.4.)
>>
>> In particular, the EFI_SMM_CONTROL2_PROTOCOL.Trigger() specification
>> makes many references to SMI handling and dispatching. But I cannot make
>> a mental connection between an SMI "broadcast", and "handling and
>> dispatching" in edk2, since all "handling and dispatching" code in edk2
>> that is exposed via protocols, is non-reentrant with regard to multiple
>> VCPUs, to my knowledge.
>>
>> One could argue that whatever code handles the SMI on the BP is
>> responsible for bringing the APs into SMM as well, before doing any
>> sensitive work. I'm not sure.
> 
> Traditionally, SMI handling has been global.  If the h/w didn't
> broadcast the SMI to all CPUs, the SMI handler did so itself.  The BSP
> would wait for all APs to "check in" to SMM, then it would do whatever
> work the SMI required, and signal the APs to resume.  That ensured that
> the OS wasn't active on the machine while the BSP was handling the SMI,
> which was required for certain uses of SMI.
> 
> However, this (obviously) doesn't scale well, so Intel has been moving
> towards signaling SMI to only a single processor, and avoiding the
> machine-wide rendezvous when it isn't necessary.  BIOS implementations
> may be lagging behind.

But... when is it necessary? Paolo implied it might not be necessary for
us because MTRR changes are not relevant on our virtual platform --
otherwise all CPUs would have to agree on the MTRR settings --, but
isn't there a security aspect to it as well?

All UefiCpuPkg/UefiCpuPkg.dec says is:

## Indicates the CPU synchronization method used when processing an SMI.
#   0x00  - Traditional CPU synchronization method.
#   0x01  - Relaxed CPU synchronization method.
# @Prompt SMM CPU Synchronization Method.
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x6014

Uhm. Thanks?...

> 
>>
>> Second, if writing to ioport 0xb2 should *automatically* raise an SMI on
>> all processors, then the QEMU code could be incorrect. However I could
>> never derive such an "imperative" from the ICH9 spec.
> 
> I too am having a hard time finding a clear statement of whether or not
> ioport 0xb2 should *automatically* raise an SMI on all processors. Maybe
> it's platform-specific?

Something should state "it's platform-specific" then :)

Thank you!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Brian J. Johnson 

On 10/13/2015 08:26 AM, Laszlo Ersek wrote:


First of all, if the edk2 reference code (in the SMM core and in
PiSmmCpuDxeSmm) depends on such behavior justifiedly, then I think we
have a bug in the PI specification. Namely, version 1.4 thereof does not
seem to require that EFI_SMM_CONTROL2_PROTOCOL.Trigger() raise an SMI on
*all* processors. (See volume 4, section 5.4.)

In particular, the EFI_SMM_CONTROL2_PROTOCOL.Trigger() specification
makes many references to SMI handling and dispatching. But I cannot make
a mental connection between an SMI "broadcast", and "handling and
dispatching" in edk2, since all "handling and dispatching" code in edk2
that is exposed via protocols, is non-reentrant with regard to multiple
VCPUs, to my knowledge.

One could argue that whatever code handles the SMI on the BP is
responsible for bringing the APs into SMM as well, before doing any
sensitive work. I'm not sure.


Traditionally, SMI handling has been global.  If the h/w didn't 
broadcast the SMI to all CPUs, the SMI handler did so itself.  The BSP 
would wait for all APs to "check in" to SMM, then it would do whatever 
work the SMI required, and signal the APs to resume.  That ensured that 
the OS wasn't active on the machine while the BSP was handling the SMI, 
which was required for certain uses of SMI.


However, this (obviously) doesn't scale well, so Intel has been moving 
towards signaling SMI to only a single processor, and avoiding the 
machine-wide rendezvous when it isn't necessary.  BIOS implementations 
may be lagging behind.




Second, if writing to ioport 0xb2 should *automatically* raise an SMI on
all processors, then the QEMU code could be incorrect. However I could
never derive such an "imperative" from the ICH9 spec.


I too am having a hard time finding a clear statement of whether or not 
ioport 0xb2 should *automatically* raise an SMI on all processors. 
Maybe it's platform-specific?

--

Brian J. Johnson



  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Andrew Fish 

> On Oct 13, 2015, at 9:35 AM, Brian J. Johnson  wrote:
> 
> On 10/13/2015 08:26 AM, Laszlo Ersek wrote:
>> 
>> First of all, if the edk2 reference code (in the SMM core and in
>> PiSmmCpuDxeSmm) depends on such behavior justifiedly, then I think we
>> have a bug in the PI specification. Namely, version 1.4 thereof does not
>> seem to require that EFI_SMM_CONTROL2_PROTOCOL.Trigger() raise an SMI on
>> *all* processors. (See volume 4, section 5.4.)
>> 
>> In particular, the EFI_SMM_CONTROL2_PROTOCOL.Trigger() specification
>> makes many references to SMI handling and dispatching. But I cannot make
>> a mental connection between an SMI "broadcast", and "handling and
>> dispatching" in edk2, since all "handling and dispatching" code in edk2
>> that is exposed via protocols, is non-reentrant with regard to multiple
>> VCPUs, to my knowledge.
>> 
>> One could argue that whatever code handles the SMI on the BP is
>> responsible for bringing the APs into SMM as well, before doing any
>> sensitive work. I'm not sure.
> 
> Traditionally, SMI handling has been global.  If the h/w didn't broadcast the 
> SMI to all CPUs, the SMI handler did so itself.  The BSP would wait for all 
> APs to "check in" to SMM, then it would do whatever work the SMI required, 
> and signal the APs to resume.  That ensured that the OS wasn't active on the 
> machine while the BSP was handling the SMI, which was required for certain 
> uses of SMI.
> 
> However, this (obviously) doesn't scale well, so Intel has been moving 
> towards signaling SMI to only a single processor, and avoiding the 
> machine-wide rendezvous when it isn't necessary.

Which is kind of scary since it takes a single CPU away from the OS, and the 
SMM code and OS would be running at the same time. 

Thanks,

Andrew Fish

>  BIOS implementations may be lagging behind.
> 
>> 
>> Second, if writing to ioport 0xb2 should *automatically* raise an SMI on
>> all processors, then the QEMU code could be incorrect. However I could
>> never derive such an "imperative" from the ICH9 spec.
> 
> I too am having a hard time finding a clear statement of whether or not 
> ioport 0xb2 should *automatically* raise an SMI on all processors. Maybe it's 
> platform-specific?
> -- 
> 
>   Brian J. Johnson
> 
> 
> 
>  My statements are my own, are not authorized by SGI, and do not
>  necessarily represent SGI’s positions.
> ___
> 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

Re: [edk2] [PATCH v2 11/41] OvmfPkg: implement EFI_SMM_CONTROL2_PROTOCOL with a DXE_RUNTIME_DRIVER

2015-10-13 Thread Brian J. Johnson 

On 10/13/2015 11:49 AM, Laszlo Ersek wrote:

On 10/13/15 18:35, Brian J. Johnson wrote:

On 10/13/2015 08:26 AM, Laszlo Ersek wrote:


First of all, if the edk2 reference code (in the SMM core and in
PiSmmCpuDxeSmm) depends on such behavior justifiedly, then I think we
have a bug in the PI specification. Namely, version 1.4 thereof does not
seem to require that EFI_SMM_CONTROL2_PROTOCOL.Trigger() raise an SMI on
*all* processors. (See volume 4, section 5.4.)

In particular, the EFI_SMM_CONTROL2_PROTOCOL.Trigger() specification
makes many references to SMI handling and dispatching. But I cannot make
a mental connection between an SMI "broadcast", and "handling and
dispatching" in edk2, since all "handling and dispatching" code in edk2
that is exposed via protocols, is non-reentrant with regard to multiple
VCPUs, to my knowledge.

One could argue that whatever code handles the SMI on the BP is
responsible for bringing the APs into SMM as well, before doing any
sensitive work. I'm not sure.


Traditionally, SMI handling has been global.  If the h/w didn't
broadcast the SMI to all CPUs, the SMI handler did so itself.  The BSP
would wait for all APs to "check in" to SMM, then it would do whatever
work the SMI required, and signal the APs to resume.  That ensured that
the OS wasn't active on the machine while the BSP was handling the SMI,
which was required for certain uses of SMI.

However, this (obviously) doesn't scale well, so Intel has been moving
towards signaling SMI to only a single processor, and avoiding the
machine-wide rendezvous when it isn't necessary.  BIOS implementations
may be lagging behind.


But... when is it necessary? Paolo implied it might not be necessary for
us because MTRR changes are not relevant on our virtual platform --
otherwise all CPUs would have to agree on the MTRR settings --, but
isn't there a security aspect to it as well?


Sometimes the hardware requires it.  For instance, there have been 
situations where a platform needed a periodic SMI to allow the BIOS to 
kick a buggy I/O controller.  The "kick" was not atomic, and normal OS 
driver activity could interfere with it.  So BIOS used a system-wide SMI 
rendezvous to make sure that the OS wasn't running while it tweaked the 
hardware.


Security can be an aspect of it too.  For instance, modern platforms 
support write protecting the system flash, with a bypass for operations 
initiated from SMM.  If flash needs to be updated (eg. by a BIOS flash 
utility, or the EFI variable driver) the code doing the write uses a SMI 
to pass the request to a driver in SMM.  The SMM driver validates the 
request, unlocks the flash, and performs the update.  However, it would 
be a security hole if the OS was allowed to continue executing while the 
flash was unlocked.  So the SMM code triggers a global rendezvous to 
make sure no unknown code is running.


But there are plenty of situations where a global rendezvous isn't 
necessary.  For example, logging and clearing various non-fatal hardware 
errors (think corrected memory errors) can be done by a single thread 
while the rest of the system continues running.  Avoiding the rendezvous 
in those cases saves a *ton* of time in the SMI handler, reducing the 
system-wide performance impact.  As long as the SMI handler is quick 
enough, the OS doesn't mind losing a few cycles on one CPU.


(Or something like that.  SMM isn't my specialty, so I may be off on the 
details.  But that's the general idea.)

--

Brian J. Johnson



  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel