Re: [V12 PATCH 3/4] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Wed, 14 May 2014 07:59:08 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 14.05.14 at 02:55, <mukesh.rathor@oracle.com> wrote:
> > On Tue, 13 May 2014 08:09:42 +0100
...
> >> >> And a more general question: How is the insertion of p2m_foreign
> >> >> entries working together with the controlled domain (i.e. the
> >> >> one owning the page) being subject to paging/sharing? I only
> >> >> recall fixme-s having got added for the two features presently
> >> >> not being supported for PVH domains...
> >> > 
> >> > Right, the two features are not supported presently, the caller
> >> > will get -EINVAL if attempted. No further progress. 
> >> 
> >> Will it? Where is that being enforced? I just went down (as an
> > 
> > In p2m_add_foreign() we return -EINVAL if the foreign gfn is not
> > one of: ram_rw | ram_logdirty | ram_ro | paging_out.
> > 
> > Also, patch 8ff5c1d added checks in set_typed_p2m_entry() and
> > p2m_change_type_one().
> 
> But that's way too late, isn't it? You ought to disallow
> paging/sharing (and whatever else you can't support right now) from
> the beginning, i.e. it shouldn't even get enabled on a DomU if the
> controlling domain is PVH.

Ah, I see. The concern here is an HVM gfn say p2m_ram_rw, being mapped 
foreign into dom0, then going into sharing/paging. Yes, we should just
disable/disallow from the beginning for now. Long term, we would need to 
keep track somehow of guest gfns that are mapped foreign so we could
just disallow operations on those...  I frankly see no reason to support 
these features for foreign types.

Code wise several options, but seems mem_event.c would be the best 
place to put checks (would ENOSYS be more appropriate?):

+++ b/xen/arch/x86/mm/mem_event.c
@@ -538,6 +538,13 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event
         case XEN_DOMCTL_MEM_EVENT_OP_PAGING_ENABLE:
         {
             struct p2m_domain *p2m = p2m_get_hostp2m(d);
+            struct domain *hwdom = rcu_lock_domain_by_id(hardware_domid);
+
+            rcu_unlock_domain(hwdom);
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: support paging */
+            if ( is_pvh_domain(hwdom) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )
@@ -620,6 +627,13 @@ int mem_event_domctl(struct domain *d, xen_domctl_mem_event
         {
         case XEN_DOMCTL_MEM_EVENT_OP_SHARING_ENABLE:
         {
+            struct domain *hwdom = rcu_lock_domain_by_id(hardware_domid);
+
+            rcu_unlock_domain(hwdom);
+            rc = -EOPNOTSUPP;
+            /* pvh fixme: support sharing */
+            if ( is_pvh_domain(hwdom) )
+                break;
+
             rc = -ENODEV;
             /* Only HAP is supported */
             if ( !hap_enabled(d) )


BTW, how come you let these get away without "fixme" tags :).. :

            /* Only HAP is supported */
            if ( !hap_enabled(d) )
                break;

            /* Currently only EPT is supported */
            if ( !cpu_has_vmx )

thanks
Mukesh