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

[Jan Beulich <jbeulich@xxxxxxxx>]

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[].

>> --- /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.

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.

>> +#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;

Can do, sure.

Jan