Re: Design doc of adding ACPI support for arm64 on Xen - version 5

From: Leif Lindholm <leif.lindholm@linaro.org>
To: Christoffer Dall <christoffer.dall@linaro.org>
Cc: "Huangpeng (Peter)" <peter.huangpeng@huawei.com>,
	Hangaohuai <hangaohuai@huawei.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	andrew@fubar.geek.nz, xen-devel <xen-devel@lists.xen.org>,
	Julien Grall <julien.grall@citrix.com>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Shannon Zhao <shannon.zhao@linaro.org>,
	Jan Beulich <jbeulich@suse.com>,
	Shannon Zhao <zhaoshenglong@huawei.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Roger Pau Monne <roger.pau@citrix.com>,
	Parth Dixit <parth.dixit@linaro.org>,
	David Vrabel <david.vrabel@citrix.com>
Subject: Re: Design doc of adding ACPI support for arm64 on Xen - version 5
Date: Wed, 2 Sep 2015 16:27:53 +0100	[thread overview]
Message-ID: <20150902152753.GB10728@bivouac.eciton.net> (raw)
In-Reply-To: <CAMJs5B_daNZgObg9Gk+HXXUNwUPgp84qRwL_KLDQaP0-g3mjrA@mail.gmail.com>

On Wed, Sep 02, 2015 at 03:57:51PM +0200, Christoffer Dall wrote:
> On Wed, Sep 2, 2015 at 3:54 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-09-02 at 14:48 +0100, Julien Grall wrote:
> >> On 02/09/15 14:26, Ian Campbell wrote:
> >> > > > > I think the problem is how you reserved this region in the EFI
> >> > > > > memory
> >> > > > > table. From what I saw, you marked this new memory with
> >> > > > > EFI_MEMORY_WB
> >> > > > > (which means that the region can be usable by Linux).
> >> > > > >
> >> > > > Yes, I mark it with EFI_MEMORY_WB. Is this right?
> >> > >
> >> > > I would say no, but it's only because I looked at the kernel code
> >> > > quickly.
> >> > >
> >> > > You have to looks how ACPI region/UEFI tables are described in the
> >> > > host
> >> > > EFI memory map and mimicking for the DOM0 EFI memory map.
> >> >
> >> > Surely it is the type (EfiACPIReclaimMemory, EfiACPIMemoryNVS etc) and
> >> > not
> >> > the mapping attributes which should control whether an OS considers a
> >> > region usable? At least until the OS is done parsing tables neither of
> >> > those are usable (which implies we want NVS as our type, unless the
> >> > memory
> >> > is intended to be reclaimed by dom0, implying it should own it).

The mapping attributes are checked to see whether a page _could_ be
used as generic RAM or not. is_reserve_region() determines whether it
should.

> >> It looks like that Linux on ARM64 is considering any region with
> >> EFI_MEMORY_WB set as normal RAM and will try to add as System RAM (see
> >> reserve_regions in arch/arm64/kernel/efi.c).

The current state of things ends up being basically:
    if (EFI_MEMORY_WB)
       memblock_add()
    if (EFI_MEMORY_WB && !reclaimable_region)
       memblock_reserve()

That is, apart from counterintuitive, a bug.
It should be using memblock_remove() instead.

> > It's hard to believe this isn't a bug... It's probably worth asking the
> > Linux maintainers about this.
> 
> wasn't this that whole workaround to make sure Linux maps the data as
> regular RAM, because otherwise architecture generic code would map it
> as IO memory,

I hope not.

> and generic routines such as memcpy would fault on
> unaligned accesses, or am I confusing ACPI with EFI here?

Even ACPI tables should need to be in Normal memory in order to work
as expected.

> Leif (added to the to-field) had some insight on this earlier on.

(Adding Ard as well.)
Ard wrote a series end of last year to clean much of this up, but it's
not been merged:
http://thread.gmane.org/gmane.linux.kernel.efi/5133

We should probably push for this to go in as a bugfix, and those
interested in seeing this can weigh in in public.

/
    Leif