diff mbox

[edk2,2/2] ArmVirtPkg/ArmVirtQemu: allow firmware to be built in ACPI-only mode

Message ID 1459423216-2415-3-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel March 31, 2016, 11:20 a.m. UTC 
This introduces the .DSC define 'PURE_ACPI_BOOT_ENABLE', defaulting to
FALSE, which controls the value of the feature PCD 'PcdPureAcpiBoot'.

This allows a ArmVirtQemu image to be built that restricts the OS to
booting in ACPI mode.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmVirtPkg/ArmVirtQemu.dsc | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.5.0

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

Comments

Laszlo Ersek March 31, 2016, 12:48 p.m. UTC | #1 
(4) for the subject of this patch, I would prefer something less
sensational :), such as

ArmVirtPkg/ArmVirtQemu: gate FDT config table install with build option

On 03/31/16 13:20, Ard Biesheuvel wrote:
> This introduces the .DSC define 'PURE_ACPI_BOOT_ENABLE', defaulting to

> FALSE, which controls the value of the feature PCD 'PcdPureAcpiBoot'.

> 

> This allows a ArmVirtQemu image to be built that restricts the OS to


(5) "a[n] ArmVirtQemu image"

> booting in ACPI mode.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmVirtPkg/ArmVirtQemu.dsc | 5 +++++

>  1 file changed, 5 insertions(+)

> 

> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

> index fafad7751e6d..e626df768f85 100644

> --- a/ArmVirtPkg/ArmVirtQemu.dsc

> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

> @@ -34,6 +34,7 @@ [Defines]

>    # -D FLAG=VALUE

>    #

>    DEFINE SECURE_BOOT_ENABLE      = FALSE

> +  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE

>  

>  !include ArmVirtPkg/ArmVirt.dsc.inc

>  

> @@ -99,6 +100,10 @@ [PcdsFeatureFlag.common]

>    # Activate KVM workaround for now.

>    gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|TRUE

>  

> +!if $(PURE_ACPI_BOOT_ENABLE) == TRUE

> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE

> +!endif

> +

>  [PcdsFixedAtBuild.common]

>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1

>  !if $(ARCH) == AARCH64

> 


(6) my only question that would justify / require a v2 is: do you have
any reason for not doing the same in "ArmVirtQemuKernel.dsc"?

Loading UEFI "as a payload by a previous bootloader stage such as ARM
Trusted Firmware/OP-TEE" (from 8de84d424221) doesn't seem to require
PcdPureAcpiBoot==FALSE, does it? That config still implies QEMU, hence
ACPI tables will be available.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 31, 2016, 12:53 p.m. UTC | #2 
On 31 March 2016 at 14:48, Laszlo Ersek <lersek@redhat.com> wrote:
> (4) for the subject of this patch, I would prefer something less

> sensational :), such as

>

> ArmVirtPkg/ArmVirtQemu: gate FDT config table install with build option

>

> On 03/31/16 13:20, Ard Biesheuvel wrote:

>> This introduces the .DSC define 'PURE_ACPI_BOOT_ENABLE', defaulting to

>> FALSE, which controls the value of the feature PCD 'PcdPureAcpiBoot'.

>>

>> This allows a ArmVirtQemu image to be built that restricts the OS to

>

> (5) "a[n] ArmVirtQemu image"

>

>> booting in ACPI mode.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmVirtPkg/ArmVirtQemu.dsc | 5 +++++

>>  1 file changed, 5 insertions(+)

>>

>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

>> index fafad7751e6d..e626df768f85 100644

>> --- a/ArmVirtPkg/ArmVirtQemu.dsc

>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

>> @@ -34,6 +34,7 @@ [Defines]

>>    # -D FLAG=VALUE

>>    #

>>    DEFINE SECURE_BOOT_ENABLE      = FALSE

>> +  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE

>>

>>  !include ArmVirtPkg/ArmVirt.dsc.inc

>>

>> @@ -99,6 +100,10 @@ [PcdsFeatureFlag.common]

>>    # Activate KVM workaround for now.

>>    gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|TRUE

>>

>> +!if $(PURE_ACPI_BOOT_ENABLE) == TRUE

>> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE

>> +!endif

>> +

>>  [PcdsFixedAtBuild.common]

>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1

>>  !if $(ARCH) == AARCH64

>>

>

> (6) my only question that would justify / require a v2 is: do you have

> any reason for not doing the same in "ArmVirtQemuKernel.dsc"?

>

> Loading UEFI "as a payload by a previous bootloader stage such as ARM

> Trusted Firmware/OP-TEE" (from 8de84d424221) doesn't seem to require

> PcdPureAcpiBoot==FALSE, does it? That config still implies QEMU, hence

> ACPI tables will be available.

>


The main reason is that ArmVirtQemuKernel is mostly intended for
development work, where the burden of adding 'acpi=force' if you need
it is much more tolerable than when trying to boot an installer on a
production KVM guest instance. So the question is not whether it can
tolerate this change, but whether it has use for it.

So I am fine adding it if you insist, but I am not aware of any use
cases that require it at the moment.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 31, 2016, 1:11 p.m. UTC | #3 
On 03/31/16 14:53, Ard Biesheuvel wrote:
> On 31 March 2016 at 14:48, Laszlo Ersek <lersek@redhat.com> wrote:

>> (4) for the subject of this patch, I would prefer something less

>> sensational :), such as

>>

>> ArmVirtPkg/ArmVirtQemu: gate FDT config table install with build option

>>

>> On 03/31/16 13:20, Ard Biesheuvel wrote:

>>> This introduces the .DSC define 'PURE_ACPI_BOOT_ENABLE', defaulting to

>>> FALSE, which controls the value of the feature PCD 'PcdPureAcpiBoot'.

>>>

>>> This allows a ArmVirtQemu image to be built that restricts the OS to

>>

>> (5) "a[n] ArmVirtQemu image"

>>

>>> booting in ACPI mode.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>> ---

>>>  ArmVirtPkg/ArmVirtQemu.dsc | 5 +++++

>>>  1 file changed, 5 insertions(+)

>>>

>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

>>> index fafad7751e6d..e626df768f85 100644

>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc

>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

>>> @@ -34,6 +34,7 @@ [Defines]

>>>    # -D FLAG=VALUE

>>>    #

>>>    DEFINE SECURE_BOOT_ENABLE      = FALSE

>>> +  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE

>>>

>>>  !include ArmVirtPkg/ArmVirt.dsc.inc

>>>

>>> @@ -99,6 +100,10 @@ [PcdsFeatureFlag.common]

>>>    # Activate KVM workaround for now.

>>>    gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|TRUE

>>>

>>> +!if $(PURE_ACPI_BOOT_ENABLE) == TRUE

>>> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE

>>> +!endif

>>> +

>>>  [PcdsFixedAtBuild.common]

>>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1

>>>  !if $(ARCH) == AARCH64

>>>

>>

>> (6) my only question that would justify / require a v2 is: do you have

>> any reason for not doing the same in "ArmVirtQemuKernel.dsc"?

>>

>> Loading UEFI "as a payload by a previous bootloader stage such as ARM

>> Trusted Firmware/OP-TEE" (from 8de84d424221) doesn't seem to require

>> PcdPureAcpiBoot==FALSE, does it? That config still implies QEMU, hence

>> ACPI tables will be available.

>>

> 

> The main reason is that ArmVirtQemuKernel is mostly intended for

> development work, where the burden of adding 'acpi=force' if you need

> it is much more tolerable than when trying to boot an installer on a

> production KVM guest instance. So the question is not whether it can

> tolerate this change, but whether it has use for it.

> 

> So I am fine adding it if you insist, but I am not aware of any use

> cases that require it at the moment.


My primary goal here is to decrease / eliminate confusion when someone
compares the two DSC files against each other, and sees the
PcdPureAcpiBoot difference. There are two ways to lift that confusion:
- eliminate the difference (= add the same to "ArmVirtQemuKernel.dsc")
- explain the difference (= add your above paragraph to the commit
message of the second patch)

Since option #1 would introduce DSC code that was practically dead on
arrival, I agree option #2 is better. Please add your explanation above
to the commit message of this patch.

With (1) through (6') addressed:

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>


Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 31, 2016, 2:39 p.m. UTC | #4 
On 31 March 2016 at 15:11, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/31/16 14:53, Ard Biesheuvel wrote:

>> On 31 March 2016 at 14:48, Laszlo Ersek <lersek@redhat.com> wrote:

>>> (4) for the subject of this patch, I would prefer something less

>>> sensational :), such as

>>>

>>> ArmVirtPkg/ArmVirtQemu: gate FDT config table install with build option

>>>

>>> On 03/31/16 13:20, Ard Biesheuvel wrote:

>>>> This introduces the .DSC define 'PURE_ACPI_BOOT_ENABLE', defaulting to

>>>> FALSE, which controls the value of the feature PCD 'PcdPureAcpiBoot'.

>>>>

>>>> This allows a ArmVirtQemu image to be built that restricts the OS to

>>>

>>> (5) "a[n] ArmVirtQemu image"

>>>

>>>> booting in ACPI mode.

>>>>

>>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>>>> ---

>>>>  ArmVirtPkg/ArmVirtQemu.dsc | 5 +++++

>>>>  1 file changed, 5 insertions(+)

>>>>

>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

>>>> index fafad7751e6d..e626df768f85 100644

>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc

>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

>>>> @@ -34,6 +34,7 @@ [Defines]

>>>>    # -D FLAG=VALUE

>>>>    #

>>>>    DEFINE SECURE_BOOT_ENABLE      = FALSE

>>>> +  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE

>>>>

>>>>  !include ArmVirtPkg/ArmVirt.dsc.inc

>>>>

>>>> @@ -99,6 +100,10 @@ [PcdsFeatureFlag.common]

>>>>    # Activate KVM workaround for now.

>>>>    gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|TRUE

>>>>

>>>> +!if $(PURE_ACPI_BOOT_ENABLE) == TRUE

>>>> +  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE

>>>> +!endif

>>>> +

>>>>  [PcdsFixedAtBuild.common]

>>>>    gArmPlatformTokenSpaceGuid.PcdCoreCount|1

>>>>  !if $(ARCH) == AARCH64

>>>>

>>>

>>> (6) my only question that would justify / require a v2 is: do you have

>>> any reason for not doing the same in "ArmVirtQemuKernel.dsc"?

>>>

>>> Loading UEFI "as a payload by a previous bootloader stage such as ARM

>>> Trusted Firmware/OP-TEE" (from 8de84d424221) doesn't seem to require

>>> PcdPureAcpiBoot==FALSE, does it? That config still implies QEMU, hence

>>> ACPI tables will be available.

>>>

>>

>> The main reason is that ArmVirtQemuKernel is mostly intended for

>> development work, where the burden of adding 'acpi=force' if you need

>> it is much more tolerable than when trying to boot an installer on a

>> production KVM guest instance. So the question is not whether it can

>> tolerate this change, but whether it has use for it.

>>

>> So I am fine adding it if you insist, but I am not aware of any use

>> cases that require it at the moment.

>

> My primary goal here is to decrease / eliminate confusion when someone

> compares the two DSC files against each other, and sees the

> PcdPureAcpiBoot difference. There are two ways to lift that confusion:

> - eliminate the difference (= add the same to "ArmVirtQemuKernel.dsc")

> - explain the difference (= add your above paragraph to the commit

> message of the second patch)

>

> Since option #1 would introduce DSC code that was practically dead on

> arrival, I agree option #2 is better. Please add your explanation above

> to the commit message of this patch.

>

> With (1) through (6') addressed:

>

> series

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>


Thanks

Pushed as

7a63d29151ae ArmVirtPkg/VirtFdtDxe: make installation of FDT as config
table optional
b359fb9115b2 ArmVirtPkg/ArmVirtQemu: gate FDT config table install
with build option
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index fafad7751e6d..e626df768f85 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -34,6 +34,7 @@  [Defines]
   # -D FLAG=VALUE
   #
   DEFINE SECURE_BOOT_ENABLE      = FALSE
+  DEFINE PURE_ACPI_BOOT_ENABLE   = FALSE
 
 !include ArmVirtPkg/ArmVirt.dsc.inc
 
@@ -99,6 +100,10 @@  [PcdsFeatureFlag.common]
   # Activate KVM workaround for now.
   gArmVirtTokenSpaceGuid.PcdKludgeMapPciMmioAsCached|TRUE
 
+!if $(PURE_ACPI_BOOT_ENABLE) == TRUE
+  gArmVirtTokenSpaceGuid.PcdPureAcpiBoot|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
   gArmPlatformTokenSpaceGuid.PcdCoreCount|1
 !if $(ARCH) == AARCH64