Re: [Xen-devel] PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jun 23, 2016 at 11:00:42AM -0400, Daniel De Graaf wrote:
> On 06/23/2016 09:25 AM, Marek Marczykowski-Górecki wrote:
> [...]
> > Ok, after drawing a flowchart of the control in this function after your
> > change, on a piece of paper, this case looks fine. But depending on how
> > the domain was found (explicit loop or rcu_lock_domain_by_id), different
> > locks are held, which makes it harder to follow what is going on.
> > 
> > Crazy idea: how about making the code easy/easier to read instead of
> > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is
> > convolved enough. How about this version (2 patches):
> [...]
> > xen: allow XEN_DOMCTL_getdomaininfo for device model
> > 
> > Allow device model domain to get info about its target domain.
> > It is used during PCI passthrough setup (xc_domain_memory_mapping
> > checks for guest being auto-translated). While it happens in stubdomain,
> > it failed, breaking PCI passthrough in such setup.
> > 
> > While it is possible to workaround this at toolstack side, it seems
> > logical to allow device model to get information about its target
> > domain.
> > 
> > The problem was exposed by c428c9f "tools/libxl: handle the iomem
> > parameter with the memory_mapping hcall".
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  xen/include/xsm/dummy.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> > index 406cd18..70a1633 100644
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG 
> > struct domain *d, int cmd)
> >      case XEN_DOMCTL_unbind_pt_irq:
> >          return xsm_default_action(XSM_DM_PRIV, current->domain, d);
> >      case XEN_DOMCTL_getdomaininfo:
> > -        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
> > +        if (current->domain->target)
> > +            return xsm_default_action(XSM_DM_PRIV, current->domain, d);
> > +        else
> > +            return xsm_default_action(XSM_XS_PRIV, current->domain, d);
> >      default:
> >          return xsm_default_action(XSM_PRIV, current->domain, d);
> >      }
> 
> I would prefer testing for the xenstore flag instead of testing for the
> target field.  It ends up being the same thing in reality, since nobody
> sane would make the xenstore also a device model (and not also dom0).
> 
>       case XEN_DOMCTL_getdomaininfo:
>           if ( src->is_xenstore )
>               return 0;
>           return xsm_default_action(XSM_DM_PRIV, current->domain, d);
> 
> This makes it clear that xenstore is the special case, and removes the
> need for the one-off XSM_XS_PRIV constant.

This was my initial idea, but I don't really understand the comment
about link-time verification if the behaviour is the same for xsm not
compiled vs disabled. But if skipping xsm_default_action here doesn't
break this magic, I'm for it.

Updated patch (with removal of XSM_XS_PRIV):

xen: allow XEN_DOMCTL_getdomaininfo for device model domains

Allow device model domain to get info about its target domain.
It is used during PCI passthrough setup (xc_domain_memory_mapping
checks for guest being auto-translated). While it happens in stubdomain,
it failed, breaking PCI passthrough in such setup.

While it is possible to workaround this at toolstack side, it seems
logical to allow device model to get information about its target
domain.

Also, since this was the only usage of XSM_XS_PRIV, which now gets
handled inline, drop it.

The problem was exposed by c428c9f "tools/libxl: handle the iomem
parameter with the memory_mapping hcall".

Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
---
 xen/include/xsm/dummy.h | 8 +++-----
 xen/include/xsm/xsm.h   | 1 -
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 406cd18..2768861 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -71,10 +71,6 @@ static always_inline int xsm_default_action(
         if ( src->is_privileged )
             return 0;
         return -EPERM;
-    case XSM_XS_PRIV:
-        if ( src->is_xenstore || src->is_privileged )
-            return 0;
-        return -EPERM;
     default:
         LINKER_BUG_ON(1);
         return -EPERM;
@@ -128,7 +124,9 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct 
domain *d, int cmd)
     case XEN_DOMCTL_unbind_pt_irq:
         return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     case XEN_DOMCTL_getdomaininfo:
-        return xsm_default_action(XSM_XS_PRIV, current->domain, d);
+        if ( current->domain->is_xenstore )
+            return 0;
+        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     default:
         return xsm_default_action(XSM_PRIV, current->domain, d);
     }
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 0d525ec..09672e7 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -38,7 +38,6 @@ enum xsm_default {
     XSM_DM_PRIV,  /* Device model can perform on its target domain */
     XSM_TARGET,   /* Can perform on self or your target domain */
     XSM_PRIV,     /* Privileged - normally restricted to dom0 */
-    XSM_XS_PRIV,  /* Xenstore domain - can do some privileged operations */
     XSM_OTHER     /* Something more complex */
 };
 typedef enum xsm_default xsm_default_t;
-- 
2.5.5


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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