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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> >>>>>> > > >> > 
> >>>>>> > > >> > Map all other tables to Dom0 using 1:1 mappings.
> >>>>>> > > >> > 
> >>>>>> > > >> > Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> >>>>>> > > >> > ---
> >>>>>> > > >> > 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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel