Re: [hybrid]: code review for function mapping pfn to foreign mfn

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> On Thu, 2012-04-19 at 00:29 +0100, Mukesh Rathor wrote:
>> On Tue, 17 Apr 2012 10:05:28 +0100
>> Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>
>> > On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
>> > > On Mon, 16 Apr 2012 14:53:22 +0100
>> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> >
>> > Sorry, I meant why a whole new subcall instead of a new
>> > XENMAPSPACE ;-)
>> >
>> > e.g. On ARM I did:
>> >
>> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>> > index 86d02c8..b2adfbe 100644
>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -212,11 +212,13 @@ struct xen_add_to_physmap {
>> >      uint16_t    size;
>> >
>> >      /* Source mapping space. */
>> > -#define XENMAPSPACE_shared_info 0 /* shared info page */
>> > -#define XENMAPSPACE_grant_table 1 /* grant table page */
>> > -#define XENMAPSPACE_gmfn        2 /* GMFN */
>> > -#define XENMAPSPACE_gmfn_range  3 /* GMFN range */
>> > -    unsigned int space;
>> > +#define XENMAPSPACE_shared_info  0 /* shared info page */
>> > +#define XENMAPSPACE_grant_table  1 /* grant table page */
>> > +#define XENMAPSPACE_gmfn         2 /* GMFN */
>> > +#define XENMAPSPACE_gmfn_range   3 /* GMFN range */
>> > +#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
>> > +    uint16_t space;
>> > +    domid_t foreign_domid; /* IFF gmfn_foreign */
>>
>>
>> Well, for several reasons, I didn't use it, mainly, it doesn't allow
>> for count. So requests have to come in one frame at a time.
>
> I've got it that way on ARM too at the moment but I was planning to make
> it behave like gmfn_range and use the size parameter to map multiple at
> a time (unless I've misunderstood what the gmfn_range variant does).
>
>>  Second, none of the common code can be used by my new request,
>
> I don't think that's an impediment to the API, XENMAPSPACE_gmfn_foreign
> is in pretty much the same position.
>
>>  because:
>>    - frame is not removed from foreign domain in my case
>>    - i don't want to update the m2p with new info.
>>
>> Anyways, I put it there for now. With ballooning change in dom0, I'm
>> now doing the hcall one frame at a time anyways. We can always enhance
>> in the future.
>>
>>         case XENMAPSPACE_gmfn_foreign:
>>         {
>>             rc = _add_foreign_to_pmap_batch(&xatp);
>>             rcu_unlock_domain(d);
>>             return rc;
>>         }
>>
>>
>> >> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void)
>> >> arg)
>>
>> >Can't XENMEM_remove_from_physmap be used here?
>>
>> Well, that calls guest_physmap_remove_page() which calls
>> p2m_remove_page which updates the M2P with INVALID_M2P_ENTRY. Whereas,
>> i just need to remove from the dom0 p2m and leave M2P as is (mfn to
>> domU gmfn). I could add a flag to the struct causing it to just call
>> set_p2m_entry() directly?
>
> Would it be useful to track the fact that a p2m entry is foreign
> somewhere? That would let you do the appropriate type of teardown etc
> without relying on the tools to get it right.
>
> Are there s/w bits available in the p2m entry itself on x86 or do we use
> them all already?

A few still available. We're at 14 types and EPT uses 4 bits.

NPT has even more room, unless it's sharing its p2m tables with iommu
tables -- in which case it can't really do fancy p2m typing. That would be
an unwelcome side-effect for a hybrid dom0 on amd, but I'm not aware of
how bad it would be.

The more general value here is to allow hvm backends arbitrary mapping
capabilities, correct? Right now all hvm backends/driver domains are
supposed to use grant table code (and there are p2m types for those).

A domain builder/checkpointing hvm domain would require equivalent
capabilities to those being discussed here, if I'm not mistaken.

So, a p2m_foreign_map would be the way to go.

My two cents
Andres


>
> Ian.