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
