Re: [PATCH] KVM: x86: obey KVM_X86_QUIRK_CD_NW_CLEARED in kvm_set_cr0()

2015-11-03 Thread Janusz Mocek
W dniu 03.11.2015 o 19:57, Laszlo Ersek pisze:
> On 11/03/15 19:34, Laszlo Ersek wrote:
>> Commit b18d5431acc7 ("KVM: x86: fix CR0.CD virtualization") was
>> technically correct, but it broke OVMF guests by slowing down various
>> parts of the firmware.
>>
>> Commit fb279950ba02 ("KVM: vmx: obey KVM_QUIRK_CD_NW_CLEARED") quirked the
>> first function modified by b18d5431acc7, vmx_get_mt_mask(), for OVMF's
>> sake. This restored the speed of the OVMF code that runs before
>> PlatformPei (including the memory intensive LZMA decompression in SEC).
>>
>> This patch extends the quirk to the second function modified by
>> b18d5431acc7, kvm_set_cr0(). It eliminates the intrusive slowdown that
>> hits the EFI_MP_SERVICES_PROTOCOL implementation of edk2's
>> UefiCpuPkg/CpuDxe -- which is built into OVMF --, when CpuDxe starts up
>> all APs at once for initialization, in order to count them.
>>
>> We also carry over the kvm_arch_has_noncoherent_dma() sub-condition from
>> the other half of the original commit b18d5431acc7.
>>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Janusz Mocek <janusz...@gmail.com>
>> Cc: Alex Williamson <alex.william...@redhat.com>
>> Cc: Xiao Guangrong <guangrong.x...@linux.intel.com>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>  arch/x86/kvm/x86.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a24bae0..30723a4 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -625,7 +625,9 @@ 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 (((cr0 ^ old_cr0) & X86_CR0_CD) &&
>> +kvm_arch_has_noncoherent_dma(vcpu->kvm) &&
>> +!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_CD_NW_CLEARED))
>>  kvm_zap_gfn_range(vcpu->kvm, 0, ~0ULL);
>>  
>>  return 0;
>>
>
> I had notes on this patch, but I forgot to format it with --notes. They
> were:
>
> - People on the CC list, please reply with your Tested-by, Reported-by,
>   etc tags as appropriate; it's getting blurry who participated in what
>   and how.
>
> - This patch is *not* necessary for the OVMF SMM work; instead it
>   addresses an independent OVMF boot regression seen by users.
>
> Thanks
> Laszlo
Tested-by: Janusz Mocek <janusz...@gmail.com>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [edk2] KVM: MTRR: fix memory type handling if MTRR is completely disabled

2015-10-20 Thread Janusz Mocek
W dniu 20.10.2015 o 19:44, Laszlo Ersek pisze:
> Hi,
>
> On 10/20/15 19:27, Janusz wrote:
>> W dniu 15.10.2015 o 20:46, Laszlo Ersek pisze:
>>> On 10/15/15 18:53, Kinney, Michael D wrote:
 Laszlo,

 There is already a PCD for this timeout that is used by CpuMpPei.

gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds

 I noticed that CpuDxe is using a hard coded AP timeout.  I think we should 
 just use this same PCD for both the PEI and DXE CPU module and then set it 
 for OVMF to the compatible value.
>>> Perfect, thank you!
>>>
>>> (I notice the default in the DEC file is 5, which is half of what
>>> the DXE driver hardcodes.)
>>>
>>> Now we only need a recommended (or experimental) value for it, and an
>>> explanation why 100*1000 is no longer sufficient on KVM :)
>>>
>>> Thanks!
>>> Laszlo
>>>
>>>
>>>
>> Laszlo,
>>
>> I saw that there is already some change in ovmf for MicroSecondDelay
>> https://github.com/tianocore/edk2/commit/1e410eadd80c328e66868263b3006a274ce81ae0
>> Is that a fix for it? Because I tried it and it still doesn't work for
>> me: https://bpaste.net/show/2514b51bf41f
>> I still get internal error
> I think you guys are now "mature enough OVMF users" to start employing
> the correct terminology.

Sory for that :)

> "edk2" (also spelled as "EDK II") is: "a modern, feature-rich,
> cross-platform firmware development environment for the UEFI and PI
> specifications".
>
> The source tree contains a whole bunch of modules (drivers,
> applications, libraries), organized into packages.
>
> "OVMF" usually denotes a firmware binary built from one of the
> OvmfPkg/OvmfPkg*.dsc "platform description files". Think of them as "top
> level makefiles". The difference between them is the target architecture
> (there's Ia32, X64, and Ia32X64 -- the last one means that the SEC and
> PEI phases are 32-bit, whereas the DXE and later phases are 64-bit.) In
> practice you'll only care about full X64.
>
> Now, each of OvmfPkg/OvmfPkg*.dsc builds the following three kinds of
> modules into the final binary:
> - platform-independent modules from various top-level packages
> - platform- (ie. Ia32/X64-) dependent modules from various top-level
>   packages
> - modules from under OvmfPkg that are specific to QEMU/KVM (and Xen, if
>   you happen to use OVMF with Xen)
>
> Now, when you reference a commit like 1e410ead above, you can look at
> the diffstat, and decide if it is OvmfPkg-specific (third category
> above) or not. Here you see UefiCpuPkg, which happens to be the second
> category.
>
> The important point is: please do *not* call any and all edk2 patches
> "OVMF changes", indiscriminately. That's super confusing for people who
> understand the above distinctions. Which now you do too. :)
>
> Let me add that in edk2, patches that straddle top level packages are
> generally forbidden -- you can't have a patch that modifies OvmfPkg and
> UefiCpuPkg at the same time, modulo *very* rare exceptions. If a feature
> or bugfix needs to touch several top-level packages, the series must be
> built up carefully in stages.
>
> Knowing all of the above, you can tell that the patch you referenced had
> only *enabled* OvmfPkg to customize UefiCpuPkg, via
> "PcdCpuApInitTimeOutInMicroSeconds". But for that customization to occur
> actually, a small patch for OvmfPkg will be necessary too, in order to
> set "PcdCpuApInitTimeOutInMicroSeconds" differently from the default.
>
> I plan to send that patch soon. If you'd like to be CC'd, that's great
> (reporting back with a Tested-by is even better!), but I'll need your
> real name for that. (Or any name that looks like a real name.)
would be great if you could add me to cc list, thanks
>
> Thanks!
> Laszlo

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html