On 18 May 2015 at 20:28, Julien Grall <julien.gr...@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....@linaro.org>
> > Signed-off-by: Naresh Bhat <naresh.b...@linaro.org>
> > Signed-off-by: Parth Dixit <parth.di...@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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
