Re: [edk2] [PATCH v2 00/16] unify GCC command line options

[Paolo Bonzini]

On 18/08/2015 08:52, Ard Biesheuvel wrote:
  Personally, I would not mind deprecating GCC44, but the biggest
  question I would have is what toolchains do the latest UDK releases
  claim to support.
 
  We also have the issue that every time I ask about deprecating a
  toolchain, Larry looks at me like I'm crazy. :)

 Well, perhaps he can chime in and explain his motivation behind this?
 At some point, we need to start removing things, surely. Larry just
 has a higher tolerance for pain :-)

RHEL 6 is shipping GCC 4.4.  True, there are software collections to
overcome that, but I think supporting GCC 4.4 is a good idea for at
least a couple more years.

Laszlo, do you still use RHEL 6?  Are you building with GCC 4.4?

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

Re: [edk2] BaseTools features: multiple workspaces

[Paolo Bonzini]

On 03/08/2015 05:56, Gao, Liming wrote:
 Hi, all
   We will update BaseTools feature to allow more than one workspaces. The 
 detail design in the below. Please help review it. If you have any comments, 
 please let me know.
 
 1.   Keep $(WORKSPACE) environment as is
 
 a.   $(WORKSPACE) determines location of Build and Conf directory.
 
 2.   New optional $(WORKSPACE_MULTIPLE) environment is added to include 
 more directories with the separator ';', like $(PATH)

Why is $(WORKSPACE_MULTIPLE) necessary?  On Linux it is okay to break
preexisting paths that contain a :; on Windows semicolons are allowed
in theory but in practice break in several ways.

Paolo

 a.   Produce the same behavior if $(WORKSPACE_MULTIPLE) is not set.
 
 3.   Update edksetup.bat/edksetup.sh to find BaseTools directory from 
 $(WORKSPACE) and $(WORKSPACE_MULTIPLE) directories.
 
 4.   Update BaseTools to support multiple workspaces
 
 a.   For the relative file path (INF, DSC and FDF), BaseTools will search 
 them from $(WORKSPACE) and $(WORKSPACE_MULTIPLE) directories.
 
 b.  Search priority from high to low: $(WORKSPACE), $(WORKSPACE_MULTIPLE).
 
 This is a compatible feature. It has no impact on current EDKII project.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Paolo Bonzini]

On 28/07/2015 20:44, Laszlo Ersek wrote:
 I have a significant update for this patch. On S3 resume, the APMC_EN
 bit (and other bits) are cleared in SMI_EN (which is necessary, see qemu
 commit be66680e). For the trigger method to work right after S3 resume,
 the APMC_EN bit must be set again (otherwise a working-as-expected OS
 will not be able to use the variable services after S3 resume).
 
 I decided to put the S3 boot script to actual use this time, and I am
 now writing the necessary opcodes to the bootscript in the entry point
 (more precisely, in a protocol notify callback). This way chipset state
 is restored by the time the OS gets the control back.

Ah, so the lockbox is accessed straight from TSEG to retrieve the S3
bootscript, without going through the SMM handler?

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

Re: [edk2] apparent SMBASE relocation issue with noexec enabled [was: MdeModulePkg DxeIpl: Add stack NX support]

[Paolo Bonzini]

On 07/08/2015 01:02, Laszlo Ersek wrote:
  The trace covers the full lifetime of the guest (I started tracing
  before launching the guest, and I passed -no-reboot to qemu, so when the
  guest crashed, QEMU exited.)
  
  This was on 3.10.0-299.el7.x86_64.
 I repeated the test with EPT off. The guest doesn't crash this way, it spins 
 in a busy loop.
 
  qemu-system-i38-32767 [002] 55142.911133: kvm_emulate_insn: 0:7ffd790b: 
 0f aa
  qemu-system-i38-32767 [002] 55142.911139: kvm_cpuid:func 
 8001 rax 6e8 rbx 0 rcx 0 rdx 10
  qemu-system-i38-32767 [002] 55142.911148: kvm_enter_smm:vcpu 0: 
 leaving SMM, smbase 0x7ffc
  qemu-system-i38-32767 [002] 55142.911150: kvm_mmu_get_page: existing sp 
 gfn 7fe65 1/2 q3 --- !pge !nxe root 0 sync
  qemu-system-i38-32767 [002] 55142.911151: kvm_mmu_get_page: existing sp 
 gfn 7fe66 1/2 q3 --- !pge !nxe root 0 sync
  qemu-system-i38-32767 [002] 55142.911151: kvm_mmu_get_page: existing sp 
 gfn 7fe67 1/2 q3 --- !pge !nxe root 0 sync
  qemu-system-i38-32767 [002] 55142.911151: kvm_mmu_get_page: existing sp 
 gfn 7fe68 1/2 q3 --- !pge !nxe root 0 sync
  qemu-system-i38-32767 [002] 55142.911152: kvm_entry:vcpu 0
  qemu-system-i38-32767 [002] 55142.911152: kvm_exit: reason 
 EXCEPTION_NMI rip 0x7ffdb6b2 info 7fe88760 8b0e
  qemu-system-i38-32767 [002] 55142.911153: kvm_page_fault:   address 
 7fe88760 error_code b
 
 And then the last triplet is repeated infinitely.
 
 0x7ffdb6b2 is the address of the same first instruction executed after the 
 RSM.
 
 The address 0x7fe88760 seems to fall into an EfiBootServicesData allocation, 
 made in PEI (via a suitable HOB):
 
 Memory Allocation 0x0004 0x7FE69000 - 0x7FE88FFF

You probably should use -cpu model,-lm,-nx.  EFER is not part of the
32-bit state save map, so the EFER.NXE bit is not restored correctly on
exit from SMM if you emulate a 32-bit CPU.

I have not debugged yet why it works without KVM, nor why the symptoms
are different between EPT and non-EPT.

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

Re: [edk2] apparent SMBASE relocation issue with noexec enabled [was: MdeModulePkg DxeIpl: Add stack NX support]

[Paolo Bonzini]

On 06/08/2015 16:31, Laszlo Ersek wrote:
 kvm_cpuid:func 8001 rax 6e8 rbx 0 rcx 0 rdx 10
 kvm_enter_smm:vcpu 0: leaving SMM, smbase 0x7ffc
 kvm_entry:vcpu 0
 kvm_exit: reason TRIPLE_FAULT rip 0x7ffdb6b2 info 0 0
 kvm_userspace_exit:   reason KVM_EXIT_SHUTDOWN (8)

Can you provide a trace with both kvm and kvmmmu events enabled?

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

Re: [edk2] [PATCH v3 27/52] OvmfPkg: use relaxed AP SMM synchronization mode

[Paolo Bonzini]

On 23/10/2015 17:29, Laszlo Ersek wrote:
> I plan to drop this patch, dependent on testing, and on how a new QEMU
> patch I've written will be received on qemu-devel.

I'm not sure why it can't be fixed within the firmware.  Your patch
to QEMU to use current_cpu obviously makes sense (that's why it
has been merged already :)), but otherwise the firmware should
adjust to the hardware, not the other way round.

Perhaps we can use a new sync mode (a new PCD would be neater, but
you'd have to pass it to BSPHandler and APHandler) to send the
IPI from SMM.  A simple implementation of the former would be this:

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index dd333a1..1be1a4d 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -377,7 +377,7 @@
 !endif
 
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x02
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|1
 !endif
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9f1ed34..430f1f4 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -383,7 +383,7 @@
 
 [PcdsFixedAtBuild.X64]
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x02
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|2
 !endif
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c0cc92b..b052806 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -382,7 +382,7 @@
 !endif
 
 !if $(SMM_REQUIRE) == TRUE
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x02
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|2
 !endif
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index b0191cb..7b20e27 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -191,6 +191,29 @@ AllCpusInSmmWithExceptions (
 }
 
 
+VOID
+SmmSendSmiToAPs (
+  VOID
+  )
+{
+  UINTN Index;
+
+  ASSERT (mSmmMpSyncData->Counter <= mNumberOfCpus);
+
+  if (mSmmMpSyncData->Counter < mNumberOfCpus) {
+//
+// Send SMI IPIs to bring outside processors in
+//
+for (Index = mMaxNumberOfCpus; Index-- > 0;) {
+  if (!mSmmMpSyncData->CpuData[Index].Present && 
gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId != INVALID_APIC_ID) {
+SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
+  }
+}
+  }
+
+  return;
+}
+
 /**
   Given timeout constraint, wait for all APs to arrive, and insure when this 
function returns, no AP will execute normal mode code before
   entering SMM, except SMI disabled APs.
@@ -344,6 +367,16 @@ BSPHandler (
   //
   gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex;
   
+  //
+  // Manual AP Sync Mode: SmmControl2DxeTrigger only triggers an SMI
+  // on the processor that executed it, call all other APs.  Otherwise
+  // this is the same as Relaxed mode.
+  //
+  if (SyncMode == SmmCpuSyncModeManualAp) {
+SmmSendSmiToAPs();
+  }
+
   //
   // If Traditional Sync Mode or need to configure MTRRs: gather all available 
APs.
   //
@@ -450,12 +482,11 @@ BSPHandler (
   ClearSmi();
 
   //
-  // If Relaxed-AP Sync Mode: gather all available APs after BSP SMM handlers 
are done, and
-  // make those APs to exit SMI synchronously. APs which arrive later will be 
excluded and 
+  // If Relaxed-AP or Manual-AP Sync Mode: gather all available APs after BSP 
SMM handlers
+  // are done, and make those APs to exit SMI synchronously. APs which arrive 
later will be
   // will run through freely.
   //
   if (SyncMode != SmmCpuSyncModeTradition && 
!SmmCpuFeaturesNeedConfigureMtrrs()) {
-
 //
 // Lock the counter down and retrieve the number of APs
 //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 71f9b1b..a631811 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -297,6 +297,7 @@ typedef struct {
 typedef enum {
   SmmCpuSyncModeTradition,
   SmmCpuSyncModeRelaxedAp,
+  SmmCpuSyncModeManualAp,
   SmmCpuSyncModeMax
 } SMM_CPU_SYNC_MODE;
 

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

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Paolo Bonzini]

On 03/11/2015 15:35, Xiao Guangrong wrote:
>
> -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.)
>>
>> Great job.  I look forward to the testing results.
>>
>> It should also have the noncoherent DMA check, in fact, though that's
>> just an optimization and it would have masked the bug on your system.
> 
> Hmm... but kvm_zap_gfn_range even other shadow page zapping operations
> are really usual behaviour in host - it depends on how we handle memory
> overcommit/memory-layout-change on host.
> 
> I doubt it is really a right way to go. kvm_zap_gfn_range() is just a time
> delay factor to trigger OVMF boot issue but there must have other factors
> have such delay too, for example, more vcpus in OVMF, high overload on
> host, etc.

But it's pointless if the quirk is enabled.  Also, bringing up APs will
cause heavy contention on mmu_lock as Laszlo pointed out.

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

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Paolo Bonzini]

On 04/11/2015 21:08, Laszlo Ersek wrote:
> On 11/04/15 17:55, Kinney, Michael D wrote:
>> Laszlo,
>>
>> BaseXApicX2ApicLib is intended to be used by platforms that support more 
>> >=256 CPUs.
>>
>> If the current system configuration is < 256 CPUs, then the platform will 
>> typically stay in APIC mode.  If >= 256 CPUs are detected, then X2 APIC mode 
>> can be enabled using the following API.
>>
>> VOID
>> EFIAPI
>> SetApicMode (
>>   IN UINTN  ApicMode
>>   )
>>
>> So just adding BaseXApicX2ApicLib to the DSC does not enable X2 APIC mode.  
>> You have to add logic to enable X2 APIC mode.
>>
>> I see that QEMU is limited to 255 VCPUs, so the use of BaseXApicLib makes 
>> sense.  Are OVMF configurations supported with >= 256 VCPUs?
> 
> I don't think so, but 64-bit Linux guest kernels enable x2apic mode
> anyway, apparently regardless of the VCPU count and topology.

Yup, x2apic-style accesses (via MSRs) are faster because you do not have
to walk the page tables.

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

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Paolo Bonzini]

On 05/11/2015 02:04, Laszlo Ersek wrote:
> On 11/04/15 22:35, Kinney, Michael D wrote:
>> Laszlo,
>>
>> Yes.  They are compatible.  And I do recommend switching to 
>> BaseXApicX2ApicLib unconditionally.
> 
> Thanks everyone for the feedback, I'll update the patch.
> 
> Paolo, in case this turns out the only code update after v4 (I can
> dream...), are you okay if I fix up the patch without posting v5?

Of course.

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

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Paolo Bonzini]

On 03/11/2015 20:42, Jordan Justen wrote:
> On 2015-11-03 05:45:52, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2015 14:25, Laszlo Ersek wrote:
>>>   - 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?
> 
> Obviously we'll have to follow what QEMU decides, so I'm not sure how
> much we can actually influence this.
> 
> Aside from the desire to better emulate chipset/platform designs of
> the actual hardware, I do have a related question.
> 
> If we use the local apic to initiate IPIs to other processors, what
> impact might that have on the state of the local apic if the OS is
> also trying to use it? For example, what if the OS is in the middle of
> trying to send an IPI?

A sequence like

   write to ICR
   out to 0xb2
   write to ICR2

is invalid.

On the other hand, the state of the _destination_ APICs is unaffected by
any IPIs that are delivered between writes to ICR and ICR2.

So it's okay to use the APIC from SMM, as long as it's only done on the
processor that received the synchronous SMI.

> Since it involves multiple projects, could it take longer to
> coordinate a change?

Perhaps, but on the other hand it's just the final 1%.  It's worthless
if we do not have the rest.

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

Re: [edk2] [Patch 3/3] UefiCpuPkg/CpuDxe: Place APs into protected mode when ExitBootService

[Paolo Bonzini]

On 26/10/2015 23:31, Laszlo Ersek wrote:
> > If QEMU could evaluate the AP state and not send an SMI to an AP in
> > Wait-forSIPI, then updating SMIs to broadcast to all AP should work
> > for SeaBios and OVMF.

Yup, this has to be fixed in both QEMU and KVM (separately).

I'm not 100% sure of how the bug happens in KVM, because KVM does
something really weird (and unintended).  The vCPU _is_ in SMM, but it
doesn't start running.  When you get the next SIPI, the vCPU runs the
startup vector in SMM rather than real mode.  Still has to be fixed, of
course.

> >  All APs in SeaBios are in Wait-for-SIPI, so
> > the BSP will be the only CPU that receives the SMI.  In OVMF, all APs
> > can be in the HLT loop that Jeff Fan provided a patch for (and I see
> > you have a good update for), so the SMI can be delivered to BSP and
> > all APs.
>
> I'd like to run this idea by Paolo (CC'd). Theoretically I might be able
> to respin my QEMU patch so that it looks at the APs' states right when
> one of the VCPUs writes to APM_CNT. But, as far as I recall, in QEMU it
> is two separate actions to generate / make an interrupt pending, and
> then to deliver it. Filtering out the SMI at generation time might not
> be the right thing to do. Doing it in the delivery / injection code
> seems possible, but that code is super quirky and I don't dare touch it.
> (Worse, I think that code might exist separately for TCG (in QEMU) and
> KVM (in the host kernel).)

I'm not sure where it's better to filter it out.  I'll have to look a
bit more at the code.

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

Re: [edk2] [Patch 3/3] UefiCpuPkg/CpuDxe: Place APs into protected mode when ExitBootService

[Paolo Bonzini]

On 27/10/2015 03:12, Fan, Jeff wrote:
> Yes. On physical hw, Aps will not response SMI if Aps received SMI in
> WFSI state. But Aps will have one pending SMI and will enter into SMM
> once Aps receive Startup IPI.

Interesting... so if the BIOS doesn't do SMBASE relocation, an
INIT-SMI-SIPI sequence will run code at 0x3 in system management
mode---thus letting the OS poke at SMRAM?

Related to this, how is SMBASE relocation handled in the case where CPUs
are hotplugged?  Is there a race between any firmware code that does
SMBASE relocation for the new code, and the OS which could overwrite the
SMBASE relocation stub at address 0x3?

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

Re: [edk2] [PATCH v4 13/41] OvmfPkg: LockBox: -D SMM_REQUIRE excludes our fake lockbox

[Paolo Bonzini]

On 03/11/2015 22:00, Laszlo Ersek wrote:
> When the user builds OVMF with -D SMM_REQUIRE, our LockBox implementation
> must not be used, since it doesn't actually protect data in the LockBox
> from the runtime guest OS. Add an according assert to
> LockBoxLibInitialize().
> 
> Furthermore, since our LockBox must not be selected with -D SMM_REQUIRE,
> it makes sense to set aside memory for it only if -D SMM_REQUIRE is
> absent. Modify InitializeRamRegions() accordingly.
> 
> This patch completes the -D SMM_REQUIRE-related tweaking of the special
> OVMF memory areas.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>  OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf |  3 ++
>  OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf  |  3 ++
>  OvmfPkg/Library/LockBoxLib/LockBoxLib.c   |  2 +
>  OvmfPkg/PlatformPei/MemDetect.c   | 40 ++--
>  4 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> index 7203d07..81c893e 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> @@ -42,3 +42,6 @@ [LibraryClasses]
>  [Pcd]
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> index a4d27a5..08973a4 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> @@ -43,3 +43,6 @@ [LibraryClasses]
>  [Pcd]
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c 
> b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> index 89050ac..45481b9 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> @@ -44,6 +44,8 @@ LockBoxLibInitialize (
>  {
>UINTN NumEntries;
>  
> +  ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
> +
>if (PcdGet32 (PcdOvmfLockBoxStorageSize) < sizeof (LOCK_BOX_GLOBAL)) {
>  return RETURN_UNSUPPORTED;
>}
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 1bdc2df..455fcbb 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -407,25 +407,27 @@ InitializeRamRegions (
>}
>  
>if (mBootMode != BOOT_ON_S3_RESUME) {
> -//
> -// Reserve the lock box storage area
> -//
> -// Since this memory range will be used on S3 resume, it must be
> -// reserved as ACPI NVS.
> -//
> -// If S3 is unsupported, then various drivers might still write to the
> -// LockBox area. We ought to prevent DXE from serving allocation requests
> -// such that they would overlap the LockBox storage.
> -//
> -ZeroMem (
> -  (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> -  (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
> -  );
> -BuildMemoryAllocationHob (
> -  (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> -  (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
> -  mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> -  );
> +if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +  //
> +  // Reserve the lock box storage area
> +  //
> +  // Since this memory range will be used on S3 resume, it must be
> +  // reserved as ACPI NVS.
> +  //
> +  // If S3 is unsupported, then various drivers might still write to the
> +  // LockBox area. We ought to prevent DXE from serving allocation 
> requests
> +  // such that they would overlap the LockBox storage.
> +  //
> +  ZeroMem (
> +(VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
> +);
> +  BuildMemoryAllocationHob (
> +(EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +(UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
> +mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> +);
> +}
>  
>  if (FeaturePcdGet (PcdSmmSmramRequire)) {
>UINT32 TsegSize;
> 

Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 12/41] OvmfPkg: AcpiS3SaveDxe: don't fake LockBox protocol if SMM_REQUIRE

[Paolo Bonzini]

On 03/11/2015 22:00, Laszlo Ersek wrote:
> In SVN r15306 (git commit d4ba06df), "OvmfPkg: S3 Resume: fake LockBox
> protocol for BootScriptExecutorDxe", we installed a fake LockBox protocol
> in OVMF's AcpiS3SaveDxe clone. While our other AcpiS3SaveDxe
> customizations remain valid (or harmless), said change is invalid when
> OVMF is built with -D SMM_REQUIRE and includes the real protocol provider,
> "MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf".
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>  OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf |  3 ++-
>  OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c  | 14 --
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf 
> b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> index 4cc0713..a288b95 100644
> --- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> +++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> @@ -59,7 +59,7 @@ [Guids]
>gEfiEndOfDxeEventGroupGuid## CONSUMES  ## Event
>  
>  [Protocols]
> -  gEfiLockBoxProtocolGuid   # PROTOCOL ALWAYS_PRODUCED
> +  gEfiLockBoxProtocolGuid   # PROTOCOL SOMETIMES_PRODUCED
>gEfiLegacyBiosProtocolGuid# PROTOCOL ALWAYS_CONSUMED
>gEfiLegacyRegion2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
>gFrameworkEfiMpServiceProtocolGuid# PROTOCOL SOMETIMES_CONSUMED
> @@ -71,6 +71,7 @@ [Pcd]
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize
> ## CONSUMES
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3BootScriptStackSize   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> ## CONSUMES
>  
>  [Depex]
>gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
> diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c 
> b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> index f20560f..e3ff234 100644
> --- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> +++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> @@ -538,12 +538,14 @@ InstallEndOfDxeCallback (
>  return EFI_LOAD_ERROR;
>}
>  
> -  Status = gBS->InstallMultipleProtocolInterfaces (
> -  ,
> -  , NULL,
> -  NULL
> -  );
> -  ASSERT_EFI_ERROR (Status);
> +  if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +Status = gBS->InstallMultipleProtocolInterfaces (
> +,
> +    , NULL,
> +NULL
> +);
> +ASSERT_EFI_ERROR (Status);
> +  }
>  
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> 

Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 35/41] OvmfPkg: port CpuS3DataDxe to X64

[Paolo Bonzini]

On 03/11/2015 22:01, Laszlo Ersek wrote:
> From: Paolo Bonzini <pbonz...@redhat.com>
> 
> The descriptor format is different and the assembly source is
> converted to nasm, but otherwise there is no difference.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>

Missing your SoB.

Paolo

> ---
> 
> Notes:
> v3:
> - new in v3
> 
>  OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf  |   5 +
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h |  59 +++
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c| 108 
> 
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/CpuAsm.nasm   |  58 +++
>  4 files changed, 230 insertions(+)
> 
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> index 2610a63..4bec056 100644
> --- a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -63,6 +63,11 @@ [Sources.Ia32]
>IA32/ArchSpecificDef.h
>IA32/ArchSpecific.c
>  
> +[Sources.X64]
> +  X64/CpuAsm.nasm
> +  X64/ArchSpecificDef.h
> +  X64/ArchSpecific.c
> +
>  [Packages]
>MdePkg/MdePkg.dec
>IntelFrameworkPkg/IntelFrameworkPkg.dec
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
> new file mode 100644
> index 000..5ea4cf6
> --- /dev/null
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
> @@ -0,0 +1,59 @@
> +/** @file
> +
> +Copyright (C) 2015, Red Hat, Inc.
> +Copyright (c) 2013-2015 Intel Corporation.
> +
> +Redistribution and use in source and binary forms, with or without
> +modification, are permitted provided that the following conditions
> +are met:
> +
> +* Redistributions of source code must retain the above copyright
> +notice, this list of conditions and the following disclaimer.
> +* Redistributions in binary form must reproduce the above copyright
> +notice, this list of conditions and the following disclaimer in
> +the documentation and/or other materials provided with the
> +distribution.
> +* Neither the name of Intel Corporation nor the names of its
> +contributors may be used to endorse or promote products derived
> +from this software without specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +
> +Module Name:
> +
> +  ProcessorDef.h
> +
> +Abstract:
> +
> +  Definition for X64 processor
> +
> +**/
> +
> +#ifndef _PROCESSOR_DEF_H_
> +#define _PROCESSOR_DEF_H_
> +
> +#pragma pack(1)
> +
> +typedef struct {
> +  UINT16  OffsetLow;
> +  UINT16  SegmentSelector;
> +  UINT16  Attributes;
> +  UINT16  OffsetHigh;
> +  UINT32  OffsetUpper;
> +  UINT32  Zero;
> +} INTERRUPT_GATE_DESCRIPTOR;
> +
> +#pragma pack()
> +
> +#endif
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c
> new file mode 100644
> index 000..544816d
> --- /dev/null
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c
> @@ -0,0 +1,108 @@
> +/** @file
> +
> +  Memory Operation Functions for IA32 Architecture.
> +
> +  Copyright (C) 2015, Red Hat, Inc.
> +  Copyright (c) 2013-2015 Intel Corporation.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +
> +  * Redistributions of source code must retain the above copyright
> +  notice, this list of conditions and the following disclaimer.
> +  * Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in
> +  the documentation and/or other materials provided with the
> +  distribution.
> +  * Neither the name of Intel Corporation nor the names 

Re: [edk2] [PATCH 6/6] OvmfPkg/PlatformPei: Set PcdCpuMaxLogicalProcessorNumber using QEMU fw_cfg

[Paolo Bonzini]

On 03/11/2015 14:25, Laszlo Ersek wrote:
>   - 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?

Actually, I was *de*prioritizing it because things _more or less_ work
without it, especially if the timeout is temporarily reduced.  We
probably agree that timeouts are evil in a virt setting, but even
without this issue you can commit the ~70 patches that bring us to 99%
of the way.

Once the slate is clean and everybody is focused on the few remaining
problems, we can tackle them---including broadcast SMIs.

In fact, the rest of your email lists some more pressing problems than
broadcast SMIs, which I'm quoting below to further remind other readers. :)

Paolo

>   - 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.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v3 26/52] OvmfPkg: SmmCpuFeaturesLib: customize state save map format

[Paolo Bonzini]

On 18/10/2015 09:38, Jordan Justen wrote:
> > This adjusts the previously introduced state save map access functions, to
> > account for QEMU and KVM's 64-bit state save map following the AMD spec
> > rather than the Intel one.
>
> Shouldn't this layout match the processor being emulated? I think I
> recall hearing something about documentation?

Yes, exactly.  Intel doesn't document the placement of the descriptor
cache registers in the SMM state save map, so it's not possible to match
the processor anyway.

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

[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 v3 03/52] UefiCpuPkg: PiSmmCpuDxeSmm: do not execute RSM from 64-bit mode

[Paolo Bonzini]

On 15/10/2015 03:31, Fan, Jeff wrote:
> Ersek & Bonzini,
> 
> From SDM 34.5.2, SMI Handler Operating Mode Switching.
> "Any required change to operating mode is performed by the RSM instruction; 
> there is no need for the SMI
> handler to change modes explicitly prior to executing RSM."
> 
> So, I don't think we need to go back to 32-bit PM before RSM.

The instruction set reference, however, says that RSM is invalid in
64-bit mode.

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

Re: [edk2] [PATCH v3 03/52] UefiCpuPkg: PiSmmCpuDxeSmm: do not execute RSM from 64-bit mode

[Paolo Bonzini]

On 16/10/2015 09:41, Yao, Jiewen wrote:
> Hello According to "IA32 SDM, page 1428, 4-330 Vol. 2B, RSM?Resume
> from System Management Mode", I do not find word say: 64bit mode is
> invalid. Would you please point out where you find "RSM is invalid in
> 64-bit mode "?

It's in the heading.  It says

64-bit mode   Invalid
Compat/Leg mode   Valid

Paolo
___
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)

[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

[edk2] [PATCH 5/4] OvmfPkg: use relaxed AP SMM synchronization mode

[Paolo Bonzini]

Port 0xb2 on QEMU only sends an SMI to the currently executing processor.
The SMI handler, however, and in particular SmmWaitForApArrival, currently
expects that SmmControl2DxeTrigger triggers an SMI IPI on all processors
rather than just the BSP.  Thus all SMM invocations loop for a second (the
default value of PcdCpuSmmApSyncTimeout) before SmmWaitForApArrival sends
another SMI IPI to the APs.

With the default SmmCpuFeaturesLib, 32-bit machines must broadcast SMIs
because 32-bit machines must reset the MTRRs on each entry to system
management modes (they have no SMRRs).  However, our virtual platform
does not have problems with cacheability of SMRAM, so we can use "directed"
SMIs instead.  To do this, just set gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode
to 1 (aka SmmCpuSyncModeRelaxedAp).  This fixes SMM on multiprocessor virtual
machines.

Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
 OvmfPkg/OvmfPkgIa32.dsc| 4 
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 
 OvmfPkg/OvmfPkgX64.dsc | 4 
 3 files changed, 12 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 81adb31..96bdc73 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -373,6 +373,10 @@
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
 !endif
 
+!if $(SMM_REQUIRE) == TRUE
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+!endif
+
 !if $(SECURE_BOOT_ENABLE) == TRUE
   # override the default values from SecurityPkg to ensure images from all 
sources are verified in secure boot
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 1a8bfd2..4e4d65b 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -379,6 +379,10 @@
 !endif
 
 [PcdsFixedAtBuild.X64]
+!if $(SMM_REQUIRE) == TRUE
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+!endif
+
 !if $(SECURE_BOOT_ENABLE) == TRUE
   # override the default values from SecurityPkg to ensure images from all 
sources are verified in secure boot
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 9bf720a..54353d6 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -378,6 +378,10 @@
   gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 
0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
 !endif
 
+!if $(SMM_REQUIRE) == TRUE
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
+!endif
+
 !if $(SECURE_BOOT_ENABLE) == TRUE
   # override the default values from SecurityPkg to ensure images from all 
sources are verified in secure boot
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy|0x04
-- 
2.5.0

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

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Paolo Bonzini]

On 05/10/2015 01:57, Michael Kinney wrote:
> Add module that initializes a CPU for the SMM envirnment and
> installs the first level SMI handler.  This module along with the
> SMM IPL and SMM Core provide the services required for
> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
> 
> CPU specific features are abstracted through the SmmCpuFeaturesLib
> 
> Platform specific features are abstracted through the
> SmmCpuPlatformHookLib
> 
> Several PCDs are added to enable/disable features and configure
> settings for the PiSmmCpuDxeSmm module
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 

Hi Michael,

I'm happy to report the first bug! :)

InitPaging() is setting a page directory entry before initializing the
corresponding page table.  This works on real hardware (including KVM),
but the TLB of QEMU's emulation mode is different (possibly it has
different  associativity, I don't really know) so at some point
execution goes to nowhere's land.

The fix is really simple:

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 9463e97..6ee9256 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -555,12 +555,12 @@ InitPaging (
   Pt = AllocatePages (1);
   ASSERT (Pt != NULL);

-  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
-
   // Split it
-  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++,
Pt++) {
-*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
+  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) {
+Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW |
IA32_PG_P);
   } // end for PT
+
+  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
 } // end if IsAddressSplit
   } // end for PTE
 } // end for PDE

Thanks,

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

[Paolo Bonzini]

G 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 APs.

This could also be fixed by setting gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode
to 1 (aka SmmCpuSyncModeRelaxedAp), but this only takes effect on X64 CPUs
which also ironically we don't support yet.  Therefore, the following seems to
be the correct fix.  With it, TCG still takes a while to boot, but KVM gets
to the UEFI shell very fast.

[pbonz...@redhat.com: Use LocalApicLib to send trigger an SMI on all
 processors]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>

diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c 
b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index e895fd6..17fbb88 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -88,6 +89,15 @@ SmmControl2DxeTrigger (
   }
 
   //
+  // 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 ();
+
+  //
   // 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
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf 
b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
index bc66a27..f9c4821 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
@@ -44,10 +44,12 @@
 [Packages]
   MdePkg/MdePkg.dec
   OvmfPkg/OvmfPkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   DebugLib
   IoLib
+  LocalApicLib
   PcdLib
   PciLib
   QemuFwCfgLib

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

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Paolo Bonzini]

On 12/10/2015 18:23, Paolo Bonzini wrote:
> 
> 
> On 05/10/2015 01:57, Michael Kinney wrote:
>> Add module that initializes a CPU for the SMM envirnment and
>> installs the first level SMI handler.  This module along with the
>> SMM IPL and SMM Core provide the services required for
>> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
>>
>> CPU specific features are abstracted through the SmmCpuFeaturesLib
>>
>> Platform specific features are abstracted through the
>> SmmCpuPlatformHookLib
>>
>> Several PCDs are added to enable/disable features and configure
>> settings for the PiSmmCpuDxeSmm module
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney <michael.d.kin...@intel.com>
> 
> Hi Michael,
> 
> I'm happy to report the first bug! :)
> 
> InitPaging() is setting a page directory entry before initializing the
> corresponding page table.  This works on real hardware (including KVM),
> but the TLB of QEMU's emulation mode is different (possibly it has
> different  associativity, I don't really know) so at some point
> execution goes to nowhere's land.
> 
> The fix is really simple:

As suggested by Jordan, here's the patch again but with all the
standard signoffs.

[pbonz...@redhat.com: InitPaging: prepare PT before filling in PDE]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 9463e97..6ee9256 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -555,12 +555,12 @@ InitPaging (
   Pt = AllocatePages (1);
   ASSERT (Pt != NULL);
   
-  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
-  
   // Split it
-  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) {
-*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
+  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) {
+Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
   } // end for PT
+
+  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
 } // end if IsAddressSplit
   } // end for PTE
 } // end for PDE
___
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

[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

[edk2] [PATCH 2/4] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits

[Paolo Bonzini]

SMRR and MTRR support is not needed on a virtual platform.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 180 +++--
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf|   4 -
 2 files changed, 18 insertions(+), 166 deletions(-)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c 
b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6ab01e8..97e2010 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,46 +15,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
-//
-// Machine Specific Registers (MSRs)
-//
-#define  SMM_FEATURES_LIB_IA32_MTRR_CAP0x0FE
-#define  SMM_FEATURES_LIB_IA32_FEATURE_CONTROL 0x03A
-#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE   0x1F2
-#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK   0x1F3
-#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE  0x0A0
-#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK  0x0A1
-#defineEFI_MSR_SMRR_MASK   0xF000
-#defineEFI_MSR_SMRR_PHYS_MASK_VALIDBIT11
-
-//
-// Set default value to assume SMRR is not supported
-//
-BOOLEAN  mSmrrSupported = FALSE;
-
-//
-// Set default value to assume IA-32 Architectural MSRs are used
-//
-UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
-UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
-
-//
-// Set default value to assume MTRRs need to be configured on each SMI
-//
-BOOLEAN  mNeedConfigureMtrrs = TRUE;
-
-//
-// Array for state of SMRR enable on all CPUs
-//
-BOOLEAN  *mSmrrEnabled;
-
 /**
   The constructor function 
 
@@ -68,91 +33,9 @@ SmmCpuFeaturesLibConstructor (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  UINT32  RegEax;
-  UINT32  RegEdx;
-  UINTN   FamilyId;
-  UINTN   ModelId;
-
-  //
-  // Retrieve CPU Family and Model 
-  //
-  AsmCpuid (CPUID_VERSION_INFO, , NULL, NULL, );
-  FamilyId = (RegEax >> 8) & 0xf;
-  ModelId  = (RegEax >> 4) & 0xf;
-  if (FamilyId == 0x06 || FamilyId == 0x0f) {
-ModelId = ModelId | ((RegEax >> 12) & 0xf0);
-  }
-
-  //
-  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
-  //
-  if ((RegEdx & BIT12) != 0) {
-//
-// Check MTRR_CAP MSR bit 11 for SMRR support
-//
-if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
-  mSmrrSupported = TRUE;
-}
-  }
-
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
-  // 
-  // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then 
-  // SMRR Physical Base and SMM Physical Mask MSRs are not available.
-  //
-  if (FamilyId == 0x06) {
-if (ModelId == 0x1C || ModelId == 0x26 || ModelId == 0x27 || ModelId == 
0x35 || ModelId == 0x36) {
-  mSmrrSupported = FALSE;
-}
-  }
-
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
-  //
-  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2 
-  // Processor Family MSRs 
-  //
-  if (FamilyId == 0x06) {
-if (ModelId == 0x17 || ModelId == 0x0f) {
-  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
-  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
-}
-  }
-  
   //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 34.4.2 SMRAM Caching
-  //   An IA-32 processor does not automatically write back and invalidate its 
-  //   caches before entering SMM or before exiting SMM. Because of this 
behavior, 
-  //   care must be taken in the placement of the SMRAM in system memory and 
in 
-  //   the caching of the SMRAM to prevent cache incoherence when switching 
back 
-  //   and forth between SMM and protected mode operation.
+  // No need to program SMRRs on our virtual platform.
   //
-  // An IA-32 processor is a processor that does not support the Intel 64 
-  // Architecture.  Support for the Intel 64 Architecture can be detected from 
-  // CPUID(CPUID_EXTENDED_CPU_SIG).EDX[29]
-  //
-  // If an IA-32 processor is detected, then set mNeedConfigureMtrrs to TRUE, 
-  // so caches are flushed on SMI entry and SMI exit, the interrupted code 
-  // MTRRs are saved/restored, and MTRRs for SMM are loaded.
-  //
-  AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL, NULL);
-  if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
-AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, );
-if ((RegEdx & BIT29) != 0) {
-  mNeedConfigureMtrrs = FALSE;
-}
-  }
-  
-  //
-  // Allocate array for state of SMRR enable on all C

[edk2] [PATCH 4/4] OvmfPkg: SmmCpuFeaturesLib: customize state save map format

[Paolo Bonzini]

This adjusts the previously introduced state save map access functions,
to account for QEMU and KVM's 64-bit state save map following the AMD
spec rather than the Intel one.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
 OvmfPkg/Include/Register/QemuSmramSaveStateMap.h   | 184 +
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  50 +++---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf|   1 +
 3 files changed, 212 insertions(+), 23 deletions(-)
 create mode 100644 OvmfPkg/Include/Register/QemuSmramSaveStateMap.h

diff --git a/OvmfPkg/Include/Register/QemuSmramSaveStateMap.h 
b/OvmfPkg/Include/Register/QemuSmramSaveStateMap.h
new file mode 100644
index 000..33ab07e
--- /dev/null
+++ b/OvmfPkg/Include/Register/QemuSmramSaveStateMap.h
@@ -0,0 +1,184 @@
+/** @file
+SMRAM Save State Map Definitions.
+
+SMRAM Save State Map definitions based on contents of the 
+Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+  Volume 3C, Section 34.4 SMRAM
+  Volume 3C, Section 34.5 SMI Handler Execution Environment
+  Volume 3C, Section 34.7 Managing Synchronous and Asynchronous SMIs
+
+and the AMD64 Architecture Programmer's Manual
+  Volume 2, Section 10.2 SMM Resources
+
+Copyright (c) 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2015, Red Hat, Inc.
+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.
+
+**/
+
+#ifndef __QEMU_SMRAM_SAVE_STATE_MAP_H__
+#define __QEMU_SMRAM_SAVE_STATE_MAP_H__
+
+#pragma pack (1)
+
+///
+/// 32-bit SMRAM Save State Map
+///
+typedef struct {
+  UINT8   Reserved0[0x200]; // 7c00h
+  UINT8   Reserved1[0xf8];  // 7e00h
+  UINT32  SMBASE;   // 7ef8h
+  UINT32  SMMRevId; // 7efch
+  UINT16  IORestart;// 7f00h
+  UINT16  AutoHALTRestart;  // 7f02h
+  UINT8   Reserved2[0x9C];  // 7f08h
+  UINT32  IOMemAddr;// 7fa0h
+  UINT32  IOMisc;   // 7fa4h
+  UINT32  _ES;  // 7fa8h
+  UINT32  _CS;  // 7fach
+  UINT32  _SS;  // 7fb0h
+  UINT32  _DS;  // 7fb4h
+  UINT32  _FS;  // 7fb8h
+  UINT32  _GS;  // 7fbch
+  UINT32  Reserved3;// 7fc0h
+  UINT32  _TR;  // 7fc4h
+  UINT32  _DR7; // 7fc8h
+  UINT32  _DR6; // 7fcch
+  UINT32  _EAX; // 7fd0h
+  UINT32  _ECX; // 7fd4h
+  UINT32  _EDX; // 7fd8h
+  UINT32  _EBX; // 7fdch
+  UINT32  _ESP; // 7fe0h
+  UINT32  _EBP; // 7fe4h
+  UINT32  _ESI; // 7fe8h
+  UINT32  _EDI; // 7fech
+  UINT32  _EIP; // 7ff0h
+  UINT32  _EFLAGS;  // 7ff4h
+  UINT32  _CR3; // 7ff8h
+  UINT32  _CR0; // 7ffch
+} QEMU_SMRAM_SAVE_STATE_MAP32;
+
+///
+/// 64-bit SMRAM Save State Map
+///
+typedef struct {
+  UINT8   Reserved0[0x200];  // 7c00h
+
+  UINT16  _ES;   // 7e00h
+  UINT16  _ESAccessRights;   // 7e02h
+  UINT32  _ESLimit;  // 7e04h
+  UINT64  _ESBase;   // 7e08h
+
+  UINT16  _CS;   // 7e10h
+  UINT16  _CSAccessRights;   // 7e12h
+  UINT32  _CSLimit;  // 7e14h
+  UINT64  _CSBase;   // 7e18h
+
+  UINT16  _SS;   // 7e20h
+  UINT16  _SSAccessRights;   // 7e22h
+  UINT32  _SSLimit;  // 7e24h
+  UINT64  _SSBase;   // 7e28h
+
+  UINT16  _DS;   // 7e30h
+  UINT16  _DSAccessRights;   // 7e32h
+  UINT32  _DSLimit;  // 7e34h
+  UINT64  _DSBase;   // 7e38h
+
+  UINT16  _FS;   // 7e40h
+  UINT16  _FSAccessRights;   // 7e42h
+  UINT32  _FSLimit;  // 7e44h
+  UINT64  _FSBase;   // 7e48h
+
+  UINT16  _GS;   // 7e50h
+  UINT16  _GSAccessRights;   // 7e52h
+  UINT32  _GSLimit;  // 7e54h
+  UINT64  _GSBase;   // 7e58h
+
+  UINT32  _GDTRReserved1;// 7e60h
+  UINT16  _GDTRLimit;// 7e64h
+  UINT16  _GDTRReserved2;// 7e66h
+  UINT64  _GDTRBase; // 7e68h
+
+  UINT16  _LDTR; // 7e70h
+  UINT16  _LDTRAccessRights; // 7e72h
+  UINT32  _LDTRLimit;// 7e74h
+  UINT64  _LDTRBase; // 7e78h
+
+  UINT32  _IDTRReserved1;// 7e80h
+  UINT16  _IDTRLimit;// 7e84h
+  UINT16  _IDTRReserved2;// 7e86h
+  UINT64  _IDTRBase; // 7e88h
+
+  UINT16  _TR;   // 7e90h
+  UINT16  _TRAccessRights;   // 7e92h
+  UINT32  _TRLimit;  // 7e94h
+  UINT64  _TRBase;   // 7e98h
+
+  UINT64  IO_RIP;// 7ea0h
+  UINT64  IO_RCX;// 7ea8h
+  UINT64  IO_RSI;//

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

[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

[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

[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] MdeModulePkg: ScsiDiskDxe: adapt SectorCount when shortening transfers

[Paolo Bonzini]

On 08/09/2015 09:02, Tian, Feng wrote:
> Hi, Laszlo
> 
> 1. I don't think the code in VirtioScsi.c is correct. It hardcodes
> the block size as 512bytes. The driver should send READ_CAPACITY cmd to get
> block size, then convert it to InTransferLength & OutTransferLength.

This is the controller's maximum transfer size, not the LUN.  The LUN's
limits are available from VPD and depend on the block size.  The
controller's limit apply to all LUNs and to all commands, even those
that transfer bytes rather than blocks.

For historical reasons the controller's maximum transfer size is
provided in 512-byte units.  In fact, Laszlo raised this point to the
virtio committee just to be sure that his code is correct, which it is.

> 2. Although the UEFI spec doesn't say the relationship of
> InTransferLength/OutTransferLength and CDB.TransferLength, but I think
> it's implied that InTransferLength/OutTransferLength =
> CDB.TransferLength * BlockSize. Otherwise how the lowest layer host
> controller driver constructs transfer descriptor? How many bytes to
> read? Is InTransferLength/OutTransferLength or the one of
> CDB.TransferLength * BlockSize?

It's always InTransferLength/OutTransferLength.  If it is < or >
CDB.TransferLength * BlockSize you get respectively an underrun or an
overrun.  An underrun is fatal, an overrun is not.

InTransferLength/OutTransferLength exist because the controller may not
know how to infer the transfer length for all CDB opcodes (some express
it in bytes, some express it in blocks, some are vendor specific, some
are just crazy and you have to inspect multiple CDB fields to compute
the transfer length).

Of course ScsiDisk *does* know how to infer the transfer length for the
CDBs it builds.  Therefore, it makes sense for ScsiDisk's own READ and
WRITE CDBs to always have InTransferLength/OutTransferLength =
CDB.TransferLength * BlockSize.  However, the controller driver must not
rely on this.

> 3. If we don't think #2 is implied, why we can assume "if pass thru
> driver returns EFI_BAD_BUFFER_SIZE and leave HostAdapterStatus at
> EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK, but *then* it must set either
> TargetStatus to some error code (different from
> EFI_EXT_SCSI_STATUS_TARGET_GOOD), *or* it must report some problem in
> the sense data."? IMHO, a pass thru driver could only return
> EFI_BAD_BUFFER_SIZE and leave HostAdpterStatus/TargetStatus to ok and
> no sense data.

I agree that:

- there's no need for the controller driver to set HostAdapterStatus to
anything

- even if it sets it to something, TargetStatus should not be set and
there should be no sense data.

Regarding the first point, this is not exactly an underrun, though it
may remind of one.  EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER might be just
as good, and even EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK might be okay
because the request has never reached the controller.  The important bit
is EFI_BAD_BUFFER_SIZE.  Thus, Laszlo's incremental patch in
http://article.gmane.org/gmane.comp.bios.edk2.devel/2007 is necessary if
I understand it correctly.

Regarding the second point, the TargetStatus and sense data *could* be
set if the transfer length in the CDB exceeds the maximum transfer
length in the VPD. In this case, the sense data will be filled and the
target status will be EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION.  This
is the case where the LUN's maximum transfer length is exceeded, which
is why it is based on the CDB rather than
InTransferLength/OutTransferLength.  However, in this case you cannot
expect EFI_BAD_BUFFER_SIZE to be set, because it's not the controller
driver's business to inspect target status and sense data (it's called
"passthru driver" for a reason :)).

Thus, the best you can do is to always back off to smaller sector sizes
when you get an unexpected error, like you did in r15491.

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

Re: [edk2] [Xen-devel] OVMF/Xen, Debian wheezy can't boot with NX on stack (Was: Re: [PATCH] OvmfPkg: prevent code execution from DXE stack)

[Paolo Bonzini]

On 09/09/2015 13:07, Ian Campbell wrote:
> I have a question: What attack vector is setting the stack as Nx in OVMF
> (or even UEFI generally) trying to protect against? Or is this being done
> for a reason other than security?
> 
> I understand why it is done for kernels and apps, but where does the
> untrusted element which is being protected against come from when running
> UEFI?

I guess something could attack shim.efi or GRUB, and subvert secure
boot's chain of trust.

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

Re: [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Paolo Bonzini]

On 10/09/2015 16:24, Kevin Davis wrote:
> Further leading me to guess that any actual use of those
> implementations could lead to you actually needing to hire a real
> attorney and not one that you find on YouTube.

The good thing is that attorneys have already figured it out.  IBM
figured out a few years ago how to work around Microsoft's patents, and
that's how FAT32 (and more specifically long file names) are implemented
in Linux.

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

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

[Paolo Bonzini]

On 01/10/2015 16:12, Janusz wrote:
> Now, I can also add, that the problem is only when I allow VM to use
> more than one core, so with option  for example:
> -smp 8,cores=4,threads=2,sockets=1 and other combinations like -smp
> 4,threads=1 its not working, and without it I am always running VM
> without problems
> 
> Any ideas what can it be? or any idea what would help to find out what
> is causing this?

I am going to send a revert of the patch tomorrow.

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

Re: [edk2] TFTP issue, Ping 1st packet reply missing

[Paolo Bonzini]

On 28/09/2015 12:24, Leekha Shaveta wrote:
> Hi Laszlo,
> 
> Please find the detailed "tcpdump" on my setup:

You still aren't following Laszlo's instructions: "your dump is missing
the DHCP packets that should be captured while the ifconfig program
(launched from the UEFI shell) brings up the interface on which you
later use TFTP"

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

Re: [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Paolo Bonzini]

Well, FatPkg is only superficially permissive and not even open source, so 
there is a precedent. (A precedent that, I might add, happens to violate 
SourceForge's the off service).

When we import edk2 into Fedora we just remove FatBinPkg. We would think twice 
before contributing to it, but do not make any kind of fuss about it.

GPL is just the same. For example, it would be possible to have an 
automatically-updated git repo that omits the GPL directory; and development 
would then be easier for people whose legal departments tend not to influence 
the engineers' productivity.

In fact:

1) it is not like, among non-Intel contributors, proprietary software companies 
have the lion's share of edk2 commits, and they probably use Tiano releases. 
Intel could strip any GPL pieces as part of the Tiano release process.

2) the GPL is working just fine for Linux, which is not that different from 
UEFI. So, picture me skeptical. If anything, what Linux can teach edk2 is that 
a closed prices and balkanized trees are a direct cause of the abysmal security 
of those implementations.

Paolo

-Original Message-
From: El-Haj-Mahmoud, Samer [samer.el-haj-mahm...@hpe.com]
Received: mercoledì, 09 set 2015, 21:12
To: Jordan Justen [jordan.l.jus...@intel.com]; Andrew Fish [af...@apple.com]
CC: Lenny Szubowicz [lenn...@redhat.com]; Karen Noel [kn...@redhat.com]; Ard 
Biesheuvel [ard.biesheu...@linaro.org]; edk2-devel-01 [edk2-de...@ml01.01.org]; 
Reza Jelveh [reza.jel...@tuhh.de]; Alexander Graf [ag...@suse.de]; qemu devel 
list [qemu-de...@nongnu.org]; Hannes Reinecke [h...@suse.de]; Gabriel L. Somlo 
(GMail) [gso...@gmail.com]; Peter Jones [pjo...@redhat.com]; Peter Batard 
[p...@akeo.ie]; Gerd Hoffmann [kra...@redhat.com]; Cole Robinson 
[crobi...@redhat.com]; Paolo Bonzini [pbonz...@redhat.com]; 
xen-de...@lists.xen.org [xen-de...@lists.xen.org]; Laszlo Ersek 
[ler...@redhat.com]; Ademar de Souza Reis Jr. [ar...@redhat.com]
Subject: RE: [edk2] EDK II & GPL - Re:  OVMF BoF @ KVM Forum 2015

The recent expansions beyond BSD where all permissive licenses (BSD like) as 
far as I can tell.

I agree with Andrew, opening the door for GPL licensed code in EDK2 will have 
severe consequences for products that are built using EDK2. 



-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan 
Justen
Sent: Wednesday, September 09, 2015 12:58 PM
To: Andrew Fish <af...@apple.com>
Cc: Lenny Szubowicz <lenn...@redhat.com>; Karen Noel <kn...@redhat.com>; Ard 
Biesheuvel <ard.biesheu...@linaro.org>; edk2-devel-01 
<edk2-devel@lists.01.org>; Reza Jelveh <reza.jel...@tuhh.de>; Alexander Graf 
<ag...@suse.de>; qemu devel list <qemu-de...@nongnu.org>; Hannes Reinecke 
<h...@suse.de>; Gabriel L. Somlo (GMail) <gso...@gmail.com>; Peter Jones 
<pjo...@redhat.com>; Peter Batard <p...@akeo.ie>; Gerd Hoffmann 
<kra...@redhat.com>; Cole Robinson <crobi...@redhat.com>; Paolo Bonzini 
<pbonz...@redhat.com>; xen-de...@lists.xen.org; Laszlo Ersek 
<ler...@redhat.com>; Ademar de Souza Reis Jr. <ar...@redhat.com>
Subject: Re: [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

On 2015-09-09 10:04:50, Andrew Fish wrote:
> 
> > On Sep 9, 2015, at 9:17 AM, Jordan Justen <jordan.l.jus...@intel.com> wrote:
> > 
> > So, related to this, I wonder how the community would feel about a 
> > GplDriverPkg. Would the community allow it as a new package in EDK 
> > II directly, or would a separate repo be required?
> > 
> 
> I think we would need a separate repo, like the FAT driver. That is 
> the only way to deal with the license issues.

There doesn't seem to be any guiding rules here. For example, I think some 
people are not comfortable with the FatBinPkg being in the tree due to the 
license. Why is that okay?

> > With regards to adding it directly into the EDK II tree, here are 
> > some potential concerns that I might anticipate hearing from the community:
> > 
> > * It will make it easier for contributors to choose GPL compared to 
> > a  permissive license, thereby limiting some users of the 
> > contribution
> > 
> > * GPL code will more easily be copied into the permissively licensed  
> > packages
> > 
> > * Some might refuse to look at EDK II entirely if it has a directory  
> > with GPL source code in it
> > 
> 
> Or have their rights to contribute revoked since this is a fundamental 
> change, and would require employees to get reauthorized by their legal 
> departments to contribute.

We've recently expanded beyond just allowing BSD code into the tree, and that 
appeared to be no big deal. No one brought this up as a fundamental change.

Just to be clear, are you saying Apple probably won't be able to contribute to 
EDK II if there is a

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

[Paolo Bonzini]

On 18/09/2015 11:37, Janusz wrote:
> Hello,
> 
> I am writting about this patch that was posted by Xiao:
> http://www.spinics.net/lists/kvm/msg119044.html and this:
> http://www.spinics.net/lists/kvm/msg119045.html
> I've tested both kernel 4.2 and 4.3 and problem still exists when I use
> OVMF - 100% cpu usage, VM resetting, while it works properly on kernel 4.1

Is this an Intel or AMD CPU?

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

Re: [edk2] [PATCH v4 05/10] OvmfPkg: SataControllerDxe: enable IO / mem access and DMA when binding

[Paolo Bonzini]

On 16/09/2015 04:57, Laszlo Ersek wrote:
> (OvmfPkg's copy of SataControllerDxe must differ from the same in DuetPkg
> because Duet inherits a pre-configured SATA controller from the BIOS, as
> explained by Feng.)

It's not true that it _must_ differ.  Duet _may_ avoid setting the bits
because it's using the pre-configured SATA controller, but it also _may_
set them.

Definitely both Windows and Linux set the bits on their own, for example.

In fact if the BIOS doesn't have an AHCI driver (you can try with
SeaBIOS) Duet would need exactly the same patches 5 and 6.

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

Re: [edk2] [PATCH] BaseTools GCC: avoid the use of COMMON symbols

[Paolo Bonzini]

On 04/12/2015 11:39, Laszlo Ersek wrote:
> (4) Linking those two files into a complete program is a violation of
> "6.7 External definitions":
> 
> [...] If an identifier declared with external linkage is used in an
> expression (other than as part of the operand of a *sizeof*
> operator), somewhere in the entire program there shall be exactly
> one external definition for the identifier [...]
> 
> Again, how does the argument go?
> 
> - In each file we have exactly one tentative definition, and no
>   declaration that would be, in its own right, an external definition.
>   Based on (2), the files must behave exactly as-if there was one
>   external *definition* for "x" in each.
> 
> - Argument (3) explains why "x" has external *linkage*.
> 
> - Argument (4) applies to "x" because "x" has external linkage, and is
>   used in a non-sizeof expression. And the requirement set forth in (4)
>   is broken by the program because the program is required to behave
>   exactly as if "x" had two external definitions.
> 
> In practical terms:
> 
> - If you compile the first version of the program (without any special
>   options), it links together successfully:
> 
>   $ gcc -std=c89 -pedantic -Wall -Wextra -o f f1.c f2.c
> 
> - If you compile the second (equivalent) version of the program,
>   linkage fails:
> 
>   $ gcc -std=c89 -pedantic -Wall -Wextra -o f-b f1-b.c f2-b.c
> 
>   /tmp/cc8Isbn8.o:(.bss+0x0): multiple definition of `x'
>   /tmp/cciQUlDz.o:(.bss+0x0): first defined here
>   collect2: error: ld returned 1 exit status
> 
> - The gcc bug is that linking the first version *too* should fail,
>   without any particular options.

That's true, but it would break existing code that declares variables in
headers without "extern".  That's why Visual Studio does the same.

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

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 12:16, Ni, Ruiyu wrote:
> Scott, I debugged the issue further and had the below findings: 
> According to the ACPI spec 6.0 5.2.9 Fixed ACPI Description Table
> (FADT), the FADT.Century can be set to 0 indicating the RTC doesn't
> support to store century value. But the Win7 boot loader
> hal!HalpInitializeCmos() will firstly read and save the FADT.Century
> to a 4-byte location pointed by hal!_HalpCmosCenturyOffset. When the
> FADT.Century is 0, it will save 0x32 (default value) to that
> location. Later hal!HalpReadCmosTime() skips to read the century
> value from CMOS when BIT 7 of the value poited by
> hal!_HalpCmosCenturyOffset is set. So in order to tell Windows that
> RTC doesn't support to store century, we need to set the FADT.Century
> to 0x80 other than 0. In summary, if FADT.Century is 0, Win7 boot
> loader reads century from 0x32; if FADT.Century & BIT7 != 0, it
> doesn't read century from CMOS; otherwise, it reads century from CMOS
> index FADT.Century.
> 
> But that caused another issue. Linux code strictly follows the ACPI
> spec which reads the century value from FADT.Century if it doesn't
> equal to zero. If we leave 0x80 in FADT.Century, Linux kernel will
> reads century from 0x80 location (actually from location 0 because
> CMOS address is 7-bit). Location 0 stores the seconds of the time
> which means Linux will read random century value.
> 
> So do you agree if a platform needs to support booting both Windows
> and Linux, it had better to set FADT.Century to 0x32 and save correct
> century value (0x20) to CMOS address 0x32?

That's fair enough, but you should not use RTC_ADDRESS_CENTURY
unconditionally in PcRtcSetTime.  Instead you should read the FADT
yourself and use the FADT.Century value if it is non-zero.  If it is
zero, I suppose writing the century to 0x32 is the only thing you can do.

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

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 18:37, Laszlo Ersek wrote:
> - A DXE driver that runs before *both* the ACPI platform DXE driver, and
> this runtime DXE driver -- to be ordered by any means necessary --, *or*
> a PEIM, sets a dynamic PCD that keys off *both* the ACPI platform DXE
> driver and this runtime DXE driver. I think this is the best approach.

Yes, replacing RTC_ADDRESS_CENTURY with a PCD sounds like a very good idea.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 09/12/2015 18:11, Kinney, Michael D wrote:
> Paolo,
> 
> I agree SetTime() is not called in very many places.  But since the
> SetTime() service is added to Runtime Services Table when the RTC
> driver runs, the logic in SetTime() must be implemented to handle
> case where SetTime() is called before ACPI Tables are published.

Right, but it can be implemented by simply skipping the century byte
(it's dead code anyway...).

Similarly, GetTime() could use the century byte as soon as ACPI tables
are published.

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

Re: [edk2] [patch 2/2] UefiCpuPkg/PiSmmCpu: Always set WP in CR0.

[Paolo Bonzini]

On 27/11/2015 02:14, Yao, Jiewen wrote:
> [Jiewen] Do you mean KVM reject SMM write BIT16 of CR0 ? It is odd,
> because my patch sets W+P bit page table entries.

That's odd indeed.  All common OSes run with CR0.WP=1.  I'll try to
reproduce...

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

Re: [edk2] [patch 1/2] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.

[Paolo Bonzini]

On 27/11/2015 15:21, Laszlo Ersek wrote:
> On 11/27/15 15:07, Yao, Jiewen wrote:
>> > So quick!
>> > Thank you very much to catch this!
> You'll get used to Paolo... the only reason he opens the SDM is not
> because he needs to look up the details. He remembers those. He looks at
> the SDM in order to find the containing chapter & section numbers for
> the details he already knows, so that others can look up the reference too.
> 
> ... That's my impression anyway! ;)

Not really, but the next time you get a failed vmentry, check out the
VMCS dump in dmesg.  All the information you need is there, you "just"
have to check it against the list in the SDM.

In this case, CR0.WP was a red herring, as the same mov to cr0 was
setting CR0.PG as well.

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

Re: [edk2] [patch 1/2] UefiCpuPkg/PiSmmCpu: Always set RW+P bit for page table by default.

[Paolo Bonzini]

On 25/11/2015 13:34, jiewen yao wrote:
> @@ -785,7 +785,7 @@ Gen4GPageTable (
>// Set Page Directory Pointers
>//
>for (Index = 0; Index < 4; Index++) {
> -Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + IA32_PG_P;
> +Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + 
> PAGE_ATTRIBUTE_BITS;
>}
>Pte += EFI_PAGE_SIZE / sizeof (*Pte);
>  

For PAE you must not set bit 1 here.  See the fix I've posted, it can be
included directly in this patch when you repost (so you still have two
patches, "Always set RW+P bit" and "Always set WP").

Thanks,

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

[edk2] [PATCH] UefiCpuPkg/PiSmmCpu: fix generation of 32-bit PAE page tables

[Paolo Bonzini]

Bits 1 and 2 are reserved in 32-bit PAE Page Directory Pointer Table Entries
(PDPTEs); see Table 4-8 in the SDM.  With VMX extended page tables, the
processor notices and fails the VM entry as soon as CR0.PG is set to 1.

Fixes commit "UefiCpuPkg/PiSmmCpu: Always set WP in CR0."

Reported-by: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Yao, Jiewen <jiewen@intel.com>
Cc: Michael D Kinney <michael.d.kin...@intel.com>
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c|  2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c |  2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c   |  8 ++--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h  | 10 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c |  2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c  |  2 +-
 6 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
index edebaab..5d29904 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
@@ -60,7 +60,7 @@ SmmInitPageTable (
   if (FeaturePcdGet (PcdCpuSmmStackGuard)) {
 InitializeIDTSmmStackGuard ();
   }
-  return Gen4GPageTable (0);
+  return Gen4GPageTable (0, TRUE);
 }
 
 /**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
index 85756d0..767cb69 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c
@@ -24,7 +24,7 @@ InitSmmS3Cr3 (
   VOID
   )
 {
-  mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0);
+  mSmmS3ResumeState->SmmS3Cr3 = Gen4GPageTable (0, TRUE);
 
   return ;
 }
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 99d03c4..df3ad68 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -732,12 +732,14 @@ APHandler (
   Create 4G PageTable in SMRAM.
 
   @param  ExtraPages   Additional page numbers besides for 4G 
memory
+  @param  Is32BitPageTable Whether the page table is 32-bit PAE
   @return PageTable Address
 
 **/
 UINT32
 Gen4GPageTable (
-  IN  UINTN ExtraPages
+  IN  UINTN ExtraPages,
+  IN  BOOLEAN   Is32BitPageTable
   )
 {
   VOID*PageTable;
@@ -785,7 +787,9 @@ Gen4GPageTable (
   // Set Page Directory Pointers
   //
   for (Index = 0; Index < 4; Index++) {
-Pte[Index] = (UINTN)PageTable + EFI_PAGE_SIZE * (Index + 1) + 
PAGE_ATTRIBUTE_BITS;
+Pte[Index] = (UINTN)PageTable
+   + EFI_PAGE_SIZE * (Index + 1)
+   + (Is32BitPageTable ? PAE_PDPTE_ATTRIBUTE_BITS : 
PAGE_ATTRIBUTE_BITS);
   }
   Pte += EFI_PAGE_SIZE / sizeof (*Pte);
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 133165d..764189d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -85,6 +85,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #define PAGE_ATTRIBUTE_BITS (IA32_PG_RW | IA32_PG_P)
 
 //
+// Bits 1 and 2 are reserved in the PAE PDPTE
+//
+#define PAE_PDPTE_ATTRIBUTE_BITS(IA32_PG_P)
+
+
+//
 // Size of Task-State Segment defined in IA32 Manual
 //
 #define TSS_SIZE  104
@@ -368,12 +374,14 @@ extern IA32_DESCRIPTOR gcSmiInitGdtr;
   Create 4G PageTable in SMRAM.
 
   @param  ExtraPages   Additional page numbers besides for 4G 
memory
+  @param  Is32BitPageTable Whether the page table is 32-bit PAE
   @return PageTable Address
 
 **/
 UINT32
 Gen4GPageTable (
-  IN  UINTN ExtraPages
+  IN  UINTN ExtraPages,
+  IN  BOOLEAN   Is32BitPageTable
   );
 
 
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index d242e06..5b11e5e 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -113,7 +113,7 @@ SmmInitPageTable (
   //
   // Generate PAE page table for the first 4GB memory space
   //
-  Pages = Gen4GPageTable (PAGE_TABLE_PAGES + 1);
+  Pages = Gen4GPageTable (PAGE_TABLE_PAGES + 1, FALSE);
 
   //
   // Set IA32_PG_PMNT bit to mask this entry
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c
index b3cd629..79e23ef 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c
@@ -45,7 +45,7 @@ InitSmmS3Cr3 (
   //
   // Generate PAE page table for the first 4GB memory space
   //
-  Pages = Gen4GPageTable (1);
+  Pages = Gen4GPageTable (1, FALSE);
 
   //
   // Fi

Re: [edk2] please DO NOT commit unreviewed patches to subversion!

[Paolo Bonzini]

On 27/11/2015 13:07, Laszlo Ersek wrote:
> On 11/27/15 12:31, Yao, Jiewen wrote:
>> Hi Laszlo and Ard
>> First of all, I apologize the confusing brought.
>> You are right. I am still using SVN to commit patch, until it is finally 
>> moved to GIT. :-(
>> The patch is reviewed and I did adopt the suggestion from reviewer, before I 
>> finally commit it, after 24hours.
>> I am sorry that I did not aware the difference of commit between SVN and GIT.
>>
>> Laszlo
>> I am fine to revert them and commit in the way reviewed. I appreciate your 
>> help!
> 
> Thank you very much.
> 
> I have now reverted the two patches in question, in SVN.

Ouch, I'm an hour late.  I have posted a fix.

Jiewen, feel free to take the necessary bits of my fix and include them
in your patch.

Laszlo, you said you would take some time off, didn't you?

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

Re: [edk2] [Patch] PcAtChipsetPkg/Rtc: Fix a UEFI Win7 boot hang issue

[Paolo Bonzini]

On 18/11/2015 06:08, Zeng, Star wrote:
> 
> @@ -508,6 +509,7 @@ PcRtcSetTime (
> RtcWrite (RTC_ADDRESS_DAY_OF_THE_MONTH, RtcTime.Day);
> RtcWrite (RTC_ADDRESS_MONTH, RtcTime.Month);
> RtcWrite (RTC_ADDRESS_YEAR, (UINT8) RtcTime.Year);
> +  RtcWrite (RTC_ADDRESS_CENTURY, Century);

Should it be written only if the FADT CenturyOffset is zero?

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

Re: [edk2] a "strange" branch taken in the SMM fault handler in PiSmmCpuDxe

[Paolo Bonzini]

On 01/06/2016 12:50, Laszlo Ersek wrote:
> In other words, the fault is raised and delivered (== the handler is
> entered) entirely within non-root operation, inside the VM. I find this
> amazing. (Amazingly annoying.)

It is indeed.  You can try using ept=0 to see the page faults.

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

Re: [edk2] a "strange" branch taken in the SMM fault handler in PiSmmCpuDxe

[Paolo Bonzini]

On 01/06/2016 14:26, Laszlo Ersek wrote:
> On 06/01/16 13:30, Paolo Bonzini wrote:
>>
>>
>> On 01/06/2016 12:50, Laszlo Ersek wrote:
>>> In other words, the fault is raised and delivered (== the handler is
>>> entered) entirely within non-root operation, inside the VM. I find
>>> this amazing. (Amazingly annoying.)
>>
>> It is indeed.  You can try using ept=0 to see the page faults.
> 
> Given that it reproduces with TCG, I added the following option to the
> QEMU command line:
> 
>   -d int,pcall
> 
> It absolutely spams the output, but the last two state dumps preceding
> the fault are:
> 
> RIP (7feee48d) and CR2 (ffe00020) are consistent with the guest's own
> report:
> 
>> ~SmiPFHandler: PFAddress=0xFFE00020 Rip=0x7FEEE48D
>> PageTable = 7FFA7000, PTIndex = 1FF, PageTable[PTIndex] = 7FFF31BD
>> New page table overlapped with old page table!
>> ASSERT .../UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c(618): ((BOOLEAN)(0==1))
> 
> Is this useful information?

Yes, it is.  If the PT address is constant, perhaps you can attach gdb
to QEMU and set a hardware watchpoint on the PTE?

Paolo

> Error code 0xe is EXCP0E_PAGE, right? Maybe I can investigate it a bit
> more.
> 
> Hm, according to <http://wiki.osdev.org/Exceptions#Page_Fault> -- hey,
> why consult the Intel SDM when you can read wiki pages on the net,
> right? :) --, and assuming that "e=0009" is the error code, it seems we
> have:
> 
> - Present (bit #0, value 1): "When set, the page fault was caused by a
>   page-protection violation. When not set, it was caused by a
>   non-present page."
> 
> - Reserved write  (bit #3, value 8): "When set, the page fault was
>   caused by reading a 1 in a reserved field."
> 
> Does this imply that the (various level) page table entries that cover
> the varstore pflash chip get corrupted at some point? (How else would
> those "reserved bits" get set?)
> 
> This would be consistent with Jeff's statement
> 
>> PageTable[PTIndex] = 7FFF2251 is not one valid page entry.
>> Usually, we encountered the similar issue due to SMM page table room
>> was crashed.
> 
> (NB: this is the only invocation of SmiPFHandler() that I see in the
> OVMF log. Also, in this case, the PTE is 7FFF31BD.)
> 
> Thanks,
> Laszlo
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] KVM Forum 2016: Call For Participation

[Paolo Bonzini]

On 10/03/2016 19:09, Paolo Bonzini wrote:
> ===
> IMPORTANT DATES
> ===
> Notification: May 27, 2015

On behalf of the program committee, I apologize for the delay in sending
out the notifications.  If you need to know in advance whether your talk
has been accepted, please contact me offlist.

Thanks,

Paolo

> Schedule announced: June 3, 2015
> Event dates: August 24-26, 2016
> 
> Thank you for your interest in KVM. We're looking forward to your
> submissions and seeing you at the KVM Forum 2016 in August!
> 
> -your KVM Forum 2016 Program Committee
> 
> Please contact us with any questions or comments at
> kvm-forum-2016...@redhat.com
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] An unkempt git guide for edk2 contributors and maintainers

[Paolo Bonzini]

On 12/02/2016 02:09, Laszlo Ersek wrote:
> (23) Now we'll format the patches as email messages, and send them to
>  the list. Standing in the root of your edk2 directory, run the
>  following (note that the "-O" option needs customization: please
>  update the pathname to the file created in step (10)):
> 
>rm -f -- *.patch
> 
>git format-patch   \
>  --notes  \
>  -O"/fully/qualified/path/to/edk2.diff.order" \
>  --cover-letter   \
>  --numbered   \
>  --subject-prefix="PATCH v1"  \
>  --stat=1000  \
>  --stat-graph-width=20\
>master..implement_foo_for_bar_v1

You can add

   git config diff.orderFile "/fully/qualified/path/to/edk2.diff.order"

to step (10) instead of adding -O.

Also you can add

   git config --global alias.format-series "format-patch --notes --cover-letter 
--numbered --stat=1000 --stat-graph-width=20"

to step (05) and simplify the above to

   git format-series --subject-prefix="PATCH v1" master..

> 
>  git send-email \
>--suppress-cc=author \
>--suppress-cc=self   \
>--suppress-cc=cc \
>--suppress-cc=sob\
>--to=edk2-devel@lists.01.org \
>*.patch

Same here.  You can add more "git config" commands to step (05),
particularly

   git config send-email.to = edk2-devel@lists.01.org

and simplify this to

   git send-email *.patch

(Not sure why you're adding the Why the "--suppress-cc").

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

Re: [edk2] [patch 1/2] MdeModulePkg/Ide: return correct status when DRQ is not ready for ATAPI

[Paolo Bonzini]

On 19/01/2016 00:29, Laszlo Ersek wrote:
>> INQUIRY works slightly different from other commands. It has a maximum
>> length instead of a deterministic length; once HCYL/LCYL are read back,
>> should RequiredBytes not be adjusted in this case? (This way we don't
>> short the loop, we just finish gracefully.)

I like John's suggestion, and it can be applied to _any_ command, not
just INQUIRY.

However, ByteCount should be changed to a UINT32* and updated by
AtaPacketReadWrite.  Then the ActualWordCount can be reflected into the
InTransferLength or OutTransferLength of struct
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.

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

[edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

[Paolo Bonzini]

For some SCSI commands, notably INQUIRY, it's relatively common for
the device to provide less data than we intended to read, and for
this reason EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET makes
InTransferLength and OutTransferLength read-write.  Make ATAPI
aware of this.

This makes it possible to handle EFI_NOT_READY always, not just
for read as done in r19685.

I've chosen to use a break statement instead of calling
CheckStatusRegister directly; the break statement reaches a
pre-existing call the CheckStatusRegister function.  This
ensures that the assignment to *ByteCount is not missed, and
adds a further sanity check to DRQClear.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
***UNTESTED***

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 32 ++---
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 320ee90..6478f7b 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -1936,7 +1936,7 @@ AtaPacketReadWrite (
   IN EFI_PCI_IO_PROTOCOL   *PciIo,
   IN EFI_IDE_REGISTERS *IdeRegisters,
   IN OUT VOID  *Buffer,
-  IN UINT64ByteCount,
+  IN OUT UINT32*ByteCount,
   IN BOOLEAN   Read,
   IN UINT64Timeout
   )
@@ -1947,17 +1947,18 @@ AtaPacketReadWrite (
   EFI_STATUS  Status;
   UINT16  *PtrBuffer;
 
+  PtrBuffer = Buffer;
+  RequiredWordCount = *ByteCount >> 1;
+
   //
   // No data transfer is premitted.
   //
-  if (ByteCount == 0) {
+  if (RequiredWordCount == 0) {
 return EFI_SUCCESS;
   }
 
-  PtrBuffer = Buffer;
-  RequiredWordCount = (UINT32)RShiftU64(ByteCount, 1);
   //
-  // ActuralWordCount means the word count of data really transferred.
+  // ActualWordCount means the word count of data really transferred.
   //
   ActualWordCount = 0;
 
@@ -1967,14 +1968,16 @@ AtaPacketReadWrite (
 // to see whether indicates device is ready to transfer data.
 //
 Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
-if ((Status == EFI_NOT_READY) && Read) {
-  //
-  // Device provided less data than we intended to read -- exit early.
-  //
-  return CheckStatusRegister (PciIo, IdeRegisters);
-}
 if (EFI_ERROR (Status)) {
-  return Status;
+  if (Status == EFI_NOT_READY) {
+//
+// Device provided less data than we intended to read, or wanted less
+// data than we intended to write, but it may still be successful.
+//
+break;
+  } else {
+return Status;
+  }
 }
 
 //
@@ -2040,6 +2043,7 @@ AtaPacketReadWrite (
 return EFI_DEVICE_ERROR;
   }
 
+  *ByteCount = ActualWordCount << 1;
   return Status;
 }
 
@@ -2138,7 +2142,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->InDataBuffer,
-   Packet->InTransferLength,
+   >InTransferLength,
TRUE,
Packet->Timeout
);
@@ -2147,7 +2151,7 @@ AtaPacketCommandExecute (
PciIo,
IdeRegisters,
Packet->OutDataBuffer,
-   Packet->OutTransferLength,
+   >OutTransferLength,
FALSE,
Packet->Timeout
);
-- 
2.5.0
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/.../IdeMode: correctly report length of returned data

[Paolo Bonzini]

On 21/01/2016 02:27, Tian, Feng wrote:
> Paolo,
> 
> I think for short write case it means the data length to be written in 
> AtaPacketReadWrite, that is ByteCount, is less than the one shipped in ATA 
> cmd, for example, CDB (READ10.byte7&8).
> 
> For such case, it should jump out the while loop in AtaPacketReadWrite and 
> send EFI_DEVICE_ERROR as DRQ is not clear.
> 
> IMHO, Laszo & your patch all looks ok to me. of course, your patch did an 
> enhancement on how to reflect the real transfer data length :) 

Thanks!  Laszlo, can you commit it?

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

[edk2] KVM Forum 2016: Call For Participation

[Paolo Bonzini]

=
KVM Forum 2016: Call For Participation
August 24-26, 2016 - Westin Harbor Castle - Toronto, Canada

(All submissions must be received before midnight May 1, 2016)
=

KVM Forum is an annual event that presents a rare opportunity
for developers and users to meet, discuss the state of Linux
virtualization technology, and plan for the challenges ahead. 
We invite you to lead part of the discussion by submitting a speaking
proposal for KVM Forum 2016.

At this highly technical conference, developers driving innovation
in the KVM virtualization stack (Linux, KVM, QEMU, libvirt) can
meet users who depend on KVM as part of their offerings, or to
power their data centers and clouds.

KVM Forum will include sessions on the state of the KVM
virtualization stack, planning for the future, and many
opportunities for attendees to collaborate. As we celebrate ten years
of KVM development in the Linux kernel, KVM continues to be a
critical part of the FOSS cloud infrastructure.

This year, KVM Forum is joining LinuxCon and ContainerCon in Toronto, 
Canada. Selected talks from KVM Forum will be presented on Wednesday
August 24 to the full audience of LinuxCon and ContainerCon. Also,
attendees of KVM Forum will have access to all of the LinuxCon and
ContainerCon talks on Wednesday.

http://events.linuxfoundation.org/cfp

Suggested topics:

KVM and Linux
* Scaling and optimizations
* Nested virtualization
* Linux kernel performance improvements
* Resource management (CPU, I/O, memory)
* Hardening and security
* VFIO: SR-IOV, GPU, platform device assignment
* Architecture ports

QEMU
* Management interfaces: QOM and QMP
* New devices, new boards, new architectures
* Scaling and optimizations
* Desktop virtualization and SPICE
* Virtual GPU
* virtio and vhost, including non-Linux or non-virtualized uses
* Hardening and security
* New storage features
* Live migration and fault tolerance
* High availability and continuous backup
* Real-time guest support
* Emulation and TCG
* Firmware: ACPI, UEFI, coreboot, u-Boot, etc.
* Testing

Management and infrastructure
* Managing KVM: Libvirt, OpenStack, oVirt, etc.
* Storage: glusterfs, Ceph, etc.
* Software defined networking: Open vSwitch, OpenDaylight, etc.
* Network Function Virtualization
* Security
* Provisioning
* Performance tuning


===
SUBMITTING YOUR PROPOSAL
===
Abstracts due: May 1, 2016

Please submit a short abstract (~150 words) describing your presentation
proposal. Slots vary in length up to 45 minutes. Also include the proposal
type -- one of:
- technical talk
- end-user talk

Submit your proposal here:
http://events.linuxfoundation.org/cfp
Please only use the categories "presentation" and "panel discussion"

You will receive a notification whether or not your presentation proposal
was accepted by May 27, 2016.

Speakers will receive a complimentary pass for the event. In the instance
that your submission has multiple presenters, only the primary speaker for a
proposal will receive a complementary event pass. For panel discussions, all
panelists will receive a complimentary event pass.

TECHNICAL TALKS

A good technical talk should not just report on what has happened over
the last year; it should present a concrete problem and how it impacts
the user and/or developer community. Whenever applicable, focus on
work that needs to be done, difficulties that haven't yet been solved,
and on decisions that other developers should be aware of. Summarizing
recent developments is okay but it should not be more than a small
portion of the overall talk.

END-USER TALKS

One of the big challenges as developers is to know what, where and how
people actually use our software. We will reserve a few slots for end
users talking about their deployment challenges and achievements.

If you are using KVM in production you are encouraged submit a speaking
proposal. Simply mark it as an end-user talk. As an end user, this is a
unique opportunity to get your input to developers.

HANDS-ON / BOF SESSIONS

We will reserve some time for people to get together and discuss
strategic decisions as well as other topics that are best solved within
smaller groups.

These sessions will be announced during the event. If you are interested
in organizing such a session, please add it to the list at

  http://www.linux-kvm.org/page/KVM_Forum_2016_BOF

Let people you think might be interested know about it, and encourage
them to add their names to the wiki page as well. Please try to
add your ideas to the list before KVM Forum starts.


PANEL DISCUSSIONS

If you are proposing a panel discussion, please make sure that you list
all of your potential panelists in your abstract. We will request full
biographies if a panel is accepted.


===
HOTEL / TRAVEL
===

This year's event will take place at the Westin Harbour Castle Toronto.
For 

Re: [edk2] Software SMI STS bit is not set when writing port B2 in QEMU Q35

[Paolo Bonzini]

On 15/03/2016 10:10, Ni, Ruiyu wrote:
> Paolo, Laszlo,
> As I mentioned in previous mail, the EAX I got from CpuSaveState
> is different from what I set before entering SMM.
> Because the failure was seen in a QEMU launched in Windows
> using the following command:
> qemu-system-x86_64.exe \
>   -machine q35,smm=on,accel=tcg \
>   -smp 1 \
>   -drive if=pflash,format=raw,unit=0,file=OVMF_CODE.fd,readonly=on \
>   -drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd \
>   --serial COM5
> 
> 
> I guess you should use Linux to run QEMU. So I switched to Ubuntu 14.04.

There should be no difference between Linux and Windows (only between
TCG and KVM).

> 1. Upgraded the kernel to 4.4.1.
> 2. Download the QEMU 2.5 source and make
> 3. run the following command:
> qemu-system-x86_64 \
>-display none \
>-machine q35,smm=on,accel=kvm \
>-global driver=cfi.pflash01,property=secure,value=on \
>-drive 
> if=pflash,format=raw,unit=0,file=Build/Ovmf3264/DEBUG_GCC49/FV/OVMF_CODE.fd,readonly=on
>  \
>-drive 
> if=pflash,format=raw,unit=1,file=Build/Ovmf3264/DEBUG_GCC49/FV/OVMF_VARS.fd \
>--serial file:ovmf.log

I'm not sure.  The above command line works for me after building OVMF
like this:

build -p OvmfPkg/OvmfPkgIa32X64.dsc -a IA32 -a X64 -b DEBUG -t GCC49 -n 4

I'm using commit 89a8115 ("BaseTools: Support recent versions of cx_freeze.",
2016-03-15) of OVMF.

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

Re: [edk2] Software SMI STS bit is not set when writing port B2 in QEMU Q35

[Paolo Bonzini]

On 15/03/2016 12:59, Ni, Ruiyu wrote:
>> > I'm not sure.  The above command line works for me after building OVMF
>> > like this:
>> > 
>> >build -p OvmfPkg/OvmfPkgIa32X64.dsc -a IA32 -a X64 -b DEBUG -t GCC49 -n 
>> > 4
>> > 
>> > I'm using commit 89a8115 ("BaseTools: Support recent versions of 
>> > cx_freeze.",
>> > 2016-03-15) of OVMF.
> I supplied additional flag when build: 
> -D CSM_ENABLE=TRUE 
> -D SMM_REQUIRE=TRUE
> I guess the second flag matters.

I also tested with -D SMM_REQUIRE and it works.

Can you send me your Csm16.bin file (offlist)?

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

Re: [edk2] Software SMI STS bit is not set when writing port B2 in QEMU Q35

[Paolo Bonzini]

On 15/03/2016 16:48, Ni, Ruiyu wrote:
> I don't think CSM matters and the bin I am using cannot be
> distributed. Does the qemu build steps matters? I ran configure
> --target-list=x86_64-softmmu. I traced the code and found the code
> hung when SMM is relocating. The code was waiting for mRebased flag
> be set.

First of all, can you reproduce the problem without CSM?

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

Re: [edk2] Software SMI STS bit is not set when writing port B2 in QEMU Q35

[Paolo Bonzini]

On 14/03/2016 09:18, Ni, Ruiyu wrote:
> I tried to hook a software SMI (triggered by B2) but the handler/callback
> was never called.
> 
> I know that when booting to ACPI OS, OS writes to B2 with certain value
> to tell firmware to enable SCI. That is achieved through the software SMI.
> The software SMI handler gets called when a certain value is written to B2.
> I am wondering how that is implemented in OVMF.

Because of the historical lack of SMI support in the firmware, this was
implemented directly in QEMU.  In the case of Q35:

static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
{
ICH9LPCState *lpc = arg;

/* ACPI specs 3.0, 4.7.2.5 */
acpi_pm1_cnt_update(>pm.acpi_regs,
val == ICH9_APM_ACPI_ENABLE,
val == ICH9_APM_ACPI_DISABLE);
if (val == ICH9_APM_ACPI_ENABLE || val == ICH9_APM_ACPI_DISABLE) {
return;
}

/* SMI_EN = PMBASE + 30. SMI control and enable register */
if (lpc->pm.smi_en & ICH9_PMIO_SMI_EN_APMC_EN) {
cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
}
}

There are other parts of SMI support where QEMU is incomplete compared
to a real chipset, most notably EOS.

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

Re: [edk2] Software SMI STS bit is not set when writing port B2 in QEMU Q35

[Paolo Bonzini]

On 14/03/2016 10:51, Ni, Ruiyu wrote:
> 
> The layout of CpuSaveState is different from what is described in
> Intel IA32 manual. Seems QEMU specific.
> The CpuSaveState pointer is correct.
> I dumped the CpuSaveState content. The SMMBase and SMMRevId
> is correct. But EAX is incorrect.

I have already explained many times that the different CpuSaveState
layout is because Intel refuses to document in the SDM the _actual_
contents of the SMM save state area, most notably the placement of the
descriptor cache registers.  Since AMD's documentation is crystal clear
(except that it's partly split between the programmer's manual and the
BIOS/kernel writer manual), we went with the AMD format.

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

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[Paolo Bonzini]

On 18/03/2016 22:53, Laszlo Ersek wrote:
> > The correct character to use in that situation is the emdash, If you
> > *absolutely* must, then rewrite the whole sentence to avoid using it.
> > Do *not* replace it with hyphens.
> 
> Okay. I've googled the use of emdash in the English language, and it
> seems to be more or less interchangeable with parens. Is that okay?

Actually there is a good reason to replace the emdash with hyphens, and
the reason is that the emdash looks like crap (to me at least) in
monospaced fonts.

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

Re: [edk2] [PATCH 00/62] Add FatPkg with 2-clause BSD license

[Paolo Bonzini]

On 30/03/2016 10:42, Laszlo Ersek wrote:
>> > Contributed-under: TianoCore Contribution Agreement 1.0
>> > Signed-off-by: Jordan Justen 
> This is huge. It will enable Fedora to ship OvmfPkg and ArmVirtPkg
> builds. It will enable RHEL to ship OVMF in Main.
> 
> Of course other GNU/Linux distros will benefit similarly. For example,
> this should fix Debian bug
>  too, so I'm
> CC'ing the commenters on that bug as well.
> 
> Mark, Jordan, and everyone else who made this happen -- thank you.

Echoed!  Thanks to all involved.

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 18/05/2016 01:57, Kinney, Michael D wrote:
>   Core
> CorebootModulePkg
> CorebootPayloadPkg

I think that anything with a .fdf file should be under Platform.
CorebootPayloadPkg is the only outlier in your proposal.

> Emulated
>   DuetPkg
>   EmulatorPkg
>   Nt32Pkg
>   OvmfPkg

I think OvmfPkg is not emulated; certainly not in the same sense as
EmulatorPkg, Nt32 or UnixPkg.  DuetPkg also seems more similar to
OvmfPkg than to EmulatorPkg (and definitely most similar to
CorebootPayloadPkg, which should be under Platform according to the rule
I proposed above).

In addition, I think that separation by architecture is more useful than
separation by vendor.  This yields the following:

Platform
  Arm
ArmPlatformPkg
ArmVirtPkg
BeagleBoardPkg
  Emulated
EmulatorPkg
Nt32Pkg
UnixPkg
  IA32X64
CorebootPayloadPkg
DuetPkg
Intel
  QuarkPlatformPkg
  Vlv2TbltDevicePkg
OvmfPkg

IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
only.

In addition, I am not sure about the separation between "Drivers" and
"Silicon".  My proposal here is to unify them as follows:

Drivers
  FatPkg
  NetworkPkg
  OptionRomPkg
  Arm
ArmPkg
Omap35xxPkg
  IA32X64
PcAtChipsetPkg
QuarkSocPkg
UefiCpuPkg
Vlv2DeviceRefCodePkg

or alternatively Omap35xxPkg, QuarkSocPkg and Vlv2DeviceRefCode could
move under Drivers/Vendor/{Arm,Intel}.

Thanks,

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 18:03, Ryan Harkin wrote:
> > IA32X64 is not a great name, but neither is Intel.  X86 suggests 32-bit
> > only.
>
> I prefer the idea of separating by vendor.  One vendor may have
> multiple architectures, for example.

That's exactly why I want to separate by architecture. :)

When doing a change across the entire tree, it's way more common to care
about 'all ARM boards' than 'all SoCs from Qualcomm'.

Thanks,

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 18:21, Kinney, Michael D wrote:
> This is one of the reasons I wanted to have both a "Silicon" and a "Driver" 
> top level directory.
> 
> We can change names, but the idea is that the "Silicon" one would contains 
> CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
> complex, turn on caches, enable memory subsystems, and do basic init of the 
> I/O subsystems built into the CPU/Chipset/SoC.
> 
> The "Drivers" area is for modules that manage hardware and peripherals that 
> attach to standard I/O subsystems such as PCI, USB, I2C, etc.

Okay, that makes sense.  Indeed they are different, for example outside
Silicon/ the architecture is not particularly important.

At the same time, with Drivers/ reduced to just three packages, I wonder
if it would make sense to move FatPkg and NetworkPkg to Core/, and
OptionRomPkg to Silicon/.  Most drivers are part of MdeModulePkg, which
is in Core/.

For Silicon/, I think //FooPkg (which can be
reduced to /FooPkg or even just FooPkg) can be more useful
than Vendor//FooPkg.  This would give

Silicon/
  Arm/   (architecture)
ArmPkg
ArmPlatformPkg
ArmVirtPkg
Arm/ (vendor)
  ArmJunoPkg (currently in ArmPlatformPkg/ArmJunoPkg)
  ArmVExpressPkg (currently in ArmPlatformPkg/ArmVExpressPkg)
  Omap35xxPkg
  IA32X64/
CorebootModulePkg
PcAtChipsetPkg
UefiCpuPkg
Intel/
  QuarkSocPkg
  Vlv2DeviceRefCodePkg
  OptionRomPkg

> If we have an area for CPU/Chipset/SoC, then we can use some directory paths 
> that clearly identify CPU architecture content as well as a Vendor specific 
> content.
> 
> I would hope we can concentrate the CPU architecture content that applies to
> all vendors in its own area, and only the vendor specific extensions go into 
> vendor specific directories.

This sounds good!

Thanks,

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

Re: [edk2] [RFC] Proposal to organize packages into directories

[Paolo Bonzini]

On 19/05/2016 20:21, Kinney, Michael D wrote:
> Paolo,
> 
> For this proposal, I am not only looking at existing packages/modules/libs, I 
> am also considering where new content might go over time.  For example, the
> MdeModulePkg has grown in size over the years.  The PEI Core, DXE Core, SMM
> Core content in MdeModulePkg is "Core" content, but much of the other content
> in that package would really fall into the "Drivers" category.  Instead of 
> adding more content to MdeModulePkg in the future, we should consider adding 
> it to "Drivers" instead.
> 
> There have also been questions on edk2-devel on where to add vendor specific
> drivers for devices/peripherals that attach to PCI/USB/I2C/etc. "Drivers" 
> seems like a better location than "Silicon".
> 
> We could even consider in the future figuring out ways to move some of the 
> content out packages like MdeModulePkg into "Drivers".

That would be even better. :)

Paolo

> So instead of removing "Drivers" from the proposal because there are only a
> couple packages in that category today, let's keep it and make sure we 
> add new content to the right directory going forward.
> 
> Mike
> 
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paolo 
>> Bonzini
>> Sent: Thursday, May 19, 2016 10:21 AM
>> To: Kinney, Michael D <michael.d.kin...@intel.com>; Ryan Harkin
>> <ryan.har...@linaro.org>
>> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
>> Subject: Re: [edk2] [RFC] Proposal to organize packages into directories
>>
>>
>>
>> On 19/05/2016 18:21, Kinney, Michael D wrote:
>>> This is one of the reasons I wanted to have both a "Silicon" and a "Driver"
>>> top level directory.
>>>
>>> We can change names, but the idea is that the "Silicon" one would contains
>>> CPU/Chipset/SoC content that is usually contains the drivers to init the CPU
>>> complex, turn on caches, enable memory subsystems, and do basic init of the
>>> I/O subsystems built into the CPU/Chipset/SoC.
>>>
>>> The "Drivers" area is for modules that manage hardware and peripherals that
>>> attach to standard I/O subsystems such as PCI, USB, I2C, etc.
>>
>> Okay, that makes sense.  Indeed they are different, for example outside
>> Silicon/ the architecture is not particularly important.
>>
>> At the same time, with Drivers/ reduced to just three packages, I wonder
>> if it would make sense to move FatPkg and NetworkPkg to Core/, and
>> OptionRomPkg to Silicon/.  Most drivers are part of MdeModulePkg, which
>> is in Core/.
>>
>> For Silicon/, I think //FooPkg (which can be
>> reduced to /FooPkg or even just FooPkg) can be more useful
>> than Vendor//FooPkg.  This would give
>>
>> Silicon/
>>   Arm/   (architecture)
>> ArmPkg
>> ArmPlatformPkg
>> ArmVirtPkg
>> Arm/ (vendor)
>>   ArmJunoPkg (currently in ArmPlatformPkg/ArmJunoPkg)
>>   ArmVExpressPkg (currently in ArmPlatformPkg/ArmVExpressPkg)
>>   Omap35xxPkg
>>   IA32X64/
>> CorebootModulePkg
>> PcAtChipsetPkg
>> UefiCpuPkg
>> Intel/
>>   QuarkSocPkg
>>   Vlv2DeviceRefCodePkg
>>   OptionRomPkg
>>
>>> If we have an area for CPU/Chipset/SoC, then we can use some directory paths
>>> that clearly identify CPU architecture content as well as a Vendor specific
>>> content.
>>>
>>> I would hope we can concentrate the CPU architecture content that applies to
>>> all vendors in its own area, and only the vendor specific extensions go into
>>> vendor specific directories.
>>
>> This sounds good!
>>
>> Thanks,
>>
>> 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] minimum NASM version

[Paolo Bonzini]

On 14/07/2016 19:11, Laszlo Ersek wrote:
> * I didn't say, but I also tried "mov ax, ds". The SDM writes, "The
>   upper 56 bits or 48 bits (respectively) of the destination
>   general-purpose register are not modified by the operation". In this
>   context, those bits were known to be zero, and I hoped that NASM 2.07
>   might support a 16->16 MOV.
> 
> I vaguely recall that I tried "mov eax, ds" too (it may have been my
> very first try, I don't remember any longer). I think "mov ax, ds" was
> my second try, and "movzx eax, ds" the third?...
> 
> However, none of these work, because NASM 2.07 (apparently) lacks the
> *general* ability to MOV from/to segment registers in 64-bit mode.

Ugh, this is so wrong. :)  I guess you could also use a macro that
expands to

bits 32
mov  src, dst
bits 64

because the encoding is the same in 32-bit and 64-bit.

>> In fact if you check the SDM only movsx supports 64-bit destinations,
>> while for movzx you just use a 32-bit destination.
> 
> MOV seems to support 16-bit segment register --> 64 bit register or memory:
> 
> Opcode InstructionOp/  64-Bit  Compat/
>   En   ModeLeg Mode
> -     ---  --  
> REX.W + 8C /r  MOV r/m64,Sreg**   AValid   Valid
> 
> Description: Move zero extended 16-bit segment register to r/m64.

Right, but for 64-bit registers it's pointless.

Paolo

> ** In 32-bit mode, the assembler may insert the 16-bit operand-size
>prefix with this instruction (see the following “Description”
>section for further information).
> 
> (I.e., I believe that the original NASM source code is correct.)
> 
> Thanks
> Laszlo
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] minimum NASM version

[Paolo Bonzini]

On 14/07/2016 13:19, Laszlo Ersek wrote:
> The problem is that NASM wouldn't support segment register MOVs in
> 64-bit mode until the following commit:
> 
>   http://repo.or.cz/nasm.git/commitdiff/21d4ccc3c338
> 
>   Wed, 25 Aug 2010 02:28:00 +0200 (24 17:28 -0700)
> 
> However, that change was first released in NASM 2.10:
> 
>   http://repo.or.cz/nasm.git/commitdiff/ff62f33da0a2
> 
>   Mon, 12 Mar 2012 21:36:02 +0100 (12 13:36 -0700)
> 
> I tried to work around this; for example by using
> 
>   movzx   eax, ds

You can just say "mov eax, ds" (and "mov ds, eax").  The reason is that
32-bit destination operands are automatically zero-extended to 64-bit.
In fact if you check the SDM only movsx supports 64-bit destinations,
while for movzx you just use a 32-bit destination.

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

Re: [edk2] minimum NASM version

[Paolo Bonzini]

On 14/07/2016 19:25, Laszlo Ersek wrote:
>> > Ugh, this is so wrong. :)  I guess you could also use a macro that
>> > expands to
>> > 
>> >bits 32
>> >mov  src, dst
>> >bits 64
>> > 
>> > because the encoding is the same in 32-bit and 64-bit.
> Nice trick :), but the point of using NASM would be to avoid such hacks,
> and to say what we mean, on Windows and Linux build hosts identically.

Sure---though in this case something like

movseg  ds, ax

would obey two of three requirements (say what you mean, and treat
Windows+Linux equally).

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

Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Get more accurate TSC Frequency

[Paolo Bonzini]

On 11/08/2016 04:37, Star Zeng wrote:
> Minimize the code overhead between the two TSC reads by adding
> new internal API to calculate TSC Frequency instead of reusing
> MicroSecondDelay ().
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Paul A Lohr 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng 
> ---
>  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 56 
> +-
>  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 33 -
>  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 31 
>  3 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index 806a4f7ce24c..e6fea231123d 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>ACPI Timer implements one instance of Timer Library.
>  
> -  Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.
> +  Copyright (c) 2013 - 2016, 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
> @@ -335,3 +335,57 @@ GetTimeInNanoSecond (
>  
>return NanoSeconds;
>  }
> +
> +/**
> +  Calculate TSC frequency.
> +
> +  The TSC counting frequency is determined by comparing how far it counts
> +  during a 100us period as determined by the ACPI timer. The ACPI timer is
> +  used because it counts at a known frequency.
> +  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> +  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> +  difference multiplied by 1 is the TSC frequency. There will be a small
> +  error because of the overhead of reading the ACPI timer. An attempt is
> +  made to determine and compensate for this error.
> +
> +  @return The number of TSC counts per second.
> +
> +**/
> +UINT64
> +InternalCalculateTscFrequency (
> +  VOID
> +  )
> +{
> +  UINT64  StartTSC;
> +  UINT64  EndTSC;
> +  UINT16  TimerAddr;
> +  UINT32  Ticks;
> +  UINT64  TscFrequency;
> +  BOOLEAN InterruptState;
> +
> +  InterruptState = SaveAndDisableInterrupts ();
> +
> +  TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> +  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
> Ticks to 100us in the future

ACPI_TIMER_FREQUENCY is 3579545, thus you're waiting 357 ticks but the
actual result of the division is much closer to 358.  The error is only
0.26%, but it's so simple to reduce it that I think it's worth it.  Just
change (ACPI_TIMER_FREQUENCY / 1) to (ACPI_TIMER_FREQUENCY + 5000) /
1.

Another possibility is to count 343 ticks and multiply by 10436; 343 *
10436 is almost exactly ACPI_TIMER_FREQUENCY.

Paolo

> +  StartTSC = AsmReadTsc (); // Get 
> base value for the TSC
> +  //
> +  // Wait until the ACPI timer has counted 100us.
> +  // Timer wrap-arounds are handled correctly by this function.
> +  // When the current ACPI timer value is greater than 'Ticks', the while 
> loop will exit.
> +  //
> +  while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
> +CpuPause();
> +  }
> +  EndTSC = AsmReadTsc ();   // TSC 
> value 100us later
> +
> +  TscFrequency = MultU64x32 (
> +   (EndTSC - StartTSC), // 
> Number of TSC counts in 100us
> +   1// 
> Number of 100us in a second
> +   );
> +
> +  SetInterruptState (InterruptState);
> +
> +  return TscFrequency;
> +}
> +
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> index 21fdb79908b8..8819ebcfccef 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>ACPI Timer implements one instance of Timer Library.
>  
> -  Copyright (c) 2013 - 2014, Intel Corporation. All rights reserved.
> +  Copyright (c) 2013 - 2016, 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
> @@ -17,6 +17,26 @@
>  #include 
>  
>  /**
> +  Calculate TSC frequency.
> +
> +  The TSC counting frequency is determined by comparing how far it counts
> +  during a 100us period as determined by the ACPI timer. The ACPI timer is
> + 

Re: [edk2] [PATCH 0/5] MdePkg BaseTools: GCC optimization for X64

[Paolo Bonzini]

On 14/07/2016 16:57, Ard Biesheuvel wrote:
>> >   On patch 5, I don't see any change for IA32 arch. is there no mode for 
>> > IA32 arch? Here, small and pic must be enabled together, right? Otherwise, 
>> > the assumption is to load driver below 2G address. Have you collected size 
>> > data before and after this change?
>> >
> To be honest, I know very little about the IA32 ABI, but I don't think
> it has different code models, does it? Since the ELF relocations are
> stripped by the PE/COFF conversion, I don't think it is necessary to
> take into account that X64 and IA32 modules may interact with each
> other when running on a X64 platform.

IIUC, IA32 cannot use memory above 4GB because it doesn't use virtual
addresses.  Also, IA32 doesn't have PC-relative addressing at all.

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

Re: [edk2] [PATCH v2 3/5] BaseTools/tools_def: enable Os optimization for GCC X64 builds

[Paolo Bonzini]

On 16/07/2016 14:29, Laszlo Ersek wrote:
> 
> However, I recall from the thread that -Os enables -fomit-frame-pointer,
> which might make source level debugging impossible (according to the GCC
> manual).

This is only with very old debuggers.  Current debuggers use DWARF
annotations which support generation of backtraces even in the lack of a
frame pointer.

That said, the best debugging experience on recent GCC (4.8 I think) is
with -Og if you want to use it for DEBUG builds.  -O1 and -Os are
probably the best for firmware though, where code size is important.

Paolo

> Now, we're not big on source level debugging in GCC builds, at least
> right now, plus I also cannot claim that that -fomit-frame-pointer is
> never enabled *otherwise*. Much as I know -fomit-frame-pointer could be
> enabled with -O1, -O2, even with -O0?...
> 
> I'd just like to avoid a setting that *guarantees* that source level
> debugging would be impossible or garbled. Ard, can you comment on that?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] PcAtChipsetPkg AcpiTimerLib: Wait 363 ACPI timer counts to get TSC Freq

[Paolo Bonzini]

On 17/08/2016 04:26, Star Zeng wrote:
> Compute the number of ticks to wait to measure TSC frequency.
> Instead of (ACPI_TIMER_FREQUENCY / 1) = 357 and 357 * 1 = 357,
> use 363 * 9861 = 3579543 Hz which is within 2 Hz of ACPI_TIMER_FREQUENCY.
> 363 counts is a calibration time of 101.4 uS.
> 
> The idea comes from Michael and Paolo.
> 
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Liming Gao <liming@intel.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Paul A Lohr <paul.a.l...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.z...@intel.com>
> ---
>  PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c | 32 
> +-
>  .../Library/AcpiTimerLib/BaseAcpiTimerLib.c| 14 +-
>  .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 14 +-
>  3 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> index e6fea231123d..020031e3f4a5 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
> @@ -340,13 +340,13 @@ GetTimeInNanoSecond (
>Calculate TSC frequency.
>  
>The TSC counting frequency is determined by comparing how far it counts
> -  during a 100us period as determined by the ACPI timer. The ACPI timer is
> -  used because it counts at a known frequency.
> -  The TSC is sampled, followed by waiting for ACPI_TIMER_FREQUENCY / 1
> -  clocks of the ACPI timer, or 100us. The TSC is then sampled again. The
> -  difference multiplied by 1 is the TSC frequency. There will be a small
> -  error because of the overhead of reading the ACPI timer. An attempt is
> -  made to determine and compensate for this error.
> +  during a 101.4 us period as determined by the ACPI timer.
> +  The ACPI timer is used because it counts at a known frequency.
> +  The TSC is sampled, followed by waiting 363 counts of the ACPI timer,
> +  or 101.4 us. The TSC is then sampled again. The difference multiplied by
> +  9861 is the TSC frequency. There will be a small error because of the
> +  overhead of reading the ACPI timer. An attempt is made to determine and
> +  compensate for this error.
>  
>@return The number of TSC counts per second.
>  
> @@ -366,22 +366,28 @@ InternalCalculateTscFrequency (
>InterruptState = SaveAndDisableInterrupts ();
>  
>TimerAddr = InternalAcpiGetAcpiTimerIoPort ();
> -  Ticks = IoRead32 (TimerAddr) + (ACPI_TIMER_FREQUENCY / 1);// Set 
> Ticks to 100us in the future
> +  //
> +  // Compute the number of ticks to wait to measure TSC frequency.
> +  // Use 363 * 9861 = 3579543 Hz which is within 2 Hz of 
> ACPI_TIMER_FREQUENCY.
> +  // 363 counts is a calibration time of 101.4 uS.
> +  //
> +  Ticks = IoRead32 (TimerAddr) + 363;
>  
>StartTSC = AsmReadTsc (); // Get 
> base value for the TSC
>//
> -  // Wait until the ACPI timer has counted 100us.
> +  // Wait until the ACPI timer has counted 101.4 us.
>// Timer wrap-arounds are handled correctly by this function.
> -  // When the current ACPI timer value is greater than 'Ticks', the while 
> loop will exit.
> +  // When the current ACPI timer value is greater than 'Ticks',
> +  // the while loop will exit.
>//
>while (((Ticks - IoRead32 (TimerAddr)) & BIT23) == 0) {
>  CpuPause();
>}
> -  EndTSC = AsmReadTsc ();   // TSC 
> value 100us later
> +  EndTSC = AsmReadTsc ();   // TSC 
> value 101.4 us later
>  
>TscFrequency = MultU64x32 (
> -   (EndTSC - StartTSC), // 
> Number of TSC counts in 100us
> -   1// 
> Number of 100us in a second
> +   (EndTSC - StartTSC), // 
> Number of TSC counts in 101.4 us
> +   9861 // 
> Number of 101.4 us in a second
> );
>  
>SetInterruptState (InterruptState);
> diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c 
> b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> index 8819ebcfccef..29521f8b220b 100644
> --- a/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> +++ b/PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
> @@ -20,13 +20,13 @@
>Calculate TSC frequency.
>  
>The TSC counting frequency is determined by comparing h

Re: [edk2] [PATCH] VfrCompile: fix invalid comparison between pointer and integer

[Paolo Bonzini]

On 15/02/2017 09:46, Zhu, Yonghong wrote:
> 
> "-s ''" is an error, current the error message is not same as no option, 
> because the content after the " all be treated as -s 's input.
> May I know what's your comment on  Nikolai SAOUKH's patch ?
> 
> -  if (mStringFileName == '\0' ) {
> +  if (mStringFileName == NULL || *mStringFileName == '\0' ) {

If "-s ''" is an error, it should be pointless to check *mStringFileName.

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

Re: [edk2] [PATCH] VfrCompile: fix invalid comparison between pointer and integer

[Paolo Bonzini]

On 13/02/2017 14:55, Zhu, Yonghong wrote:
> Hi Paolo Bonzini,
> 
> We already had another patch for this issue.  Please help to check the 
> attachment. Thanks.

Is it intended that "-s ''" is not an error, rather it is the same as no
option at all?

Paolo

> Best Regards,
> Zhu Yonghong
> 
> 
> -----Original Message-
> From: Paolo Bonzini [mailto:pbonz...@redhat.com] 
> Sent: Monday, February 13, 2017 8:54 PM
> To: edk2-de...@ml01.01.org
> Cc: Zhu, Yonghong <yonghong@intel.com>; Gao, Liming <liming@intel.com>
> Subject: [PATCH] VfrCompile: fix invalid comparison between pointer and 
> integer
> 
> This would be valid C but is not valid C++, so change the comparison to do 
> what it has always been doing.
> 
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
>  BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp 
> b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
> index 3ca57ed..2f97975 100644
> --- a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
> +++ b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
> @@ -3372,7 +3372,7 @@ CVfrStringDB::GetVarStoreNameFormStringId (
>UINT8   BlockType;
>EFI_HII_STRING_PACKAGE_HDR *PkgHeader;
>
> -  if (mStringFileName == '\0' ) {
> +  if (mStringFileName == NULL) {
>  return NULL;
>}
>  
> --
> 2.9.3
> 
> 
> 
> ___
> 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] VfrCompile: fix invalid comparison between pointer and integer

[Paolo Bonzini]

This would be valid C but is not valid C++, so change the comparison
to do what it has always been doing.

Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
 BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp 
b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
index 3ca57ed..2f97975 100644
--- a/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
+++ b/BaseTools/Source/C/VfrCompile/VfrUtilityLib.cpp
@@ -3372,7 +3372,7 @@ CVfrStringDB::GetVarStoreNameFormStringId (
   UINT8   BlockType;
   EFI_HII_STRING_PACKAGE_HDR *PkgHeader;
   
-  if (mStringFileName == '\0' ) {
+  if (mStringFileName == NULL) {
 return NULL;
   }
 
-- 
2.9.3

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 09/11/2016 16:54, Paolo Bonzini wrote:
>> > and 2) AP is in protected mode with paging disabled.
> It is not clear to me what the (4) SIPI done is there for, and why it is
> triggered in S3Resume.c rather than CpuS3.c.  And why does it take so
> much for APs to complete it?

SMI of course, not SIPI.

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 09/11/2016 16:01, Yao, Jiewen wrote:
> 1)  CpuS3.c – EarlyInitializeCpu()
> 2)  CpuS3.c – SmmRelocateBases()
> 3)  CpuS3.c – InitializeCpu()
> 4)  S3Resume.c – SendSmiIpiAllExcludingSelf()
> 
> I believe we can guarantee 1/2/3 is good, because I found we check BSP
> check mNumberToFinish.
> 
> 4 is a risk, because there is no AP finish check. If the AP is in below
> 1M with CR3 in SMRAM, it will be a trouble.
> 
> Once the AP executes RSM and return to non-SMM, the CR3 is no longer
> valid and AP must be crashed immediately. WoW!
> 
> The fix, I believe, is same.
> 
> We should make 1) AP is in above 1M reserved memory,

Is this because of the NMI case?

> and 2) AP is in protected mode with paging disabled.

It is not clear to me what the (4) SIPI done is there for, and why it is
triggered in S3Resume.c rather than CpuS3.c.  And why does it take so
much for APs to complete it?

That said, by the time you close and lock SMRAM, you aren't even sure
that you have reached the cli;hlt loop in the rendezvous funnel.  In
practice you will be there, but there is still a theoretical race.

InterlockedDecrement () should be moved from
EarlyMPRendezvousProcedure/MPRendezvousProcedure to GoToSleep, and
GoToSleep should leave 64-bit mode before doing it.  This will fix the
S3 bug as well.  It's only needed for 64-bit mode, but it is doable for
the Ia32 version as well.

Perhaps EarlyMPRendezvousProcedure and MPRendezvousProcedure can return
 what do you think?

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

> Another question I have -- and I feel I should really know it, but I
> don't... -- is *why* the APs are executing code from the page at
> 0x9f000.

This I can answer. :)

The APs have done their INIT-SIPI-SIPI, and then went into the CLI;HLT;JMP
loop.  When the AP exits SMM, it is in the JMP instruction.

As suggested by Jiewen, edk2 could jump to a 32-bit loop that is _not_
in the 0-640K area (perhaps it could be in what your doc calls the
"permanent PEI memory for the S3 resume path"?).  After thinking a
bit more about it, it seems simplest to me if CpuS3.c just uses
SwitchStack or AsmDisablePaging64 at the end of MPRendezvousProcedure,
to jump to a small stub like

POP EAX   ; pop return address
POP EAX   ; pop Context1 which is 
DEC [EAX]
 1: CLI
HLT
JMP 1

> I could be utterly and inexcusably wrong, but I think that the
> RIP=0x9f0fd symptom is a red herring.

I wouldn't call it a red herring.  After all, CR3 points to SMM
exactly because the CR3 that was set up for the 0x9f000 stub is
CpuS3.c's SMRAM page table root.

What _is_ a red herring is KVM's trace showing a RSM instruction
at RIP=0x9f0fd.  That is clearly bogus, RSM was rather the last
instruction executed _before_ getting to that RIP.

> >   vcpu#0  vcpu#1  vcpu#2  vcpu#3
> >   --  --  --  --
> >   enter   enter
> >enter| enter |
> >  |  |   |   |
> >leave|   |   |
> > <--- BAD
> >enter|   |   |
> >  |  |   |   |
> >leave  leave   leave   leave
> 
> I don't understand why we don't get horrible faults on the APs
> *immediately* when the BSP closes/locks down SMRAM. Everything in SMRAM,
> page tables, executable code, everything, will read as 0xff on QEMU. How
> can the APs continue in SMM long enough to
> 
> (a) time out and pull the BSP back into SMM,
> (b) complete the rendezvous and exit SMM?

Because the "0xff" only applies when you're out of SMM.  The three
states (open, closed, closed/locked) only apply when you're not in SMM.
While the AP is in SMM they are executing in a separate address space
where SMRAM is "not closed".  (In QEMU that's a separate AddressSpace
struct, smram_address_space in target-i386/kvm.c).

> I think I sort of answered question (2). (Apologies if Paolo and Jiewen
> explained the exact same thing before; I had to spell it out for
> myself.) That leaves question (1) open. Why enter SMM in
> S3ResumeExecuteBootScript() at all?
> 
> Anyway, I think if the BSP and the APs are properly synchronized around
> the SMI injections in S3ResumeExecuteBootScript(), then this bug is
> fixed. In that case, the APs' RSMs will restore the full context for the
> APs, including their sleep in the HLT instruction, in CpuMpPei's wakeup
> buffer. The BSP will proceed, exit PEI (restoring the CpuMpPei wakeup
> buffer -- but the APs will sleep on), and then Linux will bring up the
> APs, after taking control.

Agreed.

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

>   * Second, the instruction that causes things to blow up is <0f aa>,
> i.e., RSM. I have absolutely no clue why RSM is executed:

It's probably not RSM.  RSM is probably the last instruction executed
before, and it's still in the buffer because, as you said, there's no
way that you can fetch an instruction while CR3 points into SMM.

My first thought was that the MMU is for some reason out of contact
with reality, but actually the CR3 write is correct:

 CPU-24446 [002] 39841.871040: kvm_exit: reason 
CR_ACCESS rip 0x9f05e info 103 0
 CPU-24446 [002] 39841.871040: kvm_cr:   cr_write 3 = 
0x7ff7f000

and it's coming from the stub as well.  So the second thought was that
the wakeup buffer has the wrong CR3 put into the wakeup buffer's Cr3 location.
I'm glad I kept looking because it was much more entertaining.  Especially
knowing that I (probably) will not have to fix it. :)

The basic idea for debugging was to look for interesting events and
use 0x402 writes to correlate them to the debug log.  For example, most
accesses to 0x9f??? are obviously not traced by KVM, but the first ones
are:

31519-  CPU-2 [006] 39841.783344: kvm_exit: reason 
EPT_VIOLATION rip 0x855d82 info 181 0
31520:  CPU-2 [006] 39841.783344: kvm_page_fault:   address 
9f000 error_code 181
280224- CPU-2 [006] 39841.860940: kvm_exit: reason 
EPT_VIOLATION rip 0x7ffd0d15 info 182 0
280225: CPU-2 [006] 39841.860940: kvm_page_fault:   address 
9f000 error_code 182

(The number is just the line number in the trace).  Luckily your machine
didn't have EPT accessed/dirty bits, so KVM trapped both the first read
and the first write.

The read is at

WakeupBufferStart = 9F000, WakeupBufferSize = 1000

but it's not too interesting.  The second is a good one to start debugging
because it's from SMRAM (though not from SMM, since the first kvm_enter_smm
happens later at 305930).  So it makes sense that it writes an SMRAM CR3.
There is a write to the debug log just before, at 279993, and it writes
"SmmRestoreCpu()".  As expected, the write is followed by a flurry of MSR
writes, the APIC programming at 280131, so I am pretty sure that the write to
mExchangeInfo->Cr3 comes from PrepareApStartupVector.

FWIW, I first looked at the call chain up from BackupAndPrepareWakeupBuffer,
but that led me nowhere for an hour.  So I was a bit lucky indeed. :)

Anyhow, SmmRestoreCpu is the SmmS3ResumeEntryPoint for S3Resume2Pei, and
indeed, earlier in the log you have this debugging output from S3Resume2Pei:

SMM S3 CR3  = 7FF7F000

Doh, maybe I should have looked at the log before the trace.  Who knows.
Anyway, the SMM_S3_RESUME_STATE is initialized by InitSmmS3ResumeState,
so the CR3 is the one that is initialized by InitSmmS3Cr3 in
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c.  At this point I
was still thinking that this CR3 was wrong, but by looking at the
places where SMM is entered, and correlating that with debug log writes,
the puzzle was relatively easy to solve:

1) SMBASE relocation, done by SmmRestoreCpu:

305930: CPU-24445 [005] 39841.871264: kvm_enter_smm:vcpu 1: 
entering SMM, smbase 0x3
306000: CPU-24445 [005] 39841.871318: kvm_enter_smm:vcpu 1: 
leaving SMM, smbase 0x7ffb3000
306051: CPU-24446 [002] 39841.871349: kvm_enter_smm:vcpu 2: 
entering SMM, smbase 0x3
306108: CPU-24446 [002] 39841.871390: kvm_enter_smm:vcpu 2: 
leaving SMM, smbase 0x7ffb5000
306161: CPU-24447 [004] 39841.871421: kvm_enter_smm:vcpu 3: 
entering SMM, smbase 0x3
306218: CPU-24447 [004] 39841.871463: kvm_enter_smm:vcpu 3: 
leaving SMM, smbase 0x7ffb7000
306254: CPU-2 [006] 39841.871473: kvm_enter_smm:vcpu 0: 
entering SMM, smbase 0x3
306311: CPU-2 [006] 39841.871512: kvm_enter_smm:vcpu 0: 
leaving SMM, smbase 0x7ffb1000

2) S3ResumeExecuteBootScript (again, the previous 0x402 write ends
at 334597 and promptly gives us a clue):

334698: CPU-24445 [005] 39841.882706: kvm_enter_smm:vcpu 1: 
entering SMM, smbase 0x7ffb3000
334699: CPU-24447 [004] 39841.882706: kvm_enter_smm:vcpu 3: 
entering SMM, smbase 0x7ffb7000
334741: CPU-2 [006] 39841.882723: kvm_enter_smm:vcpu 0: 
entering SMM, smbase 0x7ffb1000
334742: CPU-24446 [002] 39841.882724: kvm_enter_smm:vcpu 2: 
entering SMM, smbase 0x7ffb5000
334875: CPU-2 [006] 39841.882755: kvm_enter_smm:vcpu 0: 
leaving SMM, smbase 0x7ffb1000

Here I think that it's where things go awry.  The lines after
S3ResumeExecuteBootScript() are

   Close all SMRAM regions before executing boot script
   Lock all SMRAM regions before executing boot script

and indeed the first is at 334898, 

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 09/11/2016 07:25, Yao, Jiewen wrote:
> Current BSP just uses its own context to initialize AP. So that AP
> takes BSP CR3, which is SMM CR3, unfortunately. After BSP initialized
> APs, the AP is put to HALT-LOOP in X64 mode. It is the last straw,
> because X64 mode halt still need paging.
> 
> 3)  The error happen, once the AP receives an interrupt (for
> whatever reason), AP starts executing code. However, that that time
> the AP might not be in SMM mode. It means SMM CR3 is not available.
> And then we see this.
> 
> 4)  I guess we did not see the error, or this is RANDOM issue,
> because it depends on if AP receives an interrupt before BSP send
> INIT-SIPI-SIPI.
> 
> 5)  The fix, I think, should be below: We should always put AP to
> protected mode, so that no paging is needed. We should put AP in
> above 1M reserved memory, instead of <1M memory, because <1M memory
> is restored.

For what it's worth, this is not what I observed.  What I found is that
the BSP doesn't wait for the AP rendezvous before closing SMRAM.

I'm not sure if the two things are related, but (3) would be a much
worse bug.  APs should not be receiving an interrupt.  Perhaps an NMI if
API is sitting in a CLI;HLT loop, but this is not what is happening.

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

Re: [edk2] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Decrease mNumberToFinish in AP safe code

[Paolo Bonzini]

On 11/11/2016 06:45, Jeff Fan wrote:
> We will put APs into hlt-loop in safe code. But we decrease mNumberToFinish
> before APs enter into the safe code. Paolo pointed out this gap.
> 
> This patch is to move mNumberToFinish decreasing to the safe code. It could
> make sure BSP could wait for all APs are running in safe code.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=216
> 
> Reported-by: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Jiewen Yao <jiewen@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 17 +++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c |  6 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h|  4 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  |  6 --
>  4 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index e53e096..34547e0 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -79,9 +79,11 @@ BOOLEAN  mAcpiS3Enable = TRUE;
>  
>  UINT8*mApHltLoopCode = NULL;
>  UINT8mApHltLoopCodeTemplate[] = {
> -   0xFA,// cli
> -   0xF4,// hlt
> -   0xEB, 0xFC   // jmp $-2
> +   0x8B, 0x44, 0x24, 0x04,  // mov  eax, dword 
> ptr [esp+4]
> +   0xF0, 0xFF, 0x08,// lock dec  dword 
> ptr [eax]
> +   0xFA,// cli
> +   0xF4,// hlt
> +   0xEB, 0xFC   // jmp $-2
> };
>  
>  /**
> @@ -399,17 +401,12 @@ MPRendezvousProcedure (
>}
>  
>//
> -  // Count down the number with lock mechanism.
> -  //
> -  InterlockedDecrement ();
> -
> -  //
> -  // Place AP into the safe code
> +  // Place AP into the safe code, count down the number with lock mechanism 
> in the safe code.
>//
>TopOfStack  = (UINT32) (UINTN) Stack + sizeof (Stack);
>TopOfStack &= ~(UINT32) (CPU_STACK_ALIGNMENT - 1);
>CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate, sizeof 
> (mApHltLoopCodeTemplate));
> -  TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack);
> +  TransferApToSafeState ((UINT32) (UINTN) mApHltLoopCode, TopOfStack, 
> );
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> index 8b880d6..9760373 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
> @@ -100,17 +100,19 @@ InitGdt (
>  
>@param[in] ApHltLoopCodeThe 32-bit address of the safe hlt-loop 
> function.
>@param[in] TopOfStack   A pointer to the new stack to use for the 
> ApHltLoopCode.
> +  @param[in] NumberToFinish   Semaphore of APs finish count.
>  
>  **/
>  VOID
>  TransferApToSafeState (
>IN UINT32 ApHltLoopCode,
> -  IN UINT32 TopOfStack
> +  IN UINT32 TopOfStack,
> +  IN UINT32 *NumberToFinish
>)
>  {
>SwitchStack (
>  (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode,
> -NULL,
> +NumberToFinish,
>  NULL,
>  (VOID *) (UINTN) TopOfStack
>  );
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 6c98659..88d9c85 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -830,12 +830,14 @@ GetAcpiS3EnableFlag (
>  
>@param[in] ApHltLoopCodeThe 32-bit address of the safe hlt-loop 
> function.
>@param[in] TopOfStack   A pointer to the new stack to use for the 
> ApHltLoopCode.
> +  @param[in] NumberToFinish   Semaphore of APs finish count.
>  
>  **/
>  VOID
>  TransferApToSafeState (
>IN UINT32 ApHltLoopCode,
> -  IN UINT32 TopOfStack
> +  IN UINT32 TopOfStack,
> +  IN UINT32 *NumberToFinish
>);
>  
>  #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> index 62338b7..6844c3f 100644
> --- a

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

[Paolo Bonzini]

On 10/11/2016 11:41, Laszlo Ersek wrote:
> Here's an excerpt from the KVM trace:
> 
>>  CPU-23509 [002]  8406.908787: kvm_enter_smm:vcpu 1: entering SMM, 
>> smbase 0x3
>>  CPU-23509 [002]  8406.908836: kvm_enter_smm:vcpu 1: leaving SMM, 
>> smbase 0x7ffb3000
>>  CPU-23510 [003]  8406.908850: kvm_enter_smm:vcpu 2: entering SMM, 
>> smbase 0x3
>>  CPU-23510 [003]  8406.908881: kvm_enter_smm:vcpu 2: leaving SMM, 
>> smbase 0x7ffb5000
>>  CPU-23511 [001]  8406.908908: kvm_enter_smm:vcpu 3: entering SMM, 
>> smbase 0x3
>>  CPU-23511 [001]  8406.908941: kvm_enter_smm:vcpu 3: leaving SMM, 
>> smbase 0x7ffb7000
>>  CPU-23508 [005]  8406.908951: kvm_enter_smm:vcpu 0: entering SMM, 
>> smbase 0x3
>>  CPU-23508 [005]  8406.908989: kvm_enter_smm:vcpu 0: leaving SMM, 
>> smbase 0x7ffb1000
>>  CPU-23511 [001]  8406.920215: kvm_enter_smm:vcpu 3: entering SMM, 
>> smbase 0x7ffb7000
>>  CPU-23509 [002]  8406.920225: kvm_enter_smm:vcpu 1: entering SMM, 
>> smbase 0x7ffb3000
>>  CPU-23510 [003]  8406.920225: kvm_enter_smm:vcpu 2: entering SMM, 
>> smbase 0x7ffb5000
>>  CPU-23508 [005]  8406.920227: kvm_enter_smm:vcpu 0: entering SMM, 
>> smbase 0x7ffb1000
>>  CPU-23508 [005]  8406.920262: kvm_enter_smm:vcpu 0: leaving SMM, 
>> smbase 0x7ffb1000
>>  CPU-23511 [001]  8406.920263: kvm_enter_smm:vcpu 3: leaving SMM, 
>> smbase 0x7ffb7000
>>  CPU-23508 [005]  8407.020292: kvm_enter_smm:vcpu 0: entering SMM, 
>> smbase 0x7ffb1000
>>  CPU-23509 [006]  8407.020338: kvm_enter_smm:vcpu 1: leaving SMM, 
>> smbase 0x7ffb3000
>>  CPU-23510 [003]  8407.020338: kvm_enter_smm:vcpu 2: leaving SMM, 
>> smbase 0x7ffb5000
>>  CPU-23508 [005]  8407.020338: kvm_enter_smm:vcpu 0: leaving SMM, 
>> smbase 0x7ffb1000
> 
> It seems that VCPU#0 still leaves (and then re-enters) SMM while VCPU#1 and 
> VCPU#2 are firmly in SMM.
> 
> So this series is a clear improvement, but something else remains amiss.
> 
> If I remove Jiewen's v2 series, and apply only this one, then the symptom 
> shows up much less frequently, but it does exist:
> - With (Jiewen's v2 + this one), testing case 13, I hit the symptom on the 
> second resume,
> - With just this set applied, I hit the symptom (= one AP disappearing from 
> Linux after resume) only on the 24th resume.

Any trace I can look at?  What about case 14, with
PcdCpuSmmStaticPageTable=TRUE?

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 10/11/2016 11:41, Yao, Jiewen wrote:
> I also did not quite understand below log.
> 
> * CPU #0: pc=0xc1844555 (halted) thread_id=7835
>   CPU #1: pc=0xc1844555 (halted) thread_id=7836
>   CPU #2: pc=0xc1844555 (halted) thread_id=7837
>   CPU #3: pc=0x7ffd17ca thread_id=7838
> 
> As I recall, writing to B2 only cause BSP get SMI on OVMF. AP does not enter 
> SMM mode.

It's not BSP that enters SMM, it's the currently executing processor.

So this means that CPU #3 has written to B2.

Thanks,

Paolo


> So why #3 can enter SMM mode? Is that expected behavior? Or unexpected 
> behavior?
> If this is expected, how this happened? Does OS send 
> SendSmiIpiAllExcludingSelf, after ExitBootServices()?
> 
> I will see if I can finish QEMU/KVM installation tomorrow.
> 
> If you have some idea on why and how #3 enter SMM, please let us know.
>
> 
> Thank you
> Yao Jiewen
> 
> 
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, November 10, 2016 4:46 AM
> To: Yao, Jiewen <jiewen@intel.com>
> Cc: Tian, Feng <feng.t...@intel.com>; edk2-de...@ml01.01.org; Kinney, Michael 
> D <michael.d.kin...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; Fan, 
> Jeff <jeff@intel.com>; Zeng, Star <star.z...@intel.com>
> Subject: Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.
> 
> On 11/09/16 07:25, Yao, Jiewen wrote:
>> Hi Laszlo
>> I will fix DEBUG message issue in V3 patch.
>>
>> Below is rest issues:
>>
>>
>> l  Case 13: S3 fails randomly.
>> A good news: I worked with Jeff Fan to root-cause the S3 resume issue. Here 
>> is detail.
>>
>>
>> 1)  We believe the dead CPU is AP. Not BSP.
>> The reason is that:
>>
>> 1.1)   The BSP already transfer control to OS waking vector. The GDT/IDT/CR3 
>> should be set by OS.
>>
>> 1.2)   The current dead CPU still has GDT/IDT point to a BIOS reserved 
>> memory. The CS/DS/SS is typical BIOS X64 mode setting.
>>
>> 1.3)   The current dead CPU still has CR3 in SMM. (Which is obvious wrong)
>>
>>
>> 2)  Based upon the 1), we reviewed S3 resume AP flow.
>> Current BSP will wake up AP in SMRAM, for security consideration. At that 
>> time, we are using SMM mode CR3. It is OK for BSP because BSP is NOT in SMM 
>> mode yet. Even after SMM rebase, we can still use it because SMRR is not set 
>> in first SMM rebase.
>> Current BSP just uses its own context to initialize AP. So that AP takes BSP 
>> CR3, which is SMM CR3, unfortunately.
>> After BSP initialized APs, the AP is put to HALT-LOOP in X64 mode. It is the 
>> last straw, because X64 mode halt still need paging.
>>
>>
>> 3)  The error happen, once the AP receives an interrupt (for whatever 
>> reason), AP starts executing code. However, that that time the AP might not 
>> be in SMM mode. It means SMM CR3 is not available. And then we see this.
>>
>>
>> 4)  I guess we did not see the error, or this is RANDOM issue, because 
>> it depends on if AP receives an interrupt before BSP send INIT-SIPI-SIPI.
>>
>>
>> 5)  The fix, I think, should be below:
>> We should always put AP to protected mode, so that no paging is needed.
>> We should put AP in above 1M reserved memory, instead of <1M memory, because 
>> <1M memory is restored.
>>
>>
>> Would you please file a bugzillar? I think we need assign CPU owner to fix 
>> that critical issue.
>>
>> There is no need to do more investigation. Thanks for your great help on 
>> that. :)
> 
> Thank you for your help!
> 
> I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=216>. The title is
> 
> BSP exits SMM and closes SMRAM on the S3 resume path before
> meeting with AP(s)
> 
> I hope the title is mostly right. I didn't add any other details (I
> haven't gone through the thread in detail yet, and without that I can't
> even write up a semi-reasonable report myself). Instead, I referenced
> this message of yours in the report, and I also linked Paolo's analysis
> from elsewhere in the thread. I hope this will do for the report.
> 
> (Also, thank you Paolo, from the amazing analysis -- I haven't digested
> it yet, but I can already tell it's amazing! :))
> 
>> l  Case 17 - I do not think it is a real issue, because SMM is out of 
>> resource.
>>
>>
>> l  Case 8 - that is a very weird issue. I talk with Jeff again. I do not 
>> have a clear clue yet.
>>> ASSERT MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.c(73): 
>>> SpinLock != ((v

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

> And, in my recent KVM / QEMU usage instructions for Jiewen:
> 
>   https://www.mail-archive.com/edk2-devel@lists.01.org/msg19446.html
> 
> I provided the following settings:
> 
> > # Settings for Ia32 only:
> > [...]
> > QEMU_COMMAND="qemu-system-i386 -cpu coreduo,-nx"
> >
> > # Settings for Ia32X64 only:
> > [...]
> > QEMU_COMMAND=qemu-system-x86_64
> 
> I guess the "-nx" bit can be left off with TCG, but AFAIR it is required
> for KVM.

Oh right now I remember.  The same problem exists: EFER is not saved in the
32-bit state save map.  AFAIK all processors with XD also have long mode.

That said, qemu-system-x86_64 and no -cpu option should work even with Ia32
PEI/DXE/SMM and no -cpu option.  In that case you could use XD.

Now if only Intel made the *full* format of the state save map public, we
could emulate everything more accurately...  I'm told it's in the BIOS
writers guide.

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

Re: [edk2] [PATCH V2 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 10/11/2016 15:48, Yao, Jiewen wrote:
> I cannot reproduce it before, because all my real hardware supports XD.
> My Windows QEMU also supports XD (to my surprise.)

QEMU can be configured to support XD or not.  Possibly Laszlo was using
some different default, or testing both cases.

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

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

[Paolo Bonzini]

On 10/11/2016 07:07, Jeff Fan wrote:
> On S3 path, we will wake up APs to restore CPU context in PiSmmCpuDxeSmm
> driver. In case, one NMI or SMI happens, APs may exit from hlt state and
> execute the instruction after HLT instruction.
> 
> But APs are not running on safe code, it leads OVMF S3 boot unstable.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=216
> 
> I tested real platform with 64bit DXE.
> 
> Jeff Fan (2):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Put AP into safe hlt-loop code on S3 path
>   UefiCpuPkg/PiSmmCpuDxeSmm: Place AP to 32bit protected mode on S3 path
> 
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 31 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 25 
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h| 13 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c  | 59 
> +++
>  4 files changed, 128 insertions(+)
> 

Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>

It would be slightly more robust to do the "InterlockedDecrement
();" while in safe state, but the race window is really
really small.

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

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

[Paolo Bonzini]

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.

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

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

[Paolo Bonzini]

On 14/11/2016 12:27, Laszlo Ersek wrote:
> 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.

It's hard to say no when someone has written the code already. :)

Paolo

> 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):
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

[Paolo Bonzini]

On 14/11/2016 19:07, Laszlo Ersek wrote:
> On 11/14/16 13:00, Paolo Bonzini wrote:
>>
>>
>> On 14/11/2016 12:27, Laszlo Ersek wrote:
>>> 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.
>>
>> It's hard to say no when someone has written the code already. :)
> 
> Thanks. I refreshed both patches (OVMF and QEMU -- no code changes just
> more precise commit messages). Unfortunately, quite a few things seem
> broken, although these patches worked a year ago.
> 
> My QEMU base commit is current master 83c83f9a5266. My host kernel is
> 3.10.0-514.el7.x86_64.
> 
> *** So, when I test these two patches, based on edk2 master (no on-list
> patches), Ia32 target, my boot hangs (spins) with the log ending in:
> 
>> SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
> 
> That is, MpInitChangeApLoopCallback() is entered, but it never finishes.
> "info cpus" prints:
> 
> * CPU #0: pc=0x7f1f7763 thread_id=17395
>   CPU #1: pc=0x7f2ce01e (halted) thread_id=17396
>   CPU #2: pc=0x7f2ce01e (halted) thread_id=17397
>   CPU #3: pc=0xfff0 thread_id=17398
> 
> and I've also seen a case where all the APs were stuck at the reset
> vector (0xfff0), *not* halted, like VCPU#3 above. They don't
> spin, they're just stuck. The spinning comes from CPU#0, apparently in
> MpInitChangeApLoopCallback.
> 
> *** I flipped the AP sync mode to traditional (considering the relaxed
> mode shouldn't be required with the broadcast SMIs). This time the log
> ends with:
> 
>> SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
>> MpInitChangeApLoopCallback() done!
> 
> but then QEMU abort()s:
> 
>> kvm_io_ioeventfd_add: error adding ioeventfd: File exists
>> 2016-11-14 17:00:41.405+: shutting down, reason=crashed
> 
> I see some ioeventfd stuff in the recent QEMU history; do you think it's
> related?

Yes, just try 2.7 for now or disable vhost.

Paolo

> *** My last attempt was even more strange. I applied Jeff's v2 (this
> series), returned to the relaxed (= currently in-tree) sync mode, and
> (of course) the broadcast SMI patches on both sides. This time I didn't
> even boot an OS, I just entered the setup TUI, and selected the Reset
> option. QEMU crashed again with:
> 
>> kvm_io_ioeventfd_add: error adding ioeventfd: File exists
>> 2016-11-14 17:00:41.405+: shutting down, reason=crashed
> 
> I don't know what to look at, honestly. I think I'll check the reflog
> for my local QEMU master branch, and return to one of my earlier pulls,
> or else use v2.7.0 for testing.
> 
> FWIW, the broadcast SMIs work just fine as long as I'm in the firmware
> (not booting an OS and not resetting, just browsing around); I verified
> with GDB that the broadcast SMI branch was taken in QEMU repeatedly.
> 
> Thanks
> Laszlo
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] using UEFI logo as part of another logo

[Paolo Bonzini]

On 31/10/2016 05:59, Michael Zimmermann wrote:
> Hi,
> 
> since the uefi logo guidlines are mainly targeted at "pure logo" usage I'm
> not sure if the following would be legal:
> https://raw.githubusercontent.com/efidroid/android_app_efidroidmanager/9d364dc4e8d59381d6c60404153c5dbf5aa4711a/app/src/main/res/mipmap-xxxhdpi/ic_launcher.png
> 
> I'm not working for a company but I need a logo which I can use everywhere
> and however I want so I really need to know if it's allowed to use the logo
> like this.

White on red falls outside the guidelines, I think.

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

Re: [edk2] [PATCH 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 04/11/2016 14:28, Yao, Jiewen wrote:
> I tried below way. But it does not help too much. It still takes more
> than 1 minutes to boot with SMP=8.
> 
>   SendSmiIpiAllExcludingSelf ();
>   IoWrite8 (ICH9_APM_STS, DataPort== NULL ? 0 : *DataPort);
>   IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);
> 
> I also tried to reduce the timeout PCD to:
> 
>   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|1000
> 
> However, I find CPU-2 is still missing.
> 
> Maybe it is caused by QEMU emulate AP in serial mode, not parallel mode.

Yeah, that's possible without KVM.  Do you issue a PAUSE instruction in
the spin loops?  That could help.

Paolo

> I think it might be best choice to set PcdCpuSmmSyncMode|0x1
> 
> It also helps cover a very corner case in SMM. J
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/6] Enable SMM page level protection.

[Paolo Bonzini]

On 04/11/2016 16:22, Laszlo Ersek wrote:
>> > What does this *KVM internal error. Suberror: 1* mean?
> The key message is "emulation failure" -- it means that the processor
> exits to the hypervisor (KVM) because it finds some code that it cannot
> execute in guest mode natively, so the hypervisor needs to emulate it.
> And, this emulation fails. The reasons can be:
> - the code is valid, but KVM lacks the emulation code for it,
> - the code is actually garbage (not code) -- there was some corruption
> in the guest (the location used to contain code but it was corrupted, or
> the guest jumped to non-code data).
> 
> Usually the register dump contains a short hexadecimal snippet from the
> instruction stream (near Code=...), pinpointing the byte that caused the
> problem. However, in this case, all we have is question marks, and this
> is the very first time I see those. That's why I CC'd Paolo and Radim :)

The question marks usually mean that the page tables do not map a page
at that address, but I don't know offhand why KVM would fail emulation
instead of triple-faulting.

Try "info tlb" to dump the page tables (huge output of course, you may
want to use the GTK+ backend which has scrollable consoles).

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

Re: [edk2] SmmCommunicationCommunicate question?

[Paolo Bonzini]

On 13/10/2016 11:07, Laszlo Ersek wrote:
> 
> Instead, once the first CPU enters SMM, it brings all the other CPUs
> into SMM as well, where they will be executing known, secure code --
> i.e., the first CPU to enter SMM forces the other CPUs to temporarily
> abandon any (possibly malicious) code the runtime OS may have prepared.
> Only *then* will the verification of the communication buffer commence.
> If the malicious code managed to race the unpriv part of the service
> successfully, now the privileged part will catch that, undisturbed.

Even this is not strictly necessary if you can set aside some memory in
SMRAM for a copy the communication buffer.  Then you can do:

   tmp = comm buffer size
   if tmp > sizeof(privileged buffer)
   return error
   copy tmp bytes from comm buffer to privileged buffer

and not look anymore at the buffer provided by the user.

Of course, "bring all CPUs into SMM" can double as a poor man's mutex,
so there may be reasons to do that anyway.

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

Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: fix to support EFI_ERASE_BLOCK_PROTOCOL

[Paolo Bonzini]

On 15/04/2017 15:44, Haojian Zhuang wrote:
> If bit TPZ and bit TPRZ are set, the erase feature is implemented.
> If bit TPZ is set and bit TPRZ is clear, the discard feature is
> implemented. And discard is a non-secure variant of the erase
> functionality.
> 
> So the detecting operation of EFI_ERASE_BLOCK_PROTOCOL, we should
> consider to support both functionality. Since discard functionality is
> only supported in some UFS devices.
> 
> And both of these two features are relied on UNMAP command.

Hi,

you need to use WRITE SAME, with a zero payload and the UNMAP bit set in
the command descriptor, in order to achieve a "secure" erase
functionality.  UNMAP is only an advisory command, and does not
guarantee that the blocks are unmapped.

Discard can use either WRITE SAME or UNMAP.

Also,

>  // Bits TPE and TPRZ should both be set to enable the erase feature on 
> UFS.
> +// Setting bit TPE and clearing bit TPRZ to enable the discard feature 
> on UFS.
>  //
> -if (((CapacityData16->LowestAlignLogic2 & BIT7) == 0) ||
> -((CapacityData16->LowestAlignLogic2 & BIT6) == 0)) {
> +if ((CapacityData16->LowestAlignLogic2 & BIT7) == 0) {
>DEBUG ((
>  EFI_D_VERBOSE,
>  "ScsiDisk EraseBlock: Either TPE or TPRZ is not set: 0x%x.\n",

The debug message is now wrong.

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

Re: [edk2] [PATCH 1/1] BaseTools/tools_def.template: revert to large code model for X64/GCC5/LTO

[Paolo Bonzini]

On 22/08/2017 16:03, Ard Biesheuvel wrote:
> On 22 August 2017 at 14:27, Paolo Bonzini <pbonz...@redhat.com> wrote:
>> On 22/08/2017 13:59, Laszlo Ersek wrote:
>>> This seems to suggest that "-pie" is the *master* switch (used only when
>>> linking), and "-fpie" is a *prerequisite* for it (to be used both when
>>> linking and compiling). Is this right?
>>>
>>> If so, then I think this is a gcc usability bug. We don't generally
>>> start our thinking from the linker side. The above implies that the
>>> simple (hosted) command line:
>>>
>>> $ gcc -o example -fpie source1.c source2.c
>>>
>>> could also result in miscompilation, because "-pie" is not given, only
>>> "-fpie".
>>
>> No, GCC should add -pie on its own.
>>
> 
> I disagree. PIE linking and PIE code generation are two completely
> different things.

What I'm saying is that GCC should add -pie on its own if you add -fpie
to the linker command line.  But that would require changes to the
compiler driver.

That said, the extra "-Wl," in "-Wl,-pie" is not necessary; the compiler
driver knows "-pie" and swallows it when compiling (and passes it to the
linker).

Paolo

> PIE linking (in the absence of LTO) only involves emitting the
> sections containing the metadata required by the loader at runtime.
> Because dynamic ELF relocations are more restricted than static ones,
> and only operate on native pointer sized quantities, this imposes
> constraints on the code generation, which is why we need the -fpic or
> -fpie compiler switch. On ARM, this means you should not emit absolute
> symbol references where the address is encoded in the immediate field
> of a sequence of movw/movt/movz/movk instructions. I'm sure there are
> similar restrictions on other architectures. Note that the arm64 KASLR
> kernel does use PIE linking but omits -fpic/-fpie simply because the
> default small code model never uses such instructions, but always uses
> relative references (and statically initialized function pointers etc
> are guaranteed to be dynamically relocatable)
> 
> IIRC, PIE linking predates the availability of the -fpie GCC flag, and
> so -fpic objects were used to create PIE binaries, because they
> happened to fulfil these requirements, given that they apply to shared
> libraries as well. However, -fpic is geared towards ELF symbol
> preemption and other restrictions that do apply to shared libraries
> but not to PIE executables, and so the -fpie switch was introduced as
> an alternative, which generates code that may not be suitable for a
> shared library but can be used in a PIE or ordinary executable.
> 
> None of this really applies to bare metal binaries, which is why we
> need the visibility tweaks when using -fpic/-fpie, in which case the
> compiler will relative references over absolute ones. Such objects
> could be combined in different ways, and PIE linking is only one of
> them.
> 

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

Re: [edk2] [PATCH 1/1] BaseTools/tools_def.template: revert to large code model for X64/GCC5/LTO

[Paolo Bonzini]

On 22/08/2017 18:04, Laszlo Ersek wrote:
>> That said, the extra "-Wl," in "-Wl,-pie" is not necessary; the compiler
>> driver knows "-pie" and swallows it when compiling (and passes it to the
>> linker).
> Now *that* I can get behind. If this works, then please let us do it --
> replace "-fpie" with "-pie" in GCC44_X64_CC_FLAGS, and add no "-Wl,"
> stuff to any DLINK defines.

Note that you still need -fpie in the CC flags (and it _should_ not need
"-pie" in CC flags, only in DLINK).

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

Re: [edk2] SMRAM sizes on large hosts

[Paolo Bonzini]

On 03/05/2017 15:35, Laszlo Ersek wrote:
>> I see.  In my other answer I tried to keep it as intact as possible.
>>
>> I'm a bit worried about the limits on the number of fw-cfg files.
> We've promoted that to a device property in QEMU commit e12f3a13e2e1
> ("fw-cfg: turn FW_CFG_FILE_SLOTS into a device property", 2017-01-12),
> and we've raised the count to 0x20 for 2.9 machtypes, in commit
> a5b3ebfd23bc ("fw-cfg: bump "x-file-slots" to 0x20 for 2.9+ machine
> types", 2017-01-12).
> 
> ... Or does your concern already account for those?

I was aware it had been bumped, though not exactly how much.  32 is
better than before, but still on the lowish side...

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