Re: [Xen-devel] [PATCH v2 1/2] x86/PoD: correctly handle non-order-0 decrease-reservation requests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.18 at 17:04, <george.dunlap@xxxxxxxxxx> wrote:
> On 12/20/2017 09:34 AM, Jan Beulich wrote:
>> p2m_pod_decrease_reservation() at the moment only returns a boolean
>> value: true for "nothing more to do", false for "something more to do".
>> If it returns false, decrease_reservation() will loop over the entire
>> range, calling guest_remove_page() for each page.
>> 
>> Unfortunately, in the case p2m_pod_decrease_reservation() succeeds
>> partially, some of the memory in the range will be not-present; at which
>> point guest_remove_page() will return an error, and the entire operation
>> will fail.
>> 
>> Fix this by:
>> 1. Having p2m_pod_decrease_reservation() return exactly the number of
>>    gpfn pages it has handled (i.e., replaced with 'not present').
>> 2. Making guest_remove_page() return -ENOENT in the case that the gpfn
>>    in question was already empty (and in no other cases).
>> 3. When looping over guest_remove_page(), expect the number of -ENOENT
>>    failures to be no larger than the number of pages
>>    p2m_pod_decrease_reservation() removed.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>> ---
>> v2: Re-written description (by George). Add comments (as suggested
>>     by George). Formatting.
> 
> One thing to double-check...
> 
>> @@ -335,10 +336,8 @@ int guest_remove_page(struct domain *d,
>>          rc = mem_sharing_unshare_page(d, gmfn, 0);
>>          if ( rc )
>>          {
>> -            put_gfn(d, gmfn);
>>              (void)mem_sharing_notify_enomem(d, gmfn, 0);
>> -
>> -            return rc;
>> +            goto out_put_gfn;
> 
> I take it you've checked to make sure that moving this put_gfn() over
> the notify call is OK?

Yes, in fact the stale-ness of the GFN is going to be slightly
reduced by the reference being held across that function (i.e.
now at least it's not stale at the time the event is put on the
ring). The function doesn't use it for anything other than
storing its value.

> I took a brief look and it seems OK to me; so if you're happy with that
> then:
> 
> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Thanks, it has gone in earlier today already anyway, based on
Andrew's ack (which admittedly he has given over irc rather
than by mail).

Jan


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