Re: [PATCH] Fixing warning cast removes address space '__iomem' of expression

[Abhinav Singh <singhabhinav9051571833@gmail.com>]

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