This is going to be another long email... Jump to bullet (6) if you only
care about my summary.

On 11/03/15 08:22, Jordan Justen wrote:
> On 2015-11-02 17:14:12, Laszlo Ersek wrote:

>> Anyway, with the following host kernel change, the AP startup problem
>> goes away (tested on top of v4.3):
>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 9a9a198..4f978ad 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -622,7 +622,8 @@ int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long 
>>> cr0)
>>>       if ((cr0 ^ old_cr0) & update_bits)
>>>               kvm_mmu_reset_context(vcpu);
>>>
>>> -     if ((cr0 ^ old_cr0) & X86_CR0_CD)
>>> +     if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED) &&
>>> +         (cr0 ^ old_cr0) & X86_CR0_CD)
>>>               kvm_zap_gfn_range(vcpu->kvm, 0, ~0ULL);
>>>
>>>       return 0;
>>
>> (Honestly I just imitated fb279950ba here; I'm not making any better
>> argument for this diff. But, independently, I wonder why this hunk
>> didn't have the noncoherent DMA check either, originally.)
>>
>> Janusz, could you rebuild your host kernel with this patch, and share
>> the results? (I'm also attaching the same as a formatted patch, so
>> you can apply it with "git am" easily.)
>
> One note related to decompression caching. I also created this patch:
>
> https://github.com/jljusten/edk2/commit/7a1d3799
>
> In my https://github.com/jljusten/edk2/tree/decompress-mtrr branch.
>
> Infernix tried it and it didn't seem to totally fix the issue. (See
> comments on https://github.com/tianocore/edk2/issues/21) I don't know
> if Janusz got a chance to try it.
>
> I didn't clean it up and send it out, since I didn't have any evidence
> that it helped...

(1) There is now evidence that the modifications that kernel commit
    b18d5431acc7

      KVM: x86: fix CR0.CD virtualization

    did to two separate functions:
    - vmx_get_mt_mask()
    - kvm_set_cr0()

    cause separate symptoms with OVMF. The vmx_get_mt_mask() change
    causes the slowdown with the LZMA decompression in SEC, and the
    kvm_set_cr0() change causes the delay with the AP startup in CpuDxe.

    Isolating the latter (i.e. the kvm_set_cr0() change's effect on the
    AP startup) was the topic of my previous email: with fixup
    fb279950ba

      KVM: vmx: obey KVM_QUIRK_CD_NW_CLEARED

    in the host kernel (which takes vmx_get_mt_mask() out of the
    picture), flipping the kvm_set_cr0() change on and off can be
    directly correlated with the AP startup problem.

    And now I checked the other symptom. I reverted the fb279950ba fixup

      KVM: vmx: obey KVM_QUIRK_CD_NW_CLEARED

    from vmx_get_mt_mask(), on top of v4.3, but I *also* applied my
    patch (see last email) entitled

      KVM: x86: obey KVM_X86_QUIRK_CD_NW_CLEARED in kvm_set_cr0().

    This makes the AP startup work again without problems, but *should*
    bring back the LZMA slowdown.

    And it does. (Tested it on my HP Z400 workstation, with an assigned
    GPU.)

(2) Using the awesome "ts" (timestamp) utility from the "moreutils"
    package, one can redirect the QEMU debug console to a *named pipe*
    (--> mkfifo), then read that pipe with:

    $ ts -i '%H:%M:%.S' /tmp/debugcon.pipe

    This will prefix each line with a relative / incremental timestamp,
    with sub-second resolution.

    (Of course you can write the debug console to /dev/stdout as well,
    and then just use the pipe operator "|" with "ts".)

    Now, this is what I get from an upstream OVMF build, with the
    kernel-side reproducer described in (1):

> 00:00:00.063287 SEC: Normal boot
> 00:01:33.306886 Register PPI Notify: DCD0BE23-9586-40F4-B643-06522CED4EDE

    The LZMA decompression takes about one and half minute.

(3) Using your patch "OvmfPkg/Sec: Program MTRRs before decompression"
    (with a fixup, see it later), I get:

> 00:00:00.013923 SEC: Normal boot
> 00:00:26.657586 Register PPI Notify: DCD0BE23-9586-40F4-B643-06522CED4EDE

    Better (approx 26 seconds), but still quite slow. In addition, the
    DXE core loading / startup takes conspicuously long as well:

> 00:00:00.001067 Loading DXE CORE at 0x000BFF60000 EntryPoint=0x000BFF60240
> 00:00:05.680312 Install PPI: 605EA650-C65C-42E1-BA80-91A52AB618C6

    More than five seconds.

(4) Whereas, if I only apply my host kernel patch

      KVM: x86: obey KVM_X86_QUIRK_CD_NW_CLEARED in kvm_set_cr0().

    on top of v4.3, and do *not* revert the fb279950ba fixup

      KVM: vmx: obey KVM_QUIRK_CD_NW_CLEARED

    then both the AP startup works, and the SEC decompression is fast.
    Measured without your OVMF patch:

> 00:00:00.000596 SEC: Normal boot
> 00:00:00.208118 Register PPI Notify: DCD0BE23-9586-40F4-B643-06522CED4EDE

  About one fifth of a second... And,

> 00:00:00.000216 Loading DXE CORE at 0x000BFF60000 EntryPoint=0x000BFF60240
> 00:00:00.015877 Install PPI: 605EA650-C65C-42E1-BA80-91A52AB618C6

  Which is unnoticeable.

(5) Review comments on your patch "OvmfPkg/Sec: Program MTRRs before
    decompression" follow.

    (Although I don't think this avenue should be pursued for now --
    I'll elaborate in the next bullet --, I also believe it's basic
    politeness to review a patch when asked to, assuming time allows for
    it.)

    The commit hash is different because I applied the patch to my tree
    (for testing it, see above) and then formatted it out of there.

> From 0fa30019127773d547f147bc987df6fd2f2714d5 Mon Sep 17 00:00:00 2001
> From: Jordan Justen <jordan.l.jus...@intel.com>
> Date: Tue, 20 Oct 2015 00:01:55 -0700
> Subject: [PATCH] OvmfPkg/Sec: Program MTRRs before decompression
>
> KVM will perform slowly in some cases if the MTRRs are not programmed
> correctly. Since the OVMF decompression uses memory extensively, we
> need to program the MTRRs before decompressing the PEI/DXE code.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
> ---
>  OvmfPkg/Sec/SecMain.inf         |  3 ++
>  OvmfPkg/PlatformPei/MemDetect.c |  6 ++--
>  OvmfPkg/Sec/SecMain.c           | 62 
> +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> index 2f78f3c..4c5392a 100644
> --- a/OvmfPkg/Sec/SecMain.inf
> +++ b/OvmfPkg/Sec/SecMain.inf
> @@ -56,11 +56,14 @@ [LibraryClasses]
>    PeCoffExtraActionLib
>    ExtractGuidedSectionLib
>    LocalApicLib
> +  MtrrLib
>
>  [Ppis]
>    gEfiTemporaryRamSupportPpiGuid                # PPI ALWAYS_PRODUCED
>
>  [Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 612bb4a..bdc16f5 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -291,9 +291,9 @@ QemuInitializeRam (
>      //
>      // MTRRs disabled, fixed MTRRs disabled, default type is uncached
>      //
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
> +    //ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
> +    //ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> +    //ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
>
>      //
>      // flip default type to writeback
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 4f87059..ee3761a 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -29,6 +29,7 @@
>  #include <Library/PeCoffExtraActionLib.h>
>  #include <Library/ExtractGuidedSectionLib.h>
>  #include <Library/LocalApicLib.h>
> +#include <Library/MtrrLib.h>
>
>  #include <Ppi/TemporaryRamSupport.h>
>
> @@ -687,6 +688,59 @@ FindAndReportEntryPoints (
>    return;
>  }
>
> +
> +/*
> +  Initialize the MTRRs for the SEC phase.
> +
> +  Most importantly this will enable caching for decompression of the FVs.
> +
> +**/
> +VOID
> +InitializeMtrrsForSec (
> +  VOID
> +  )
> +{
> +  MTRR_SETTINGS               Settings;
> +  MTRR_VARIABLE_SETTING       *Mtrr;
> +
> +  ZeroMem ((VOID *) &Settings, sizeof(Settings));

(i) Please drop the space character from within the cast.

> +
> +  Settings.MtrrDefType = MTRR_LIB_CACHE_MTRR_ENABLED |
> +    MTRR_LIB_CACHE_FIXED_MTRR_ENABLED | MTRR_CACHE_UNCACHEABLE;

Seems correct, PlatformPei has the same (BIT11 | BIT10 | 6).

> +
> +  //
> +  // Address 0 - 0xa0000: Enable Write Back Cache
> +  //
> +  SetMem (&Settings.Fixed.Mtrr[0], 2 * sizeof(Settings.Fixed.Mtrr[0]),
> +          MTRR_CACHE_WRITE_BACK);

Looks good, yes.

> +
> +  //
> +  // Set first 256MB as write-back cache
> +  //
> +  ASSERT ((PcdGet32 (PcdOvmfPeiMemFvBase) + PcdGet32 (PcdOvmfPeiMemFvSize)) <
> +          SIZE_256MB);
> +  ASSERT ((PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize)) <
> +          SIZE_256MB);

(ii) With my SMM patches in mind, this should probably cover up to
PcdOvmfDecompressionScratchEnd instead. No change in behavior though,
256MB is plenty.

> +  Mtrr = &Settings.Variables.Mtrr[0];
> +  Mtrr->Base = 0LL | MTRR_CACHE_WRITE_BACK;
> +  Mtrr->Mask = ((~(UINT64) (SIZE_256MB - 1) &
> +                 MTRR_LIB_CACHE_VALID_ADDRESS) |
> +                MTRR_LIB_CACHE_MTRR_ENABLED);

This took quite some time to review against the Intel SDM :), but it
looks correct.

(iii) Except, please, drop the space from within the cast.

> +
> +  //
> +  // Set ROM/Flash as write-protected cache
> +  //
> +  Mtrr = &Settings.Variables.Mtrr[0];

(iv) This is where I needed to fix up the patch: the subscript should be
one, not zero. (You probably copied the assignment from above, and
forgot to update it.)

> +  Mtrr->Base = PcdGet32 (PcdOvmfFdBaseAddress) | MTRR_CACHE_WRITE_PROTECTED;
> +  Mtrr->Mask =
> +    ((~(UINT64) (PcdGet32 (PcdOvmfFirmwareFdSize) - 1) &
> +      MTRR_LIB_CACHE_VALID_ADDRESS) |
> +     MTRR_LIB_CACHE_MTRR_ENABLED);

(v) Please drop the space from whitin the cast.

(vi) It should be asserted that PcdOvmfFirmwareFdSize is an integral
power of two.

(vii) It should be asserted that PcdOvmfFdBaseAddress has all bits clear
when masked with (PcdOvmfFirmwareFdSize - 1).

> +
> +  MtrrSetAllMtrrs (&Settings);
> +}
> +
> +
>  VOID
>  EFIAPI
>  SecCoreStartupWithStack (
> @@ -714,6 +768,14 @@ SecCoreStartupWithStack (
>    InitializeFloatingPointUnits ();
>
>    //
> +  // Initialize the MTRRs for the SEC phase. This will be redone more
> +  // thoroughly at a later stage.
> +  //
> +  if (IsMtrrSupported ()) {
> +    InitializeMtrrsForSec ();
> +  }
> +
> +  //
>    // Initialize IDT
>    //
>    IdtTableInStack.PeiService = NULL;
> --
> 1.8.3.1
>

(6) In closing:

    - The LZMA decompression slowdown has been quirked in the upstream
      kernel. A KVM ioctl() has been introduced that allows QEMU
      userspace to *disable* any quirks, but as far as I can see in
      QEMU, this is not being done automatically, nor is it exposed to
      the user. Therefore we can conclude that this issue has been
      solved for the mid term at least.

    - I have proved that the AP startup slowdown is a separate issue,
      one that is caused (or "exposed") by the "other" part of host
      kernel commit b18d5431acc7 -- the part that has lived on to v4.3
      un-quirked.

      I'll await Janusz's test results, and then I'll submit my proposed
      fix (= extending the quirk) to the KVM mailing list. With that we
      should consider this issue also resolved for OVMF, for the mid
      term.

    - Rather than auditing the MP services implementation in
      UefiCpuPkg/CpuDxe *right now*, can we *PLEASE* focus on the SMM
      series?

      I've been working on it (with some intermittent delays due to
      external pressure) since *April*. We are so close now. This is
      what remains to be done:

      - Agreement between Paolo, Jordan and Mike about implementing
        broadcast SMIs. I am willing to code up whatever design is
        agreed upon. Can everyone involved please prioritize this
        discussion a little?

      - Solving the MP-related ExitBootServices() handler bug in CpuDxe.
        Patches have been on the list for almost a week. While I've been
        breaking my back testing and reviewing patches for others (not
        just on edk2-devel, mind you), nobody has batted an eye about
        that series.

        [edk2] [PATCH v2 0/4] UefiCpuPkg/CpuDxe: Fix ExitBootServices()
                              callback in the presence of SMIs
        http://thread.gmane.org/gmane.comp.bios.edk2.devel/3518

        Please review it, so that I can include it at the front of my
        upcoming v4 SMM series.

      - Thirty (30) patches from the SMM series still need reviews. Once
        all of the above is covered, I'll update the OvmfPkg/README
        patch about the status, and post version 4. I wouldn't mind if
        we could commit the series still in 2015, but I'm getting less
        and less hopeful.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to