| Message ID | 1698854508-23036-1-git-send-email-niyelchu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Replace ioremap_cache() with memremap() | expand |
From: Nischala Yelchuri <niyelchu@linux.microsoft.com> 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@linux.microsoft.com> > --- > 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
On Wed, Nov 01, 2023 at 09:01:48AM -0700, Nischala Yelchuri wrote: > 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. Do you find any real world issues with the current code? How do you discover these issues? Thanks, Wei.
Wei, this is one of the Hyper-V code improvement tasks that Michael Kelley identified. Using memremap() is the right thing to do here. Abhinav Singh (cc'ed) also tried to fix this earlier as there are sparse warnings with ioremap_cache(). On Sun, Nov 12, 2023 at 11:12:33PM +0000, Wei Liu wrote: > On Wed, Nov 01, 2023 at 09:01:48AM -0700, Nischala Yelchuri wrote: > > 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. > > Do you find any real world issues with the current code? How do you > discover these issues? > > Thanks, > Wei.
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); 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); 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,
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@linux.microsoft.com> --- 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(-)