Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <19cec6f0-e176-4bcc-95a0-9d6eb0261ed1@gmail.com>
Date:   Sun, 29 Oct 2023 02:56:17 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Fixing warning cast removes address space '__iomem' of
 expression
Content-Language: en-US
To:     "Michael Kelley (LINUX)" <mikelley@microsoft.com>,
        KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        "wei.liu@kernel.org" <wei.liu@kernel.org>,
        Dexuan Cui <decui@microsoft.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        Nischala Yelchuri <Nischala.Yelchuri@microsoft.com>
Cc:     "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kernel-mentees@lists.linuxfoundation.org" 
        <linux-kernel-mentees@lists.linuxfoundation.org>
References: <20231024112832.737832-1-singhabhinav9051571833@gmail.com>
 <SN6PR2101MB16937C421EA9CDF373835360D7A3A@SN6PR2101MB1693.namprd21.prod.outlook.com>
From:   Abhinav Singh <singhabhinav9051571833@gmail.com>
In-Reply-To: <SN6PR2101MB16937C421EA9CDF373835360D7A3A@SN6PR2101MB1693.namprd21.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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