RE: [PATCH] Replace ioremap_cache() with memremap()

[Michael Kelley]

From: Nischala Yelchuri <niyelchu@xxxxxxxxxxxxxxxxxxx> Sent: Wednesday, November 1, 2023 9:02 AM
>

It's customary for the patch "Subject:" line to have a prefix indicating the
area of the code being modified.  This patch touches on multiple Hyper-V
drivers, so there's not a clear choice for prefix.  I would suggest using
"Drivers: hv:" as is commonly used for Hyper-V code, though other
reviewers might have a different suggestion.

 > Current Hyper-V code for CoCo VMs uses ioremap_cache() to map normal
> memory as decrypted.
> ioremap_cache() is ideally used to map I/O device memory when it has the characteristics
> of normal memory. It should not be used to map normal memory as the returned pointer
> has the __iomem attribute.
> 
> Fix current code by replacing ioremap_cache() with memremap().
> 
> No functional change intended.
> 
> Signed-off-by: Nischala Yelchuri <niyelchu@xxxxxxxxxxxxxxxxxxx>
> ---
>  arch/x86/hyperv/hv_init.c               |  6 +++---
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c |  2 +-
>  drivers/hv/hv.c                         | 13 +++++++------
>  drivers/video/fbdev/hyperv_fb.c         |  6 +++---
>  4 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 21556ad87..fae43c040 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -68,9 +68,9 @@ static int hyperv_init_ghcb(void)
>  	 */
>  	rdmsrl(MSR_AMD64_SEV_ES_GHCB, ghcb_gpa);
> 
> -	/* Mask out vTOM bit. ioremap_cache() maps decrypted */
> +	/* Mask out vTOM bit. memremap() maps decrypted with MEMREMAP_DEC */
>  	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 | MEMREMAP_DEC);
>  	if (!ghcb_va)
>  		return -ENOMEM;
> 
> @@ -238,7 +238,7 @@ static int hv_cpu_die(unsigned int cpu)
>  	if (hv_ghcb_pg) {
>  		ghcb_va = (void **)this_cpu_ptr(hv_ghcb_pg);
>  		if (*ghcb_va)
> -			iounmap(*ghcb_va);
> +			memunmap(*ghcb_va);
>  		*ghcb_va = NULL;
>  	}
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index d511d17c5..d6fec9bd3 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -92,7 +92,7 @@ static int hyperv_setup_vram(struct hyperv_drm_device
> *hv,
>  	 * connect to display properly for ARM64 Linux VM, as the host also maps
>  	 * the VRAM cacheable.
>  	 */
> -	hv->vram = ioremap_cache(hv->mem->start, hv->fb_size);
> +	hv->vram = memremap(hv->mem->start, hv->fb_size, MEMREMAP_WB | MEMREMAP_DEC);

This change has some additional implications that must be
accounted for.  The hv->vram field is declared as void __iomem *
in hyperv_drm.h.   The __iomem attribute should be dropped.
Then the use of IOSYS_MAP_INIT_VADDR_IOMEM() in
hyperv_blit_to_vram_rect() should be changed to
IOSYS_MAP_INIT_VADDR().  This has the desirable effect of 
allowing normal memcpy() functions to be used instead of
the _toio()/_fromio() variants.

>  	if (!hv->vram) {
>  		drm_err(dev, "Failed to map vram\n");
>  		ret = -ENOMEM;
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 51e5018ac..399bfa392 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -274,11 +274,12 @@ void hv_synic_enable_regs(unsigned int cpu)
>  	simp.simp_enabled = 1;
> 
>  	if (ms_hyperv.paravisor_present || hv_root_partition) {
> -		/* Mask out vTOM bit. ioremap_cache() maps decrypted */
> +		/* Mask out vTOM bit. memremap() maps decrypted with MEMREMAP_DEC */
>  		u64 base = (simp.base_simp_gpa << HV_HYP_PAGE_SHIFT) &
>  				~ms_hyperv.shared_gpa_boundary;
>  		hv_cpu->synic_message_page
> -			= (void *)ioremap_cache(base, HV_HYP_PAGE_SIZE);
> +			= memremap(base,
> +				   HV_HYP_PAGE_SIZE, MEMREMAP_WB | MEMREMAP_DEC);
>  		if (!hv_cpu->synic_message_page)
>  			pr_err("Fail to map synic message page.\n");
>  	} else {
> @@ -293,11 +294,11 @@ void hv_synic_enable_regs(unsigned int cpu)
>  	siefp.siefp_enabled = 1;
> 
>  	if (ms_hyperv.paravisor_present || hv_root_partition) {
> -		/* Mask out vTOM bit. ioremap_cache() maps decrypted */
> +		/* Mask out vTOM bit. memremap() maps decrypted with MEMREMAP_DEC */
>  		u64 base = (siefp.base_siefp_gpa << HV_HYP_PAGE_SHIFT) &
>  				~ms_hyperv.shared_gpa_boundary;
>  		hv_cpu->synic_event_page
> -			= (void *)ioremap_cache(base, HV_HYP_PAGE_SIZE);
> +			= memremap(base, HV_HYP_PAGE_SIZE, MEMREMAP_WB | MEMREMAP_DEC);
>  		if (!hv_cpu->synic_event_page)
>  			pr_err("Fail to map synic event page.\n");
>  	} else {
> @@ -376,7 +377,7 @@ void hv_synic_disable_regs(unsigned int cpu)
>  	 */
>  	simp.simp_enabled = 0;
>  	if (ms_hyperv.paravisor_present || hv_root_partition) {
> -		iounmap(hv_cpu->synic_message_page);
> +		memunmap(hv_cpu->synic_message_page);
>  		hv_cpu->synic_message_page = NULL;
>  	} else {
>  		simp.base_simp_gpa = 0;
> @@ -388,7 +389,7 @@ void hv_synic_disable_regs(unsigned int cpu)
>  	siefp.siefp_enabled = 0;
> 
>  	if (ms_hyperv.paravisor_present || hv_root_partition) {
> -		iounmap(hv_cpu->synic_event_page);
> +		memunmap(hv_cpu->synic_event_page);
>  		hv_cpu->synic_event_page = NULL;
>  	} else {
>  		siefp.base_siefp_gpa = 0;
> diff --git a/drivers/video/fbdev/hyperv_fb.c
> b/drivers/video/fbdev/hyperv_fb.c
> index bf59daf86..cd9ec1f6c 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1034,7 +1034,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	 * VM Connect to display properly for ARM64 Linux VM, as the host also
>  	 * maps the VRAM cacheable.
>  	 */
> -	fb_virt = ioremap_cache(par->mem->start, screen_fb_size);
> +	fb_virt = memremap(par->mem->start, screen_fb_size, MEMREMAP_WB | MEMREMAP_DEC);

There's a similar situation here:  the local variable fb_virt is
declared as void __iomem *.  The __iomem attribute should
be dropped.

>  	if (!fb_virt)
>  		goto err2;
> 
> @@ -1068,7 +1068,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	return 0;
> 
>  err3:
> -	iounmap(fb_virt);
> +	memunmap(fb_virt);
>  err2:
>  	vmbus_free_mmio(par->mem->start, screen_fb_size);
>  	par->mem = NULL;
> @@ -1086,7 +1086,7 @@ static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
> 
>  	if (par->need_docopy) {
>  		vfree(par->dio_vp);
> -		iounmap(info->screen_base);
> +		memunmap(info->screen_base);
>  		vmbus_free_mmio(par->mem->start, screen_fb_size);
>  	} else {
>  		hvfb_release_phymem(hdev, info->fix.smem_start,
> --
> 2.34.1