Re: [PATCH v2] x86/vmap: handle superpages in vmap_to_mfn()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.12.2020 12:54, Hongyan Xia wrote:
> On Mon, 2020-12-07 at 11:11 +0100, Jan Beulich wrote:
>> On 03.12.2020 12:21, Hongyan Xia wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long
>>> v)
>>>          }                                          \
>>>      } while ( false )
>>>  
>>> +/* Translate mapped Xen address to MFN. */
>>> +mfn_t xen_map_to_mfn(unsigned long va)
>>> +{
>>> +#define CHECK_MAPPED(cond_)     \
>>> +    if ( !(cond_) )             \
>>> +    {                           \
>>> +        ASSERT_UNREACHABLE();   \
>>> +        ret = INVALID_MFN;      \
>>> +        goto out;               \
>>> +    }                           \
>>
>> This should be coded such that use sites ...
>>
>>> +    bool locking = system_state > SYS_STATE_boot;
>>> +    unsigned int l2_offset = l2_table_offset(va);
>>> +    unsigned int l1_offset = l1_table_offset(va);
>>> +    const l3_pgentry_t *pl3e = virt_to_xen_l3e(va);
>>> +    const l2_pgentry_t *pl2e = NULL;
>>> +    const l1_pgentry_t *pl1e = NULL;
>>> +    struct page_info *l3page;
>>> +    mfn_t ret;
>>> +
>>> +    L3T_INIT(l3page);
>>> +    CHECK_MAPPED(pl3e)
>>> +    l3page = virt_to_page(pl3e);
>>> +    L3T_LOCK(l3page);
>>> +
>>> +    CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT)
>>
>> ... will properly require a statement-ending semicolon. With
>> additionally the trailing underscore dropped from the macro's
>> parameter name
> 
> The immediate solution that came to mind is a do-while construct. Would
> you be happy with that?

Sure.

>>> --- a/xen/include/asm-x86/mm.h
>>> +++ b/xen/include/asm-x86/mm.h
>>> @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void);
>>>  void free_xen_pagetable_new(mfn_t mfn);
>>>  
>>>  l1_pgentry_t *virt_to_xen_l1e(unsigned long v);
>>> +mfn_t xen_map_to_mfn(unsigned long va);
>>
>> This is now a pretty proper companion of map_page_to_xen(), and
>> hence imo ought to be declared next to that one rather than here.
>> Ultimately Arm may also need to gain an implementation.
> 
> Since map_pages_to_xen() is in the common header, are we okay with
> having the declaration but not an implementation on the Arm side in
> this patch? Or do we also want to introduce the Arm implementation in
> this patch?

Just a declaration is fine imo. If a use in common code appears,
it'll still be noticeable at link time that Arm will need to
have an implementation added.

Jan