Re: [PATCH v2 08/41] arm/acpi : Parse FADT table and get PSCI flags

[Parth Dixit <parth.dixit@linaro.org>]

+shannon

On 24 May 2015 at 11:35, Parth Dixit <parth.dixit@linaro.org> wrote:
>
>
> On 18 May 2015 at 20:28, Julien Grall <julien.grall@citrix.com> wrote:
>>
>> Hi Parth,
>>
>> On 17/05/15 21:03, Parth Dixit wrote:
>> > There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set,
>> > the former signals to the OS that the hardware is PSCI compliant.
>> > The latter selects the appropriate conduit for PSCI calls by
>> > toggling between Hypervisor Calls (HVC) and Secure Monitor Calls
>> > (SMC).
>> >
>> > FADT table contains such information, parse FADT to get the flags
>> > for furture usage. At the same time, only ACPI 5.1 or higher verison
>> > supports PSCI, and FADT Major.Minor version was introduced in ACPI
>> > 5.1, so we will check the version and only parse FADT table with
>> > version >= 5.1.
>> >
>> > If firmware provides ACPI tables with ACPI version less than 5.1,
>> > OS will be messed up with those information, so disable ACPI if we
>> > get an FADT table with version less that 5.1.
>> >
>> > Modify FADT table before passing it to Dom0.
>> > Set PSCI_COMPLIANT and PSCI_USE_HVC.
>> >
>> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> > Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
>> > Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
>> > ---
>> >  xen/arch/arm/acpi/boot.c   | 38 ++++++++++++++++++++++++++++++++++++++
>> >  xen/arch/arm/acpi/lib.c    | 11 +++++++++++
>> >  xen/include/asm-arm/acpi.h | 11 +++++++++++
>> >  3 files changed, 60 insertions(+)
>> >
>> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
>> > index 8dc69d5..57eb33c 100644
>> > --- a/xen/arch/arm/acpi/boot.c
>> > +++ b/xen/arch/arm/acpi/boot.c
>> > @@ -24,9 +24,40 @@
>> >
>> >  #include <xen/init.h>
>> >  #include <xen/acpi.h>
>> > +#include <xen/errno.h>
>> > +#include <acpi/actables.h>
>> > +#include <xen/mm.h>
>> >
>> >  #include <asm/acpi.h>
>> >
>> > +static int __init acpi_parse_fadt(struct acpi_table_header *table)
>> > +{
>> > +    struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
>> > +    u8 checksum;
>> > +
>> > +    /*
>> > +     * Revision in table header is the FADT Major revision, and there
>> > +     * is a minor revision of FADT which was introduced by ACPI 5.1,
>> > +     * we only deal with ACPI 5.1 or newer revision to get GIC and SMP
>> > +     * boot protocol configuration data, or we will disable ACPI.
>> > +     */
>> > +    if ( table->revision > 5 ||
>> > +            ( table->revision == 5 && fadt->minor_revision >= 1 ) )
>>
>> The indentation looks wrong here.
>>
>> > +    {
>> > +        fadt->arm_boot_flags |= ( ACPI_FADT_PSCI_COMPLIANT |
>> > ACPI_FADT_PSCI_USE_HVC );
>> > +        checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, fadt),
>> > fadt->header.length);
>> > +        fadt->header.checksum -= checksum;
>> > +        clean_dcache_va_range(fadt, sizeof(struct acpi_table_fadt));
>>
>> Most of this patch is dealing with setting up correctly DOM0 FADT
>> although the title doesn't mention it and there is only 2 lines in the
>> commit message. This would also need comment in the need explaining what
>> this code does.
>>
>> Furthermore, I don't think this code should live here. The function is
>> called by acpi_boot_table_init which should initialize ACPI and not
>> trying to modify the ACPI table.
>>
> i'll split it into two patches with modifying of fadt going into seperate
> patch.
>>
>> We should have a specific dom0 acpi function to modify/add ACPI table
>> when it's necessary.
>>
>> > +        return 0;
>> > +    }
>> > +
>> > +    printk("Unsupported FADT revision %d.%d, should be 5.1+, will
>> > disable ACPI\n",
>> > +            table->revision, fadt->minor_revision);
>> > +    disable_acpi();
>> > +
>> > +    return -EINVAL;
>> > +}
>> > +
>> >  /*
>> >   * acpi_boot_table_init() called from setup_arch(), always.
>> >   *      1. find RSDP and get its address, and then find XSDT
>> > @@ -51,6 +82,13 @@ int __init acpi_boot_table_init(void)
>> >          return error;
>> >      }
>> >
>> > +    if ( acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt) )
>> > +    {
>> > +        /* disable ACPI if no FADT is found */
>> > +        disable_acpi();
>> > +        printk("Can't find FADT\n");
>> > +    }
>> > +
>>
>> I think the code readability will be improved if we introduce
>> acpi_get_table_with_size.
>>
>> Although, this is not implemented by ACPICA but only Linux. Jan may not
>> be agree to import it.
>>
>> >      return 0;
>> >  }
>> >
>> > diff --git a/xen/arch/arm/acpi/lib.c b/xen/arch/arm/acpi/lib.c
>> > index 650beed..fd9bfa4 100644
>> > --- a/xen/arch/arm/acpi/lib.c
>> > +++ b/xen/arch/arm/acpi/lib.c
>> > @@ -6,3 +6,14 @@ acpi_os_map_iomem(acpi_physical_address phys, acpi_size
>> > size)
>> >  {
>> >      return __va(phys);
>> >  }
>>
>> missing blank line
>>
>> > +/* 1 to indicate PSCI 0.2+ is implemented */
>> > +inline bool_t acpi_psci_present(void)
>>
>> inline is not necessary. Although, I would move the function in the
>> header because it's very simple.
>>
>> > +{
>> > +    return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT;
>> > +}
>> > +
>> > +/* 1 to indicate HVC is present instead of SMC as the PSCI conduit */
>> > +inline bool_t acpi_psci_hvc_present(void)
>>
>> Ditto.
>>
>> > +{
>> > +    return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC;
>> > +}
>>
>> Regards,
>>
>> --
>> --
>> Julien Grall
>
>