[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.)


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.