Re: [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@linux.intel.com>]

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@linux.intel.com> 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@citrix.com>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>
> 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@lists.xen.org
http://lists.xen.org/xen-devel