Re: [Xen-devel] [PATCH 3/3] memory: restrict XENMEM_remove_from_physmap to translated guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.19 at 16:35, <george.dunlap@xxxxxxxxxx> wrote:
> On 3/5/19 1:28 PM, Jan Beulich wrote:
>> The commit re-introducing it (14eb3b41d0 ["xen: reinstate previously
>> unused XENMEM_remove_from_physmap hypercall"]) as well as the one having
>> originally introduced it (d818f3cb7c ["hvm: Use main memory for video
>> memory"]) and the one then purging it again (78c3097e4f ["Remove unused
>> XENMEM_remove_from_physmap"]) make clear that this operation is intended
>> for use on HVM (i.e. translated) guests only. Restrict it at least as
>> much, because for PV guests documentation (in the public header) does
>> not even match the implementation: It talks about GPFN as input, but
>> get_page_from_gfn() assumes a GMFN in the non-translated case (and hands
>> back the value passed in).
>> 
>> Also lift the check in XENMEM_add_to_physmap{,_batch} handling up
>> directly into top level hypercall handling, and clarify things in the
>> public header accordingly.
>> 
>> Take the liberty and also replace a pointless use of "current" with a
>> more efficient use of an existing local variable (or function parameter
>> to be precise).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Looks good, sorry for the delay:
> 
> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Thanks.

> A couple of comments:
> 
>> ---
>> TBD: Is using P2M_ALLOC here really appropriate? It means e.g.
>>      pointlessly populating a PoD slot just to unpopulate it again right
>>      away, with the page then free floating, i.e. no longer available
>>      for use to replace another PoD slot, and (afaict) no longer
>>      accessible by the guest in any way.
> 
> Looks like the P2M_ALLOC was introduced in c/s 06e7bfed206. I can't
> immediately see any reason to do the allocation -- I think it just must
> have been C&P without much thought given as to what was going to happen
> next.

The question is: If we remove it, what is the -ENOENT condition
going to be?

>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4470,9 +4470,6 @@ int xenmem_add_to_physmap_one(
>>      mfn_t mfn = INVALID_MFN;
>>      p2m_type_t p2mt;
>>  
>> -    if ( !paging_mode_translate(d) )
>> -        return -EACCES;
>> -
>>      switch ( space )
>>      {
>>          case XENMAPSPACE_shared_info:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -815,6 +815,8 @@ int xenmem_add_to_physmap(struct domain
>>      long rc = 0;
>>      union xen_add_to_physmap_batch_extra extra;
>>  
>> +    ASSERT(paging_mode_translate(d));
> 
> So, just trying to think through the principles behind these two.  We
> know that this is never going to be called w/o first calling
> xatp_permission_check(); if we assume that such a change will be tested
> (i.e., that something with paging_mode_translate() will call this
> hypercall before a release), then a single ASSERT() should be enough to
> make sure that both functions are updated properly?

Yes, that's my expectation.

> I guess that's good enough.  (It's hard not to start to get paranoid
> when you ask yourself these sorts of questions.)

Good. (Indeed.)

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel