Re: [Xen-devel] [PATCH v11 2/3] x86/mem_sharing: reset a fork

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.03.2020 15:00, Tamas K Lengyel wrote:
> On Wed, Mar 18, 2020 at 5:36 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 28.02.2020 19:40, Tamas K Lengyel wrote:
>>> +        mfn = page_to_mfn(page);
>>> +        if ( mfn_valid(mfn) )
>>
>> All pages on a domain's list should have a valid MFN - what are you
>> trying to protect against here?
> 
> I saw no documentation stating what you stated above. If that's the
> case it can be dropped.

Only pages coming from the allocator (or, in some special cases,
otherwise valid) get put on a domain's page list. By coming from
the allocator their MFNs are impicitly valid.

>>> +            mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &p2mt, &p2ma,
>>> +                                        0, NULL, false);
>>> +
>>> +            if ( p2m_is_ram(p2mt) && !p2m_is_shared(p2mt) )
>>> +            {
>>> +                /* take an extra reference, must work for a shared page */
>>
>> The comment (and also the next one further down) looks contradictory
>> to the if() immediately ahead, at least to me. Could you clarify the
>> situation, please?
> 
> I don't understand your question.  The comment explains exactly what
> happens. Taking an extra reference must work. If it didn't, trigger an
> ASSERT_UNREACHABLE. Which part is confusing?

The comment says "a shared page" whereas the condition includes
"!p2m_is_shared(p2mt)", which I understand to mean a page which is
not shared.

As to you dropping continuations again - please have at least a
bold comment clarifying that their addition is a requirement for
the code to ever reach "supported" status. (Any other obvious but
intentional omissions could also be named there.)

Jan

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