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

["Yu, Zhang" <yu.c.zhang@xxxxxxxxxxxxxxx>]

Thanks for your advices and good questions. :)

On 4/7/2016 1:13 AM, George Dunlap wrote:
> On Thu, Mar 31, 2016 at 11:53 AM, Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
> > let one ioreq server claim/disclaim its responsibility for the
> > handling of guest pages with p2m type p2m_ioreq_server. Users
> > of this HVMOP can specify whether the p2m_ioreq_server is supposed
> > to handle write accesses or read ones or both in a parameter named
> > flags. For now, we only support one ioreq server for this p2m type,
> > so once an ioreq server has claimed its ownership, subsequent calls
> > of the HVMOP_map_mem_type_to_ioreq_server will fail. Users can also
> > disclaim the ownership of guest ram pages with this p2m type, by
> > triggering this new HVMOP, with ioreq server id set to the current
> > owner's and flags parameter set to 0.
> >
> > For now, both HVMOP_map_mem_type_to_ioreq_server and p2m_ioreq_server
> > are only supported for HVMs with HAP enabled.
> >
> > Note that flags parameter(if not 0) of this HVMOP only indicates
> > which kind of memory accesses are to be forwarded to an ioreq server,
> > it has impact on the access rights of guest ram pages, but are not
> > the same. Due to hardware limitations, if only write operations are
> > to be forwarded, read ones will be performed at full speed, with
> > no hypervisor intervention. But if read ones are to be forwarded to
> > an ioreq server, writes will inevitably be trapped into hypervisor,
> > which means significant performance impact.
> >
> > Also note that this HVMOP_map_mem_type_to_ioreq_server will not
> > change the p2m type of any guest ram page, until HVMOP_set_mem_type
> > is triggered. So normally the steps should be the backend driver
> > first claims its ownership of guest ram pages with p2m_ioreq_server
> > type, and then sets the memory type to p2m_ioreq_server for specified
> > guest ram pages.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
> And again, review of this patch was significantly delayed because you
> didn't provide any description of the changes you made between v1 and
> v2 or why.
Sorry about the inconvenience, will change in next version.

> Overall looks good.  Just a few questions...
>
> > +static int hvmop_map_mem_type_to_ioreq_server(
> > +    XEN_GUEST_HANDLE_PARAM(xen_hvm_map_mem_type_to_ioreq_server_t) uop)
> > +{
> > +    xen_hvm_map_mem_type_to_ioreq_server_t op;
> > +    struct domain *d;
> > +    int rc;
> > +
> > +    if ( copy_from_guest(&op, uop, 1) )
> > +        return -EFAULT;
> > +
> > +    rc = rcu_lock_remote_domain_by_id(op.domid, &d);
> > +    if ( rc != 0 )
> > +        return rc;
> > +
> > +    rc = -EINVAL;
> > +    if ( !is_hvm_domain(d) )
> > +        goto out;
> > +
> > +    /* 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().

Since my intention is to resync the outdated p2m_ioreq_server pages
back to p2m_ram_rw, calling p2m_change_entry_global() directly should
be much more convenient(and correct) for me than inventing another
wrapper to cover both the HAP and shadow mode(which xengt does not use
by now).


> > +    /*
> > +     * 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."?

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

> >   /* 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.


> Really getting down to the fine details here -- thanks for all your
> work on this.
>
>   -George
B.R.
Yu

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