Re: [PATCH 1/2] make ioremap_attr() common

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 19, 2026 at 04:51:54PM +0100, Jan Beulich wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -6166,41 +6166,15 @@ void *__init arch_vmap_virt_end(void)
>      return fix_to_virt(__end_of_fixed_addresses);
>  }
>  
> -void __iomem *ioremap(paddr_t pa, size_t len)
> +void __iomem *x86_ioremap_attr(paddr_t pa, size_t len, pte_attr_t attr)
>  {
> -    mfn_t mfn = _mfn(PFN_DOWN(pa));
> -    void *va;
> -
> -    WARN_ON(page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL));
> +    WARN_ON(page_is_ram_type(PFN_DOWN(pa), RAM_TYPE_CONVENTIONAL));
>  
>      /* The low first Mb is always mapped. */
> -    if ( !((pa + len - 1) >> 20) )
> -        va = __va(pa);
> -    else
> -    {
> -        unsigned int offs = pa & (PAGE_SIZE - 1);
> -        unsigned int nr = PFN_UP(offs + len);
> -
> -        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_UCMINUS, VMAP_DEFAULT);
> -        if ( va )
> -            va += offs;
> -    }
> -
> -    return (void __force __iomem *)va;
> -}
> -
> -void __iomem *__init ioremap_wc(paddr_t pa, size_t len)
> -{
> -    mfn_t mfn = _mfn(PFN_DOWN(pa));
> -    unsigned int offs = pa & (PAGE_SIZE - 1);
> -    unsigned int nr = PFN_UP(offs + len);
> -    void *va;
> -
> -    WARN_ON(page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL));
> -
> -    va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_WC, VMAP_DEFAULT);
> +    if ( !((pa + len - 1) >> 20) && attr == PAGE_HYPERVISOR_UCMINUS )
> +        return (void __force __iomem *)__va(pa);

Sorry, I'm possibly a bit lost: I see that on the bootstrap
page-tables we map the VGA hole (0xa0000-0xc0000) as UC-, but I'm not
seeing this being done to the directmap?

>  
> -    return (void __force __iomem *)(va ? va + offs : NULL);
> +    return NULL;
>  }
>  
>  int create_perdomain_mapping(struct domain *d, unsigned long va,
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_GRANT_TABLE) += grant_table
>  obj-y += guestcopy.o
>  obj-y += gzip/
>  obj-$(CONFIG_HYPFS) += hypfs.o
> +obj-y += ioremap.o
>  obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
>  obj-y += irq.o
>  obj-y += kernel.o
> --- /dev/null
> +++ b/xen/common/ioremap.c
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/mm.h>
> +#include <xen/pfn.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/io.h>
> +
> +void __iomem *ioremap_attr(paddr_t pa, size_t len, pte_attr_t attr)
> +{
> +    void __iomem *ptr = NULL;
> +    unsigned int offs = PAGE_OFFSET(pa);
> +
> +#ifdef arch_ioremap_attr
> +    ptr = arch_ioremap_attr(pa, len, attr);
> +    if ( PAGE_OFFSET(ptr) )
> +        return ptr;

IMO it's a bit confusing to deal with the offset addition detached
form here, I would rather do:

if ( ptr != NULL )
{
    ASSERT(PAGE_OFFSET(ptr) == offs);
    return PAGE_OFFSET(ptr) ? ptr : ptr + off;
}

And get rid of the !ptr check in the if below.  So that when
arch_ioremap_attr() succeeds it's all dealt with in the #ifdef
protected block.

> +#endif
> +
> +    if ( IS_ENABLED(CONFIG_HAS_VMAP) && !ptr )
> +    {
> +        mfn_t mfn = _mfn(PFN_DOWN(pa));
> +
> +        ptr = (void __force __iomem *)__vmap(&mfn, PFN_UP(offs + len), 1, 1,
> +                                             attr, VMAP_DEFAULT);
> +    }
> +
> +    if ( !ptr )
> +        return NULL;
> +
> +    return ptr + offs;

I would also join the NULL checking using a ternary operator:

return ptr ? ptr + off : NULL;

Thanks, Roger.