acpi: Switch to dynamic mapping at SYS_STATE_boot

Commit Message

Boris Ostrovsky Jan. 30, 2017, 6:26 p.m. UTC

We can switch ACPI from using fixmap to dynamic mapping as soon as
the system enters SYS_STATE_boot. This will allow us, for example,
to map MADT on systems with large number of processors where the
table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).

Signed-off-by: Boris Ostrovsky <[e-mail]>
---
 xen/drivers/acpi/osl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Jan. 31, 2017, 11:01 a.m. UTC | #1

>>> On 30.01.17 at 19:26, <[e-mail]> wrote:
> We can switch ACPI from using fixmap to dynamic mapping as soon as
> the system enters SYS_STATE_boot. This will allow us, for example,
> to map MADT on systems with large number of processors where the
> table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).

I think this needs a little more care. While ARM switches to
SYS_STATE_boot after having called vm_init(), that's not the case
on x86. Granted there's no ACPI code being invoked in between,
but this would still be a latent bug (as soon as someone inserts any
code there).

Jan

Boris Ostrovsky Jan. 31, 2017, 3:52 p.m. UTC | #2

On 01/31/2017 06:01 AM, Jan Beulich wrote:
>>>> On 30.01.17 at 19:26, <[e-mail]> wrote:
>> We can switch ACPI from using fixmap to dynamic mapping as soon as
>> the system enters SYS_STATE_boot. This will allow us, for example,
>> to map MADT on systems with large number of processors where the
>> table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).
> I think this needs a little more care. While ARM switches to
> SYS_STATE_boot after having called vm_init(), that's not the case
> on x86. Granted there's no ACPI code being invoked in between,
> but this would still be a latent bug (as soon as someone inserts any
> code there).

Then acpi_os_map_memory() shouldn't rely on system_state at all.

How about adding something like

static bool_t vm_ready()
{
    return !!vm_base[VMAP_DEFAULT];
}

-boris

Jan Beulich Jan. 31, 2017, 3:56 p.m. UTC | #3

>>> On 31.01.17 at 16:52, <[e-mail]> wrote:
> On 01/31/2017 06:01 AM, Jan Beulich wrote:
>>>>> On 30.01.17 at 19:26, <[e-mail]> wrote:
>>> We can switch ACPI from using fixmap to dynamic mapping as soon as
>>> the system enters SYS_STATE_boot. This will allow us, for example,
>>> to map MADT on systems with large number of processors where the
>>> table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).
>> I think this needs a little more care. While ARM switches to
>> SYS_STATE_boot after having called vm_init(), that's not the case
>> on x86. Granted there's no ACPI code being invoked in between,
>> but this would still be a latent bug (as soon as someone inserts any
>> code there).
> 
> Then acpi_os_map_memory() shouldn't rely on system_state at all.

Why?

> How about adding something like
> 
> static bool_t vm_ready()
> {
>     return !!vm_base[VMAP_DEFAULT];
> }

What's wrong with just swapping the order of the two things in
x86's setup.c?

Jan

Boris Ostrovsky Jan. 31, 2017, 4:05 p.m. UTC | #4

On 01/31/2017 10:56 AM, Jan Beulich wrote:
>>>> On 31.01.17 at 16:52, <[e-mail]> wrote:
>> On 01/31/2017 06:01 AM, Jan Beulich wrote:
>>>>>> On 30.01.17 at 19:26, <[e-mail]> wrote:
>>>> We can switch ACPI from using fixmap to dynamic mapping as soon as
>>>> the system enters SYS_STATE_boot. This will allow us, for example,
>>>> to map MADT on systems with large number of processors where the
>>>> table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).
>>> I think this needs a little more care. While ARM switches to
>>> SYS_STATE_boot after having called vm_init(), that's not the case
>>> on x86. Granted there's no ACPI code being invoked in between,
>>> but this would still be a latent bug (as soon as someone inserts any
>>> code there).
>> Then acpi_os_map_memory() shouldn't rely on system_state at all.
> Why?

Because it really is only interested in whether vm_init() has been
called, nor what the overall system state is (even if vmap status can be
implied from the state).

But I can swap the two if you prefer that.

-boris

>
>> How about adding something like
>>
>> static bool_t vm_ready()
>> {
>>     return !!vm_base[VMAP_DEFAULT];
>> }
> What's wrong with just swapping the order of the two things in
> x86's setup.c?

I

Jan Beulich Jan. 31, 2017, 4:27 p.m. UTC | #5

>>> On 31.01.17 at 17:05, <[e-mail]> wrote:
> On 01/31/2017 10:56 AM, Jan Beulich wrote:
>>>>> On 31.01.17 at 16:52, <[e-mail]> wrote:
>>> On 01/31/2017 06:01 AM, Jan Beulich wrote:
>>>>>>> On 30.01.17 at 19:26, <[e-mail]> wrote:
>>>>> We can switch ACPI from using fixmap to dynamic mapping as soon as
>>>>> the system enters SYS_STATE_boot. This will allow us, for example,
>>>>> to map MADT on systems with large number of processors where the
>>>>> table might not fit into NUM_FIXMAP_ACPI_PAGES (currently set to 4).
>>>> I think this needs a little more care. While ARM switches to
>>>> SYS_STATE_boot after having called vm_init(), that's not the case
>>>> on x86. Granted there's no ACPI code being invoked in between,
>>>> but this would still be a latent bug (as soon as someone inserts any
>>>> code there).
>>> Then acpi_os_map_memory() shouldn't rely on system_state at all.
>> Why?
> 
> Because it really is only interested in whether vm_init() has been
> called, nor what the overall system state is (even if vmap status can be
> implied from the state).
> 
> But I can swap the two if you prefer that.

Seems better to me than introducing vm_ready(). But perhaps
others are of differing opinion...

Jan

Patch

diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 3616dfd..7199047 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -89,7 +89,7 @@  acpi_physical_address __init acpi_os_get_root_pointer(void)
 void __iomem *
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-	if (system_state >= SYS_STATE_active) {
+	if (system_state >= SYS_STATE_boot) {
 		mfn_t mfn = _mfn(PFN_DOWN(phys));
 		unsigned int offs = phys & (PAGE_SIZE - 1);
 
@@ -104,7 +104,7 @@  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 
 void acpi_os_unmap_memory(void __iomem * virt, acpi_size size)
 {
-	if (system_state >= SYS_STATE_active)
+	if (system_state >= SYS_STATE_boot)
 		vunmap((void *)((unsigned long)virt & PAGE_MASK));
 }