Re: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

Re: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

[Bjorn Helgaas]

+cc Tony, linux-ia64

On Sat, Jan 21, 2012 at 8:04 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, Jan 20, 2012 at 7:13 PM, Myron Stowe <myron.stowe@redhat.com> wrote:
>> From: Myron Stowe <mstowe@redhat.com>
>>
>> This patch adds support for RAM to ACPI's mapping capabilities in order
>> to support APEI error injection (EINJ) actions.
>>
>> This patch re-factors similar functionality introduced in commit
>> 76da3fb3575, bringing it into osl.c in preparation for removing
>> ./drivers/acpi/atomicio.[ch].
>>
>> Signed-off-by: Myron Stowe <myron.stowe@redhat.com>
>> ---
>>
>>  drivers/acpi/osl.c |   36 ++++++++++++++++++++++++++++++++++--
>>  1 files changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index f363a55..8ee64ea 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/slab.h>
>>  #include <linux/mm.h>
>> +#include <linux/highmem.h>
>>  #include <linux/pci.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/kmod.h>
>> @@ -321,6 +322,37 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>>        return NULL;
>>  }
>>
>> +#ifndef CONFIG_IA64
>> +#define should_use_kmap(pfn)   page_is_ram(pfn)
>> +#else
>> +/* ioremap will take care of cache attributes */
>> +#define should_use_kmap(pfn)   0
>> +#endif
>> +
>> +static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned long pg_sz)
>> +{
>> +       unsigned long pfn;
>> +
>> +       pfn = pg_off >> PAGE_SHIFT;
>> +       if (should_use_kmap(pfn)) {
>> +               if (pg_sz > PAGE_SIZE)
>> +                       return NULL;
>> +               return (void __iomem __force *)kmap(pfn_to_page(pfn));
>> +       } else
>> +               return acpi_os_ioremap(pg_off, pg_sz);
>
> This implies that ioremap() works differently on ia64 than on x86.
> Apparently one can ioremap() RAM on x86, but not on ia64.  Why is this
> different?  Shouldn't we instead fix ioremap() on ia64 so it works the
> same as on x86?
>
> I looked at the ia64 ioremap(), and I can't see the reason it fails
> for RAM.  Huang, do you remember the details from 76da3fb3575?
>
>> +}
>> +
>> +static void acpi_unmap(acpi_physical_address pg_off, void __iomem *vaddr)
>> +{
>> +       unsigned long pfn;
>> +
>> +       pfn = pg_off >> PAGE_SHIFT;
>> +       if (page_is_ram(pfn))
>> +               kunmap(pfn_to_page(pfn));
>> +       else
>> +               iounmap(vaddr);
>
> I hope we can resolve the ioremap() question so we don't need this
> patch at all.  But if we do need this, I don't like the asymmetry here
> -- on x86 RAM, I think we use ioremap() and kunmap(), which seems
> wrong.  We should be able to use ioremap() and iounmap().
>
>> +}
>> +
>>  void __iomem *__init_refok
>>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>  {
>> @@ -353,7 +385,7 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>
>>        pg_off = round_down(phys, PAGE_SIZE);
>>        pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
>> -       virt = acpi_os_ioremap(pg_off, pg_sz);
>> +       virt = acpi_map(pg_off, pg_sz);
>>        if (!virt) {
>>                mutex_unlock(&acpi_ioremap_lock);
>>                kfree(map);
>> @@ -384,7 +416,7 @@ static void acpi_os_map_cleanup(struct acpi_ioremap *map)
>>  {
>>        if (!map->refcount) {
>>                synchronize_rcu();
>> -               iounmap(map->virt);
>> +               acpi_unmap(map->phys, map->virt);
>>                kfree(map);
>>        }
>>  }
>>

Re: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

[Thomas Renninger]

Hi,

Please ignore my previous mail, my mailer wrongly formatted
it with html tags and it got filtered out from the lists.

Firstly: This has to get in quickly if it shouldn't miss
3.3 (again). Tiny adjustings (I don't see any, beside the
issue Bjorn brought up) can still be done later, but I
guess this rather big one gets rejected by Linus after
the merge window closed.

Here again:

On Saturday, January 21, 2012 09:38:27 PM Bjorn Helgaas wrote:
> +cc Tony, linux-ia64
> 
...
> >>  {
> >> @@ -353,7 +385,7 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >>
> >>        pg_off = round_down(phys, PAGE_SIZE);
> >>        pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> >> -       virt = acpi_os_ioremap(pg_off, pg_sz);
> >> +       virt = acpi_map(pg_off, pg_sz);
Ok, that worked before, also on IA64.
That means that typically/normally/always io mem is tried to be mapped.

It's due to the new APEI infrastructure/features that ram could
get mapped via acpi_os_map_memory.

Is there any Itanium out there implementing any APEI table?
Will there ever be one?
Even if, isn't it a BIOS bug if such stuff is declared in ram and not
in reserved memory (also on X86)?
Especially on an Enterprise Itanium platform,
I guess the vendor should or better has to fix it up.

I'd do:
can_use_ioremap(pfn) instead of should_use_kmap(pfn)
and let it return false in ram + ia64 case, something like:
#ifdef IA64
#define can_use_ioremap(pfn)   !page_is_ram(pfn)
#endif

Pass the error upwards and APEI should get disabled on IA64,
if any ACPI code tries to ioremap real memory early (when
the APEI table parsing happens).
A nice FW_BUG message could be added as well (also on X86?).

Don't forget to use:
iounmap(vaddr);
only in acpi_unmap() then.

FWIW I even grepped for APEI tables on the most recent IA
machine we have -> no APEI tables.

If the rest is functionally the same as the patch series
you've send some months ago, feel free to add:

Reviewed-by: Thomas Renninger <trenn@suse.de>

I had a rather close look at those patches.

   Thomas

Re: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

[Myron Stowe]

On Mon, 2012-01-23 at 16:51 +0100, Thomas Renninger wrote:
> Hi,
> 
> Please ignore my previous mail, my mailer wrongly formatted
> it with html tags and it got filtered out from the lists.
> 
> Firstly: This has to get in quickly if it shouldn't miss
> 3.3 (again). Tiny adjustings (I don't see any, beside the
> issue Bjorn brought up) can still be done later, but I
> guess this rather big one gets rejected by Linus after
> the merge window closed.

Yes, I believe the obsolescence of atomicio.c from having taken the
conversion patch (patch 3/4 of the series) was inadvertent.  As such I
was trying to help by producing a series that addresses the two recent
additions to atomicio.c that needed ported over to osl.c so that we
would not loose such functionality with atomicio's obsolence as fast as
possible.  Linus may not allow such now that the 3.3 merge window is
closed but I think we all are thinking we should at least attempt to get
the regressions resolved immediately if possible.
> 
> Here again:
> 
> On Saturday, January 21, 2012 09:38:27 PM Bjorn Helgaas wrote:
> > +cc Tony, linux-ia64
> > 
> ...
> > >>  {
> > >> @@ -353,7 +385,7 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > >>
> > >>        pg_off = round_down(phys, PAGE_SIZE);
> > >>        pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > >> -       virt = acpi_os_ioremap(pg_off, pg_sz);
> > >> +       virt = acpi_map(pg_off, pg_sz);
> Ok, that worked before, also on IA64.
> That means that typically/normally/always io mem is tried to be mapped.
> 
> It's due to the new APEI infrastructure/features that ram could
> get mapped via acpi_os_map_memory.
> 
> Is there any Itanium out there implementing any APEI table?
> Will there ever be one?
> Even if, isn't it a BIOS bug if such stuff is declared in ram and not
> in reserved memory (also on X86)?
> Especially on an Enterprise Itanium platform,
> I guess the vendor should or better has to fix it up.
> 
> I'd do:
> can_use_ioremap(pfn) instead of should_use_kmap(pfn)
> and let it return false in ram + ia64 case, something like:
> #ifdef IA64
> #define can_use_ioremap(pfn)   !page_is_ram(pfn)
> #endif
> 
> Pass the error upwards and APEI should get disabled on IA64,
> if any ACPI code tries to ioremap real memory early (when
> the APEI table parsing happens).
> A nice FW_BUG message could be added as well (also on X86?).
> 
> Don't forget to use:
> iounmap(vaddr);
> only in acpi_unmap() then.

I must admit, while I understand the reason and concept related to
handling RAM for APEI, I do not understand memory management well enough
understand the details of the original implementation.  I was just
trying to cross-port the functionality over.  In doing such, I too, was
concerned as to the asymmetric nature of the paths (mapping vs.
unmapping) with respect to x86 as it just *felt* wrong.

Len - please let me know if you want me to adjust the patch with Thomas'
suggestions.

> 
> FWIW I even grepped for APEI tables on the most recent IA
> machine we have -> no APEI tables.
> 
> If the rest is functionally the same as the patch series
> you've send some months ago, feel free to add:
> 
> Reviewed-by: Thomas Renninger <trenn@suse.de>
> 
> I had a rather close look at those patches.

Thanks Thomas - yes, the removal patch - 3/3 - is the same.

Myron
> 
>    Thomas

RE: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

[Luck, Tony]

> Is there any Itanium out there implementing any APEI table?
> Will there ever be one?

With respect to the EINJ table - the answer is definitely no.
Itanium has a PAL mechanism for injecting errors, so there is
no need to implement EINJ.

-Tony

Re: [PATCH 2/3] ACPI, APEI: Add RAM mapping support to ACPI

[Huang Ying]

Hi, Thomas,

Sorry for late.  Just return from Chinese new year holiday.

On Mon, 2012-01-23 at 16:51 +0100, Thomas Renninger wrote:
> Hi,
> 
> Please ignore my previous mail, my mailer wrongly formatted
> it with html tags and it got filtered out from the lists.
> 
> Firstly: This has to get in quickly if it shouldn't miss
> 3.3 (again). Tiny adjustings (I don't see any, beside the
> issue Bjorn brought up) can still be done later, but I
> guess this rather big one gets rejected by Linus after
> the merge window closed.
> 
> Here again:
> 
> On Saturday, January 21, 2012 09:38:27 PM Bjorn Helgaas wrote:
> > +cc Tony, linux-ia64
> > 
> ...
> > >>  {
> > >> @@ -353,7 +385,7 @@ acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> > >>
> > >>        pg_off = round_down(phys, PAGE_SIZE);
> > >>        pg_sz = round_up(phys + size, PAGE_SIZE) - pg_off;
> > >> -       virt = acpi_os_ioremap(pg_off, pg_sz);
> > >> +       virt = acpi_map(pg_off, pg_sz);
> Ok, that worked before, also on IA64.
> That means that typically/normally/always io mem is tried to be mapped.
> 
> It's due to the new APEI infrastructure/features that ram could
> get mapped via acpi_os_map_memory.
> 
> Is there any Itanium out there implementing any APEI table?
> Will there ever be one?
> Even if, isn't it a BIOS bug if such stuff is declared in ram and not
> in reserved memory (also on X86)?
> Especially on an Enterprise Itanium platform,
> I guess the vendor should or better has to fix it up.

The reason for this patch can be found in description of my original
patch: 

    ACPI, Add RAM mapping support to ACPI atomic IO support
    
    On one of our testing machine, the following EINJ command lines:
    
      # echo 0x10000000 > param1
      # echo 0xfffffffffffff000 > param2
      # echo 0x8 > error_type
      # echo 1 > error_inject
    
    Will get:
    
      echo: write error: Input/output error
    
    The EIO comes from:
    
        rc = apei_exec_pre_map_gars(&trigger_ctx);
    
    The root cause is as follow.  Normally, ACPI atomic IO support is used
    to access IO memory.  But in EINJ of that machine, it is used to
    access RAM to trigger the injected error.  And the ioremap() called by
    apei_exec_pre_map_gars() can not map the RAM.
    
    This patch add RAM mapping support to ACPI atomic IO support to
    satisfy EINJ requirement.


I think that is reasonable to access the injected address to trigger
some memory error.  So it is hard to say this is a BIOS bug.

Best Regards,
Huang Ying