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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Mar 18, 2020 at 8:13 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.)
>

Sure, I had that comment in place before. There are no plans to have
this code be "supported", we are fine with it being experimental.

Tamas

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