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 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).

Jan


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