[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
On 2016/1/22 18:15, Jan Beulich wrote:
On 22.01.16 at 10:37,<zhaoshenglong@xxxxxxxxxx> wrote:
>
>On 2016/1/22 16:47, Jan Beulich wrote:
>>>>>On 22.01.16 at 09:38,<zhaoshenglong@xxxxxxxxxx> wrote:
>>> >
>>> >On 2016/1/18 21:33, Jan Beulich wrote:
>>>>>>> >>>>>On 16.01.16 at 06:01,<zhaoshenglong@xxxxxxxxxx> wrote:
>>>>>> >>> >--- a/xen/drivers/acpi/osl.c
>>>>>> >>> >+++ b/xen/drivers/acpi/osl.c
>>>>>> >>> >@@ -86,17 +86,7 @@ acpi_physical_address __init
>>> >acpi_os_get_root_pointer(void)
>>>>>> >>> > void __iomem *
>>>>>> >>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>>>>> >>> > {
>>>>>> >>> >- if (system_state >= SYS_STATE_active) {
>>>>>> >>> >- mfn_t mfn = _mfn(PFN_DOWN(phys));
>>>>>> >>> >- unsigned int offs = phys & (PAGE_SIZE - 1);
>>>>>> >>> >-
>>>>>> >>> >- /* The low first Mb is always mapped. */
>>>>>> >>> >- if ( !((phys + size - 1) >> 20) )
>>>>>> >>> >- return __va(phys);
>>>>>> >>> >- return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>>>>> >>> >- PAGE_HYPERVISOR_NOCACHE) + offs;
>>>>>> >>> >- }
>>>>>> >>> >- return __acpi_map_table(phys, size);
>>>>>> >>> >+ return arch_acpi_os_map_memory(phys, size);
>>>>>> >>> > }
>>>> >>I'm quite sure I've said before that this goes too far: The __vmap()
>>>> >>part and likely also the early-boot __acpi_map_table() one already
>>>> >>are architecture independent and hence should stay. The factoring
>>>> >>hence should only concern the first Mb handling and maybe the
>>>> >>the mapping attributes passed to __vmap().
>>> >
>>> >Yes, the first MB handling and __vmap() should refactor. So if it only
>>> >moves them to an architecture function, how about below patch?
>>> >
>>> >diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>> >index cc15ea3..5885a3a 100644
>>> >--- a/xen/arch/x86/acpi/lib.c
>>> >+++ b/xen/arch/x86/acpi/lib.c
>>> >@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>> > u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>> > {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>> >
>>> >+void __iomem *
>>> >+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>> >+{
>>> >+ mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> >+ unsigned int offs = phys & (PAGE_SIZE - 1);
>>> >+
>>> >+ /* The low first Mb is always mapped. */
>>> >+ if ( !((phys + size - 1) >> 20) )
>>> >+ return __va(phys);
>>> >+ return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> >+ PAGE_HYPERVISOR_NOCACHE) + offs;
>>> >+}
>>Well, I had clearly said the vmap() part is generic; if there's
>>anything architecture dependent here, then the mapping
>>attributes (and hence only_those_ should be factored out,
>>not the entire function invocation).
>I know what you said. But how can we change the attribute for ARM in
>acpi_os_map_memory() without moving these codes out?
By having each arch #define their value, and use that constant here?
You really want that? Even though the way here I use is not too many
dunplicated codes (and I think it looks clearer).
--
Shannon
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
