Re: [PATCH] ACPI: OSL: Handle the return value of acpi_os_map_generic_address() for a non-register GAS

[James Liu]

On Fri, Oct 22, 2021 at 04:55:59PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 22, 2021 at 3:18 AM <james.liu@xxxxxxx> wrote:
> >
> > From: James Liu <james.liu@xxxxxxx>
> >
> > Modify acpi_os_map_generic_address() to correctly handle a non-register
> > GAS (i.e., a pointer to a data structure), whose bit width is expected
> > to be 0 according to Table 5.1 in ACPI spec. 6.4.
> >
> > For example, the RegisterRegion field in SET_ERROR_TYPE_WITH_ADDRESS is a
> > non-register GAS, which points to a data structure whose format is defined
> > in Table 18.30 in ACPI Spec. 6.4.
> 
> IIUC, the RegisterRegion field is defined in Section 18.6.2 that says
> "This structure describes the physical address of a register as well
> as the bit range that corresponds to a desired region of the
> register".  This doesn't appear to be a non-register GAS to me.
>
Sorry, my description is improper.
I will rewrite it as "a GAS of a data structure".
 
> > This fix can prevent EINJ (Error Injection module) from loading failure
> > when a platform firmware can correctly support the format of Injection
> > Instruction Entry in an EINJ table.
> >
> > Signed-off-by: James Liu <james.liu@xxxxxxx>
> > ---
> >  drivers/acpi/osl.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 45c5c0e45e33..74de9341fadf 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -456,13 +456,23 @@ void __iomem *acpi_os_map_generic_address(struct acpi_generic_address *gas)
> >
> >         if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
> >                 return NULL;
> > +       /* Handle a non-register GAS (i.e., a pointer to a data structure),
>

Thanks, I will update it in the next version.
 
> Check CodingStyle for the expected format of comments.
> 
> > +        * whose bit width is expected to be 0 according to ACPI spec. 6.4.
> > +        * For example, The RegisterRegion field in SET_ERROR_TYPE_WITH_ADDRESS
> > +        * points to a data structure whose format is defined in Table 18.30 in
> > +        * ACPI Spec. 6.4
> > +        */
> > +       if (!gas->bit_width) {
> > +               pr_info("Mapping IOMEM for a non-register GAS.\n");
> > +               return  acpi_os_map_iomem(addr, sizeof(unsigned long long));
> 
> It is not clear to me at all why sizeof(unsigned long long) is the
> right size to use here.
>
It can be set as sizeof(addr) to avoid returning NULL when gas->bit_width equals 0,  
which means a GAS is used as a pointer to a data structure.
 
> IIUC, there is a data structure located at addr that needs to be
> accessed, but what if it is larger than the above size?
>
Your concern is correct. The size of iomem mapped at addr depends on the data 
structure used in the ACPI context (e.g., 36 bytes is for the data structure of
SET_ERROR_TYPE_WITH_ADDRESS). 

We could better defer the exact mapping to user/platform modules since the data 
structure information is neither defined in Address Space ID nor passed as an 
argument of acpi_os_map_generic_address().

Thus, this fix just makes sure EINJ can *still* be successfully loaded if OS modules
only use SET_ERROR_TYPE_WITHOUT_ADDRESS even though a platform firmware claims to
support SET_ERROR_TYPE_WITH_ADDRESS. To be more specific, this can avoid
pr_err("Error pre-mapping GARs.\n")in drivers/acpi/apei/einj.c.

> > +       }
> >
> >         /* Handle possible alignment issues */
> >         memcpy(&addr, &gas->address, sizeof(addr));
> > -       if (!addr || !gas->bit_width)
> > +       if (!addr)
> >                 return NULL;
> > -
> > -       return acpi_os_map_iomem(addr, gas->bit_width / 8);
> > +       else
> > +               return acpi_os_map_iomem(addr, gas->bit_width / 8);
> >  }
> >  EXPORT_SYMBOL(acpi_os_map_generic_address);
> >
> > --