Re: [edk2] [PATCH v3 00/52] OvmfPkg: support SMM for better security (steps towards MP and X64)

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 17:11, Paolo Bonzini wrote:
> 
> 
> On 21/10/2015 17:04, Laszlo Ersek wrote:
>> Now, on TCG, reading the APIC ID register (for device "apic") happens in:
>>
>> apic_mem_readl() [hw/intc/apic.c]
>>   val = s->id << 24
>>
>> Whereas on KVM, the same occurs in:
>>
>> kvm_apic_mem_read() [hw/i386/kvm/apic.c]
>>   return ~(uint64_t)0;
>>
>> However, such reads don't seem to reach QEMU (and the above read stub);
>> they are handled within KVM (I don't know where the distinction is made
>> in KVM).
> 
> It's here:
> 
> static u32 __apic_read(struct kvm_lapic *apic, unsigned int offset)
> {
> u32 val = 0;
> 
> if (offset >= LAPIC_MMIO_LENGTH)
> return 0;
> 
> switch (offset) {
> case APIC_ID:
> if (apic_x2apic_mode(apic))
> val = kvm_apic_id(apic);
> else
> val = kvm_apic_id(apic) << 24;
> break;

Sorry, I don't understand -- is this the distinction between normal and
x2apic mode, or the distinction whether the read access will be
forwarded to userspace vs. handled in KVM? My statement was about the
latter question.

> I'll try following the equally dizzying chains in OVMF

(not contesting that! :))

> and see why KVM
> is in x2apic mode.

I don't think KVM is in x2apic mode. I have no proof either way, but I
saw nothing implying x2apic. FWIW, LocalApicLib has two instances in edk2:

UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
("supports xAPIC mode only")

UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
("supports x2APIC capable processors which have xAPIC and x2APIC modes")

and all client modules in OVMF are linked against the former.

> In any case, "-cpu foo,-x2apic" is the next thing to
> try.

Tried it just now, no observable change.

... I gather it works on upstream or kvm/next host kernels; is that
correct? Maybe I should try a reverse bisection. *Shudder*.

Thanks!
Laszlo

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

Re: [edk2] [PATCH v3 00/52] OvmfPkg: support SMM for better security (steps towards MP and X64)

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 14:10, Laszlo Ersek wrote:
> On 10/15/15 00:25, Laszlo Ersek wrote:
> 
>> Test environment and results:
>>
>> Host kernel:
>> - latest RHEL-7 development kernel (3.10.0-323.el7), with Paolo's
>>   following patches backported by yours truly:
>>   - KVM: x86: clean up kvm_arch_vcpu_runnable
>>   - KVM: x86: fix SMI to halted VCPU
>>
>> QEMU:
>> - current upstream (c49d3411faae), with Paolo's patch applied:
>>   - target-i386: allow any alignment for SMBASE
>>
>> Below, the meaning of "bitness=32" is:
>> * qemu-system-i386
>> * -cpu coreduo,-nx
>>
>> Whereas "bitness=64" means:
>> * qemu-system-x86_64
>> * no special -cpu flag
>>
>> For variable access verification, "efibootmgr" is invoked (without
>> options) at the guest OS (Fedlet 20141209) root prompt.
>>
>>   bitness  accel  VCPUs  result
>>   ---  -  -  ---
>>   32   KVM1  Fedlet 20141209 boots, S3 works, variables work
>>
>>   32   KVM2  stuck in SMBASE relocation, APIC IDs look valid
> 
> Alright, so I've dug into this. It's very interesting.
> 
> First, here's the debug patch for edk2:
> 
> -
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 0e39173..bcfa075 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -442,11 +442,15 @@ SmmRelocateBases (
>for (Index = 0; Index < mNumberOfCpus; Index++) {
>  mRebased[Index] = FALSE;
>  if (ApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
> +  DEBUG ((EFI_D_VERBOSE, "%a: sending SMI IPI to APIC ID 0x%Lx\n",
> +__FUNCTION__, gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId));
>SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
>//
>// Wait for this AP to finish its 1st SMI
>//
>while (!mRebased[Index]);
> +  DEBUG ((EFI_D_VERBOSE, "%a: APIC ID 0x%Lx has processed its first 
> SMI\n",
> +__FUNCTION__, gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId));
>  } else {
>//
>// BSP will be Relocated later
> -
> 
> As one can expect, the first message appears in the log:
> 
> 
> SMRAM TileSize = 0800
> CPU[000]  APIC ID=  SMBASE=7FFC1000  SaveState=7FFD0C00  Size=0400
> CPU[001]  APIC ID=0001  SMBASE=7FFC1800  SaveState=7FFD1400  Size=0400
> SmmRelocateBases: sending SMI IPI to APIC ID 0x1
> 
> 
> but the second message doesn't; the (!mRebased[Index]) condition
> never evaluates to false, so the loop is never exited.
> 
> Second, I sought to analyze the KVM trace very carefully, against the
> SendSmiIpi() source code in edk2, and against the KVM source code.
> Here comes the kicker: KVM interprets the APIC ICR (high, low) writes
> correctly, injects the SMI, VCPU#1 wakes and enters SMM (!), then
> leaves SMM with a relocated SMBASE field (!!!).
> 
> *However*, according to the KVM trace, the relocated SMBASE field is
> *wrong* -- the value being reported below, 0x7ffc1000, corresponds to
> CPU#0 above!
> 
> 
>  qemu-system-i38-22085 [000] 13634.057590: kvm_enter_smm:vcpu 1: 
> leaving SMM, smbase 0x7ffc1000
> 
> 
> Then VCPU#1 goes on to do various things (I'm too lazy to analyze all
> those trace entries), but ultimately it reaches a HLT. And the busy
> wait in SmmRelocateBases() never completes, because vcpu #1 seems to
> have looked at VCPU#0's area.
> 
> Given that this works with TCG, I *guess* it is either a KVM bug, or
> some visibility race. I'll have to look at more.

I added more debug messages to edk2, and compared the logs generated
when running on KVM against those generated when running on TCG. The
point where things go sideways in edk2 is this:

The SmmInitHandler() function
[UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] has the following leading
comment:

"C function for SMI handler. To change all processor's SMMBase Register."

It is called from the assembly language entry point code, in System
Management Mode, for the purposes of initial SMBASE relocation.

In this function, the processor that executes the function identifies
itself by calling GetApicId(). The retrieved APIC ID is located within
the ProcessorInfo array (in order to map the APIC ID to a CPU index),
and then the rest of the relocation code uses this CPU index, for poking
into the SMBASE and SaveState "tiles".

Now, for VCPU#1, GetApicId() returns APIC ID 0x01 on TCG, and 0x00 on
KVM. Things go downhill from there.

The GetApicId() function
[UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c] does two (interesting)
things: it checks the initial APIC ID first (with a CPUID instruction),
then -- because the former is under 0x100 -- it does:

//
// If the initial local APIC ID is less 0x100, read APIC ID from
// XAPIC_ID_OFFSET, otherwise return the initial local APIC ID.
//
ApicId

Re: [edk2] [PATCH v3 00/52] OvmfPkg: support SMM for better security (steps towards MP and X64)

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 12:27, Paolo Bonzini wrote:
> 
> 
> On 21/10/2015 12:22, Laszlo Ersek wrote:
>> On 10/21/15 11:51, Paolo Bonzini wrote:
>>>
>>>
>>> On 20/10/2015 12:08, Laszlo Ersek wrote:
>>
>>   64   KVM>=1"KVM: entry failed, hardware error 0x8021"
>>  while guest in SMBASE relocation
 Tracing KVM shows the following:

  qemu-system-x86-3236  [001] 10586.857752: kvm_enter_smm:vcpu 1: 
 entering SMM, smbase 0x3
 ...
  qemu-system-x86-3236  [001] 10586.857863: kvm_enter_smm:vcpu 1: 
 leaving SMM, smbase 0x0
  qemu-system-x86-3236  [001] 10586.857865: kvm_entry:vcpu 1
  qemu-system-x86-3236  [001] 10586.857866: kvm_exit: reason 
 UNKNOWN rip 0x0 info 0 8306
  qemu-system-x86-3236  [001] 10586.857909: kvm_userspace_exit:   reason 
 KVM_EXIT_FAIL_ENTRY (9)
>>>
>>> This seems like you do not have this patch: 
>>> https://lkml.org/lkml/2015/10/14/462
>>
>> That's right, I don't have it; but in the guest I do have your
>>
>>   UefiCpuPkg: PiSmmCpuDxeSmm: do not execute RSM from 64-bit mode
>>   http://thread.gmane.org/gmane.comp.bios.edk2.devel/3020/focus=3023
>>
>> My understanding was that either of those two patches was sufficient.
> 
> No, this one fixes return _from_ 64-bit mode.  The kernel patch fixes
> return _to_ 64-bit mode and will be in 4.3.
> 
>> IIRC you asked me to keep the guest patch around in my branch for ease
>> of testing, until the above KVM patch is released with Linux 4.4. Is
>> that correct?
> 
> This will be the kernel fix for return _from_ 64-bit mode.

I can confirm that the patch you referenced makes the above failure go away. 
Meaning, with >= 2 VCPUs, the failure is replaced with the infinite loop that 
is also seen in the 32-bit case. (In other words, with this host patch in 
place, the 32-bit and 64-bit cases, KVM, >= 2 VCPU, behave identically.)

With 1 VCPU, "Fedora-Live-Xfce-x86_64-21-5.iso" boots, but then panics with:

[0.003008] ACPI: Core revision 20140724
[0.004328] ACPI: All ACPI Tables successfully acquired
[0.005418] PANIC: double fault, error_code: 0x0
[0.005895] CPU: 0 PID: 0 Comm: (null) Not tainted 3.17.4-301.fc21.x86_64 #1
[0.006000] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 
02/06/2015
[0.006000] task:   (null) ti: c028 task.ti:   
(null)
[0.006000] RIP: 0008:[<7ffc0955>]  [<7ffc0955>] 0x7ffc0955
[0.006000] RSP: 0020:7ffaadf0  EFLAGS: 00010002
[0.006000] RAX: 0800 RBX: 0033 RCX: c080
[0.006000] RDX:  RSI: 81c03d60 RDI: 7ffa3000
[0.006000] RBP: 81c03c90 R08:  R09: 
[0.006000] R10: 7febb010 R11: 0007 R12: 81c03ed0
[0.006000] R13: 0007 R14:  R15: 0009c000
[0.006000] FS:  (0020) GS:(0020) 
knlGS:
[0.006000] CS:  0010 DS: 0020 ES: 0020 CR0: 80050033
[0.006000] CR2: 7ffaade8 CR3: 0009c000 CR4: 06b0
[0.006000] Stack:

The RIP looks suspiciously close (interpreted as a phys address) to the SMRAM 
range, but that's just a wild guess.

Anyway, we got proof that I had been missing this host kernel patch.

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

Re: [edk2] [PATCH v3 00/52] OvmfPkg: support SMM for better security (steps towards MP and X64)

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 17:04, Laszlo Ersek wrote:
> On 10/21/15 14:10, Laszlo Ersek wrote:
>> On 10/15/15 00:25, Laszlo Ersek wrote:
>>
>>> Test environment and results:
>>>
>>> Host kernel:
>>> - latest RHEL-7 development kernel (3.10.0-323.el7), with Paolo's
>>>   following patches backported by yours truly:
>>>   - KVM: x86: clean up kvm_arch_vcpu_runnable
>>>   - KVM: x86: fix SMI to halted VCPU
>>>
>>> QEMU:
>>> - current upstream (c49d3411faae), with Paolo's patch applied:
>>>   - target-i386: allow any alignment for SMBASE
>>>
>>> Below, the meaning of "bitness=32" is:
>>> * qemu-system-i386
>>> * -cpu coreduo,-nx
>>>
>>> Whereas "bitness=64" means:
>>> * qemu-system-x86_64
>>> * no special -cpu flag
>>>
>>> For variable access verification, "efibootmgr" is invoked (without
>>> options) at the guest OS (Fedlet 20141209) root prompt.
>>>
>>>   bitness  accel  VCPUs  result
>>>   ---  -  -  ---
>>>   32   KVM1  Fedlet 20141209 boots, S3 works, variables work
>>>
>>>   32   KVM2  stuck in SMBASE relocation, APIC IDs look valid
>>
>> Alright, so I've dug into this. It's very interesting.
>>
>> First, here's the debug patch for edk2:
>>
>> -
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
>> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index 0e39173..bcfa075 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -442,11 +442,15 @@ SmmRelocateBases (
>>for (Index = 0; Index < mNumberOfCpus; Index++) {
>>  mRebased[Index] = FALSE;
>>  if (ApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) 
>> {
>> +  DEBUG ((EFI_D_VERBOSE, "%a: sending SMI IPI to APIC ID 0x%Lx\n",
>> +__FUNCTION__, gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId));
>>SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
>>//
>>// Wait for this AP to finish its 1st SMI
>>//
>>while (!mRebased[Index]);
>> +  DEBUG ((EFI_D_VERBOSE, "%a: APIC ID 0x%Lx has processed its first 
>> SMI\n",
>> +__FUNCTION__, gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId));
>>  } else {
>>//
>>// BSP will be Relocated later
>> -
>>
>> As one can expect, the first message appears in the log:
>>
>> 
>> SMRAM TileSize = 0800
>> CPU[000]  APIC ID=  SMBASE=7FFC1000  SaveState=7FFD0C00  Size=0400
>> CPU[001]  APIC ID=0001  SMBASE=7FFC1800  SaveState=7FFD1400  Size=0400
>> SmmRelocateBases: sending SMI IPI to APIC ID 0x1
>> 
>>
>> but the second message doesn't; the (!mRebased[Index]) condition
>> never evaluates to false, so the loop is never exited.
>>
>> Second, I sought to analyze the KVM trace very carefully, against the
>> SendSmiIpi() source code in edk2, and against the KVM source code.
>> Here comes the kicker: KVM interprets the APIC ICR (high, low) writes
>> correctly, injects the SMI, VCPU#1 wakes and enters SMM (!), then
>> leaves SMM with a relocated SMBASE field (!!!).
>>
>> *However*, according to the KVM trace, the relocated SMBASE field is
>> *wrong* -- the value being reported below, 0x7ffc1000, corresponds to
>> CPU#0 above!
>>
>> 
>>  qemu-system-i38-22085 [000] 13634.057590: kvm_enter_smm:vcpu 1: 
>> leaving SMM, smbase 0x7ffc1000
>> 
>>
>> Then VCPU#1 goes on to do various things (I'm too lazy to analyze all
>> those trace entries), but ultimately it reaches a HLT. And the busy
>> wait in SmmRelocateBases() never completes, because vcpu #1 seems to
>> have looked at VCPU#0's area.
>>
>> Given that this works with TCG, I *guess* it is either a KVM bug, or
>> some visibility race. I'll have to look at more.
> 
> I added more debug messages to edk2, and compared the logs generated
> when running on KVM against those generated when running on TCG. The
> point where things go sideways in edk2 is this:
> 
> The SmmInitHandler() function
> [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] has the following leading
> comment:
> 
> "C function for SMI handler. To change all processor's SMMBase Register."
> 
> It is called from the assembly language entry point code, in System
> Management Mode, for the purposes of initial SMBASE relocation.
> 
> In this function, the processor that executes the function identifies
> itself by calling GetApicId(). The retrieved APIC ID is located within
> the ProcessorInfo array (in order to map the APIC ID to a CPU index),
> and then the rest of the relocation code uses this CPU index, for poking
> into the SMBASE and SaveState "tiles".
> 
> Now, for VCPU#1, GetApicId() returns APIC ID 0x01 on TCG, and 0x00 on
> KVM. Things go downhill from there.

More strangeness: after replacing all GetApicId() calls with
GetInitialApicId() in PiSmmCpuDxeSmm, the SMBASE relocation succeeds for
(32-bit, KVM, 2 VCPUs), but (64-bit, KVM, 2 VCPUs) remains stuck in the
loop. %-/

Re: [edk2] style question/Omap3530Gpio

2015-10-21 Thread Andrew Fish 

> On Oct 21, 2015, at 8:31 AM, Laszlo Ersek  wrote:
> 
> On 10/21/15 17:13, Leif Lindholm wrote:
>> Hi Andrew, (list on cc)
>> 
>> Having a look at rewriting the ARM PL061Gpio driver since I find the
>> current version too complicated to review Haojian's patches against.
>> Now, it has a PL061Gpio.h in ArmPlatformPkg/Include/Drivers/ - but
>> since the driver is registered via the EmbeddedGpio interface, this
>> header file is only ever included from PL061Gpio.c.
>> Looking at the Omap3530Gpio driver, the same separation of driver and
>> its internal definitions happens there. 
>> 
>> So to my question - is this a recommended layout split or simply a
>> personal preference? (Since my default behaviour would be to stick
>> not-externally-accessed include files with the .c file.)
> 
> (Sorry about butting in.)
> 
> If the "PL061Gpio.h" file comes from a device data sheet, then it should go 
> under "ArmPlatformPkg/Include/IndustryStandard". (Or maybe 
> "ArmPkg/Include/IndustryStandard".) A few such directories exist already:
> 
> ArmPkg/Include/IndustryStandard
> MdePkg/Include/IndustryStandard
> OvmfPkg/Include/IndustryStandard
> Vlv2DeviceRefCodePkg/ValleyView2Soc/SouthCluster/Include/IndustryStandard
> 

I don’t really think of GPIOs as IndustryStandard/. In general IndustryStandard 
means a published spec that is hardware/vendor agnostic. So things like ACPI, 
PCI, SMBUS, are in the MdePkg IndustryStandard/. IMHO a data sheet for chip is 
not IndustryStandard/ .

The PL061 is an ARM PrimCell(TM) so that feels a little more like the 
PcAtChipsetPkg/, 
https://github.com/tianocore/edk2/tree/master/PcAtChipsetPkg/Include, there is 
an Include/Register/ but no IndustryStandard dir.

The generic answer is the package really owns defining the public include 
structure. The important point is being really clear on what is public and what 
is private. The EDK was really bad about the public library .h file, also 
containing things needed for the implementation of the library. 

Thanks,

Andrew Fish


> Thanks
> Laszlo
> 
>> 
>> Regards,
>> 
>> Leif
>> ___
>> 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] OvmfPkg: increase MP services startup timeout

2015-10-21 Thread Kinney, Michael D 
Laszlo,

We have the PCD that specifies the max CPUs.

  ## Specifies max supported number of Logical Processors.
  # @Prompt Configure max supported number of Logical Processors
  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64|UINT32|0x0002

We could exit the wait loop as soon as the number of detected CPUs matches 
PcdCpuMaxLogicalProcessorNumber.

Mike


>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, October 21, 2015 1:40 AM
>To: Justen, Jordan L
>Cc: Xiao Guangrong; Eduardo Habkost; edk2-de...@ml01.01.org; Alex
>Williamson; Kinney, Michael D
>Subject: Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout
>
>CC'ing Eduardo
>
>On 10/20/15 23:39, Jordan Justen wrote:
>> On 2015-10-20 12:27:42, Laszlo Ersek wrote:
>>> Due to Linux kernel commit b18d5431acc7 ("KVM: x86: fix CR0.CD
>>> virtualization"), vCPUs need more time to start up, because:
>>>
>>> - KVM now zaps all the mappings for the guest memory in EPT or shadow
>page
>>>   table, hence more VM exits are required to rebuild the mappings for all
>>>   memory accesses.
>>>
>>> - If a physical device has been assigned to the guest, and the IOMMU lacks
>>>   the snoop control feature, guest memory will become uncacheable after
>>>   CR0.CD is set to 1.
>>>
>>> UefiCpuPkg/UefiCpuPkg.dec sets the timeout to 50ms; startup failures with
>>> 100ms have been reported. Xiao Guangrong suggested 1s, and helped
>word the
>>> commit message.
>>
>> We will wait 1 second each boot for APs to start up? Meaning we will
>> basically add 1 second to the OVMF boot time?
>
>Yes, in StartApsStackless() that's the case.
>
>> I wonder if we can get the actual max from QEMU and use it to tell
>> CpuDxe how many processors to wait for. This would allow it to bail
>> out earlier if all processors startup faster.
>
>QEMU exports two fw_cfg keys (not files -- directly keys) that are
>related to VCPU counts:
>
>  QemuFwCfgItemSmpCpuCount = 0x0005,
>
>  QemuFwCfgItemMaximumCpuCount = 0x000f,
>
>(called FW_CFG_NB_CPUS and FW_CFG_MAX_CPUS in the QEMU source,
>respectively).
>
>FW_CFG_MAX_CPUS is not really good for this, because it carries the
>maximum possible APIC ID value, plus one, not the maximum number of
>CPUs. Please see the big comment in bochs_bios_init() in QEMU
>[hw/i386/pc.c].
>
>FW_CFG_NB_CPUS exposes the "smp_cpus" variable of QEMU, which is the
>count of VCPUs present at QEMU startup (more can be hotplugged later).
>See the -smp option:
>
>  -smp [cpus=]n[,cores=cores][,threads=threads][,sockets=sockets]
>   [,maxcpus=maxcpus]
>
>"smp_cpus" corresponds to "n" in the above (where "cpus" is enforced to
>equal cores * threads * sockets ==> smp_parse()), whereas "maxcpus"
>means "maximum number of CPUs, including future hotplug" (variable
>"max_cpus", which is not exposed via fw_cfg at all).
>
>In addition, (smp_cpus - 1) is also visible in the CMOS, at offset 0x5f.
>
>Confused yet? Good; it is very confusing. Here's a summary:
>
>  quantity |QEMU command line |QEMU variable or  |guest
>interface
>   |  |function  |
>  
> -|--|--|---
>  initially|-smp cpus=|smp_cpus  
> |FW_CFG_NB_CPUS
>  plugged  |and/or|  |and
>  CPUs |-smp cores=,threads=,sockets= |  
> |cmos[0x5f] + 1
>  
> -+--+--+---
>  maximum  |-smp maxcpus= |max_cpus  |n/a
>  possible |  |  |
>  CPUs |  |  |
>  
> -+--+--+---
>  maximum  |-smp
>cores=,threads=,sockets=,maxcpus=|pc_apic_id_limit()|FW_CFG_MAX_CPUS
>  APIC ID  |(i.e., derived from topology and  |  |-1
>   |maximum possible count)   |  |
>
>SeaBIOS too counts the CPUs at startup (see smp_setup() in
>"src/fw/smp.c"). The quantity that it uses to terminate the loop is
>(cmos[0x5f] + 1) -- i.e., number of initially plugged CPUs.
>
>If we want to do something similar, then UefiCpuPkg/CpuDxe needs to
>accept and consider such a limit (from a dynamic PCD), and:
>
>(a) in OvmfPkg/PlatformPei, we have to set that PCD from (cmos[0x5f]+1)
>or (equivalently) QemuFwCfgItemSmpCpuCount, or
>(b) set the PCD (from one of the same sources) in a NULL library that we
>hook into CpuDxe in our platform DSC files.
>
>If you can write a patch for this, I'll thank you. :)
>
>Thanks,
>Laszlo
>
>>
>> -Jordan
>>
>>> Cc: Xiao Guangrong 
>>> Cc: Jordan Justen 
>>> Cc: Janusz Mocek 
>>> Cc:

Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

2015-10-21 Thread Jordan Justen 
On 2015-10-21 14:19:40, Kinney, Michael D wrote:
> Laszlo,
> 
> We have the PCD that specifies the max CPUs.
> 
>   ## Specifies max supported number of Logical Processors.
>   # @Prompt Configure max supported number of Logical Processors
>   
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64|UINT32|0x0002

It would need to be changed to a dynamic PCD.

In terms of determining the number of logical processors, can't the
cpuid function help? I'm not sure how QEMU handles that though.
(Vaguely I remember something like cpuid only help with a single
core... So, if QEMU presents as multiple cores, then maybe it can't
help.)

We'd need a reliable way to set the PCD before CpuDxe runs.

-Jordan

> We could exit the wait loop as soon as the number of detected CPUs
> matches PcdCpuMaxLogicalProcessorNumber.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [MdeModulePkg] MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf will not compile with Xcode 6.3.2

2015-10-21 Thread Andrew Fish 
"/usr/bin/clang" -target x86_64-pc-win32-macho -c -g -Os -Wall -Werror -Wextra 
-include AutoGen.h -funsigned-char -fno-ms-extensions -fno-stack-protector 
-fno-builtin -fshort-wchar -mno-implicit-float -mms-bitfields 
-Wno-unused-parameter -Wno-missing-braces -Wno-missing-field-initializers 
-Wno-tautological-compare -Wno-sign-compare 
-ftrap-function=undefined_behavior_has_been_optimized_away_by_clang  -o 
/Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/OUTPUT/./Variable.obj
 -I/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe 
-I/Users/andrewfish/work/src/edk2/Build/MdeModule/DEBUG_XCODE5/X64/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe/DEBUG
 -I/Users/andrewfish/work/src/edk2/MdePkg 
-I/Users/andrewfish/work/src/edk2/MdePkg/Include 
-I/Users/andrewfish/work/src/edk2/MdePkg/Include/X64 
-I/Users/andrewfish/work/src/edk2/MdeModulePkg 
-I/Users/andrewfish/work/src/edk2/MdeModu
 lePkg/Include 
/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
/Users/andrewfish/work/src/edk2/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c:1713:8:
 error: array type 'VA_LIST' (aka '__builtin_va_list') is not assignable
  Args = Marker;
   ^

it is NOT portable C code to use = with markers, you need to use VA_COPY() 
defined in Base.h. 

This fixes the compiler warning with Xcode.

~/work/src/edk2(master)>git diff 
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 31e1937..ef3dab3 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -1710,7 +1710,7 @@ CheckRemainingSpaceForConsistencyInternal (
   ASSERT_EFI_ERROR (Status);
 
   TotalNeededSize = 0;
-  Args = Marker;
+  VA_COPY (Args, Marker);
   VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
   while (VariableEntry != NULL) {
 //
@@ -1739,7 +1739,7 @@ CheckRemainingSpaceForConsistencyInternal (
 return FALSE;
   }
 
-  Args = Marker;
+  VA_COPY (Args, Marker);
   VariableEntry = VA_ARG (Args, VARIABLE_ENTRY_CONSISTENCY *);
   while (VariableEntry != NULL) {
 //


Contributed-under: TianoCore Contribution Agreement 1.0
Reviewed-by: Andrew Fish 

Thanks,

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

Re: [edk2] [PATCH] UefiCpuPkg: PiSmmCpuDxeSmm: Remove unused references to SmmLib

2015-10-21 Thread Yao, Jiewen 
Looks good. Reviewed by jiewen@inter.com


-Original Message-
From: Kinney, Michael D 
Sent: Thursday, October 22, 2015 7:26 AM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen; Fan, Jeff
Subject: [PATCH] UefiCpuPkg: PiSmmCpuDxeSmm: Remove unused references to SmmLib

The PiSmmCpuDxeSmm module does not use any services from the SmmLib.
This change removes the SmmLib from PiSmmCpuDxeSmm module and also removes the 
lib mapping in the UefiCpuPkg DSC file because no other modules in the 
UefiCpuPkg use the SmmLib.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney 
CC: Yao, Jiewen 
CC: Jeff Fan 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 1 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 -
 UefiCpuPkg/UefiCpuPkg.dsc| 1 -
 3 files changed, 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 9ea1189..162bdad 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -30,7 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
-#include 
 #include   #include   
#include  diff --git 
a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 45ab16c..f559947 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -101,7 +101,6 @@
   SynchronizationLib
   BaseMemoryLib
   MtrrLib
-  SmmLib
   IoLib
   TimerLib
   SmmServicesTableLib
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc index 
10197d4..756645f 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -55,7 +55,6 @@
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
   
CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
-  SmmLib|MdePkg/Library/SmmLibNull/SmmLibNull.inf
   PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
   
SmmCpuPlatformHookLib|UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf
--
1.9.5.msysgit.1

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

[edk2] [PATCH] UefiCpuPkg: PiSmmCpuDxeSmm: Remove unused references to SmmLib

2015-10-21 Thread Michael Kinney 
The PiSmmCpuDxeSmm module does not use any services from the SmmLib.
This change removes the SmmLib from PiSmmCpuDxeSmm module and also
removes the lib mapping in the UefiCpuPkg DSC file because no other
modules in the UefiCpuPkg use the SmmLib.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Michael Kinney 
CC: Yao, Jiewen 
CC: Jeff Fan 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   | 1 -
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 -
 UefiCpuPkg/UefiCpuPkg.dsc| 1 -
 3 files changed, 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 9ea1189..162bdad 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -30,7 +30,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 45ab16c..f559947 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -101,7 +101,6 @@
   SynchronizationLib
   BaseMemoryLib
   MtrrLib
-  SmmLib
   IoLib
   TimerLib
   SmmServicesTableLib
diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 10197d4..756645f 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -55,7 +55,6 @@
   
SynchronizationLib|MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
   SmmMemLib|MdePkg/Library/SmmMemLib/SmmMemLib.inf
   
CacheMaintenanceLib|MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
-  SmmLib|MdePkg/Library/SmmLibNull/SmmLibNull.inf
   PciLib|MdePkg/Library/BasePciLibPciExpress/BasePciLibPciExpress.inf
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
   
SmmCpuPlatformHookLib|UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf
-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

2015-10-21 Thread Fan, Jeff 
Jordan,

CPU ID only could get the max logical processor number supported in current 
socket. It cannot handle multi-sockets case.

Yes. We could promote PcdCpuMaxLogicalProcessorNumber to dynamic type. But we 
need find one reliable way like you said to set it before CPU MP PEI not only 
CPU MP DXE.

Thanks!
Jeff

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
Justen
Sent: Thursday, October 22, 2015 5:42 AM
To: Kinney, Michael D; Laszlo Ersek; Kinney, Michael D
Cc: Alex Williamson; edk2-de...@ml01.01.org; Xiao Guangrong; Eduardo Habkost
Subject: Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

On 2015-10-21 14:19:40, Kinney, Michael D wrote:
> Laszlo,
> 
> We have the PCD that specifies the max CPUs.
> 
>   ## Specifies max supported number of Logical Processors.
>   # @Prompt Configure max supported number of Logical Processors
>   
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64|UINT32|0x
> 0002

It would need to be changed to a dynamic PCD.

In terms of determining the number of logical processors, can't the cpuid 
function help? I'm not sure how QEMU handles that though.
(Vaguely I remember something like cpuid only help with a single core... So, if 
QEMU presents as multiple cores, then maybe it can't
help.)

We'd need a reliable way to set the PCD before CpuDxe runs.

-Jordan

> We could exit the wait loop as soon as the number of detected CPUs 
> matches PcdCpuMaxLogicalProcessorNumber.
___
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 v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when only 1 CPU detected

2015-10-21 Thread Laszlo Ersek 
Adding Jordan.

On 10/21/15 03:14, Kinney, Michael D wrote:
> Bruce,
> 
> No.  Different ASSERT().
> 
> That looks like a new bug, and it is in the new code I added to force BSP to 
> wait for all APs to enter sleeping state before StartupAllAPs() is called.
> 
> How did you reproduce this?

Actually, I'm seeing a related / similar assertion failure, from here:

  //
  // Wait for all APs to enter idle loop.
  //
  Timeout = 0;
  do {
if (CheckAllAPsSleeping ()) {
  break;
}
gBS->Stall (gPollInterval);
Timeout += gPollInterval;
  } while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
  ASSERT (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));

I'm not doing anything special, just booting a preexistent guest on KVM.

The default timeout from UefiCpuPkg is 50msec, which is apparently too
short here. I have not discovered this in practice earlier because:
- I've been focusing on TCG
- my patch

  [PATCH] OvmfPkg: increase MP services startup timeou
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/3260

  has been part of my builds recently.

So, I think we might have to apply said patch to OvmfPkg quickly, but
not strictly in response to the bug report / analysis from Janusz Mocek
& Xiao Guangrong: instead, to fix up this regression.

(See also
,
where I pointed out that the UefiCpuPkg change actually halves the
preexistent timeout.)

Then, any smarts discussed under

should be implemented on top.

Jordan, do you agree we can / should preliminarily apply my patch?
As-is, OVMF might not boot on KVM at all at the moment, due to these
timeouts.

Thanks
Laszlo

> 
> Thanks,
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Bruce Cran
>> Sent: Tuesday, October 20, 2015 5:21 PM
>> To: Laszlo Ersek; edk2-de...@ml01.01.org
>> Subject: Re: [edk2] [PATCH v3 01/52] UefiCpuPkg: CpuDxe: Fix ASSERT() when
>> only 1 CPU detected
>>
>> On 10/14/15 4:25 PM, Laszlo Ersek wrote:
>>> From: Michael Kinney 
>>>
>>> When only 1 CPU is detected and the max CPUs is greater than 1,
>>> an ASSERT() is generated because the pages associated with the
>>> AP stack have already been freed.  Only do this FreePages() call
>>> if there is more than 1 CPU detected.
>>
>> Is the ASSERT() that was being triggered "LockValue == ((UINTN) 2) ||
>> LockValue == ((UINTN) 1)" in:
>>
>>   AcquireSpinLockOrFail
>>   GetMpSpinLock
>>   TestCpuStatusFlag
>>   CheckAllAPsSleeping
>>   InitializeMpSupport
>>   InitializeCpu
>>   ...
>>
>> Or is this a separate bug?
>>
>> --
>> Bruce
>> ___
>> 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

[edk2] [PATCH] MdeModulePkg Variable: Enhance variable performance by reading from existed memory cache.

2015-10-21 Thread Derek Lin 
Current variable driver already have memory cache to improve performance.
Change the code which read from physical MMIO address to read from memory cache.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Derek Lin 
---
 .../Universal/Variable/RuntimeDxe/Variable.c   | 49 +-
 .../Universal/Variable/RuntimeDxe/VariableDxe.c| 27 ++--
 2 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 31e1937..aac3942 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -17,6 +17,7 @@
   integer overflow. It should also check attribute to avoid authentication 
bypass.
 
 Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+(C) Copyright 2015 Hewlett Packard Enterprise Development LP
 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
@@ -37,6 +38,11 @@ VARIABLE_MODULE_GLOBAL  *mVariableModuleGlobal;
 VARIABLE_STORE_HEADER  *mNvVariableCache  = NULL;
 
 ///
+/// Memory cache of Fv Header.
+///
+EFI_FIRMWARE_VOLUME_HEADER *mNvFvHeaderCache= NULL;
+
+///
 /// The memory entry used for variable statistics data.
 ///
 VARIABLE_INFO_ENTRY*gVariableInfo = NULL;
@@ -333,7 +339,7 @@ UpdateVariableStore (
 return EFI_INVALID_PARAMETER;
   }
 
-  for (PtrBlockMapEntry = FwVolHeader->BlockMap; PtrBlockMapEntry->NumBlocks 
!= 0; PtrBlockMapEntry++) {
+  for (PtrBlockMapEntry = mNvFvHeaderCache->BlockMap; 
PtrBlockMapEntry->NumBlocks != 0; PtrBlockMapEntry++) {
 for (BlockIndex2 = 0; BlockIndex2 < PtrBlockMapEntry->NumBlocks; 
BlockIndex2++) {
   //
   // Check to see if the Variable Writes are spanning through multiple
@@ -2209,7 +2215,7 @@ UpdateVariable (
 VariableStoreHeader  = (VARIABLE_STORE_HEADER *) ((UINTN) 
mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase);
 Variable = 
 Variable->StartPtr = GetStartPointer (VariableStoreHeader);
-Variable->EndPtr   = GetEndPointer (VariableStoreHeader);
+Variable->EndPtr   = (VARIABLE_HEADER *) ((UINT8 *) VariableStoreHeader + 
((UINTN) GetEndPointer (mNvVariableCache) - (UINTN) mNvVariableCache));
 Variable->CurrPtr  = (VARIABLE_HEADER *)((UINTN)Variable->StartPtr + 
((UINTN)CacheVariable->CurrPtr - (UINTN)CacheVariable->StartPtr));
 if (CacheVariable->InDeletedTransitionPtr != NULL) {
   Variable->InDeletedTransitionPtr = (VARIABLE_HEADER 
*)((UINTN)Variable->StartPtr + ((UINTN)CacheVariable->InDeletedTransitionPtr - 
(UINTN)CacheVariable->StartPtr));
@@ -2247,7 +2253,7 @@ UpdateVariable (
   //
   // Only variable that have NV attributes can be updated/deleted in 
Runtime.
   //
-  if ((Variable->CurrPtr->Attributes & EFI_VARIABLE_NON_VOLATILE) == 0) {
+  if ((CacheVariable->CurrPtr->Attributes & EFI_VARIABLE_NON_VOLATILE) == 
0) {
 Status = EFI_INVALID_PARAMETER;
 goto Done;
   }
@@ -2255,7 +2261,7 @@ UpdateVariable (
   //
   // Only variable that have RT attributes can be updated/deleted in 
Runtime.
   //
-  if ((Variable->CurrPtr->Attributes & EFI_VARIABLE_RUNTIME_ACCESS) == 0) {
+  if ((CacheVariable->CurrPtr->Attributes & EFI_VARIABLE_RUNTIME_ACCESS) 
== 0) {
 Status = EFI_INVALID_PARAMETER;
 goto Done;
   }
@@ -2273,7 +2279,7 @@ UpdateVariable (
 // Both ADDED and IN_DELETED_TRANSITION variable are present,
 // set IN_DELETED_TRANSITION one to DELETED state first.
 //
-State = Variable->InDeletedTransitionPtr->State;
+State = CacheVariable->InDeletedTransitionPtr->State;
 State &= VAR_DELETED;
 Status = UpdateVariableStore (
>VariableGlobal,
@@ -2294,7 +2300,7 @@ UpdateVariable (
 }
   }
 
-  State = Variable->CurrPtr->State;
+  State = CacheVariable->CurrPtr->State;
   State &= VAR_DELETED;
 
   Status = UpdateVariableStore (
@@ -2319,8 +2325,8 @@ UpdateVariable (
 // If the variable is marked valid, and the same data has been passed in,
 // then return to the caller immediately.
 //
-if (DataSizeOfVariable (Variable->CurrPtr) == DataSize &&
-(CompareMem (Data, GetVariableDataPtr (Variable->CurrPtr), DataSize) 
== 0) &&
+if (DataSizeOfVariable (CacheVariable->CurrPtr) == DataSize &&
+(CompareMem (Data, GetVariableDataPtr (CacheVariable->CurrPtr), 
DataSize) == 0) &&
 ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0) &&
 (TimeStamp == NULL)) {
   //
@@ -2329,8 +2335,8 @@ UpdateVariable (
   UpdateVariableInfo (VariableName, VendorGuid, Variable->Volatile, FALSE, 
TRUE, FALSE, FALSE);
   Status = EFI_SUCCESS;

Re: [edk2] Porting UEFI to a ARM platform

2015-10-21 Thread Michael Zimmermann 
I can confirm that the tutorial on the tianocore wiki works just fine
because I've just finished a port using that(it's a ARM-2ndstage platform
though).

On Wed, Oct 21, 2015 at 9:43 AM, Laszlo Ersek  wrote:

> On 10/21/15 04:44, Yehuda Yitschak wrote:
> > Hello everyone
> >
> >
> > i started reading and experimenting with UEFI a month ago and i'm
> > now considering to start porting it to one of Marvell's Aarch64 SOCs
> >
> > as a first stage i only want to get a basic shell running and start
> > adding drivers with time.
> >
> >
> > i'm wondering if there is any documentation or guide available to help
> > with the process
> >
> >
> > my initial thoughts are to clone to Juno package and start changing the
> > DSC and other configuration files but i guess there is a lot of details
> > into this.
> >
> >
> > Any help or pointer is greatly appreciated
>
> I've never done a full port myself, but I remember ArmPlatformPkg wiki
> articles. Hm... new wiki:
>
> https://github.com/tianocore/tianocore.github.io/wiki/ArmPlatformPkg
>
> old wiki (with pics):
>
> http://tianocore.sourceforge.net/wiki/ArmPlatformPkg
>
> Maybe this helps.
>
> Laszlo
>
>
> >
> >
> > Thanks in advance
> >
> >
> > Yehuda
> >
>
> ___
> 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] Porting UEFI to a ARM platform

2015-10-21 Thread Leif Lindholm 
Hi Yehuda,

On Wed, Oct 21, 2015 at 02:44:19AM +, Yehuda Yitschak wrote:
> i started reading and experimenting with UEFI a month ago and i'm
> now considering to start porting it to one of Marvell's Aarch64 SOCs
> 
> as a first stage i only want to get a basic shell running and start
> adding drivers with time.
> 
> i'm wondering if there is any documentation or guide available to
> help with the process
> 
> my initial thoughts are to clone to Juno package and start changing
> the DSC and other configuration files but i guess there is a lot of
> details into this.
> 
> Any help or pointer is greatly appreciated

The link Laszlo posted is a good overview, but it's a bit dated - so
shuoldn't be taken verbatim.

Also, to Haojian's point - I won't be adding any more ARM platforms to
EDK2 until the platform situation has been resolved. Until then, I
maintain OpenPlatformPkg separately to prototype my preferred layout.

Main things from my perspective:
- Unless you're explicitly after a line-based user interface, and are
  willing to effectively take up responsibility for maintaining it,
  don't use the ArmBds.
- Base your port on upstream EDK2 and OpenPlatformPkg - do *not* base
  it on linaro-edk2.
- The PcdSystemMemoryBase/PcdSystemMemorySize stuff is a pure hack,
  but in a platform with a fixed-at-build amount of RAM it's a simple
  way to describe your available memory.
  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibCTA15-A7/CTA15-A7Mem.c
  provides an example for how to deal with multiple regions, although
  it's still a bit overly simplistic.
- Don't forget to describe your unavailable regions of memory
  (reserved for EL3 or otherwise).

This is definitely an area in which more public HOWTOs would be
useful.

/
Leif

p.s.
If you can stand it (I can't), here's a recent presentation by me
ranting about things wrong with many existing ARM UEFI platform ports:
https://sfo15.pathable.com/meetings/302843
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 00/52] OvmfPkg: support SMM for better security (steps towards MP and X64)

2015-10-21 Thread Paolo Bonzini 


On 21/10/2015 12:22, Laszlo Ersek wrote:
> On 10/21/15 11:51, Paolo Bonzini wrote:
>>
>>
>> On 20/10/2015 12:08, Laszlo Ersek wrote:
>
>   64   KVM>=1"KVM: entry failed, hardware error 0x8021"
>  while guest in SMBASE relocation
>>> Tracing KVM shows the following:
>>>
>>>  qemu-system-x86-3236  [001] 10586.857752: kvm_enter_smm:vcpu 1: 
>>> entering SMM, smbase 0x3
>>> ...
>>>  qemu-system-x86-3236  [001] 10586.857863: kvm_enter_smm:vcpu 1: 
>>> leaving SMM, smbase 0x0
>>>  qemu-system-x86-3236  [001] 10586.857865: kvm_entry:vcpu 1
>>>  qemu-system-x86-3236  [001] 10586.857866: kvm_exit: reason 
>>> UNKNOWN rip 0x0 info 0 8306
>>>  qemu-system-x86-3236  [001] 10586.857909: kvm_userspace_exit:   reason 
>>> KVM_EXIT_FAIL_ENTRY (9)
>>
>> This seems like you do not have this patch: 
>> https://lkml.org/lkml/2015/10/14/462
> 
> That's right, I don't have it; but in the guest I do have your
> 
>   UefiCpuPkg: PiSmmCpuDxeSmm: do not execute RSM from 64-bit mode
>   http://thread.gmane.org/gmane.comp.bios.edk2.devel/3020/focus=3023
> 
> My understanding was that either of those two patches was sufficient.

No, this one fixes return _from_ 64-bit mode.  The kernel patch fixes
return _to_ 64-bit mode and will be in 4.3.

> IIRC you asked me to keep the guest patch around in my branch for ease
> of testing, until the above KVM patch is released with Linux 4.4. Is
> that correct?

This will be the kernel fix for return _from_ 64-bit mode.

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

Re: [edk2] [PATCH v2] OvmfPkg: XenPvBlkDxe: handle empty cdrom drives

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 13:39, Stefano Stabellini wrote:
> Empty cdroms are not going to connect, avoid waiting for the backend to
> switch to state 4, which is never going to happen, and return
> error instead from XenPvBlockFrontInitialization(). Detect an
> empty cdrom by looking at the "params" node on xenstore, which is set to
> "" or "aio:" for empty drives by libxl.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Stefano Stabellini 
> 
> ---
> Changes in v2:
> - better commit message
> - return EFI_DEVICE_ERROR instead of EFI_NO_MEDIA
> - check the return status of XenBusIo->XsBackendRead
> ---
>  OvmfPkg/XenPvBlkDxe/BlockFront.c |   15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c 
> b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> index 256ac55..d07e980 100644
> --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> @@ -169,6 +169,7 @@ XenPvBlockFrontInitialization (
>XEN_BLOCK_FRONT_DEVICE *Dev;
>XenbusState State;
>UINT64 Value;
> +  CHAR8 *Params;
>  
>ASSERT (NodeName != NULL);
>  
> @@ -186,6 +187,20 @@ XenPvBlockFrontInitialization (
>}
>FreePool (DeviceType);
>  
> +  if (Dev->MediaInfo.CdRom) {
> +Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", 
> (VOID**));
> +if (Status != XENSTORE_STATUS_SUCCESS) {
> +  DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, 
> Status));
> +  goto Error;
> +}
> +if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
> +  FreePool (Params);
> +  DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
> +  goto Error;
> +}
> +FreePool (Params);
> +  }
> +
>Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, );
>if (Status != XENSTORE_STATUS_SUCCESS || Value > MAX_UINT16) {
>  DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to get backend-id (%d)\n",
> 

Reviewed-by: Laszlo Ersek 

I test-built this for X64 and Ia32, and then committed it to SVN as rev
18651.

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

[edk2] [PATCH v2] OvmfPkg: XenPvBlkDxe: handle empty cdrom drives

2015-10-21 Thread Stefano Stabellini 
Empty cdroms are not going to connect, avoid waiting for the backend to
switch to state 4, which is never going to happen, and return
error instead from XenPvBlockFrontInitialization(). Detect an
empty cdrom by looking at the "params" node on xenstore, which is set to
"" or "aio:" for empty drives by libxl.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Stefano Stabellini 

---
Changes in v2:
- better commit message
- return EFI_DEVICE_ERROR instead of EFI_NO_MEDIA
- check the return status of XenBusIo->XsBackendRead
---
 OvmfPkg/XenPvBlkDxe/BlockFront.c |   15 +++
 1 file changed, 15 insertions(+)

diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
index 256ac55..d07e980 100644
--- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
+++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
@@ -169,6 +169,7 @@ XenPvBlockFrontInitialization (
   XEN_BLOCK_FRONT_DEVICE *Dev;
   XenbusState State;
   UINT64 Value;
+  CHAR8 *Params;
 
   ASSERT (NodeName != NULL);
 
@@ -186,6 +187,20 @@ XenPvBlockFrontInitialization (
   }
   FreePool (DeviceType);
 
+  if (Dev->MediaInfo.CdRom) {
+Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", 
(VOID**));
+if (Status != XENSTORE_STATUS_SUCCESS) {
+  DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, 
Status));
+  goto Error;
+}
+if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
+  FreePool (Params);
+  DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
+  goto Error;
+}
+FreePool (Params);
+  }
+
   Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, );
   if (Status != XENSTORE_STATUS_SUCCESS || Value > MAX_UINT16) {
 DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to get backend-id (%d)\n",
-- 
1.7.10.4

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

Re: [edk2] [Patch] MdeModulePkg SetupBrowserDxe: Save global variable values before nest function called.

2015-10-21 Thread Gao, Liming 
Reviewed-by: Liming Gao 

-Original Message-
From: Dong, Eric 
Sent: Friday, October 16, 2015 9:34 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming; Bi, Dandan
Subject: [Patch] MdeModulePkg SetupBrowserDxe: Save global variable values 
before nest function called.

The SendForm function can be called again before this function exist. This 
function also uses some global variables. So we must save global variable 
values before the function been called again. Old implementation miss to save 
some global variable, this patch fixed it.

Cc: Liming Gao 
Cc: Dandan Bi 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong 
---
 MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 8   
MdeModulePkg/Universal/SetupBrowserDxe/Setup.h | 8 +++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c 
b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
index 43cfc87..4a6758a 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
@@ -5566,10 +5566,14 @@ SaveBrowserContext (
   Context->CallbackReconnect= gCallbackReconnect;
   Context->ExitRequired = gExitRequired;
   Context->HiiHandle= mCurrentHiiHandle;
   Context->FormId   = mCurrentFormId;
   CopyGuid (>FormSetGuid, );
+  Context->SystemLevelFormSet   = mSystemLevelFormSet;
+  Context->CurFakeQestId= mCurFakeQestId;
+  Context->HiiPackageListUpdated = mHiiPackageListUpdated;
+  Context->FinishRetrieveCall   = mFinishRetrieveCall;
 
   //
   // Save the menu history data.
   //
   InitializeListHead(>FormHistoryList);
@@ -5623,10 +5627,14 @@ RestoreBrowserContext (
   gCallbackReconnect= Context->CallbackReconnect;
   gExitRequired = Context->ExitRequired;
   mCurrentHiiHandle = Context->HiiHandle;
   mCurrentFormId= Context->FormId;
   CopyGuid (, >FormSetGuid);
+  mSystemLevelFormSet   = Context->SystemLevelFormSet;
+  mCurFakeQestId= Context->CurFakeQestId;
+  mHiiPackageListUpdated = Context->HiiPackageListUpdated;
+  mFinishRetrieveCall   = Context->FinishRetrieveCall;
 
   //
   // Restore the menu history data.
   //
   while (!IsListEmpty (>FormHistoryList)) { diff --git 
a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h 
b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
index 61e706a..81e2a62 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.h
@@ -536,11 +536,14 @@ typedef struct {
   BOOLEAN  ExitRequired;
   EFI_HII_HANDLE   HiiHandle;
   EFI_GUID FormSetGuid;
   EFI_FORM_ID  FormId;
   UI_MENU_SELECTION*Selection;
-
+  FORM_BROWSER_FORMSET *SystemLevelFormSet;
+  EFI_QUESTION_ID  CurFakeQestId;
+  BOOLEAN  HiiPackageListUpdated;
+  BOOLEAN  FinishRetrieveCall;
   LIST_ENTRY   FormHistoryList;
 } BROWSER_CONTEXT;
 
 #define BROWSER_CONTEXT_FROM_LINK(a)  CR (a, BROWSER_CONTEXT, Link, 
BROWSER_CONTEXT_SIGNATURE)
 
@@ -584,10 +587,13 @@ extern SETUP_DRIVER_PRIVATE_DATA mPrivateData;  // 
Browser Global Strings  //
 extern CHAR16*gEmptyString;
 
 extern UI_MENU_SELECTION  *gCurrentSelection;
+extern BOOLEANmHiiPackageListUpdated;
+extern UINT16 mCurFakeQestId;
+extern BOOLEANmFinishRetrieveCall;
 
 //
 // Global Procedure Defines
 //
 #include "Expression.h"
--
1.9.5.msysgit.1

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

Re: [edk2] Porting UEFI to a ARM platform

2015-10-21 Thread Laszlo Ersek 
On 10/21/15 04:44, Yehuda Yitschak wrote:
> Hello everyone 
> 
> 
> i started reading and experimenting with UEFI a month ago and i'm
> now considering to start porting it to one of Marvell's Aarch64 SOCs
> 
> as a first stage i only want to get a basic shell running and start
> adding drivers with time. 
> 
> 
> i'm wondering if there is any documentation or guide available to help
> with the process
> 
> 
> my initial thoughts are to clone to Juno package and start changing the
> DSC and other configuration files but i guess there is a lot of details
> into this. 
> 
> 
> Any help or pointer is greatly appreciated 

I've never done a full port myself, but I remember ArmPlatformPkg wiki
articles. Hm... new wiki:

https://github.com/tianocore/tianocore.github.io/wiki/ArmPlatformPkg

old wiki (with pics):

http://tianocore.sourceforge.net/wiki/ArmPlatformPkg

Maybe this helps.

Laszlo


> 
> 
> Thanks in advance
> 
> 
> Yehuda 
> 

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

Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

2015-10-21 Thread Laszlo Ersek 
CC'ing Eduardo

On 10/20/15 23:39, Jordan Justen wrote:
> On 2015-10-20 12:27:42, Laszlo Ersek wrote:
>> Due to Linux kernel commit b18d5431acc7 ("KVM: x86: fix CR0.CD
>> virtualization"), vCPUs need more time to start up, because:
>>
>> - KVM now zaps all the mappings for the guest memory in EPT or shadow page
>>   table, hence more VM exits are required to rebuild the mappings for all
>>   memory accesses.
>>
>> - If a physical device has been assigned to the guest, and the IOMMU lacks
>>   the snoop control feature, guest memory will become uncacheable after
>>   CR0.CD is set to 1.
>>
>> UefiCpuPkg/UefiCpuPkg.dec sets the timeout to 50ms; startup failures with
>> 100ms have been reported. Xiao Guangrong suggested 1s, and helped word the
>> commit message.
>
> We will wait 1 second each boot for APs to start up? Meaning we will
> basically add 1 second to the OVMF boot time?

Yes, in StartApsStackless() that's the case.

> I wonder if we can get the actual max from QEMU and use it to tell
> CpuDxe how many processors to wait for. This would allow it to bail
> out earlier if all processors startup faster.

QEMU exports two fw_cfg keys (not files -- directly keys) that are
related to VCPU counts:

  QemuFwCfgItemSmpCpuCount = 0x0005,

  QemuFwCfgItemMaximumCpuCount = 0x000f,

(called FW_CFG_NB_CPUS and FW_CFG_MAX_CPUS in the QEMU source,
respectively).

FW_CFG_MAX_CPUS is not really good for this, because it carries the
maximum possible APIC ID value, plus one, not the maximum number of
CPUs. Please see the big comment in bochs_bios_init() in QEMU
[hw/i386/pc.c].

FW_CFG_NB_CPUS exposes the "smp_cpus" variable of QEMU, which is the
count of VCPUs present at QEMU startup (more can be hotplugged later).
See the -smp option:

  -smp [cpus=]n[,cores=cores][,threads=threads][,sockets=sockets]
   [,maxcpus=maxcpus]

"smp_cpus" corresponds to "n" in the above (where "cpus" is enforced to
equal cores * threads * sockets ==> smp_parse()), whereas "maxcpus"
means "maximum number of CPUs, including future hotplug" (variable
"max_cpus", which is not exposed via fw_cfg at all).

In addition, (smp_cpus - 1) is also visible in the CMOS, at offset 0x5f.

Confused yet? Good; it is very confusing. Here's a summary:

  quantity |QEMU command line |QEMU variable or  |guest 
interface
   |  |function  |
  
-|--|--|---
  initially|-smp cpus=|smp_cpus  
|FW_CFG_NB_CPUS
  plugged  |and/or|  |and
  CPUs |-smp cores=,threads=,sockets= |  
|cmos[0x5f] + 1
  
-+--+--+---
  maximum  |-smp maxcpus= |max_cpus  |n/a
  possible |  |  |
  CPUs |  |  |
  
-+--+--+---
  maximum  |-smp 
cores=,threads=,sockets=,maxcpus=|pc_apic_id_limit()|FW_CFG_MAX_CPUS
  APIC ID  |(i.e., derived from topology and  |  |-1
   |maximum possible count)   |  |

SeaBIOS too counts the CPUs at startup (see smp_setup() in
"src/fw/smp.c"). The quantity that it uses to terminate the loop is
(cmos[0x5f] + 1) -- i.e., number of initially plugged CPUs.

If we want to do something similar, then UefiCpuPkg/CpuDxe needs to
accept and consider such a limit (from a dynamic PCD), and:

(a) in OvmfPkg/PlatformPei, we have to set that PCD from (cmos[0x5f]+1)
or (equivalently) QemuFwCfgItemSmpCpuCount, or
(b) set the PCD (from one of the same sources) in a NULL library that we
hook into CpuDxe in our platform DSC files.

If you can write a patch for this, I'll thank you. :)

Thanks,
Laszlo

>
> -Jordan
>
>> Cc: Xiao Guangrong 
>> Cc: Jordan Justen 
>> Cc: Janusz Mocek 
>> Cc: Alex Williamson 
>> Reported-by: Janusz Mocek 
>> Suggested-by: Xiao Guangrong 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  OvmfPkg/OvmfPkgIa32.dsc| 6 ++
>>  OvmfPkg/OvmfPkgIa32X64.dsc | 8 +++-
>>  OvmfPkg/OvmfPkgX64.dsc | 6 ++
>>  3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 0d044c2..670a80f 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -358,6 +358,12 @@ [PcdsFixedAtBuild]
>>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>  !endif
>>
>> +  # Initial AP detection may take a long time on KVM; dependent on device
>> +  #

Re: [edk2] [PATCH] OvmfPkg: increase MP services startup timeout

2015-10-21 Thread Laszlo Ersek 
On 10/20/15 22:09, Janusz wrote:
> W dniu 20.10.2015 o 21:27, Laszlo Ersek pisze:
>> Due to Linux kernel commit b18d5431acc7 ("KVM: x86: fix CR0.CD
>> virtualization"), vCPUs need more time to start up, because:
>>
>> - KVM now zaps all the mappings for the guest memory in EPT or shadow page
>>   table, hence more VM exits are required to rebuild the mappings for all
>>   memory accesses.
>>
>> - If a physical device has been assigned to the guest, and the IOMMU lacks
>>   the snoop control feature, guest memory will become uncacheable after
>>   CR0.CD is set to 1.
>>
>> UefiCpuPkg/UefiCpuPkg.dec sets the timeout to 50ms; startup failures with
>> 100ms have been reported. Xiao Guangrong suggested 1s, and helped word the
>> commit message.
>>
>> Cc: Xiao Guangrong 
>> Cc: Jordan Justen 
>> Cc: Janusz Mocek 
>> Cc: Alex Williamson 
>> Reported-by: Janusz Mocek 
>> Suggested-by: Xiao Guangrong 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  OvmfPkg/OvmfPkgIa32.dsc| 6 ++
>>  OvmfPkg/OvmfPkgIa32X64.dsc | 8 +++-
>>  OvmfPkg/OvmfPkgX64.dsc | 6 ++
>>  3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 0d044c2..670a80f 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -358,6 +358,12 @@ [PcdsFixedAtBuild]
>>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>  !endif
>>  
>> +  # Initial AP detection may take a long time on KVM; dependent on device
>> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
>> interval
>> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the 
>> APs,
>> +  # and to wait until they become idle again.
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
>> +
>>  !ifndef $(USE_OLD_SHELL)
>>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
>> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
>> 0xB4, 0xD1 }
>>  !endif
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 19d2221..8c00096 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -51,7 +51,7 @@ [BuildOptions]
>>  
>>  [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
>>GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
>> -  
>> +
>>  
>> 
>>  #
>>  # SKU Identification section - list of all SKU IDs supported by this 
>> Platform.
>> @@ -363,6 +363,12 @@ [PcdsFixedAtBuild]
>>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>  !endif
>>  
>> +  # Initial AP detection may take a long time on KVM; dependent on device
>> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
>> interval
>> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the 
>> APs,
>> +  # and to wait until they become idle again.
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
>> +
>>  !ifndef $(USE_OLD_SHELL)
>>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
>> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
>> 0xB4, 0xD1 }
>>  !endif
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index b8df1dc..eb4da4f 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -363,6 +363,12 @@ [PcdsFixedAtBuild]
>>gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>>  !endif
>>  
>> +  # Initial AP detection may take a long time on KVM; dependent on device
>> +  # assignment, IOMMU features, VCPU count, and more. Allow a generous 
>> interval
>> +  # for the MP services initialization in UefiCpuPkg/CpuDxe to count the 
>> APs,
>> +  # and to wait until they become idle again.
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|100
>> +
>>  !ifndef $(USE_OLD_SHELL)
>>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 
>> 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 
>> 0xB4, 0xD1 }
>>  !endif
> Tested-by: Janusz Mocek 
> 
> Thanks
> 

Thanks for testing it. We'll need a better design (see the discussion
with Jordan), but until we implement that in upstream, we can perhaps
ask Gerd to add this patch to his RPMs -- this should work as a
short-term band-aid for assigned GPU users.

For upstream, Jordan's suggestion is clearly superior, so we shall do
that; once ready, this patch can be backed out of Gerd's build.

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