[Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-02-28 Thread Shannon Zhao 
From: Shannon Zhao 

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

Signed-off-by: Shannon Zhao 
---
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);
+}
+}
+}
+
 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



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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-02-29 Thread Stefano Stabellini 
On Sun, 28 Feb 2016, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Map all other tables to Dom0 using 1:1 mappings.
> 
> Signed-off-by: Shannon Zhao 
> ---
> 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
> 
> 

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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-02-29 Thread Shannon Zhao 


On 2016/2/29 22:15, Stefano Stabellini wrote:
> On Sun, 28 Feb 2016, Shannon Zhao wrote:
>> > From: Shannon Zhao 
>> > 
>> > Map all other tables to Dom0 using 1:1 mappings.
>> > 
>> > Signed-off-by: Shannon Zhao 
>> > ---
>> > 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,
-- 
Shannon


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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-03-01 Thread Stefano Stabellini 
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 
> >> > 
> >> > Map all other tables to Dom0 using 1:1 mappings.
> >> > 
> >> > Signed-off-by: Shannon Zhao 
> >> > ---
> >> > 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 

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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-03-01 Thread Stefano Stabellini 
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 
> > >> > 
> > >> > Map all other tables to Dom0 using 1:1 mappings.
> > >> > 
> > >> > Signed-off-by: Shannon Zhao 
> > >> > ---
> > >> > 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 

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.

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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-03-02 Thread Shannon Zhao 
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 
>> > > >> > 
>> > > >> > Map all other tables to Dom0 using 1:1 mappings.
>> > > >> > 
>> > > >> > Signed-off-by: Shannon Zhao 
>> > > >> > ---
>> > > >> > 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 
> 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

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

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-03-02 Thread Stefano Stabellini 
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 
> >> > > >> > 
> >> > > >> > Map all other tables to Dom0 using 1:1 mappings.
> >> > > >> > 
> >> > > >> > Signed-off-by: Shannon Zhao 
> >> > > >> > ---
> >> > > >> > 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 
> >
> > 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.___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 13/24] arm/acpi: Map all other tables for Dom0

2016-03-02 Thread Jan Beulich 
>>> On 02.03.16 at 16:00,  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 
>> >> > > >> > 
>> >> > > >> > Map all other tables to Dom0 using 1:1 mappings.
>> >> > > >> > 
>> >> > > >> > Signed-off-by: Shannon Zhao 
>> >> > > >> > ---
>> >> > > >> > 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 
>> >
>> > 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

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