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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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?

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel