Messages in this thread | | | Date | Sun, 29 Oct 2023 02:56:17 +0530 | Subject | Re: [PATCH] Fixing warning cast removes address space '__iomem' of expression | From | Abhinav Singh <> |
| |
On 10/29/23 01:40, Michael Kelley (LINUX) wrote: > From: Abhinav Singh <singhabhinav9051571833@gmail.com> Sent: Tuesday, October 24, 2023 4:29 AM >> > > Subject lines usually have a prefix to indicate the area of the kernel > the patch is for. We're not always super consistent with the prefixes, > but you can look at the commit log for a file to see what is > typically used. In this case, the prefix is usually "x86/hyperv:" > >> >> This patch fixes sparse complaining about the removal of __iomem address >> space when casting the return value of this function ioremap_cache(...) >> from `void __ioremap*` to `void*`. > > Should avoid wording like "this patch" in commit messages. See > the commit message guidelines in the "Describe your changes" > section of Documentation/process/submitting-patches.rst. A > better approach is to just state the problem: "Sparse complains > about the removal .....". Then describe the fix. Also avoid > pronouns like "I" or "you". > >> >> I think there are two way of fixing it, first one is changing the >> datatype of variable `ghcb_va` from `void*` to `void __iomem*` . >> Second way of fixing it is using the memremap(...) which is >> done in this patch. >> >> Signed-off-by: Abhinav Singh <singhabhinav9051571833@gmail.com> >> --- >> arch/x86/hyperv/hv_init.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c >> index 21556ad87f4b..c14161add274 100644 >> --- a/arch/x86/hyperv/hv_init.c >> +++ b/arch/x86/hyperv/hv_init.c >> @@ -70,7 +70,7 @@ static int hyperv_init_ghcb(void) >> >> /* Mask out vTOM bit. ioremap_cache() maps decrypted */ > > This comment mentions ioremap_cache(). Since you are changing > to use memremap() instead, the comment should be updated to > match. > >> ghcb_gpa &= ~ms_hyperv.shared_gpa_boundary; >> - ghcb_va = (void *)ioremap_cache(ghcb_gpa, HV_HYP_PAGE_SIZE); >> + ghcb_va = memremap(ghcb_gpa, HV_HYP_PAGE_SIZE, MEMREMAP_WB); > > As noted in the comment, ioremap_cache() provides a mapping that > accesses the memory as decrypted. To be equivalent, the call to > memremap() should include the MEMREMAP_DEC flag so that it > also is assured of producing a decrypted mapping. > > Also, corresponding to the current ioremap_cache() call here, > there's an iounmap() call in hv_cpu_die(). To maintain proper > pairing, that iounmap() call should be changed to memunmap(). > > It turns out there are other occurrences of this same pattern in > Hyper-V specific code in the Linux kernel. See hv_synic_enable_regs(), > for example.Did "sparse" flag the same problem in those > occurrences?
The particular warning msg for this case is like this "warning: cast removes address space '__iomem' of expression". I only saw these warning one time inside the arch/x86/ directory.
>It turns out that Nischala Yelchuri at Microsoft is > concurrently working on fixing this occurrence as well as the > others we know about in Hyper-V specific code.
So should I continue or not with this patch?
> > Michael > >> >> -- >> 2.39.2 >
Thanks for taking out the time for reviewing this and giving the suggestions.
Thank You, Abhinav Singh
| |