Re: [PATCH v2 0/3] Put AP into safe hlt-loop code on S3 path

[Laszlo Ersek <lersek@redhat.com>]

On 11/14/16 12:09, Paolo Bonzini wrote:
> 
> 
> On 14/11/2016 11:39, Laszlo Ersek wrote:
>> You've tried that:
>>
>> https://www.mail-archive.com/edk2-devel@lists.01.org/msg02840.html
>> https://www.mail-archive.com/edk2-devel@lists.01.org/msg02923.html
> 
> Uh, right. :)
> 
>> Do you suggest to make the LocalApicLib instances usable at runtime?
>> For that I think we'll need to cover the LAPIC address range with a
>> runtime-marked EfiMemoryMappedIO area. This can be done in
>> "OvmfPkg/SmmControl2Dxe".
>>
>> Also, we'll need a LocalApicLib instance that registers a callback for
>> SetVirtualAddressMap() and converts the LAPIC base address pointer.
>>
>> Currently BaseXApicX2ApicLib.c's GetLocalApicBaseAddress() function uses
>> the MSR_IA32_APIC_BASE register if it's available -- based on CPUID --,
>> and falls back to PcdCpuLocalApicBaseAddress otherwise. And only
>> PcdCpuLocalApicBaseAddress is what we could replace with the virtual
>> pointer. We can't accommodate a guest OS that reprograms the LAPIC base
>> address.
>>
>> Jeff, what do you think?
>>
>> Anyway, I believe KVM doesn't support moving the LAPIC window; is that
>> right?
> 
> No, it doesn't.  Let's add a backdoor in QEMU, where writing a given
> value to port 0xb2 would start generating SMIs to all CPUs.  Then you
> can write this somewhere in the initialization code, and in the S3 boot
> script.

Well...

http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05658.html
http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00125.html
http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00563.html

Are you suggesting that I resurrect this patch? That would be my pleasure. Please say yes.

Also, no special handling would be necessary on the S3 resume path, because after resume, SMM clients like the variable driver would continue calling the Trigger() method. SmmControl2Dxe is a Runtime DXE Driver. The OVMF side patch looks like this (I think I never posted it, but I preserved it):

> From 2d0cf255858c265d8b0af36ab586b0dd6a9140fa Mon Sep 17 00:00:00 2001
> From: Laszlo Ersek <lersek@redhat.com>
> Date: Fri, 23 Oct 2015 17:10:38 +0200
> Subject: [PATCH] (SQUASH) OvmfPkg: SmmControl2Dxe: select broadcast SMI
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h |  6 ++++--
>  OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c       | 12 +++++++++++-
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> index 18b34a3d4f4e..3fa26395adc7 100644
> --- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> +++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
> @@ -83,8 +83,10 @@
>  //
>  // IO ports
>  //
> -#define ICH9_APM_CNT 0xB2
> -#define ICH9_APM_STS 0xB3
> +#define ICH9_APM_CNT                    0xB2
> +
> +#define ICH9_APM_STS                    0xB3
> +#define QEMU_ICH9_APM_STS_BROADCAST_SMI   'Q'
>
>  //
>  // IO ports relative to PMBASE
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> index e895fd638d48..4f60ca48bdea 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> @@ -95,10 +95,20 @@ SmmControl2DxeTrigger (
>    // report about hardware status, while this register is fully governed by
>    // software.
>    //
> +  // On the QEMU platform, we use the status register to request a "broadcast"
> +  // SMI; i.e., to bring all VCPUs into SMM at once. Edk2 handles the case when
> +  // the SMI is raised only on the processor that calls Trigger(), but that
> +  // case has much worse performance.
> +  //
> +  // Note that we exploit the fact that the SMM_CORE never passes a non-NULL
> +  // DataPort pointer (that is, we exploit that it doesn't care about the
> +  // status register value).
> +  //
>    // 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);
> +  ASSERT (DataPort == NULL);
> +  IoWrite8 (ICH9_APM_STS, QEMU_ICH9_APM_STS_BROADCAST_SMI);
>    IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
>    return EFI_SUCCESS;
>  }
> --
> 2.9.2
>

(I guess there's a teeny-tiny window between the two IoWrite8() calls, but I don't think any guest OS would permit (or try) an S3 suspend while some firmware call is in flight.)

If you wish, I can apply these patches (both QEMU and OVMF) locally, and then retest Jeff's v2 and Jiewen's v3 patch sets.

Thank you!
Laszlo