| Message ID | 56A1EA87.5050404@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote: > > On 2016/1/18 21:33, Jan Beulich wrote: >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> 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
On 2016/1/22 16:47, Jan Beulich wrote: >>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote: >> > >> > On 2016/1/18 21:33, Jan Beulich wrote: >>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> 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,
>>> On 22.01.16 at 10:37, <zhaoshenglong@huawei.com> wrote: > > On 2016/1/22 16:47, Jan Beulich wrote: >>>>> On 22.01.16 at 09:38, <zhaoshenglong@huawei.com> wrote: >>> > >>> > On 2016/1/18 21:33, Jan Beulich wrote: >>>>>>> >>>>> On 16.01.16 at 06:01, <zhaoshenglong@huawei.com> 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
On 2016/1/22 18:15, Jan Beulich wrote: >>>> On 22.01.16 at 10:37,<zhaoshenglong@huawei.com> wrote: >> > >> >On 2016/1/22 16:47, Jan Beulich wrote: >>>>>> >>>>>On 22.01.16 at 09:38,<zhaoshenglong@huawei.com> wrote: >>>>> >>> > >>>>> >>> >On 2016/1/18 21:33, Jan Beulich wrote: >>>>>>>>>>>>> >>>>>>> >>>>>On 16.01.16 at 06:01,<zhaoshenglong@huawei.com> 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).
>>> On 22.01.16 at 12:55, <shannon.zhao@linaro.org> wrote: > > On 2016/1/22 18:15, Jan Beulich wrote: >>>>> On 22.01.16 at 10:37,<zhaoshenglong@huawei.com> wrote: >>> > >>> >On 2016/1/22 16:47, Jan Beulich wrote: >>>>>>> >>>>>On 22.01.16 at 09:38,<zhaoshenglong@huawei.com> wrote: >>>>>> >>> > >>>>>> >>> >On 2016/1/18 21:33, Jan Beulich wrote: >>>>>>>>>>>>>> >>>>>>> >>>>>On 16.01.16 at 06:01,<zhaoshenglong@huawei.com> 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
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; +} + /* * Important Safety Note: The fixed ACPI page numbers are *subtracted* * from the fixed base. That's why we start at FIX_ACPI_END and diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c index a2fc8c4..e2dda2e 100644 --- a/xen/drivers/acpi/osl.c +++ b/xen/drivers/acpi/osl.c @@ -88,16 +88,8 @@ 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; - } + if (system_state >= SYS_STATE_active) + return arch_acpi_os_map_memory(phys, size); return __acpi_map_table(phys, size); }