[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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.