Re: [PATCH v2 01/12] x86: introduce ioremap_wc()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 27/05/2021 14:09, Jan Beulich wrote:
> On 27.05.2021 14:48, Julien Grall wrote:
> > On 27/05/2021 13:30, Jan Beulich wrote:
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > > @@ -5881,6 +5881,20 @@ void __iomem *ioremap(paddr_t pa, size_t
> > >        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);
> > > +
> > > +    return (void __force __iomem *)(va + offs);
> > > +}
> > Arm is already providing ioremap_wc() which is a wrapper to
> > ioremap_attr().
> I did notice this, yes.
>
> > Can this be moved to the common code to avoid duplication?
> If by "this" you mean ioremap_attr(), then I wasn't convinced we want
> a function of this name on x86.
I am open to other name.

> In particular you may note that
> x86'es ioremap() is sort of the equivalent of Arm's ioremap_nocache(),
> but is different from the new ioremap_wc() by more than just the
> different PTE attributes.
That's because ioremap() will not vmap() the first MB, am I correct? If 
so, I am not sure why you want to do that in ioremap() but not 
ioremap_wc(). Wouldn't this result access the memory with mismatched 
attributes?
> Also I was specifically asked to make ioremap_wc() __init; ioremap()
> cannot be, because of at least the use from pci_vtd_quirk().
I am not sure this is relevant to the conversation here. I am sure there 
are other function that would benefits to be __init in one arch but 
can't in the other. Yet, common code can be beneficials.
> Plus I'd need to clean up Arm's lack of __iomem if I wanted to fold
> things. 
__iomem is NOP on Xen. So while the annotation may not be consistently 
used, I don't see the clean-up a requirement to consolidate the code...
> Or wait - it's declaration and definition which are out of
> sync there, i.e. a pre-existing issue.
We don't usually add __init on both the declaration and definition. So 
why would it be necessary to add __iomem in both cases?
Cheers,

--
Julien Grall