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

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 18 May 2015 at 20:28, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> ---
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel