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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.03.16 at 16:00, <stefano.stabellini@xxxxxxxxxxxxx> 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@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.

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