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

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

On Thu, Apr 02, 2026 at 02:42:54PM +0200, Jan Beulich wrote:
> On 02.04.2026 14:15, Roger Pau Monné wrote:
> > 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?
> 
> I'm not changing that aspect here, do I? ioremap() used the direct map
> (apparently assuming the cache attribute is suitable), and so do I now.
> 
> Yet then, to address the point you make anyway, I certainly see use of
> __PAGE_HYPERVISOR_UCMINUS in l1_directmap[].

Yes, sorry, I know you are not adding this here.  Just took a look at
the code, and I couldn't find where the directmap entries for the low
1M get adjusted to use UC-.  I should have made the comment clearer.

> >> --- /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.
> 
> Possible, yet I deemed the other variant better.

OK, I'm not going to insist then.  It just took me a bit to notice the
extra !ptr check in the if condition below.

> The assertion you suggest isn't valid anyway (see the last sentence of the
> description and xen/arch/arm/mpu/mm.c:ioremap_attr() prior to Michal's
> "xen/arm: mpu: Fix ioremap_attr region size and return value"). Now that
> that's changing, things may indeed be possible to simplify here.

Oh, I see, right you are.  It should be:

ASSERT(!PAGE_OFFSET(ptr) || PAGE_OFFSET(ptr) == offs);

Thanks, Roger.