When tboot is disabled via menuconfig we get undefined reference error for
g_tboot_shared. This patch fixes that by disabling the causing source code
when tboot is disabled.

Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro>
---
 xen/arch/x86/acpi/power.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 91a8c4d0bd..29d9775aed 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -407,6 +407,7 @@ static int acpi_get_wake_status(void)
     return val;
 }
 
+#ifdef CONFIG_TBOOT
 static void tboot_sleep(u8 sleep_state)
 {
     uint32_t shutdown_type;
@@ -451,18 +452,21 @@ static void tboot_sleep(u8 sleep_state)
 
     tboot_shutdown(shutdown_type);
 }
+#endif
 
 /* System is really put into sleep state by this stub */
 acpi_status acpi_enter_sleep_state(u8 sleep_state)
 {
     acpi_status status;
 
+#ifdef CONFIG_TBOOT
     if ( tboot_in_measured_env() )
     {
         tboot_sleep(sleep_state);
         printk(XENLOG_ERR "TBOOT failed entering s3 state\n");
         return_ACPI_STATUS(AE_ERROR);
     }
+#endif
 
     ACPI_FLUSH_CPU_CACHE();
 
-- 
2.20.1

On 29.04.2021 14:11, Costin Lupu wrote:
> When tboot is disabled via menuconfig we get undefined reference error for
> g_tboot_shared. This patch fixes that by disabling the causing source code
> when tboot is disabled.

There must be more to this: Our shim config also builds with tboot
disabled, without running into any build issue. Furthermore ...

> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -407,6 +407,7 @@ static int acpi_get_wake_status(void)
>      return val;
>  }
>  
> +#ifdef CONFIG_TBOOT
>  static void tboot_sleep(u8 sleep_state)
>  {
>      uint32_t shutdown_type;
> @@ -451,18 +452,21 @@ static void tboot_sleep(u8 sleep_state)
>  
>      tboot_shutdown(shutdown_type);
>  }
> +#endif
>  
>  /* System is really put into sleep state by this stub */
>  acpi_status acpi_enter_sleep_state(u8 sleep_state)
>  {
>      acpi_status status;
>  
> +#ifdef CONFIG_TBOOT
>      if ( tboot_in_measured_env() )
>      {
>          tboot_sleep(sleep_state);
>          printk(XENLOG_ERR "TBOOT failed entering s3 state\n");
>          return_ACPI_STATUS(AE_ERROR);
>      }
> +#endif

... tboot_in_measured_env() already has a stub returning 0 when
!TBOOT (which is what I would have recommended instead of the
#ifdef-ary).

If there is a specific case where the compiler fails to DCE the
offending code, then you need to describe this in sufficient
detail.

Jan

On 4/29/21 3:40 PM, Jan Beulich wrote:
> On 29.04.2021 14:11, Costin Lupu wrote:
>> When tboot is disabled via menuconfig we get undefined reference error for
>> g_tboot_shared. This patch fixes that by disabling the causing source code
>> when tboot is disabled.
> 
> There must be more to this: Our shim config also builds with tboot
> disabled, without running into any build issue. Furthermore ...
> 
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -407,6 +407,7 @@ static int acpi_get_wake_status(void)
>>      return val;
>>  }
>>  
>> +#ifdef CONFIG_TBOOT
>>  static void tboot_sleep(u8 sleep_state)
>>  {
>>      uint32_t shutdown_type;
>> @@ -451,18 +452,21 @@ static void tboot_sleep(u8 sleep_state)
>>  
>>      tboot_shutdown(shutdown_type);
>>  }
>> +#endif
>>  
>>  /* System is really put into sleep state by this stub */
>>  acpi_status acpi_enter_sleep_state(u8 sleep_state)
>>  {
>>      acpi_status status;
>>  
>> +#ifdef CONFIG_TBOOT
>>      if ( tboot_in_measured_env() )
>>      {
>>          tboot_sleep(sleep_state);
>>          printk(XENLOG_ERR "TBOOT failed entering s3 state\n");
>>          return_ACPI_STATUS(AE_ERROR);
>>      }
>> +#endif
> 
> ... tboot_in_measured_env() already has a stub returning 0 when
> !TBOOT (which is what I would have recommended instead of the
> #ifdef-ary).
> 
> If there is a specific case where the compiler fails to DCE the
> offending code, then you need to describe this in sufficient
> detail.

Yes, indeed. My bad, it is for a debug build with -O0, so without DCE.

Cheers,
Costin

On 29.04.2021 15:22, Costin Lupu wrote:
> On 4/29/21 3:40 PM, Jan Beulich wrote:
>> If there is a specific case where the compiler fails to DCE the
>> offending code, then you need to describe this in sufficient
>> detail.
> 
> Yes, indeed. My bad, it is for a debug build with -O0, so without DCE.

Iirc there's a series pending to switch to -Og; I don't think we
build with -O0 under any circumstances (for this very reason).

Jan

On 29/04/2021 14:42, Jan Beulich wrote:
> On 29.04.2021 15:22, Costin Lupu wrote:
>> On 4/29/21 3:40 PM, Jan Beulich wrote:
>>> If there is a specific case where the compiler fails to DCE the
>>> offending code, then you need to describe this in sufficient
>>> detail.
>> Yes, indeed. My bad, it is for a debug build with -O0, so without DCE.
> Iirc there's a series pending to switch to -Og; I don't think we
> build with -O0 under any circumstances (for this very reason).

-Og is roughly about -O0.9.  There is certainly some trivial DCE
involved, but no optimisations which radically rearrange the code
structure.  The -Og series did survive some randconfig testing, but I
can't guarantee that it was comprehensive.

As for -O0, I think causality is the other way around there.  Wei
elected to rely on DCE for conditional compilation because we didn't
seem to care about compiling at -O0.


Part of me thinks that we ought to cope compiling at any optimisation
level, including -O0, but I doubt I'd like the extra ifdef-ary required
to make that work.

I think we absolutely should have Kconfig to select between -Og/1/2/3,
as well as binutils new microarch levels

~Andrew