Re: [PATCH v3 07/62] arm/acpi: Add arch_acpi_os_map_memory helper function for ARM

From: Shannon Zhao <zhaoshenglong@huawei.com>
To: Julien Grall <julien.grall@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Jan Beulich <JBeulich@suse.com>,
	shannon.zhao@linaro.org
Cc: hangaohuai@huawei.com, xen-devel@lists.xen.org,
	stefano.stabellini@citrix.com, peter.huangpeng@huawei.com,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH v3 07/62] arm/acpi: Add arch_acpi_os_map_memory helper function for ARM
Date: Tue, 29 Dec 2015 20:01:29 +0800	[thread overview]
Message-ID: <56827619.7040000@huawei.com> (raw)
In-Reply-To: <56657554.20205@citrix.com>

Hi,

On 2015/12/7 20:02, Julien Grall wrote:
> On 07/12/15 10:38, Ian Campbell wrote:
>> On Mon, 2015-12-07 at 03:32 -0700, Jan Beulich wrote:
>>>>>> On 07.12.15 at 09:58, <zhaoshenglong@huawei.com> wrote:
>>>> On 2015/11/30 22:47, Julien Grall wrote:
>>>>> On 23/11/15 11:37, Stefano Stabellini wrote:
>>>>>>> On Tue, 17 Nov 2015, shannon.zhao@linaro.org wrote:
>>>>>>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>> could you please add a couple of lines to the commit message
>>>>>>> mentioning
>>>>>>> why __va(phys) is an acceptable implementation of
>>>>>>> arch_acpi_os_map_memory?
>>>>> FWIW, I already asked this question multiple time on the previous
>>>>> series
>>>>> without clear answer.
>>>>>
>>>>> __va should only be used when the memory is direct-mapped to Xen (i.e
>>>>> accessible directly). On ARM64, this is only the case for the RAM.
>>>>> Can
>>>>> someone confirm the ACPI will always reside to the RAM?
>>>> I checked this with the UEFI SPEC. It says in 2.3.6 AArch64 Platforms:
>>>> "If ACPI is supported :
>>>> ● ACPI Tables loaded at boot time can be contained in memory of type
>>>> EfiACPIReclaimMemory (recommended) or EfiACPIMemoryNVS."
>>>>
>>>> So I think it means the ACPI tables will always reside in RAM.
>>>
>>> I think NVS doesn't necessarily mean RAM.
>>
>> That's what I thought/expected too (sounds like it could be flash, CMOS,
>> ROM etc), but looking at the ACPI 6.0 spec ("16.3.2 BIOS Initialization of
>> Memory") it says:
>>
>>     Memory identified by the BIOS as being     reserved by the BIOS for its use.
>>     OSPM is required to tag this memory as cacheable, and to save     and
>>     restore its image before entering an S4 state.
>>
>> IOW, if I'm reading that correctly, it actually is "memory", and it is only
>> "NV" by virtue of requiring the OSPM to save/restore it over suspend (so
>> not at all "NV" in the usual sense).
> 
> I think it would be safer to use vmap. It won't impact much Xen as the
> mapping are mostly used during Xen boot and DOM0 creation.
> 

If it uses vmap, it needs to move acpi_boot_table_init after vm_init(),
right?

I move acpi_boot_table_init after vm_init() and rewrite
arch_acpi_os_map_memory like below.

 void __iomem *
 arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
-    return __va(phys);
+    mfn_t mfn = _mfn(PFN_DOWN(phys));
+    unsigned int offs = phys & (PAGE_SIZE - 1);
+
+    return __vmap(&mfn, PFN_UP(offs + size), 1, 1, PAGE_HYPERVISOR)
+           + offs;
 }

But there is a weird problem. All the ACPI tables are installed
correctly within acpi_boot_table_init() and it prints with below output:

(XEN) ACPI: RSDP FEC90014, 0024 (r2 LINARO)
(XEN) ACPI: XSDT FEC800E8, 0044 (r1 LINARO RTSMVEV8        0       1000013)
(XEN) ACPI: FACP FEC50000, 0114 (r6 LINARO RTSMVEV8        0 INTL 20150619)
(XEN) ACPI: DSDT FEC60000, 0248 (r2 LINARO RTSMVEV8        4 INTL 20150619)
(XEN) ACPI: APIC FEC70000, 02D4 (r3 LINARO RTSMVEV8        1 INTL 20150619)
(XEN) ACPI: GTDT FEC40000, 009C (r2 LINARO RTSMVEV8        1 INTL 20150619)
(XEN) ACPI: SPCR FEC30000, 0050 (r1 LINARO RTSMVEV8        0 INTL 20150619)

As you see, the physical address of DSDT table is FEC60000, but when I
print acpi_gbl_root_table_list.tables[0].address after
acpi_boot_table_init(), it becomes aafec60000. And it even becomes
d6aaaaaafec60000 when it's used in acpi_map_rest_tables(see patch 49).

But others of acpi_gbl_root_table_list don't change. E.g.
acpi_gbl_root_table_list.tables[2].address is always FEC50000.

-- 
Shannon

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel