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*`.
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 <[email protected]>
---
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 */
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);
if (!ghcb_va)
return -ENOMEM;
--
2.39.2
From: Abhinav Singh <[email protected]> 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 <[email protected]>
> ---
> 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? 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.
Michael
>
> --
> 2.39.2
On 10/29/23 01:40, Michael Kelley (LINUX) wrote:
> From: Abhinav Singh <[email protected]> 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 <[email protected]>
>> ---
>> 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
From: Abhinav Singh <[email protected]> Sent: Saturday, October 28, 2023 2:26 PM
>
> >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?
>
I'll suggest deferring to Nischala's pending patch. Her patch
covers all 5 occurrences. Also, I don't know if you were able
to test your patch in a Hyper-V Confidential VM. She did test
in that configuration and confirmed that nothing broke.
Of course, feel free to correspond directly with Nischala
on how best to move forward. I'm an unnecessary intermediary
who is just pointing out the overlap. :-)
Michael
On 10/30/23 22:08, Michael Kelley (LINUX) wrote:
> From: Abhinav Singh <[email protected]> Sent: Saturday, October 28, 2023 2:26 PM
>>
>>> 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?
>>
>
> I'll suggest deferring to Nischala's pending patch. Her patch
> covers all 5 occurrences. Also, I don't know if you were able
> to test your patch in a Hyper-V Confidential VM. She did test
> in that configuration and confirmed that nothing broke.
>
> Of course, feel free to correspond directly with Nischala
> on how best to move forward. I'm an unnecessary intermediary
> who is just pointing out the overlap. :-)
>
> Michael
Okay will ask her about the patch.
Thank you for your time to review and guide me :)
Abhinav