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

Jan


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