Re: [Xen-devel] [PATCH RFC 29/35] arm : acpi enable PSCI and hvc in acpi FADT table

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 6 Feb 2015, Julien Grall wrote:
> Hi Parth,
> 
> On 06/02/2015 01:12, Stefano Stabellini wrote:
> > On Wed, 4 Feb 2015, parth.dixit@xxxxxxxxxx wrote:
> > > From: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > > 
> > > Enable PSCI and hvc flags in FADT table so that dom0 uses PSCI to
> > > boot vcpu's
> > > 
> > > Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > > ---
> > >   xen/arch/arm/arm64/acpi/arm-core.c | 16 ++++++++++++++++
> > >   1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/xen/arch/arm/arm64/acpi/arm-core.c
> > > b/xen/arch/arm/arm64/acpi/arm-core.c
> > > index 6707e4c..9a26202 100644
> > > --- a/xen/arch/arm/arm64/acpi/arm-core.c
> > > +++ b/xen/arch/arm/arm64/acpi/arm-core.c
> > > @@ -28,6 +28,7 @@
> > >   #include <xen/errno.h>
> > >   #include <xen/stdbool.h>
> > >   #include <xen/cpumask.h>
> > > +#include <acpi/actables.h>
> > > 
> > >   #include <asm/cputype.h>
> > >   #include <asm/acpi.h>
> > > @@ -242,6 +243,19 @@ static int __init acpi_parse_fadt(struct
> > > acpi_table_header *table)
> > >          return 0;
> > >   }
> > > 
> > > +static void set_psci_fadt(void)
> > > +{
> > > +    struct acpi_table_fadt *fadt=NULL;
> > > +    struct acpi_table_header *table=NULL;
> > > +    u8 checksum;
> > > +
> > > +    acpi_get_table(ACPI_SIG_FADT, 0, &table);
> > > +    fadt = (struct acpi_table_fadt *)table;
> > > +    fadt->arm_boot_flags |= ( ACPI_FADT_PSCI_COMPLIANT |
> > > ACPI_FADT_PSCI_USE_HVC );
> > 
> > Are we actually allowed to modify the real acpi table passed by the
> > firmware?
> > Could it be read-only?
> > 
> > If we can change it,  do we need a dsb() before the acpi_tb_checksum?
> 
> IIRC, the ACPI memory region is mapped cached and preparing the ACPI blob is
> only done in one processor. So the dsb() is not necessary here.

I was being paranoid and thinking that the memory dependency might not
be explicit enough and could get a very keen compiler confused, ending
up with reordering the checksum calculation and the field update.

However as we specify -fno-strict-aliasing to compile Xen, I think that
we are allowed to assume that the compiler won't do that.


> But ... that made me think that we should clear the cache after changes in the
> table. We can't assume that the guest will map with cache attribute the ACPI
> tables.

You are right and because we cannot guess what caching attributes dom0
is going to use, we also need to make sure to clean the dcache and unmap
the relative pages in Xen because we don't want to have the same page
mapped with two different caching attributes in the system.

The alternative would be to mandate that dom0 maps all the acpi pages as
cached memory always.  In practice I think it is going to be the case
for Linux. But I don't like to set this kind of restrictions to Dom0.

To summarize, the code sequence should be something like:

change a field in an acpi table
update checksum
clean_dcache_va_range on the table

[...]

unmap acpi table pages in Xen
map acpi pages in the guest

[...]

boot dom0

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