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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

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?

Thanks,
-- 
Shannon


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