Re: [PATCH 12/24] x86/mm: get_page_from_l1e() is PV-or-shadow-only

On 8/3/26 6:11 AM, Jan Beulich wrote:
> On 02.08.2026 17:55, Daniel P. Smith wrote:
>> On 7/28/26 9:18 AM, Jan Beulich wrote:
>>> Otherwise the function is unreachable, violating MISRA C:2012 rule 2.1.
>>> With the function compiled out, its dedicated XSM hook also becomes
>>> unreachable, so it is similarly guarded.
>>>
>>> Signed-off-by: Jan Beulich <[email protected]>
>>> ---
>>> It feels suspicious that the .priv_mapping() check is used for HVM guests
>>> in shadow mode, but not for ones in HAP mode.
>>
>> I believe a hint to it is laying in the comment,
>>
>>    /*
>>     * Let privileged domains transfer the right to map their target
>>     * domain's pages. This is used to allow stub-domain pvfb export to
>>     * dom0, until pvfb supports granted mappings. At that time this
>>     * minor hack can go away.
>>     */
>>
>> Correct me if I am wrong, but get_page_from_l1e() is only used by PV and
>> HVM + Shadow. When in HVM + HAP is mapping a guest page, it is done
>> through p2m_get_foreign() which will then be covered by
>> xsm_map_gmfn_foreign(). So only HVM + Shadow can hit TARGET_HACK check.
> 
> Yes, sure; that wasn't the point of my comment. The point was that I'd
> expect _the same_ hook to be used by the other path. Aiui if you make a
> policy, you want same situations dealt with the same. Hence there shouldn't
> be a need to express the same thing two ways.
> 

But it's not the same, the enforcement mechanism is different. FLASK is 
an evaluation of Subject/Object/Predicate. In this case the mechanism 
(software enforced access) that provides the Predicate has enough risk 
that it warranted itself a separate check to allow fine grained 
assignment of the operation to a specific domain which was driven by a 
specific use case.

>> I think the question is how to address the TARGET_HACK situation.
> 
> I fear I don't really know what exactly you mean here.
> 

Is this path still needed for the pvfb or is it now in use by other use 
cases. If the former, then close the ability otherwise TARGET_HACK 
should be renamed to something sensible for general case. Some code 
documentation might be necessary to help understand why/

>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -837,6 +837,8 @@ static int cf_check print_mmio_emul_rang
>>>    }
>>>    #endif
>>>    
>>> +#if defined(CONFIG_PV) || defined(CONFIG_SHADOW_PAGING)
>>> +
>>>    /*
>>>     * get_page_from_l1e returns:
>>>     *   0  => success (page not present also counts as such)
>>> @@ -1038,6 +1040,8 @@ get_page_from_l1e(
>>>        return -EBUSY;
>>>    }
>>>    
>>> +#endif /* CONFIG_PV || CONFIG_SHADOW_PAGING */
>>> +
>>
>> Would it also not be prudent to #ifdef out the declaration in asm/mm.h?
> 
> Ah, yes, this looks possible for this function - the decl isn't needed for any
> DCE-ing by the compiler.
> 
>> I think it would be a good defensive approach to condition out the
>> header declaration. Otherwise,
>>
>> Acked-by: Daniel P. Smith <[email protected]>
> 
> Thanks, also for all the others.
> 
> Jan