From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shannon Zhao Subject: Re: [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent Date: Fri, 22 Jan 2016 19:55:39 +0800 Message-ID: <56A218BB.9030808@linaro.org> References: <1452920477-13916-1-git-send-email-zhaoshenglong@huawei.com> <1452920477-13916-6-git-send-email-zhaoshenglong@huawei.com> <569CF7AF02000078000C806A@prv-mh.provo.novell.com> <56A1EA87.5050404@huawei.com> <56A1FABA02000078000C9E74@prv-mh.provo.novell.com> <56A1F863.1000507@huawei.com> <56A20F6502000078000C9F58@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56A20F6502000078000C9F58@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Shannon Zhao Cc: julien.grall@citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, peter.huangpeng@huawei.com List-Id: xen-devel@lists.xenproject.org On 2016/1/22 18:15, Jan Beulich wrote: >>>> On 22.01.16 at 10:37, wrote: >> > >> >On 2016/1/22 16:47, Jan Beulich wrote: >>>>>> >>>>>On 22.01.16 at 09:38, wrote: >>>>> >>> > >>>>> >>> >On 2016/1/18 21:33, Jan Beulich wrote: >>>>>>>>>>>>> >>>>>>> >>>>>On 16.01.16 at 06:01, 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). -- Shannon