Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.16 at 12:55, <shannon.zhao@xxxxxxxxxx> wrote:

> 
> 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?

Yes.

Jan


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