[PATCH 0/4] x86: ACPI and DMI table mapping fixes

[PATCH 0/4] x86: ACPI and DMI table mapping fixes

[Jan Beulich]

The first three patches fix fallout from the re-work of
acpi_os_{,un}map_memory(): Direct uses of __acpi_map_table() are now
no longer valid once we've reached SYS_STATE_boot. This was originally
noticed by system shutdown no longer working (patch 1), but clearly
extends beyond of this (patches 2 and 3). The last patch relaxes
things such that entering S5 would still work even if there was a
problem with FACS (information collected from there is only needed for
entering S3 and, once we support it, S4 via S4BIOS_REQ).

1: x86/ACPI: fix mapping of FACS
2: x86/ACPI: fix S3 wakeup vector mapping
3: x86/DMI: fix table mapping when one lives above 1Mb
4: x86/ACPI: don't invalidate S5 data when S3 wakeup vector cannot be determined

Jan

[PATCH 1/4] x86/ACPI: fix mapping of FACS

[Jan Beulich]

acpi_fadt_parse_sleep_info() runs when the system is already in
SYS_STATE_boot. Hence its direct call to __acpi_map_table() won't work
anymore. This call should probably have been replaced long ago already,
as the layering violation hasn't been necessary for quite some time.

Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -422,8 +422,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 	if (!facs_pa)
 		goto bad;
 
-	facs = (struct acpi_table_facs *)
-		__acpi_map_table(facs_pa, sizeof(struct acpi_table_facs));
+	facs = acpi_os_map_memory(facs_pa, sizeof(*facs));
 	if (!facs)
 		goto bad;
 
@@ -448,11 +447,16 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 		offsetof(struct acpi_table_facs, firmware_waking_vector);
 	acpi_sinfo.vector_width = 32;
 
+	acpi_os_unmap_memory(facs, sizeof(*facs));
+
 	printk(KERN_INFO PREFIX
 	       "            wakeup_vec[%"PRIx64"], vec_size[%x]\n",
 	       acpi_sinfo.wakeup_vector, acpi_sinfo.vector_width);
 	return;
-bad:
+
+ bad:
+	if (facs)
+		acpi_os_unmap_memory(facs, sizeof(*facs));
 	memset(&acpi_sinfo, 0,
 	       offsetof(struct acpi_sleep_info, sleep_control));
 	memset(&acpi_sinfo.sleep_status + 1, 0,

Re: [PATCH 1/4] x86/ACPI: fix mapping of FACS

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:39:55PM +0100, Jan Beulich wrote:
> acpi_fadt_parse_sleep_info() runs when the system is already in
> SYS_STATE_boot. Hence its direct call to __acpi_map_table() won't work
> anymore. This call should probably have been replaced long ago already,
> as the layering violation hasn't been necessary for quite some time.
> 
> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/4] x86/ACPI: fix mapping of FACS

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:39:55PM +0100, Jan Beulich wrote:
> acpi_fadt_parse_sleep_info() runs when the system is already in
> SYS_STATE_boot. Hence its direct call to __acpi_map_table() won't work
> anymore. This call should probably have been replaced long ago already,
> as the layering violation hasn't been necessary for quite some time.
> 
> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -422,8 +422,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>  	if (!facs_pa)
>  		goto bad;
>  
> -	facs = (struct acpi_table_facs *)
> -		__acpi_map_table(facs_pa, sizeof(struct acpi_table_facs));
> +	facs = acpi_os_map_memory(facs_pa, sizeof(*facs));
>  	if (!facs)
>  		goto bad;
>  
> @@ -448,11 +447,16 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>  		offsetof(struct acpi_table_facs, firmware_waking_vector);
>  	acpi_sinfo.vector_width = 32;
>  
> +	acpi_os_unmap_memory(facs, sizeof(*facs));

Nit: looking at this again, I think you could move the
acpi_os_unmap_memory further up, just after the last usage of facs
(ie: before setting the wakeup_vector field).

Roger.

Re: [PATCH 1/4] x86/ACPI: fix mapping of FACS

[Jan Beulich]

On 29.12.2020 11:56, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 01:39:55PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -422,8 +422,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>>  	if (!facs_pa)
>>  		goto bad;
>>  
>> -	facs = (struct acpi_table_facs *)
>> -		__acpi_map_table(facs_pa, sizeof(struct acpi_table_facs));
>> +	facs = acpi_os_map_memory(facs_pa, sizeof(*facs));
>>  	if (!facs)
>>  		goto bad;
>>  
>> @@ -448,11 +447,16 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>>  		offsetof(struct acpi_table_facs, firmware_waking_vector);
>>  	acpi_sinfo.vector_width = 32;
>>  
>> +	acpi_os_unmap_memory(facs, sizeof(*facs));
> 
> Nit: looking at this again, I think you could move the
> acpi_os_unmap_memory further up, just after the last usage of facs
> (ie: before setting the wakeup_vector field).

Indeed I had it this way first, but then realized placing it here
results in less code churn in patch 4 of this series.

Jan

[PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

Use of __acpi_map_table() here was at least close to an abuse already
before, but it will now consistently return NULL here. Drop the layering
violation and use set_fixmap() directly. Re-use of the ACPI fixmap area
is hopefully going to remain "fine" for the time being.

Add checks to acpi_enter_sleep(): The vector now needs to be contained
within a single page, but the ACPI spec requires 64-byte alignment of
FACS anyway. Also bail if no wakeup vector was determined in the first
place, in part as preparation for a subsequent relaxation change.

Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -443,6 +443,11 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 			"FACS is shorter than ACPI spec allow: %#x",
 			facs->length);
 
+	if (facs_pa % 64)
+		printk(KERN_WARNING PREFIX
+			"FACS is not 64-byte aligned: %#lx",
+			facs_pa);
+
 	acpi_sinfo.wakeup_vector = facs_pa + 
 		offsetof(struct acpi_table_facs, firmware_waking_vector);
 	acpi_sinfo.vector_width = 32;
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
     if ( state != ACPI_STATE_S3 )
         return;
 
-    wakeup_vector_va = __acpi_map_table(
-        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
-
     /* TBoot will set resume vector itself (when it is safe to do so). */
     if ( tboot_in_measured_env() )
         return;
 
+    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
+    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
+                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
+
     if ( acpi_sinfo.vector_width == 32 )
         *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
     else
         *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
+
+    clear_fixmap(FIX_ACPI_END);
 }
 
 static void acpi_sleep_post(u32 state) {}
@@ -331,6 +334,12 @@ static long enter_state_helper(void *dat
  */
 int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep)
 {
+    if ( sleep->sleep_state == ACPI_STATE_S3 &&
+         (!acpi_sinfo.wakeup_vector || !acpi_sinfo.vector_width ||
+          (PAGE_OFFSET(acpi_sinfo.wakeup_vector) >
+           PAGE_SIZE - acpi_sinfo.vector_width / 8)) )
+        return -EOPNOTSUPP;
+
     if ( sleep->flags & XENPF_ACPI_SLEEP_EXTENDED )
     {
         if ( !acpi_sinfo.sleep_control.address ||

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
> Use of __acpi_map_table() here was at least close to an abuse already
> before, but it will now consistently return NULL here. Drop the layering
> violation and use set_fixmap() directly. Re-use of the ACPI fixmap area
> is hopefully going to remain "fine" for the time being.
> 
> Add checks to acpi_enter_sleep(): The vector now needs to be contained
> within a single page, but the ACPI spec requires 64-byte alignment of
> FACS anyway. Also bail if no wakeup vector was determined in the first
> place, in part as preparation for a subsequent relaxation change.
> 
> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -443,6 +443,11 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>  			"FACS is shorter than ACPI spec allow: %#x",
>  			facs->length);
>  
> +	if (facs_pa % 64)
> +		printk(KERN_WARNING PREFIX
> +			"FACS is not 64-byte aligned: %#lx",
> +			facs_pa);
> +
>  	acpi_sinfo.wakeup_vector = facs_pa + 
>  		offsetof(struct acpi_table_facs, firmware_waking_vector);
>  	acpi_sinfo.vector_width = 32;
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>      if ( state != ACPI_STATE_S3 )
>          return;
>  
> -    wakeup_vector_va = __acpi_map_table(
> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
> -
>      /* TBoot will set resume vector itself (when it is safe to do so). */
>      if ( tboot_in_measured_env() )
>          return;
>  
> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
> +
>      if ( acpi_sinfo.vector_width == 32 )
>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>      else
>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> +
> +    clear_fixmap(FIX_ACPI_END);

Why not use vmap here instead of the fixmap?

Thanks, Roger.

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 23.11.2020 16:24, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>      if ( state != ACPI_STATE_S3 )
>>          return;
>>  
>> -    wakeup_vector_va = __acpi_map_table(
>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>> -
>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>      if ( tboot_in_measured_env() )
>>          return;
>>  
>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>> +
>>      if ( acpi_sinfo.vector_width == 32 )
>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>      else
>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>> +
>> +    clear_fixmap(FIX_ACPI_END);
> 
> Why not use vmap here instead of the fixmap?

Considering the S3 path is relatively fragile (as in: we end up
breaking it more often than about anything else) I wanted to
make as little of a change as possible. Hence I decided to stick
to the fixmap use that was (indirectly) used before as well.

Jan

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
> On 23.11.2020 16:24, Roger Pau Monné wrote:
> > On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/acpi/power.c
> >> +++ b/xen/arch/x86/acpi/power.c
> >> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
> >>      if ( state != ACPI_STATE_S3 )
> >>          return;
> >>  
> >> -    wakeup_vector_va = __acpi_map_table(
> >> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
> >> -
> >>      /* TBoot will set resume vector itself (when it is safe to do so). */
> >>      if ( tboot_in_measured_env() )
> >>          return;
> >>  
> >> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
> >> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
> >> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
> >> +
> >>      if ( acpi_sinfo.vector_width == 32 )
> >>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> >>      else
> >>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> >> +
> >> +    clear_fixmap(FIX_ACPI_END);
> > 
> > Why not use vmap here instead of the fixmap?
> 
> Considering the S3 path is relatively fragile (as in: we end up
> breaking it more often than about anything else) I wanted to
> make as little of a change as possible. Hence I decided to stick
> to the fixmap use that was (indirectly) used before as well.

Unless there's a restriction to use the ACPI fixmap entry I would just
switch to use vmap, as it's used extensively in the code and less
likely to trigger issues in the future, or else a bunch of other stuff
would also be broken.

IMO doing the mapping differently here when it's not required will end
up turning this code more fragile in the long run.

Thanks, Roger.

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Andrew Cooper]

On 23/11/2020 16:07, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/acpi/power.c
>>>> +++ b/xen/arch/x86/acpi/power.c
>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>>>      if ( state != ACPI_STATE_S3 )
>>>>          return;
>>>>  
>>>> -    wakeup_vector_va = __acpi_map_table(
>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>>>> -
>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>>>      if ( tboot_in_measured_env() )
>>>>          return;
>>>>  
>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>>>> +
>>>>      if ( acpi_sinfo.vector_width == 32 )
>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>      else
>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>> +
>>>> +    clear_fixmap(FIX_ACPI_END);
>>> Why not use vmap here instead of the fixmap?
>> Considering the S3 path is relatively fragile (as in: we end up
>> breaking it more often than about anything else) I wanted to
>> make as little of a change as possible. Hence I decided to stick
>> to the fixmap use that was (indirectly) used before as well.
> Unless there's a restriction to use the ACPI fixmap entry I would just
> switch to use vmap, as it's used extensively in the code and less
> likely to trigger issues in the future, or else a bunch of other stuff
> would also be broken.
>
> IMO doing the mapping differently here when it's not required will end
> up turning this code more fragile in the long run.

We can't enter S3 at all until dom0 has booted, as one detail has to
come from AML.

Therefore, we're fully up and running by this point, and vmap() will be
fine.

However, why are we re-writing the wakeup vector every time?  Its fixed
by the position of the trampoline, so we'd actually simplify the S3 path
by only setting it up once.

~Andrew

(The fix for fragility is to actually test it, not shy away from making
any change)

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 23.11.2020 17:14, Andrew Cooper wrote:
> On 23/11/2020 16:07, Roger Pau Monné wrote:
>> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/acpi/power.c
>>>>> +++ b/xen/arch/x86/acpi/power.c
>>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>>>>      if ( state != ACPI_STATE_S3 )
>>>>>          return;
>>>>>  
>>>>> -    wakeup_vector_va = __acpi_map_table(
>>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>>>>> -
>>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>>>>      if ( tboot_in_measured_env() )
>>>>>          return;
>>>>>  
>>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>>>>> +
>>>>>      if ( acpi_sinfo.vector_width == 32 )
>>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>      else
>>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>> +
>>>>> +    clear_fixmap(FIX_ACPI_END);
>>>> Why not use vmap here instead of the fixmap?
>>> Considering the S3 path is relatively fragile (as in: we end up
>>> breaking it more often than about anything else) I wanted to
>>> make as little of a change as possible. Hence I decided to stick
>>> to the fixmap use that was (indirectly) used before as well.
>> Unless there's a restriction to use the ACPI fixmap entry I would just
>> switch to use vmap, as it's used extensively in the code and less
>> likely to trigger issues in the future, or else a bunch of other stuff
>> would also be broken.
>>
>> IMO doing the mapping differently here when it's not required will end
>> up turning this code more fragile in the long run.
> 
> We can't enter S3 at all until dom0 has booted, as one detail has to
> come from AML.
> 
> Therefore, we're fully up and running by this point, and vmap() will be
> fine.

That's not the point of my reservation. The code here runs when the
system already isn't "fully up and running" anymore. Secondary CPUs
have already been offlined, and we're around the point where we
disable interrupts. Granted when we disable them, we also turn off
spin debugging, but I'd still prefer a path that's not susceptible
to IRQ state. What I admit I didn't pay attention to is that
set_fixmap(), by virtue of being a thin wrapper around
map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
to vmap(). You're both aware that it, unlike set_fixmap(), can
fail, aren't you?

> However, why are we re-writing the wakeup vector every time?  Its fixed
> by the position of the trampoline, so we'd actually simplify the S3 path
> by only setting it up once.

I think the spec allows for (as in: doesn't preclude) firmware to
write this structure from scratch again each time the system comes
back up. Therefore what we've written there once may not survive
the first suspend/resume cycle.

> (The fix for fragility is to actually test it, not shy away from making
> any change)

Fair point. I'll see if I can convince my old laptop to cooperate.
I know Windows doesn't resume correctly on it ...

Jan

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 24.11.2020 12:04, Jan Beulich wrote:
> On 23.11.2020 17:14, Andrew Cooper wrote:
>> On 23/11/2020 16:07, Roger Pau Monné wrote:
>>> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>>>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/acpi/power.c
>>>>>> +++ b/xen/arch/x86/acpi/power.c
>>>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>>>>>      if ( state != ACPI_STATE_S3 )
>>>>>>          return;
>>>>>>  
>>>>>> -    wakeup_vector_va = __acpi_map_table(
>>>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>>>>>> -
>>>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>>>>>      if ( tboot_in_measured_env() )
>>>>>>          return;
>>>>>>  
>>>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>>>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>>>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>>>>>> +
>>>>>>      if ( acpi_sinfo.vector_width == 32 )
>>>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>>      else
>>>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>> +
>>>>>> +    clear_fixmap(FIX_ACPI_END);
>>>>> Why not use vmap here instead of the fixmap?
>>>> Considering the S3 path is relatively fragile (as in: we end up
>>>> breaking it more often than about anything else) I wanted to
>>>> make as little of a change as possible. Hence I decided to stick
>>>> to the fixmap use that was (indirectly) used before as well.
>>> Unless there's a restriction to use the ACPI fixmap entry I would just
>>> switch to use vmap, as it's used extensively in the code and less
>>> likely to trigger issues in the future, or else a bunch of other stuff
>>> would also be broken.
>>>
>>> IMO doing the mapping differently here when it's not required will end
>>> up turning this code more fragile in the long run.
>>
>> We can't enter S3 at all until dom0 has booted, as one detail has to
>> come from AML.
>>
>> Therefore, we're fully up and running by this point, and vmap() will be
>> fine.
> 
> That's not the point of my reservation. The code here runs when the
> system already isn't "fully up and running" anymore. Secondary CPUs
> have already been offlined, and we're around the point where we
> disable interrupts. Granted when we disable them, we also turn off
> spin debugging, but I'd still prefer a path that's not susceptible
> to IRQ state. What I admit I didn't pay attention to is that
> set_fixmap(), by virtue of being a thin wrapper around
> map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
> to vmap(). You're both aware that it, unlike set_fixmap(), can
> fail, aren't you?

Would at least one of the two of you please explicitly reply to
this last question, clarifying that you're indeed okay with this
new possible source of S3 entry failing?

Jan

Ping: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 30.11.2020 14:02, Jan Beulich wrote:
> On 24.11.2020 12:04, Jan Beulich wrote:
>> On 23.11.2020 17:14, Andrew Cooper wrote:
>>> On 23/11/2020 16:07, Roger Pau Monné wrote:
>>>> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>>>>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>>>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>>>>> --- a/xen/arch/x86/acpi/power.c
>>>>>>> +++ b/xen/arch/x86/acpi/power.c
>>>>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>>>>>>      if ( state != ACPI_STATE_S3 )
>>>>>>>          return;
>>>>>>>  
>>>>>>> -    wakeup_vector_va = __acpi_map_table(
>>>>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>>>>>>> -
>>>>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>>>>>>      if ( tboot_in_measured_env() )
>>>>>>>          return;
>>>>>>>  
>>>>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>>>>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>>>>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>>>>>>> +
>>>>>>>      if ( acpi_sinfo.vector_width == 32 )
>>>>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>>>      else
>>>>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>>> +
>>>>>>> +    clear_fixmap(FIX_ACPI_END);
>>>>>> Why not use vmap here instead of the fixmap?
>>>>> Considering the S3 path is relatively fragile (as in: we end up
>>>>> breaking it more often than about anything else) I wanted to
>>>>> make as little of a change as possible. Hence I decided to stick
>>>>> to the fixmap use that was (indirectly) used before as well.
>>>> Unless there's a restriction to use the ACPI fixmap entry I would just
>>>> switch to use vmap, as it's used extensively in the code and less
>>>> likely to trigger issues in the future, or else a bunch of other stuff
>>>> would also be broken.
>>>>
>>>> IMO doing the mapping differently here when it's not required will end
>>>> up turning this code more fragile in the long run.
>>>
>>> We can't enter S3 at all until dom0 has booted, as one detail has to
>>> come from AML.
>>>
>>> Therefore, we're fully up and running by this point, and vmap() will be
>>> fine.
>>
>> That's not the point of my reservation. The code here runs when the
>> system already isn't "fully up and running" anymore. Secondary CPUs
>> have already been offlined, and we're around the point where we
>> disable interrupts. Granted when we disable them, we also turn off
>> spin debugging, but I'd still prefer a path that's not susceptible
>> to IRQ state. What I admit I didn't pay attention to is that
>> set_fixmap(), by virtue of being a thin wrapper around
>> map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
>> to vmap(). You're both aware that it, unlike set_fixmap(), can
>> fail, aren't you?
> 
> Would at least one of the two of you please explicitly reply to
> this last question, clarifying that you're indeed okay with this
> new possible source of S3 entry failing?

I think we want to get this regression addressed, but without the
explicit consent of at least one of you that introducing a new
error source to the S3 path is indeed okay I'd prefer not to
prepare and then send v2. I expect there's going to be some code
churn (not the least because acpi_sleep_prepare() currently
returns void), and I'd rather avoid doing the conversion work
just to then be told to go back to the previous approach.

Jan

Re: Ping: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Roger Pau Monné]

On Wed, Dec 23, 2020 at 04:09:07PM +0100, Jan Beulich wrote:
> On 30.11.2020 14:02, Jan Beulich wrote:
> > On 24.11.2020 12:04, Jan Beulich wrote:
> >> On 23.11.2020 17:14, Andrew Cooper wrote:
> >>> On 23/11/2020 16:07, Roger Pau Monné wrote:
> >>>> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
> >>>>> On 23.11.2020 16:24, Roger Pau Monné wrote:
> >>>>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
> >>>>>>> --- a/xen/arch/x86/acpi/power.c
> >>>>>>> +++ b/xen/arch/x86/acpi/power.c
> >>>>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
> >>>>>>>      if ( state != ACPI_STATE_S3 )
> >>>>>>>          return;
> >>>>>>>  
> >>>>>>> -    wakeup_vector_va = __acpi_map_table(
> >>>>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
> >>>>>>> -
> >>>>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
> >>>>>>>      if ( tboot_in_measured_env() )
> >>>>>>>          return;
> >>>>>>>  
> >>>>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
> >>>>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
> >>>>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
> >>>>>>> +
> >>>>>>>      if ( acpi_sinfo.vector_width == 32 )
> >>>>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> >>>>>>>      else
> >>>>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> >>>>>>> +
> >>>>>>> +    clear_fixmap(FIX_ACPI_END);
> >>>>>> Why not use vmap here instead of the fixmap?
> >>>>> Considering the S3 path is relatively fragile (as in: we end up
> >>>>> breaking it more often than about anything else) I wanted to
> >>>>> make as little of a change as possible. Hence I decided to stick
> >>>>> to the fixmap use that was (indirectly) used before as well.
> >>>> Unless there's a restriction to use the ACPI fixmap entry I would just
> >>>> switch to use vmap, as it's used extensively in the code and less
> >>>> likely to trigger issues in the future, or else a bunch of other stuff
> >>>> would also be broken.
> >>>>
> >>>> IMO doing the mapping differently here when it's not required will end
> >>>> up turning this code more fragile in the long run.
> >>>
> >>> We can't enter S3 at all until dom0 has booted, as one detail has to
> >>> come from AML.
> >>>
> >>> Therefore, we're fully up and running by this point, and vmap() will be
> >>> fine.
> >>
> >> That's not the point of my reservation. The code here runs when the
> >> system already isn't "fully up and running" anymore. Secondary CPUs
> >> have already been offlined, and we're around the point where we
> >> disable interrupts. Granted when we disable them, we also turn off
> >> spin debugging, but I'd still prefer a path that's not susceptible
> >> to IRQ state. What I admit I didn't pay attention to is that
> >> set_fixmap(), by virtue of being a thin wrapper around
> >> map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
> >> to vmap(). You're both aware that it, unlike set_fixmap(), can
> >> fail, aren't you?
> > 
> > Would at least one of the two of you please explicitly reply to
> > this last question, clarifying that you're indeed okay with this
> > new possible source of S3 entry failing?
> 
> I think we want to get this regression addressed, but without the
> explicit consent of at least one of you that introducing a new
> error source to the S3 path is indeed okay I'd prefer not to
> prepare and then send v2. I expect there's going to be some code
> churn (not the least because acpi_sleep_prepare() currently
> returns void), and I'd rather avoid doing the conversion work
> just to then be told to go back to the previous approach.

Since this is a fix for a regression I think we should just take it in
order to avoid delaying any more, so I've Acked the patch.

My preference however would be for this to use vmap. Could the mapping
be established in acpi_fadt_parse_sleep_info instead of having to map
and unmap every time in acpi_sleep_prepare?

The physical address where the wakeup vector resides doesn't change
AFAICT, so it would be fine to keep the mapping.

Thanks, Roger.

Re: Ping: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 29.12.2020 11:54, Roger Pau Monné wrote:
> On Wed, Dec 23, 2020 at 04:09:07PM +0100, Jan Beulich wrote:
>> On 30.11.2020 14:02, Jan Beulich wrote:
>>> On 24.11.2020 12:04, Jan Beulich wrote:
>>>> On 23.11.2020 17:14, Andrew Cooper wrote:
>>>>> On 23/11/2020 16:07, Roger Pau Monné wrote:
>>>>>> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>>>>>>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>>>>>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>>>>>>> --- a/xen/arch/x86/acpi/power.c
>>>>>>>>> +++ b/xen/arch/x86/acpi/power.c
>>>>>>>>> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>>>>>>>>>      if ( state != ACPI_STATE_S3 )
>>>>>>>>>          return;
>>>>>>>>>  
>>>>>>>>> -    wakeup_vector_va = __acpi_map_table(
>>>>>>>>> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
>>>>>>>>> -
>>>>>>>>>      /* TBoot will set resume vector itself (when it is safe to do so). */
>>>>>>>>>      if ( tboot_in_measured_env() )
>>>>>>>>>          return;
>>>>>>>>>  
>>>>>>>>> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
>>>>>>>>> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
>>>>>>>>> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
>>>>>>>>> +
>>>>>>>>>      if ( acpi_sinfo.vector_width == 32 )
>>>>>>>>>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>>>>>      else
>>>>>>>>>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>>>>>>>>> +
>>>>>>>>> +    clear_fixmap(FIX_ACPI_END);
>>>>>>>> Why not use vmap here instead of the fixmap?
>>>>>>> Considering the S3 path is relatively fragile (as in: we end up
>>>>>>> breaking it more often than about anything else) I wanted to
>>>>>>> make as little of a change as possible. Hence I decided to stick
>>>>>>> to the fixmap use that was (indirectly) used before as well.
>>>>>> Unless there's a restriction to use the ACPI fixmap entry I would just
>>>>>> switch to use vmap, as it's used extensively in the code and less
>>>>>> likely to trigger issues in the future, or else a bunch of other stuff
>>>>>> would also be broken.
>>>>>>
>>>>>> IMO doing the mapping differently here when it's not required will end
>>>>>> up turning this code more fragile in the long run.
>>>>>
>>>>> We can't enter S3 at all until dom0 has booted, as one detail has to
>>>>> come from AML.
>>>>>
>>>>> Therefore, we're fully up and running by this point, and vmap() will be
>>>>> fine.
>>>>
>>>> That's not the point of my reservation. The code here runs when the
>>>> system already isn't "fully up and running" anymore. Secondary CPUs
>>>> have already been offlined, and we're around the point where we
>>>> disable interrupts. Granted when we disable them, we also turn off
>>>> spin debugging, but I'd still prefer a path that's not susceptible
>>>> to IRQ state. What I admit I didn't pay attention to is that
>>>> set_fixmap(), by virtue of being a thin wrapper around
>>>> map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
>>>> to vmap(). You're both aware that it, unlike set_fixmap(), can
>>>> fail, aren't you?
>>>
>>> Would at least one of the two of you please explicitly reply to
>>> this last question, clarifying that you're indeed okay with this
>>> new possible source of S3 entry failing?
>>
>> I think we want to get this regression addressed, but without the
>> explicit consent of at least one of you that introducing a new
>> error source to the S3 path is indeed okay I'd prefer not to
>> prepare and then send v2. I expect there's going to be some code
>> churn (not the least because acpi_sleep_prepare() currently
>> returns void), and I'd rather avoid doing the conversion work
>> just to then be told to go back to the previous approach.
> 
> Since this is a fix for a regression I think we should just take it in
> order to avoid delaying any more, so I've Acked the patch.

Thanks.

> My preference however would be for this to use vmap. Could the mapping
> be established in acpi_fadt_parse_sleep_info instead of having to map
> and unmap every time in acpi_sleep_prepare?

Establishing a permanent mapping would seem like a waste of resources
to me, no matter that it's just a single page. In particular if the
system would never really make any attempt to enter S3.

> The physical address where the wakeup vector resides doesn't change
> AFAICT, so it would be fine to keep the mapping.

True, even more so considering that if it changed, we'd have to
parse the ACPI table again to learn the new address, which we've
never done (and which aiui no OS is expected to do).

Jan

Re: Ping: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Roger Pau Monné]

On Mon, Jan 04, 2021 at 03:03:07PM +0100, Jan Beulich wrote:
> On 29.12.2020 11:54, Roger Pau Monné wrote:
> > My preference however would be for this to use vmap. Could the mapping
> > be established in acpi_fadt_parse_sleep_info instead of having to map
> > and unmap every time in acpi_sleep_prepare?
> 
> Establishing a permanent mapping would seem like a waste of resources
> to me, no matter that it's just a single page. In particular if the
> system would never really make any attempt to enter S3.

IMO I would rather have a simpler path going into suspension (ie: not
having to create a mapping to get the vector at all) with the cost of
using one page of virtual address space permanently.

Thanks, Roger.

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
> Use of __acpi_map_table() here was at least close to an abuse already
> before, but it will now consistently return NULL here. Drop the layering
> violation and use set_fixmap() directly. Re-use of the ACPI fixmap area
> is hopefully going to remain "fine" for the time being.
> 
> Add checks to acpi_enter_sleep(): The vector now needs to be contained
> within a single page, but the ACPI spec requires 64-byte alignment of
> FACS anyway. Also bail if no wakeup vector was determined in the first
> place, in part as preparation for a subsequent relaxation change.
> 
> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

See below for a comment.

> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -443,6 +443,11 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>  			"FACS is shorter than ACPI spec allow: %#x",
>  			facs->length);
>  
> +	if (facs_pa % 64)
> +		printk(KERN_WARNING PREFIX
> +			"FACS is not 64-byte aligned: %#lx",
> +			facs_pa);
> +
>  	acpi_sinfo.wakeup_vector = facs_pa + 
>  		offsetof(struct acpi_table_facs, firmware_waking_vector);
>  	acpi_sinfo.vector_width = 32;
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>      if ( state != ACPI_STATE_S3 )
>          return;
>  
> -    wakeup_vector_va = __acpi_map_table(
> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
> -
>      /* TBoot will set resume vector itself (when it is safe to do so). */
>      if ( tboot_in_measured_env() )
>          return;
>  
> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
> +
>      if ( acpi_sinfo.vector_width == 32 )
>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>      else
>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> +
> +    clear_fixmap(FIX_ACPI_END);
>  }
>  
>  static void acpi_sleep_post(u32 state) {}
> @@ -331,6 +334,12 @@ static long enter_state_helper(void *dat
>   */
>  int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep)
>  {
> +    if ( sleep->sleep_state == ACPI_STATE_S3 &&
> +         (!acpi_sinfo.wakeup_vector || !acpi_sinfo.vector_width ||
> +          (PAGE_OFFSET(acpi_sinfo.wakeup_vector) >
> +           PAGE_SIZE - acpi_sinfo.vector_width / 8)) )

Shouldn't this last check better be done in acpi_fadt_parse_sleep_info
and then avoid setting wakeup_vector in the first place?

Thanks, Roger.

Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich]

On 29.12.2020 11:51, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>> Use of __acpi_map_table() here was at least close to an abuse already
>> before, but it will now consistently return NULL here. Drop the layering
>> violation and use set_fixmap() directly. Re-use of the ACPI fixmap area
>> is hopefully going to remain "fine" for the time being.
>>
>> Add checks to acpi_enter_sleep(): The vector now needs to be contained
>> within a single page, but the ACPI spec requires 64-byte alignment of
>> FACS anyway. Also bail if no wakeup vector was determined in the first
>> place, in part as preparation for a subsequent relaxation change.
>>
>> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> See below for a comment.

For this please also note ...

>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -443,6 +443,11 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>>  			"FACS is shorter than ACPI spec allow: %#x",
>>  			facs->length);
>>  
>> +	if (facs_pa % 64)
>> +		printk(KERN_WARNING PREFIX
>> +			"FACS is not 64-byte aligned: %#lx",
>> +			facs_pa);
>> +
>>  	acpi_sinfo.wakeup_vector = facs_pa + 
>>  		offsetof(struct acpi_table_facs, firmware_waking_vector);
>>  	acpi_sinfo.vector_width = 32;

... the printk() getting added here. Violation of the spec here
implies entering S3 may fail because of ...

>> @@ -331,6 +334,12 @@ static long enter_state_helper(void *dat
>>   */
>>  int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep)
>>  {
>> +    if ( sleep->sleep_state == ACPI_STATE_S3 &&
>> +         (!acpi_sinfo.wakeup_vector || !acpi_sinfo.vector_width ||
>> +          (PAGE_OFFSET(acpi_sinfo.wakeup_vector) >
>> +           PAGE_SIZE - acpi_sinfo.vector_width / 8)) )
> 
> Shouldn't this last check better be done in acpi_fadt_parse_sleep_info
> and then avoid setting wakeup_vector in the first place?

... the check you talk about here, albeit these are independent
aspects: The spec requires even more strict alignment, and what
gets checked here is merely a precondition for the specific
implementation of ours, not tolerating the storage for the
vector to cross a page boundary. As such, I consider it more
appropriate for the check to live here, but yes, it could in
principle also be put there.

Jan

[PATCH 3/4] x86/DMI: fix table mapping when one lives above 1Mb

[Jan Beulich]

Use of __acpi_map_table() is kind of an abuse here, and doesn't work
anymore for the majority of cases if any of the tables lives outside the
low first Mb. Keep this (ab)use only prior to reaching SYS_STATE_boot,
primarily to avoid needing to audit whether any of the calls here can
happen this early in the first place; quite likely this isn't necessary
at all - at least dmi_scan_machine() gets called late enough.

For the "normal" case, call __vmap() directly, despite effectively
duplicating acpi_os_map_memory(). There's one difference though: We
shouldn't need to establish UC- mappings, WP or r/o WB mappings ought to
be fine, as the tables are going to live in either RAM or ROM. Short of
having PAGE_HYPERVISOR_WP and wanting to map the tables r/o anyway, use
the latter of the two options. The r/o mapping implies some
constification of code elsewhere in the file. For code touched anyway
also switch to void (where possible) or uint8_t.

Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/dmi_scan.c
+++ b/xen/arch/x86/dmi_scan.c
@@ -12,8 +12,6 @@
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 
-#define bt_ioremap(b,l)  ((void *)__acpi_map_table(b,l))
-#define bt_iounmap(b,l)  ((void)0)
 #define memcpy_fromio    memcpy
 #define alloc_bootmem(l) xmalloc_bytes(l)
 
@@ -111,9 +109,32 @@ enum dmi_entry_type {
 #define dmi_printk(x)
 #endif
 
-static char * __init dmi_string(struct dmi_header *dm, u8 s)
+static const void *__init bt_ioremap(paddr_t addr, unsigned int len)
 {
-	char *bp=(char *)dm;
+    mfn_t mfn = _mfn(PFN_DOWN(addr));
+    unsigned int offs = PAGE_OFFSET(addr);
+
+    if ( addr + len <= MB(1) )
+        return __va(addr);
+
+    if ( system_state < SYS_STATE_boot )
+        return __acpi_map_table(addr, len);
+
+    return __vmap(&mfn, PFN_UP(offs + len), 1, 1, PAGE_HYPERVISOR_RO,
+                  VMAP_DEFAULT) + offs;
+}
+
+static void __init bt_iounmap(const void *ptr, unsigned int len)
+{
+    if ( (unsigned long)ptr < DIRECTMAP_VIRT_START &&
+         system_state >= SYS_STATE_boot )
+        vunmap(ptr);
+}
+
+static const char *__init dmi_string(const struct dmi_header *dm, uint8_t s)
+{
+	const char *bp = (const void *)dm;
+
 	bp+=dm->length;
 	if(!s)
 		return "";
@@ -133,11 +154,10 @@ static char * __init dmi_string(struct d
  */
  
 static int __init dmi_table(paddr_t base, u32 len, int num,
-			    void (*decode)(struct dmi_header *))
+			    void (*decode)(const struct dmi_header *))
 {
-	u8 *buf;
-	struct dmi_header *dm;
-	u8 *data;
+	const uint8_t *buf, *data;
+	const struct dmi_header *dm;
 	int i=0;
 		
 	buf = bt_ioremap(base, len);
@@ -301,7 +321,7 @@ typedef union {
 
 static int __init _dmi_iterate(const struct dmi_eps *dmi,
 			       const smbios_eps_u smbios,
-			       void (*decode)(struct dmi_header *))
+			       void (*decode)(const struct dmi_header *))
 {
 	int num;
 	u32 len;
@@ -335,7 +355,7 @@ static int __init _dmi_iterate(const str
 	return dmi_table(base, len, num, decode);
 }
 
-static int __init dmi_iterate(void (*decode)(struct dmi_header *))
+static int __init dmi_iterate(void (*decode)(const struct dmi_header *))
 {
 	struct dmi_eps dmi;
 	struct smbios3_eps smbios3;
@@ -370,7 +390,7 @@ static int __init dmi_iterate(void (*dec
 	return -1;
 }
 
-static int __init dmi_efi_iterate(void (*decode)(struct dmi_header *))
+static int __init dmi_efi_iterate(void (*decode)(const struct dmi_header *))
 {
 	int ret = -1;
 
@@ -433,10 +453,11 @@ static char *__initdata dmi_ident[DMI_ST
  *	Save a DMI string
  */
  
-static void __init dmi_save_ident(struct dmi_header *dm, int slot, int string)
+static void __init dmi_save_ident(const struct dmi_header *dm, int slot, int string)
 {
-	char *d = (char*)dm;
-	char *p = dmi_string(dm, d[string]);
+	const char *d = (const void *)dm;
+	const char *p = dmi_string(dm, d[string]);
+
 	if(p==NULL || *p == 0)
 		return;
 	if (dmi_ident[slot])
@@ -629,10 +650,10 @@ static const struct dmi_blacklist __init
  *	out of here.
  */
 
-static void __init dmi_decode(struct dmi_header *dm)
+static void __init dmi_decode(const struct dmi_header *dm)
 {
 #ifdef DMI_DEBUG
-	u8 *data = (u8 *)dm;
+	const uint8_t *data = (const void *)dm;
 #endif
 	
 	switch(dm->type)

Re: [PATCH 3/4] x86/DMI: fix table mapping when one lives above 1Mb

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:40:30PM +0100, Jan Beulich wrote:
> Use of __acpi_map_table() is kind of an abuse here, and doesn't work
> anymore for the majority of cases if any of the tables lives outside the
> low first Mb. Keep this (ab)use only prior to reaching SYS_STATE_boot,
> primarily to avoid needing to audit whether any of the calls here can
> happen this early in the first place; quite likely this isn't necessary
> at all - at least dmi_scan_machine() gets called late enough.
> 
> For the "normal" case, call __vmap() directly, despite effectively
> duplicating acpi_os_map_memory(). There's one difference though: We
> shouldn't need to establish UC- mappings, WP or r/o WB mappings ought to
> be fine, as the tables are going to live in either RAM or ROM. Short of
> having PAGE_HYPERVISOR_WP and wanting to map the tables r/o anyway, use
> the latter of the two options. The r/o mapping implies some
> constification of code elsewhere in the file. For code touched anyway
> also switch to void (where possible) or uint8_t.
> 
> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH 4/4] x86/ACPI: don't invalidate S5 data when S3 wakeup vector cannot be determined

[Jan Beulich]

We can be more tolerant as long as the data collected from FACS is only
needed to enter S3. A prior change already added suitable checking to
acpi_enter_sleep().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -420,22 +420,22 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 		facs_pa = (uint64_t)fadt->facs;
 	}
 	if (!facs_pa)
-		goto bad;
+		return;
 
 	facs = acpi_os_map_memory(facs_pa, sizeof(*facs));
 	if (!facs)
-		goto bad;
+		return;
 
 	if (strncmp(facs->signature, "FACS", 4)) {
 		printk(KERN_ERR PREFIX "Invalid FACS signature %.4s\n",
 			facs->signature);
-		goto bad;
+		goto done;
 	}
 
 	if (facs->length < 24) {
 		printk(KERN_ERR PREFIX "Invalid FACS table length: %#x",
 			facs->length);
-		goto bad;
+		goto done;
 	}
 
 	if (facs->length < 64)
@@ -452,6 +452,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 		offsetof(struct acpi_table_facs, firmware_waking_vector);
 	acpi_sinfo.vector_width = 32;
 
+ done:
 	acpi_os_unmap_memory(facs, sizeof(*facs));
 
 	printk(KERN_INFO PREFIX

Re: [PATCH 4/4] x86/ACPI: don't invalidate S5 data when S3 wakeup vector cannot be determined

[Roger Pau Monné]

On Mon, Nov 23, 2020 at 01:41:06PM +0100, Jan Beulich wrote:
> We can be more tolerant as long as the data collected from FACS is only
> needed to enter S3. A prior change already added suitable checking to
> acpi_enter_sleep().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.