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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 2 September 2015 at 17:27, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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.
>

Only the following regions are ever considered to be usable as normal memory:

EFI_LOADER_CODE:
EFI_LOADER_DATA:
EFI_BOOT_SERVICES_CODE:
EFI_BOOT_SERVICES_DATA:
EFI_CONVENTIONAL_MEMORY:
EFI_PERSISTENT_MEMORY:

The code in is_reserve_region() reads a bit backwards, but the only
reason for the is_normal_ram() in there is to prevent non-memory
regions (i.e., MMIO regions for the RTC or NOR flash the UEFI uses) to
be memblock_reserve()d.

>> >> 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.
>

Indeed. It would be much better to never add reserved memory to the
linear mapping this way, but the problem is that we lose all
annotation of this region being backed by memory. I.e., pfn_valid()
will return false, and the ACPI code will map it as a device,
resulting in alignment faults when traversing ACPI tables it contains.

So in the series Leif refers to below, I use the physmem memblock
table to record all regions, use the memory memblock table to only
record the regions available to the OS. That way, we can keep track of
which regions are backed by RAM but not covered by the linear mapping.

>> > 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.
>

Indeed.

>> 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.
>

I will brush up the series and repost.

-- 
Ard.