Re: [PATCH v2 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server

[George Dunlap <George.Dunlap@eu.citrix.com>]

Oops -- forgot to CC the list...

On Thu, Apr 7, 2016 at 10:55 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Thu, Apr 7, 2016 at 8:01 AM, Yu, Zhang <yu.c.zhang@linux.intel.com> wrote:
>>>> +    /* For now, only support for HAP enabled hvm */
>>>> +    if ( !hap_enabled(d) )
>>>> +        goto out;
>>>
>>>
>>> So before I suggested that this be restricted to HAP because you were
>>> using p2m_memory_type_changed(), which was only implemented on EPT.
>>> But since then you've switched that code to use
>>> p2m_change_entry_type_global() instead, which is implemented by both;
>>> and you implement the type in p2m_type_to_flags().  Is there any
>>> reason to keep this restriction?
>>>
>>
>> Yes. And this is a change which was not explained clearly. Sorry.
>>
>> Reason I've chosen p2m_change_entry_type_global() instead:
>> p2m_memory_type_changed() will only trigger the resynchronization for
>> the ept memory types in resolve_misconfig(). Yet it is the p2m type we
>> wanna to be recalculated, so here comes p2m_change_entry_type_global().
>>
>> Reasons I restricting the code in HAP mode:
>> Well, I guess p2m_change_entry_type_global() was only called by HAP code
>> like hap_[en|dis]able_log_dirty() etc, which were registered during
>> hap_domain_init(). As to shadow mode, it is sh_[en|dis]able_log_dirty(),
>> which do not use p2m_change_entry_type_global().
>
> Oh, right -- yes, there's an ASSERT(hap_enabled()) right at the top of
> p2m_pt_change_entry_type_global().
>
> Yes, if that functionality is not already implemented for shadow,
> there's no need for you to implement it; and restricting it to
> HAP-only is the obvious solution.
>
>>>> +    /*
>>>> +     * Each time we map/unmap an ioreq server to/from p2m_ioreq_server,
>>>> +     * we mark the p2m table to be recalculated, so that gfns which were
>>>> +     * previously marked with p2m_ioreq_server can be resynced.
>>>> +     */
>>>> +    p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
>>>
>>>
>>> This comment doesn't seem to be accurate (or if it is it's a bit
>>> confusing).  Would it be more accurate to say something like the
>>> following:
>>>
>>> "Each time we map / unmap in ioreq server to/from p2m_ioreq_server, we
>>> reset all memory currently marked p2m_ioreq_server to p2m_ram_rw."
>>>
>> Well, I agree this comment is not quite accurate. Like you said in your
>> comment, the purpose here, calling p2m_change_entry_type_global() is to
>> "reset all memory currently marked p2m_ioreq_server to p2m_ram_rw". But
>> the recalculation is asynchronous. So how about:
>>
>> "Each time we map/unmap an ioreq server to/from p2m_ioreq_server, we
>> mark the p2m table to be recalculated, so all memory currently marked
>> p2m_ioreq_server can be reset to p2m_ram_rw later."?
>
> I think we're emphasizing different things -- I'm emphasizing what the
> change will be, and you're emphasizing when the change will happen.
> :-)
>
> I think from the point of view of someone reading this code, it
> doesn't matter when the physical p2m entries get updated; *logically*
> they are updated now, since from this call onward anything that reads
> the p2m table will get the new type.  The fact that we do it lazily is
> an implementation detail -- we could change the function behind the
> scenes to do it all at once, and the semantics would be the same (it
> would just cause a long change all at once).
>
> If you really want to include when the change will happen, what about this:
>
> "Each time we map / unmap in ioreq server to/from p2m_ioreq_server, we
> reset all memory currently marked p2m_ioreq_server to p2m_ram_rw.
> (This happens lazily as the p2m entries are accessed.)"
>
> BTW, I think this also means that for the interface at the moment, you
> can't change the kinds of accesses you want to intercept very easily
> -- if you want to change from intercepting only writes to intercepting
> both reads and writes, you have to detach from the ioreq_server type
> completely (which will make all your currently-marked ioreq_server
> pages go back to ram_rw), then attach again, and re-mark all the pages
> you were watching.
>
> Which is perhaps not perfect, but I suppose it will do.  It should
> even be possible to change this in the future -- ioreq servers that
> want to change the access mode can try just changing it directly, and
> if they get -EBUSY, do it the hard way.
>
> (Just to be clear, I'm thinking out loud here in the last two
> paragraphs -- no action required unless you feel like it.)
>
>>> But of course that raises another question: is there ever any risk
>>> that an ioreq server will change some other ram type (say, p2m_ram_ro)
>>> to p2m_ioreq_server, and then have it changed back to p2m_ram_rw when
>>> it detaches?
>>>
>> Good question. And unfortunately, yes there is. :)
>> Maybe we should insist only p2m_ram_rw pages can be changed to
>> p2m_ioreq_server, and vice versa.
>
> Well I think if you only allow p2m_ram_rw pages to be changed to
> p2m_ioreq_server, that should be sufficient.  If that works for you it
> seems like it could be a reasonable approach.
>
>>
>>>>   /* Types that can be subject to bulk transitions. */
>>>>   #define P2M_CHANGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
>>>> -                              | p2m_to_mask(p2m_ram_logdirty) )
>>>> +                              | p2m_to_mask(p2m_ram_logdirty) \
>>>> +                              | p2m_to_mask(p2m_ioreq_server) )
>>>
>>>
>>> It's probably worth a comment here noting that you can do a mass
>>> change *from* p2m_ioreq_server, but you can't do a mass change *to*
>>> p2m_ioreq_server.  (And doing so would need to change a number of
>>> places in the code where it's assumed that the result of such a
>>> recalculation is either p2m_logdirty or p2m_ram_rw -- e.g.,
>>> p2m-ept.c:553, p2m-pt.c:452, &c.
>>>
>> I agree with adding a note here.
>> But adding extra code in resolve_miconfig()/do_recalc()? Is this
>> necessary? IIUC, current code already guarantees there will be no mass
>> change *to* the p2m_ioreq_server.
>
> Sorry -- in this context, when I said "And doing so would need...", I
> meant, "And also note in your comment that if someone wanted to do a
> mass change to p2m_ioreq_server, they would need to change..."  That
> is, was suggesting you write a comment giving a hint to people in the
> future about the limitations, not add any code yourself. :-)
>
> But maybe it would actually be better to add an ASSERT(nt !=
> p2m_ioreq_server) to p2m_change_entry_type_global(), with a comment
> saying that much of the lazy recalc code assumes being changed into
> only ram_logdirty or ram_rw?
>
> And then of course there's the p2m_ioreq_server -> p2m_ram_logdirty
> transition -- I assume that live migration is incompatible with this
> functionality?  Is there anything that prevents a live migration from
> being started when there are outstanding p2m_ioreq_server entries?
>
>  -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel