From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: Design doc of adding ACPI support for arm64 on Xen - version 5 Date: Wed, 2 Sep 2015 17:37:09 +0200 Message-ID: References: <55E59B77.2090905@huawei.com> <55E5AADB.70503@citrix.com> <55E690DC.6000303@huawei.com> <55E6D8EA.4020008@citrix.com> <55E6E56C.80100@linaro.org> <55E6F110.7010704@citrix.com> <1441200364.26292.190.camel@citrix.com> <55E6FE11.3060401@citrix.com> <1441202054.26292.209.camel@citrix.com> <20150902152753.GB10728@bivouac.eciton.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150902152753.GB10728@bivouac.eciton.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Leif Lindholm Cc: "Huangpeng (Peter)" , Hangaohuai , Ian Campbell , Stefano Stabellini , andrew@fubar.geek.nz, xen-devel , Julien Grall , Stefano Stabellini , Shannon Zhao , Jan Beulich , Shannon Zhao , Boris Ostrovsky , Roger Pau Monne , Parth Dixit , Christoffer Dall , David Vrabel List-Id: xen-devel@lists.xenproject.org On 2 September 2015 at 17:27, Leif Lindholm 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 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.