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
