On 10/29/23 01:40, Michael Kelley (LINUX) wrote:
From: Abhinav Singh <singhabhinav9051571833@xxxxxxxxx> 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@xxxxxxxxx>
---
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
