[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 27.05.2021 15:30, Julien Grall wrote:
> 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.

My remark wasn't so much about the name, but about there being a
"more capable" backing function for a number of wrappers.

>> 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?

UC and WC aren't really conflicting cache attributes - they both
fall in the "uncachable" category. In fact I have a TBD in the
post-commit-message area regarding this very aspect of possibly
reusing the low 1Mb mapping.

>> 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?

__init is an attribute that is meaningful only for functions and
only on their definitions (because it controls what section the
code gets emitted to by the compiler, while it is of no interest
at all to any caller of the function, as far as the compiler is
concerned). __iomem, otoh, is a modifier for pointer types, so
doesn't apply to the function as a whole but to its return types.
Such types (when they're not NOP) need to be consistent between
declaration and definition. You can try this with an about
arbitrary (but valid) __attribute__(()) of your choice and with a
not overly old compiler - you should see it complain about such
inconsistencies.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.