| Message ID | 1431893048-5214-9-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
+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 > >
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 ) ) + { + 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)); + 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"); + } + 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); } +/* 1 to indicate PSCI 0.2+ is implemented */ +inline bool_t acpi_psci_present(void) +{ + 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) +{ + return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC; +} diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h index f771056..058f343 100644 --- a/xen/include/asm-arm/acpi.h +++ b/xen/include/asm-arm/acpi.h @@ -29,6 +29,17 @@ #define COMPILER_DEPENDENT_UINT64 unsigned long long extern bool_t acpi_disabled; + +#ifdef CONFIG_ACPI +/* 1 to indicate PSCI 0.2+ is implemented */ +bool_t acpi_psci_present(void); +/* 1 to indicate HVC is present instead of SMC as the PSCI conduit */ +bool_t acpi_psci_hvc_present(void); +#else +static inline bool_t acpi_psci_present(void) { return false; } +static inline bool_t acpi_psci_hvc_present(void) {return false; } +#endif /* CONFIG_ACPI */ + /* Basic configuration for ACPI */ static inline void disable_acpi(void) {