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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx wrote:
>>>>>>>>> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>>>>>>> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel