diff mbox

[v4,13/24] arm/acpi: Map all other tables for Dom0

Message ID 1456658360-16080-14-git-send-email-zhaoshenglong@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao Feb. 28, 2016, 11:19 a.m. UTC
From: Shannon Zhao <shannon.zhao@linaro.org>

Map all other tables to Dom0 using 1:1 mappings.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
v4: fix commit message
---
 xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

Stefano Stabellini Feb. 29, 2016, 2:15 p.m. UTC | #1
On Sun, 28 Feb 2016, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Map all other tables to Dom0 using 1:1 mappings.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
> v4: fix commit message
> ---
>  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 64e48ae..6ad420c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>  }
>  
>  #ifdef CONFIG_ACPI
> +static void acpi_map_other_tables(struct domain *d)
> +{
> +    int i;
> +    unsigned long res;
> +    u64 addr, size;
> +
> +    /* Map all other tables to Dom0 using 1:1 mappings. */
> +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
> +    {
> +        addr = acpi_gbl_root_table_list.tables[i].address;
> +        size = acpi_gbl_root_table_list.tables[i].length;
> +        res = map_regions(d,
> +                          paddr_to_pfn(addr & PAGE_MASK),
> +                          DIV_ROUND_UP(size, PAGE_SIZE),
> +                          paddr_to_pfn(addr & PAGE_MASK));
> +        if ( res )
> +        {
> +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
> +                   " - 0x%"PRIx64" in domain \n",
> +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> +        }
> +    }
> +}

The problem with this function is that it is mapping all other tables to
the guest, including the unmodified FADT and MADT. This way dom0 is
going to find two different versions of the FADT and MADT, isn't that
right?


>  static int acpi_create_rsdp(struct domain *d, struct membank tbl_add[])
>  {
>  
> @@ -1661,6 +1685,8 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
>      if ( rc != 0 )
>          return rc;
>  
> +    acpi_map_other_tables(d);
> +
>      return 0;
>  }
>  #else
> -- 
> 2.0.4
> 
>
Shannon Zhao Feb. 29, 2016, 7:06 p.m. UTC | #2
On 2016/2/29 22:15, Stefano Stabellini wrote:
> On Sun, 28 Feb 2016, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Map all other tables to Dom0 using 1:1 mappings.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> > v4: fix commit message
>> > ---
>> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
>> >  1 file changed, 26 insertions(+)
>> > 
>> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> > index 64e48ae..6ad420c 100644
>> > --- a/xen/arch/arm/domain_build.c
>> > +++ b/xen/arch/arm/domain_build.c
>> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>> >  }
>> >  
>> >  #ifdef CONFIG_ACPI
>> > +static void acpi_map_other_tables(struct domain *d)
>> > +{
>> > +    int i;
>> > +    unsigned long res;
>> > +    u64 addr, size;
>> > +
>> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
>> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
>> > +    {
>> > +        addr = acpi_gbl_root_table_list.tables[i].address;
>> > +        size = acpi_gbl_root_table_list.tables[i].length;
>> > +        res = map_regions(d,
>> > +                          paddr_to_pfn(addr & PAGE_MASK),
>> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
>> > +                          paddr_to_pfn(addr & PAGE_MASK));
>> > +        if ( res )
>> > +        {
>> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
>> > +                   " - 0x%"PRIx64" in domain \n",
>> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>> > +        }
>> > +    }
>> > +}
> The problem with this function is that it is mapping all other tables to
> the guest, including the unmodified FADT and MADT. This way dom0 is
> going to find two different versions of the FADT and MADT, isn't that
> right?
>  
We've replaced the entries of XSDT table with new value. That means XSDT
points to new table. Guest will not see the old ones.

Thanks,
Stefano Stabellini March 1, 2016, 3:28 p.m. UTC | #3
On Tue, 1 Mar 2016, Shannon Zhao wrote:
> On 2016/2/29 22:15, Stefano Stabellini wrote:
> > On Sun, 28 Feb 2016, Shannon Zhao wrote:
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> > 
> >> > Map all other tables to Dom0 using 1:1 mappings.
> >> > 
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> > ---
> >> > v4: fix commit message
> >> > ---
> >> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
> >> >  1 file changed, 26 insertions(+)
> >> > 
> >> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >> > index 64e48ae..6ad420c 100644
> >> > --- a/xen/arch/arm/domain_build.c
> >> > +++ b/xen/arch/arm/domain_build.c
> >> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> >> >  }
> >> >  
> >> >  #ifdef CONFIG_ACPI
> >> > +static void acpi_map_other_tables(struct domain *d)
> >> > +{
> >> > +    int i;
> >> > +    unsigned long res;
> >> > +    u64 addr, size;
> >> > +
> >> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
> >> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
> >> > +    {
> >> > +        addr = acpi_gbl_root_table_list.tables[i].address;
> >> > +        size = acpi_gbl_root_table_list.tables[i].length;
> >> > +        res = map_regions(d,
> >> > +                          paddr_to_pfn(addr & PAGE_MASK),
> >> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
> >> > +                          paddr_to_pfn(addr & PAGE_MASK));
> >> > +        if ( res )
> >> > +        {
> >> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
> >> > +                   " - 0x%"PRIx64" in domain \n",
> >> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> >> > +        }
> >> > +    }
> >> > +}
> > The problem with this function is that it is mapping all other tables to
> > the guest, including the unmodified FADT and MADT. This way dom0 is
> > going to find two different versions of the FADT and MADT, isn't that
> > right?
> >  
> We've replaced the entries of XSDT table with new value. That means XSDT
> points to new table. Guest will not see the old ones.

All right. Of course it would be best to avoid mapping the original FADT
and MADT at all, but given that they are not likely to be page aligned,
it wouldn't be easy to do.

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Stefano Stabellini March 1, 2016, 5:01 p.m. UTC | #4
On Tue, 1 Mar 2016, Stefano Stabellini wrote:
> On Tue, 1 Mar 2016, Shannon Zhao wrote:
> > On 2016/2/29 22:15, Stefano Stabellini wrote:
> > > On Sun, 28 Feb 2016, Shannon Zhao wrote:
> > >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> > >> > 
> > >> > Map all other tables to Dom0 using 1:1 mappings.
> > >> > 
> > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > >> > ---
> > >> > v4: fix commit message
> > >> > ---
> > >> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
> > >> >  1 file changed, 26 insertions(+)
> > >> > 
> > >> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > >> > index 64e48ae..6ad420c 100644
> > >> > --- a/xen/arch/arm/domain_build.c
> > >> > +++ b/xen/arch/arm/domain_build.c
> > >> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> > >> >  }
> > >> >  
> > >> >  #ifdef CONFIG_ACPI
> > >> > +static void acpi_map_other_tables(struct domain *d)
> > >> > +{
> > >> > +    int i;
> > >> > +    unsigned long res;
> > >> > +    u64 addr, size;
> > >> > +
> > >> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
> > >> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
> > >> > +    {
> > >> > +        addr = acpi_gbl_root_table_list.tables[i].address;
> > >> > +        size = acpi_gbl_root_table_list.tables[i].length;
> > >> > +        res = map_regions(d,
> > >> > +                          paddr_to_pfn(addr & PAGE_MASK),
> > >> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
> > >> > +                          paddr_to_pfn(addr & PAGE_MASK));
> > >> > +        if ( res )
> > >> > +        {
> > >> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
> > >> > +                   " - 0x%"PRIx64" in domain \n",
> > >> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> > >> > +        }
> > >> > +    }
> > >> > +}
> > > The problem with this function is that it is mapping all other tables to
> > > the guest, including the unmodified FADT and MADT. This way dom0 is
> > > going to find two different versions of the FADT and MADT, isn't that
> > > right?
> > >  
> > We've replaced the entries of XSDT table with new value. That means XSDT
> > points to new table. Guest will not see the old ones.
> 
> All right. Of course it would be best to avoid mapping the original FADT
> and MADT at all, but given that they are not likely to be page aligned,
> it wouldn't be easy to do.
> 
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

However I have one more question: given that map_regions maps the memory
read-only to Dom0, isn't it possible that one or more of the DSDT
functions could not work as expected? I would imagine that the ACPI
bytecode is allowed to change its own memory, right?

Note that I am not suggesting to change map_regions to map memory
read-write, because that would be undesirable from a security
perspective.
Shannon Zhao March 2, 2016, 12:43 p.m. UTC | #5
On 2016?03?02? 01:01, Stefano Stabellini wrote:
> On Tue, 1 Mar 2016, Stefano Stabellini wrote:
>> > On Tue, 1 Mar 2016, Shannon Zhao wrote:
>>> > > On 2016/2/29 22:15, Stefano Stabellini wrote:
>>>> > > > On Sun, 28 Feb 2016, Shannon Zhao wrote:
>>>>>> > > >> > From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>> > > >> > 
>>>>>> > > >> > Map all other tables to Dom0 using 1:1 mappings.
>>>>>> > > >> > 
>>>>>> > > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>> > > >> > ---
>>>>>> > > >> > v4: fix commit message
>>>>>> > > >> > ---
>>>>>> > > >> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
>>>>>> > > >> >  1 file changed, 26 insertions(+)
>>>>>> > > >> > 
>>>>>> > > >> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>>> > > >> > index 64e48ae..6ad420c 100644
>>>>>> > > >> > --- a/xen/arch/arm/domain_build.c
>>>>>> > > >> > +++ b/xen/arch/arm/domain_build.c
>>>>>> > > >> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>>>>>> > > >> >  }
>>>>>> > > >> >  
>>>>>> > > >> >  #ifdef CONFIG_ACPI
>>>>>> > > >> > +static void acpi_map_other_tables(struct domain *d)
>>>>>> > > >> > +{
>>>>>> > > >> > +    int i;
>>>>>> > > >> > +    unsigned long res;
>>>>>> > > >> > +    u64 addr, size;
>>>>>> > > >> > +
>>>>>> > > >> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
>>>>>> > > >> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
>>>>>> > > >> > +    {
>>>>>> > > >> > +        addr = acpi_gbl_root_table_list.tables[i].address;
>>>>>> > > >> > +        size = acpi_gbl_root_table_list.tables[i].length;
>>>>>> > > >> > +        res = map_regions(d,
>>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK),
>>>>>> > > >> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
>>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK));
>>>>>> > > >> > +        if ( res )
>>>>>> > > >> > +        {
>>>>>> > > >> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
>>>>>> > > >> > +                   " - 0x%"PRIx64" in domain \n",
>>>>>> > > >> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>>>>>> > > >> > +        }
>>>>>> > > >> > +    }
>>>>>> > > >> > +}
>>>> > > > The problem with this function is that it is mapping all other tables to
>>>> > > > the guest, including the unmodified FADT and MADT. This way dom0 is
>>>> > > > going to find two different versions of the FADT and MADT, isn't that
>>>> > > > right?
>>>> > > >  
>>> > > We've replaced the entries of XSDT table with new value. That means XSDT
>>> > > points to new table. Guest will not see the old ones.
>> > 
>> > All right. Of course it would be best to avoid mapping the original FADT
>> > and MADT at all, but given that they are not likely to be page aligned,
>> > it wouldn't be easy to do.
>> > 
>> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> However I have one more question: given that map_regions maps the memory
> read-only to Dom0, isn't it possible that one or more of the DSDT
> functions could not work as expected? I would imagine that the ACPI
> bytecode is allowed to change its own memory, right?
> 
I'm not sure about this. But it seems that Xen or Linux always map these
tables to its memory.

Thanks,
--
Shannon
Stefano Stabellini March 2, 2016, 3 p.m. UTC | #6
On Wed, 2 Mar 2016, Shannon Zhao wrote:
> On 2016?03?02? 01:01, Stefano Stabellini wrote:
> > On Tue, 1 Mar 2016, Stefano Stabellini wrote:
> >> > On Tue, 1 Mar 2016, Shannon Zhao wrote:
> >>> > > On 2016/2/29 22:15, Stefano Stabellini wrote:
> >>>> > > > On Sun, 28 Feb 2016, Shannon Zhao wrote:
> >>>>>> > > >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >>>>>> > > >> > 
> >>>>>> > > >> > Map all other tables to Dom0 using 1:1 mappings.
> >>>>>> > > >> > 
> >>>>>> > > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>>>>> > > >> > ---
> >>>>>> > > >> > v4: fix commit message
> >>>>>> > > >> > ---
> >>>>>> > > >> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
> >>>>>> > > >> >  1 file changed, 26 insertions(+)
> >>>>>> > > >> > 
> >>>>>> > > >> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>>>>> > > >> > index 64e48ae..6ad420c 100644
> >>>>>> > > >> > --- a/xen/arch/arm/domain_build.c
> >>>>>> > > >> > +++ b/xen/arch/arm/domain_build.c
> >>>>>> > > >> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> >>>>>> > > >> >  }
> >>>>>> > > >> >  
> >>>>>> > > >> >  #ifdef CONFIG_ACPI
> >>>>>> > > >> > +static void acpi_map_other_tables(struct domain *d)
> >>>>>> > > >> > +{
> >>>>>> > > >> > +    int i;
> >>>>>> > > >> > +    unsigned long res;
> >>>>>> > > >> > +    u64 addr, size;
> >>>>>> > > >> > +
> >>>>>> > > >> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
> >>>>>> > > >> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
> >>>>>> > > >> > +    {
> >>>>>> > > >> > +        addr = acpi_gbl_root_table_list.tables[i].address;
> >>>>>> > > >> > +        size = acpi_gbl_root_table_list.tables[i].length;
> >>>>>> > > >> > +        res = map_regions(d,
> >>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK),
> >>>>>> > > >> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
> >>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK));
> >>>>>> > > >> > +        if ( res )
> >>>>>> > > >> > +        {
> >>>>>> > > >> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
> >>>>>> > > >> > +                   " - 0x%"PRIx64" in domain \n",
> >>>>>> > > >> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> >>>>>> > > >> > +        }
> >>>>>> > > >> > +    }
> >>>>>> > > >> > +}
> >>>> > > > The problem with this function is that it is mapping all other tables to
> >>>> > > > the guest, including the unmodified FADT and MADT. This way dom0 is
> >>>> > > > going to find two different versions of the FADT and MADT, isn't that
> >>>> > > > right?
> >>>> > > >  
> >>> > > We've replaced the entries of XSDT table with new value. That means XSDT
> >>> > > points to new table. Guest will not see the old ones.
> >> > 
> >> > All right. Of course it would be best to avoid mapping the original FADT
> >> > and MADT at all, but given that they are not likely to be page aligned,
> >> > it wouldn't be easy to do.
> >> > 
> >> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > However I have one more question: given that map_regions maps the memory
> > read-only to Dom0, isn't it possible that one or more of the DSDT
> > functions could not work as expected? I would imagine that the ACPI
> > bytecode is allowed to change its own memory, right?
> > 
> I'm not sure about this. But it seems that Xen or Linux always map these
> tables to its memory.

It's not mapping pages in general the problem. The potential issue comes
from the pages being mapped read-only. If an AML function in the DSDT
needs to write something to memory, I imagine that the function would
fail when called from Dom0.

I think we need to map them read-write, which is safe, even for the
original FADT and MADT, because by the time Dom0 gets to see them, Xen
won't parse them anymore (Xen completes parsing ACPI tables, before
booting Dom0).

So this patch is fine, but
http://marc.info/?l=xen-devel&m=145665887528175 needs to be changed to
use p2m_access_rw instead of p2m_access_r.
Jan Beulich March 2, 2016, 4 p.m. UTC | #7
>>> On 02.03.16 at 16:00, <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 2 Mar 2016, Shannon Zhao wrote:
>> On 2016?03?02? 01:01, Stefano Stabellini wrote:
>> > On Tue, 1 Mar 2016, Stefano Stabellini wrote:
>> >> > On Tue, 1 Mar 2016, Shannon Zhao wrote:
>> >>> > > On 2016/2/29 22:15, Stefano Stabellini wrote:
>> >>>> > > > On Sun, 28 Feb 2016, Shannon Zhao wrote:
>> >>>>>> > > >> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> >>>>>> > > >> > 
>> >>>>>> > > >> > Map all other tables to Dom0 using 1:1 mappings.
>> >>>>>> > > >> > 
>> >>>>>> > > >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> >>>>>> > > >> > ---
>> >>>>>> > > >> > v4: fix commit message
>> >>>>>> > > >> > ---
>> >>>>>> > > >> >  xen/arch/arm/domain_build.c | 26 ++++++++++++++++++++++++++
>> >>>>>> > > >> >  1 file changed, 26 insertions(+)
>> >>>>>> > > >> > 
>> >>>>>> > > >> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> >>>>>> > > >> > index 64e48ae..6ad420c 100644
>> >>>>>> > > >> > --- a/xen/arch/arm/domain_build.c
>> >>>>>> > > >> > +++ b/xen/arch/arm/domain_build.c
>> >>>>>> > > >> > @@ -1357,6 +1357,30 @@ static int prepare_dtb(struct domain *d, struct 
> kernel_info *kinfo)
>> >>>>>> > > >> >  }
>> >>>>>> > > >> >  
>> >>>>>> > > >> >  #ifdef CONFIG_ACPI
>> >>>>>> > > >> > +static void acpi_map_other_tables(struct domain *d)
>> >>>>>> > > >> > +{
>> >>>>>> > > >> > +    int i;
>> >>>>>> > > >> > +    unsigned long res;
>> >>>>>> > > >> > +    u64 addr, size;
>> >>>>>> > > >> > +
>> >>>>>> > > >> > +    /* Map all other tables to Dom0 using 1:1 mappings. */
>> >>>>>> > > >> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
>> >>>>>> > > >> > +    {
>> >>>>>> > > >> > +        addr = acpi_gbl_root_table_list.tables[i].address;
>> >>>>>> > > >> > +        size = acpi_gbl_root_table_list.tables[i].length;
>> >>>>>> > > >> > +        res = map_regions(d,
>> >>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK),
>> >>>>>> > > >> > +                          DIV_ROUND_UP(size, PAGE_SIZE),
>> >>>>>> > > >> > +                          paddr_to_pfn(addr & PAGE_MASK));
>> >>>>>> > > >> > +        if ( res )
>> >>>>>> > > >> > +        {
>> >>>>>> > > >> > +             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
>> >>>>>> > > >> > +                   " - 0x%"PRIx64" in domain \n",
>> >>>>>> > > >> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>> >>>>>> > > >> > +        }
>> >>>>>> > > >> > +    }
>> >>>>>> > > >> > +}
>> >>>> > > > The problem with this function is that it is mapping all other tables to
>> >>>> > > > the guest, including the unmodified FADT and MADT. This way dom0 is
>> >>>> > > > going to find two different versions of the FADT and MADT, isn't that
>> >>>> > > > right?
>> >>>> > > >  
>> >>> > > We've replaced the entries of XSDT table with new value. That means XSDT
>> >>> > > points to new table. Guest will not see the old ones.
>> >> > 
>> >> > All right. Of course it would be best to avoid mapping the original FADT
>> >> > and MADT at all, but given that they are not likely to be page aligned,
>> >> > it wouldn't be easy to do.
>> >> > 
>> >> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> >
>> > However I have one more question: given that map_regions maps the memory
>> > read-only to Dom0, isn't it possible that one or more of the DSDT
>> > functions could not work as expected? I would imagine that the ACPI
>> > bytecode is allowed to change its own memory, right?
>> > 
>> I'm not sure about this. But it seems that Xen or Linux always map these
>> tables to its memory.
> 
> It's not mapping pages in general the problem. The potential issue comes
> from the pages being mapped read-only. If an AML function in the DSDT
> needs to write something to memory, I imagine that the function would
> fail when called from Dom0.
> 
> I think we need to map them read-write, which is safe, even for the
> original FADT and MADT, because by the time Dom0 gets to see them, Xen
> won't parse them anymore (Xen completes parsing ACPI tables, before
> booting Dom0).
> 
> So this patch is fine, but
> http://marc.info/?l=xen-devel&m=145665887528175 needs to be changed to
> use p2m_access_rw instead of p2m_access_r.

Yes, I agree, r/w mappings ought to be fine here as long as only
Dom0 gets them.

Jan
diff mbox

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 64e48ae..6ad420c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1357,6 +1357,30 @@  static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 }
 
 #ifdef CONFIG_ACPI
+static void acpi_map_other_tables(struct domain *d)
+{
+    int i;
+    unsigned long res;
+    u64 addr, size;
+
+    /* Map all other tables to Dom0 using 1:1 mappings. */
+    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
+    {
+        addr = acpi_gbl_root_table_list.tables[i].address;
+        size = acpi_gbl_root_table_list.tables[i].length;
+        res = map_regions(d,
+                          paddr_to_pfn(addr & PAGE_MASK),
+                          DIV_ROUND_UP(size, PAGE_SIZE),
+                          paddr_to_pfn(addr & PAGE_MASK));
+        if ( res )
+        {
+             panic(XENLOG_ERR "Unable to map 0x%"PRIx64
+                   " - 0x%"PRIx64" in domain \n",
+                   addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+        }
+    }
+}
+
 static int acpi_create_rsdp(struct domain *d, struct membank tbl_add[])
 {
 
@@ -1661,6 +1685,8 @@  static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
     if ( rc != 0 )
         return rc;
 
+    acpi_map_other_tables(d);
+
     return 0;
 }
 #else