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

2015-11-19 Thread Jordan Justen 
On 2015-11-04 17:04:34, 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.

With that change:

Reviewed-by: Jordan Justen 
___
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

2015-11-05 Thread 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 v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

2015-11-04 Thread Brian J. Johnson 

On 11/04/2015 02:08 PM, 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.

This is normally not a problem, but with SMM support, the APIC library
gets used (within SMM) *after* Linux configures x2apic mode. This causes
the "basic" library instance to blow up. Therefore I flipped the library
class resolution to the one instance that supports x2apic mode, but only
for the SMM build of OVMF.

So, the question Paolo and I have is *not* whether we must use the
x2apic-capable library in the SMM build -- that is a fact, forced by the
X64 Linux guest kernel's behavior.

Neither is our question "how could we enable x2apic mode"? About that,
we don't care at the moment. :)

Instead, our question is if we can use BaseXApicX2ApicLib even in the
*non*-SMM build, without fear of regressions. In other words, if
BaseXApicX2ApicLib can safely replace BaseXApicLib in a build where
BaseXApicLib would be otherwise fully sufficient.

Because, if the answer is "yes, it is compatible", then we shouldn't
complicate the library class resolution in the OVMF DSC files, making it
dependent on the build type (i.e., SMM -> BaseXApicX2ApicLib, non-SMM ->
BaseXApicLib). Rather than that, we should indiscriminately use
BaseXApicX2ApicLib.

So... is BaseXApicX2ApicLib compatible with BaseXApicLib in the domain
where the latter would work sufficiently?

Thanks!
Laszlo


All I can say is that BaseXApicX2ApicLib works great on non-virtual 
hardware, in either legacy or X2Apic mode.


Brian






Thanks,

Mike


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, November 04, 2015 2:41 AM
To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
Justen, Jordan L
Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
with x2apic support if SMM_REQUIRE

On 11/04/15 09:48, Paolo Bonzini wrote:



On 03/11/2015 22:00, Laszlo Ersek wrote:

+
+!if $(SMM_REQUIRE) == TRUE
+

LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf

+!else
LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
+!endif
+


Can we enable BaseXApicX2ApicLib unconditionally?


I think I am technically capable of that :), but should we? We haven't
used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
it could regress stuff or not. If it causes problems with the
SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
such a global change for the traditional build.

I'm not against it, I just don't have experience with BaseXApicX2ApicLib.

Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
the former? If it has only been for simplicity's sake, and
BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
think Paolo is right, and we should just keep the OVMF DSCs simple.

Thanks
Laszlo


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




--

  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
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

2015-11-04 Thread 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

2015-11-04 Thread Kinney, Michael D 
Laszlo,

Yes.  They are compatible.  And I do recommend switching to BaseXApicX2ApicLib 
unconditionally.
 
Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, November 04, 2015 12:09 PM
>To: Kinney, Michael D; Paolo Bonzini; edk2-de...@ml01.01.org; Fan, Jeff;
>Justen, Jordan L
>Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>with x2apic support if SMM_REQUIRE
>
>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.
>
>This is normally not a problem, but with SMM support, the APIC library
>gets used (within SMM) *after* Linux configures x2apic mode. This causes
>the "basic" library instance to blow up. Therefore I flipped the library
>class resolution to the one instance that supports x2apic mode, but only
>for the SMM build of OVMF.
>
>So, the question Paolo and I have is *not* whether we must use the
>x2apic-capable library in the SMM build -- that is a fact, forced by the
>X64 Linux guest kernel's behavior.
>
>Neither is our question "how could we enable x2apic mode"? About that,
>we don't care at the moment. :)
>
>Instead, our question is if we can use BaseXApicX2ApicLib even in the
>*non*-SMM build, without fear of regressions. In other words, if
>BaseXApicX2ApicLib can safely replace BaseXApicLib in a build where
>BaseXApicLib would be otherwise fully sufficient.
>
>Because, if the answer is "yes, it is compatible", then we shouldn't
>complicate the library class resolution in the OVMF DSC files, making it
>dependent on the build type (i.e., SMM -> BaseXApicX2ApicLib, non-SMM ->
>BaseXApicLib). Rather than that, we should indiscriminately use
>BaseXApicX2ApicLib.
>
>So... is BaseXApicX2ApicLib compatible with BaseXApicLib in the domain
>where the latter would work sufficiently?
>
>Thanks!
>Laszlo
>
>
>>
>> Thanks,
>>
>> Mike
>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Wednesday, November 04, 2015 2:41 AM
>>> To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
>>> Justen, Jordan L
>>> Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>>> with x2apic support if SMM_REQUIRE
>>>
>>> On 11/04/15 09:48, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 03/11/2015 22:00, Laszlo Ersek wrote:
>>>>> +
>>>>> +!if $(SMM_REQUIRE) == TRUE
>>>>> +
>>>
>LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>>>>> +!else
>>>>>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>>>>> +!endif
>>>>> +
>>>>
>>>> Can we enable BaseXApicX2ApicLib unconditionally?
>>>
>>> I think I am technically capable of that :), but should we? We haven't
>>> used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
>>> it could regress stuff or not. If it causes problems with the
>>> SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
>>> such a global change for the traditional build.
>>>
>>> I'm not against it, I just don't have experience with BaseXApicX2ApicLib.
>>>
>>> Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
>>> and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always
>used
>>> the former? If it has only been for simplicity's sake, and
>>> BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
>>> think Paolo is right, and we should just keep the OVMF DSCs simple.
>>>
>>> Thanks
>>> Laszlo
>
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

2015-11-04 Thread Kinney, Michael D 
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?

Thanks,

Mike

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Wednesday, November 04, 2015 2:41 AM
>To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
>Justen, Jordan L
>Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>with x2apic support if SMM_REQUIRE
>
>On 11/04/15 09:48, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2015 22:00, Laszlo Ersek wrote:
>>> +
>>> +!if $(SMM_REQUIRE) == TRUE
>>> +
>LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>>> +!else
>>>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>>> +!endif
>>> +
>>
>> Can we enable BaseXApicX2ApicLib unconditionally?
>
>I think I am technically capable of that :), but should we? We haven't
>used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
>it could regress stuff or not. If it causes problems with the
>SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
>such a global change for the traditional build.
>
>I'm not against it, I just don't have experience with BaseXApicX2ApicLib.
>
>Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
>and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
>the former? If it has only been for simplicity's sake, and
>BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
>think Paolo is right, and we should just keep the OVMF DSCs simple.
>
>Thanks
>Laszlo
___
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

2015-11-04 Thread Laszlo Ersek 
On 11/04/15 09:48, Paolo Bonzini wrote:
> 
> 
> On 03/11/2015 22:00, Laszlo Ersek wrote:
>> +
>> +!if $(SMM_REQUIRE) == TRUE
>> +  LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> +!else
>>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>> +!endif
>> +
> 
> Can we enable BaseXApicX2ApicLib unconditionally?

I think I am technically capable of that :), but should we? We haven't
used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
it could regress stuff or not. If it causes problems with the
SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
such a global change for the traditional build.

I'm not against it, I just don't have experience with BaseXApicX2ApicLib.

Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
the former? If it has only been for simplicity's sake, and
BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
think Paolo is right, and we should just keep the OVMF DSCs simple.

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