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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 17:45,  wrote:
> In your patch (changing XSM_XS_PRIV semantic), you implicitly considered
> all domctls allowed for xenstore domain to be always a subset of those
> allowed for device model domain. For now this is true, but if this set
> is going to be extended in the future, your approach most likely will
> lead to an error.

I don't think so (and intentionally accepted that resulting behavior),
but in the end only the future can prove this either way.

Jan


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

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 05:45:22PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote:
> > >>> On 23.06.16 at 17:22,  wrote:
> > > 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.
> > 
> > If that's the route to go (which I'm not convinced of, as I'm not sure
> > we won't need other xenstore domain special casing later on) I'd
> > really like to ask you to mention the other broken case too, as
> > described in my original patch (unless you found I was wrong with
> > that).
> 
> So, maybe something like this:
>   case XEN_DOMCTL_getdomaininfo:
>   if ( current-domain->is_xenstore )
>   return xsm_default_action(XSM_XS_PRIV, current->domain, d);;
>   return xsm_default_action(XSM_DM_PRIV, current->domain, d);
> 
> 
> In your patch (changing XSM_XS_PRIV semantic), you implicitly considered
> all domctls allowed for xenstore domain to be always a subset of those
> allowed for device model domain. For now this is true, but if this set
> is going to be extended in the future, your approach most likely will
> lead to an error.

Hmm, but if xenstore domain will never be also device model domain, this
probably change nothing...

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


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 17:22,  wrote:
> > 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.
> 
> If that's the route to go (which I'm not convinced of, as I'm not sure
> we won't need other xenstore domain special casing later on) I'd
> really like to ask you to mention the other broken case too, as
> described in my original patch (unless you found I was wrong with
> that).

So, maybe something like this:
  case XEN_DOMCTL_getdomaininfo:
  if ( current-domain->is_xenstore )
  return xsm_default_action(XSM_XS_PRIV, current->domain, d);;
  return xsm_default_action(XSM_DM_PRIV, current->domain, d);


In your patch (changing XSM_XS_PRIV semantic), you implicitly considered
all domctls allowed for xenstore domain to be always a subset of those
allowed for device model domain. For now this is true, but if this set
is going to be extended in the future, your approach most likely will
lead to an error.

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


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 17:22,  wrote:
> 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.

If that's the route to go (which I'm not convinced of, as I'm not sure
we won't need other xenstore domain special casing later on) I'd
really like to ask you to mention the other broken case too, as
described in my original patch (unless you found I was wrong with
that).

Jan


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

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

2016-06-23 Thread Daniel De Graaf 

On 06/23/2016 11:22 AM, Marek Marczykowski-Górecki wrote:

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


That magic is just for the parameter to the XSM hook: in this case, it's
the XSM_OTHER in xsm_domctl that is being verified.  There is no magic
in xsm_default_action.


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 


This would be a good patch to include the corresponding change to the
XSM policy (tweaked from Jan's email):


--- a/tools/flask/policy/modules/xen/xen.if
+++ b/tools/flask/policy/modules/xen/xen.if
@@ -148,7 +148,7 @@ define(`device_model', `
create_channel($2, $1, $2_channel)
allow $1 $2_channel:event create;

-   allow $1 $2_target:domain shutdown;
+   allow $1 $2_target:domain { getdomaininfo shutdown };
allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack 
};
allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl 
irqlevel pciroute pcilevel cacheattr send_irq };
 ')


With that included, or elsewhere in the series:

Acked-by: Daniel De Graaf 


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

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

2016-06-23 Thread Marek Marczykowski-Górecki 
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 
> > ---
> >  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 
---
 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

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

2016-06-23 Thread Andrew Cooper 
On 23/06/16 15:59, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote:
>> On 23/06/16 14:25, Marek Marczykowski-Górecki wrote:
>>> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
>>> On 23.06.16 at 11:23,  wrote:
> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki 
> wrote:
>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>> On 23.06.16 at 10:57,  wrote:
 On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?
 Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
 existing domain with ID >= passed one? Reading various comments in code
 it looks to be used to domain enumeration. This patch changes this
 behaviour.
>>> No, it doesn't. It adjusts the behavior only for the DM case (which
>>> isn't supposed to get information on other than the target domain,
>>> i.e. in this one specific case the very domain ID needs to be passed
>>> in).
>> int xc_domain_getinfo(xc_interface *xch,
>>   uint32_t first_domid,
>>   unsigned int max_doms,
>>   xc_dominfo_t *info)
>> {
>> unsigned int nr_doms;
>> uint32_t next_domid = first_domid;
>> DECLARE_DOMCTL;
>> int rc = 0;
>>
>> memset(info, 0, max_doms*sizeof(xc_dominfo_t));
>>
>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
>> {   
>> domctl.cmd = XEN_DOMCTL_getdomaininfo;
>> domctl.domain = (domid_t)next_domid;
>> if ( (rc = do_domctl(xch, )) < 0 )
>> break;
>> info->domid  = (uint16_t)domctl.domain;
>> (...)
>> next_domid = (uint16_t)domctl.domain + 1;
>>
>>
>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
> valid
>> domain.
> Hmm, looks like I've misread you patch. Reading again...
>
> But now I see rcu_read_lock(_read_lock) is gets called only when
> looping over domains, but rcu_read_unlock is called in any case. Is it
> correct?
 How that? There is this third hunk:
>>> 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: move domain lookup for getdomaininfo to the same
>>>
>>> XEN_DOMCTL_getdomaininfo have different semantics than most of others
>>> domctls - it returns information about first valid domain with ID >=
>>> argument. But that's no excuse for having the lookup done in a different
>>> place, which made handling different corner cases unnecessary complex.
>>> Move the lookup to the first switch clause. And adjust locking to be the
>>> same as for other cases.
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki 
>> FWIW, I prefer this solution to the issue.
>>
>> Reviewed-by: Andrew Cooper , with a few style
>> nits.
> Fixed patch according to your comments:
>
> xen: move domain lookup for getdomaininfo to the same
>
> XEN_DOMCTL_getdomaininfo have different semantics than most of others
> domctls - it returns information about first valid domain with ID >=
> argument. But that's no excuse for having the lookup code in a different
> place, which made handling different corner cases unnecessary complex.
> Move the lookup to the first switch clause. And adjust locking to be the
> same as for other cases.
>
> Signed-off-by: Marek Marczykowski-Górecki 

Reviewed-by: Andrew Cooper 

> ---
>  xen/common/domctl.c | 44 +++-
>  1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e43904e..41de3e8 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>  switch ( op->cmd )
>  {
>  case XEN_DOMCTL_createdomain:
> -case XEN_DOMCTL_getdomaininfo:
>  case XEN_DOMCTL_test_assign_device:
>  case XEN_DOMCTL_gdbsx_guestmemio:
>  d = NULL;
>  break;
> +
> +case XEN_DOMCTL_getdomaininfo:
> +d =

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

2016-06-23 Thread Daniel De Graaf 

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


--
Daniel De Graaf
National Security Agency

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

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote:
> On 23/06/16 14:25, Marek Marczykowski-Górecki wrote:
> > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
> > On 23.06.16 at 11:23,  wrote:
> >>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki 
> >>> wrote:
>  On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>  On 23.06.16 at 10:57,  wrote:
> >> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >>> I wonder what good the duplication of the returned domain ID does: I'm
> >>> tempted to remove the one in the command-specific structure. Does
> >>> anyone have insight into why it was done that way?
> >> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
> >> existing domain with ID >= passed one? Reading various comments in code
> >> it looks to be used to domain enumeration. This patch changes this
> >> behaviour.
> > No, it doesn't. It adjusts the behavior only for the DM case (which
> > isn't supposed to get information on other than the target domain,
> > i.e. in this one specific case the very domain ID needs to be passed
> > in).
>  int xc_domain_getinfo(xc_interface *xch,
>    uint32_t first_domid,
>    unsigned int max_doms,
>    xc_dominfo_t *info)
>  {
>  unsigned int nr_doms;
>  uint32_t next_domid = first_domid;
>  DECLARE_DOMCTL;
>  int rc = 0;
> 
>  memset(info, 0, max_doms*sizeof(xc_dominfo_t));
> 
>  for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
>  {   
>  domctl.cmd = XEN_DOMCTL_getdomaininfo;
>  domctl.domain = (domid_t)next_domid;
>  if ( (rc = do_domctl(xch, )) < 0 )
>  break;
>  info->domid  = (uint16_t)domctl.domain;
>  (...)
>  next_domid = (uint16_t)domctl.domain + 1;
> 
> 
>  Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
> >>> valid
>  domain.
> >>> Hmm, looks like I've misread you patch. Reading again...
> >>>
> >>> But now I see rcu_read_lock(_read_lock) is gets called only when
> >>> looping over domains, but rcu_read_unlock is called in any case. Is it
> >>> correct?
> >> How that? There is this third hunk:
> > 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: move domain lookup for getdomaininfo to the same
> >
> > XEN_DOMCTL_getdomaininfo have different semantics than most of others
> > domctls - it returns information about first valid domain with ID >=
> > argument. But that's no excuse for having the lookup done in a different
> > place, which made handling different corner cases unnecessary complex.
> > Move the lookup to the first switch clause. And adjust locking to be the
> > same as for other cases.
> >
> > Signed-off-by: Marek Marczykowski-Górecki 
> 
> FWIW, I prefer this solution to the issue.
> 
> Reviewed-by: Andrew Cooper , with a few style
> nits.

Fixed patch according to your comments:

xen: move domain lookup for getdomaininfo to the same

XEN_DOMCTL_getdomaininfo have different semantics than most of others
domctls - it returns information about first valid domain with ID >=
argument. But that's no excuse for having the lookup code in a different
place, which made handling different corner cases unnecessary complex.
Move the lookup to the first switch clause. And adjust locking to be the
same as for other cases.

Signed-off-by: Marek Marczykowski-Górecki 
---
 xen/common/domctl.c | 44 +++-
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..41de3e8 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
 switch ( op->cmd )
 {
 case XEN_DOMCTL_createdomain:
-case XEN_DOMCTL_getdomaininfo:
 case XEN_DOMCTL_test_assign_device:
 case XEN_DOMCTL_gdbsx_guestmemio:
 d = NULL;
 break;
+
+case XEN_DOMCTL_getdomaininfo:
+d = rcu_lock_domain_by_id(op->domain);
+
+if ( d == NULL )
+{
+/* Search for the next available domain. */
+rcu_read_lock(_read_lock);
+
+

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 15:25,  wrote:
> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
>> >>> On 23.06.16 at 11:23,  wrote:
>> > On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
>> >> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>> >> > >>> On 23.06.16 at 10:57,  wrote:
>> >> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> >> > >> I wonder what good the duplication of the returned domain ID does: 
>> >> > >> I'm
>> >> > >> tempted to remove the one in the command-specific structure. Does
>> >> > >> anyone have insight into why it was done that way?
>> >> > > 
>> >> > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
>> >> > > existing domain with ID >= passed one? Reading various comments in 
>> >> > > code
>> >> > > it looks to be used to domain enumeration. This patch changes this
>> >> > > behaviour.
>> >> > 
>> >> > No, it doesn't. It adjusts the behavior only for the DM case (which
>> >> > isn't supposed to get information on other than the target domain,
>> >> > i.e. in this one specific case the very domain ID needs to be passed
>> >> > in).
>> >> 
>> >> int xc_domain_getinfo(xc_interface *xch,
>> >>   uint32_t first_domid,
>> >>   unsigned int max_doms,
>> >>   xc_dominfo_t *info)
>> >> {
>> >> unsigned int nr_doms;
>> >> uint32_t next_domid = first_domid;
>> >> DECLARE_DOMCTL;
>> >> int rc = 0;
>> >> 
>> >> memset(info, 0, max_doms*sizeof(xc_dominfo_t));
>> >> 
>> >> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
>> >> {   
>> >> domctl.cmd = XEN_DOMCTL_getdomaininfo;
>> >> domctl.domain = (domid_t)next_domid;
>> >> if ( (rc = do_domctl(xch, )) < 0 )
>> >> break;
>> >> info->domid  = (uint16_t)domctl.domain;
>> >> (...)
>> >> next_domid = (uint16_t)domctl.domain + 1;
>> >> 
>> >> 
>> >> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
>> > valid
>> >> domain.
>> > 
>> > Hmm, looks like I've misread you patch. Reading again...
>> > 
>> > But now I see rcu_read_lock(_read_lock) is gets called only when
>> > looping over domains, but rcu_read_unlock is called in any case. Is it
>> > correct?
>> 
>> How that? There is this third hunk:
> 
> 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: move domain lookup for getdomaininfo to the same

I don't mind this one.

> xen: allow XEN_DOMCTL_getdomaininfo for device model

But I don't really like this, and would prefer my solution here; it's
Daniel's call though.

Jan

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

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

2016-06-23 Thread Daniel De Graaf 

On 06/23/2016 04:39 AM, Jan Beulich wrote:

On 23.06.16 at 10:32,  wrote:

On 22.06.16 at 20:24,  wrote:

Either method works, and I agree allowing DM to invoke this domctl is both
useful and not going to introduce problems.  The getdomaininfo permission
will also need to be added to the device_model macro in xen.if.


What exactly this last sentence means I need to add I'm not sure
about.


Perhaps this?

--- unstable.orig/tools/flask/policy/policy/modules/xen/xen.if
+++ unstable/tools/flask/policy/policy/modules/xen/xen.if
@@ -148,7 +148,7 @@ define(`device_model', `
create_channel($2, $1, $2_channel)
allow $1 $2_channel:event create;

-   allow $1 $2_target:domain shutdown;
+   allow $1 $2_target:domain { getdomaininfo shutdown };
allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack 
};
allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl 
irqlevel pciroute pcilevel cacheattr send_irq };
 ')

Jan


Yes, that is what I meant.

--
Daniel De Graaf
National Security Agency

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

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

2016-06-23 Thread Andrew Cooper 
On 23/06/16 10:50, Jan Beulich wrote:
 On 23.06.16 at 11:44,  wrote:
>> On 23/06/16 09:32, Jan Beulich wrote:
>>> I wonder what good the duplication of the returned domain ID does: I'm
>>> tempted to remove the one in the command-specific structure. Does
>>> anyone have insight into why it was done that way?
>> This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy
>> semantics, which go out of their way to make it easy to get wrong.
>>
>> It is important that you always check the returned domid, as it may not
>> be the domain you asked for.  In particular, if a domain you are looking
>> after dies, the adjacent domain's information will be returnned.
> Same question as to Marek: How is this related to my remark?

Oh right.  I misread.  Altering the domctl domid value is pointless, as
libxc abstracts the call behind do_domctl() anyway.

~Andrew

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

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

2016-06-23 Thread Andrew Cooper 
On 23/06/16 14:25, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
> On 23.06.16 at 11:23,  wrote:
>>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
 On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
 On 23.06.16 at 10:57,  wrote:
>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>>> I wonder what good the duplication of the returned domain ID does: I'm
>>> tempted to remove the one in the command-specific structure. Does
>>> anyone have insight into why it was done that way?
>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
>> existing domain with ID >= passed one? Reading various comments in code
>> it looks to be used to domain enumeration. This patch changes this
>> behaviour.
> No, it doesn't. It adjusts the behavior only for the DM case (which
> isn't supposed to get information on other than the target domain,
> i.e. in this one specific case the very domain ID needs to be passed
> in).
 int xc_domain_getinfo(xc_interface *xch,
   uint32_t first_domid,
   unsigned int max_doms,
   xc_dominfo_t *info)
 {
 unsigned int nr_doms;
 uint32_t next_domid = first_domid;
 DECLARE_DOMCTL;
 int rc = 0;

 memset(info, 0, max_doms*sizeof(xc_dominfo_t));

 for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
 {   
 domctl.cmd = XEN_DOMCTL_getdomaininfo;
 domctl.domain = (domid_t)next_domid;
 if ( (rc = do_domctl(xch, )) < 0 )
 break;
 info->domid  = (uint16_t)domctl.domain;
 (...)
 next_domid = (uint16_t)domctl.domain + 1;


 Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
>>> valid
 domain.
>>> Hmm, looks like I've misread you patch. Reading again...
>>>
>>> But now I see rcu_read_lock(_read_lock) is gets called only when
>>> looping over domains, but rcu_read_unlock is called in any case. Is it
>>> correct?
>> How that? There is this third hunk:
> 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: move domain lookup for getdomaininfo to the same
>
> XEN_DOMCTL_getdomaininfo have different semantics than most of others
> domctls - it returns information about first valid domain with ID >=
> argument. But that's no excuse for having the lookup done in a different
> place, which made handling different corner cases unnecessary complex.
> Move the lookup to the first switch clause. And adjust locking to be the
> same as for other cases.
>
> Signed-off-by: Marek Marczykowski-Górecki 

FWIW, I prefer this solution to the issue.

Reviewed-by: Andrew Cooper , with a few style
nits.

> ---
>  xen/common/domctl.c | 41 -
>  1 file changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e43904e..6ae1fe0 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -442,11 +442,29 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>  switch ( op->cmd )
>  {
>  case XEN_DOMCTL_createdomain:
> -case XEN_DOMCTL_getdomaininfo:
>  case XEN_DOMCTL_test_assign_device:
>  case XEN_DOMCTL_gdbsx_guestmemio:
>  d = NULL;
>  break;

Newline here please.

> +case XEN_DOMCTL_getdomaininfo:
> +d = rcu_lock_domain_by_id(op->domain);

And here.

> +if ( d == NULL )
> +{
> +/* search for the next valid domain */

/* Search for the next available domain. */

> +rcu_read_lock(_read_lock);
> +
> +for_each_domain ( d )
> +if ( d->domain_id >= op->domain )
> +{
> +rcu_lock_domain(d);
> +break;
> +}
> +
> +rcu_read_unlock(_read_lock);
> +if ( d == NULL )
> +return -ESRCH;
> +}
> +break;

Another newline here please.

>  default:
>  d = rcu_lock_domain_by_id(op->domain);
>  if ( d == NULL )
> @@ -862,33 +880,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>  break;
>  
>  case XEN_DOMCTL_getdomaininfo:
> -{
> -domid_t dom

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 11:23,  wrote:
> > On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
> >> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
> >> > >>> On 23.06.16 at 10:57,  wrote:
> >> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >> > >> I wonder what good the duplication of the returned domain ID does: I'm
> >> > >> tempted to remove the one in the command-specific structure. Does
> >> > >> anyone have insight into why it was done that way?
> >> > > 
> >> > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
> >> > > existing domain with ID >= passed one? Reading various comments in code
> >> > > it looks to be used to domain enumeration. This patch changes this
> >> > > behaviour.
> >> > 
> >> > No, it doesn't. It adjusts the behavior only for the DM case (which
> >> > isn't supposed to get information on other than the target domain,
> >> > i.e. in this one specific case the very domain ID needs to be passed
> >> > in).
> >> 
> >> int xc_domain_getinfo(xc_interface *xch,
> >>   uint32_t first_domid,
> >>   unsigned int max_doms,
> >>   xc_dominfo_t *info)
> >> {
> >> unsigned int nr_doms;
> >> uint32_t next_domid = first_domid;
> >> DECLARE_DOMCTL;
> >> int rc = 0;
> >> 
> >> memset(info, 0, max_doms*sizeof(xc_dominfo_t));
> >> 
> >> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
> >> {   
> >> domctl.cmd = XEN_DOMCTL_getdomaininfo;
> >> domctl.domain = (domid_t)next_domid;
> >> if ( (rc = do_domctl(xch, )) < 0 )
> >> break;
> >> info->domid  = (uint16_t)domctl.domain;
> >> (...)
> >> next_domid = (uint16_t)domctl.domain + 1;
> >> 
> >> 
> >> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
> > valid
> >> domain.
> > 
> > Hmm, looks like I've misread you patch. Reading again...
> > 
> > But now I see rcu_read_lock(_read_lock) is gets called only when
> > looping over domains, but rcu_read_unlock is called in any case. Is it
> > correct?
> 
> How that? There is this third hunk:

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: move domain lookup for getdomaininfo to the same

XEN_DOMCTL_getdomaininfo have different semantics than most of others
domctls - it returns information about first valid domain with ID >=
argument. But that's no excuse for having the lookup done in a different
place, which made handling different corner cases unnecessary complex.
Move the lookup to the first switch clause. And adjust locking to be the
same as for other cases.

Signed-off-by: Marek Marczykowski-Górecki 
---
 xen/common/domctl.c | 41 -
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..6ae1fe0 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -442,11 +442,29 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
 switch ( op->cmd )
 {
 case XEN_DOMCTL_createdomain:
-case XEN_DOMCTL_getdomaininfo:
 case XEN_DOMCTL_test_assign_device:
 case XEN_DOMCTL_gdbsx_guestmemio:
 d = NULL;
 break;
+case XEN_DOMCTL_getdomaininfo:
+d = rcu_lock_domain_by_id(op->domain);
+if ( d == NULL )
+{
+/* search for the next valid domain */
+rcu_read_lock(_read_lock);
+
+for_each_domain ( d )
+if ( d->domain_id >= op->domain )
+{
+rcu_lock_domain(d);
+break;
+}
+
+rcu_read_unlock(_read_lock);
+if ( d == NULL )
+return -ESRCH;
+}
+break;
 default:
 d = rcu_lock_domain_by_id(op->domain);
 if ( d == NULL )
@@ -862,33 +880,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
u_domctl)
 break;
 
 case XEN_DOMCTL_getdomaininfo:
-{
-domid_t dom = op->domain;
-
-rcu_read_lock(_read_lock);
-
-for_each_domain ( d )
-if ( d->domain_id >= dom )
-break;
-
-ret = -ESRCH;
-if ( d == NULL )
-goto getdomaininfo_out;
-
 ret = xsm_getdomaininfo(XSM_HOOK, d);
 if ( ret )
-goto getdomaininfo_out;
+

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 11:44,  wrote:
> On 23/06/16 09:32, Jan Beulich wrote:
>> I wonder what good the duplication of the returned domain ID does: I'm
>> tempted to remove the one in the command-specific structure. Does
>> anyone have insight into why it was done that way?
> 
> This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy
> semantics, which go out of their way to make it easy to get wrong.
> 
> It is important that you always check the returned domid, as it may not
> be the domain you asked for.  In particular, if a domain you are looking
> after dies, the adjacent domain's information will be returnned.

Same question as to Marek: How is this related to my remark?

> Also, noone has yet addressed the issue that, strictly speaking,
> xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the
> SYSCTL interface version.  This in particular makes things interesting
> for valgrind support.

True, and afaict also the case for XEN_SCHEDULER_*. No idea how
to cleanly address this other than by folding the two versions.

Jan


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

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 11:23,  wrote:
> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>> > >>> On 23.06.16 at 10:57,  wrote:
>> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> > >> I wonder what good the duplication of the returned domain ID does: I'm
>> > >> tempted to remove the one in the command-specific structure. Does
>> > >> anyone have insight into why it was done that way?
>> > > 
>> > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
>> > > existing domain with ID >= passed one? Reading various comments in code
>> > > it looks to be used to domain enumeration. This patch changes this
>> > > behaviour.
>> > 
>> > No, it doesn't. It adjusts the behavior only for the DM case (which
>> > isn't supposed to get information on other than the target domain,
>> > i.e. in this one specific case the very domain ID needs to be passed
>> > in).
>> 
>> int xc_domain_getinfo(xc_interface *xch,
>>   uint32_t first_domid,
>>   unsigned int max_doms,
>>   xc_dominfo_t *info)
>> {
>> unsigned int nr_doms;
>> uint32_t next_domid = first_domid;
>> DECLARE_DOMCTL;
>> int rc = 0;
>> 
>> memset(info, 0, max_doms*sizeof(xc_dominfo_t));
>> 
>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
>> {   
>> domctl.cmd = XEN_DOMCTL_getdomaininfo;
>> domctl.domain = (domid_t)next_domid;
>> if ( (rc = do_domctl(xch, )) < 0 )
>> break;
>> info->domid  = (uint16_t)domctl.domain;
>> (...)
>> next_domid = (uint16_t)domctl.domain + 1;
>> 
>> 
>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
> valid
>> domain.
> 
> Hmm, looks like I've misread you patch. Reading again...
> 
> But now I see rcu_read_lock(_read_lock) is gets called only when
> looping over domains, but rcu_read_unlock is called in any case. Is it
> correct?

How that? There is this third hunk:

@@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 copyback = 1;
 
 getdomaininfo_out:
+if ( dom == DOMID_INVALID )
+break;
+
 rcu_read_unlock(_read_lock);
 d = NULL;
 break;

Jan

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

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 11:18,  wrote:
> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>> >>> On 23.06.16 at 10:57,  wrote:
>> > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> >> >>> On 22.06.16 at 20:24,  wrote:
>> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
>> >> > On 22.06.16 at 16:13,  wrote:
>> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >> >>> On 22.06.16 at 15:03,  wrote:
>> >> > I've finally found what was causing long standing issue of not 
>> >> > working
>> >> > PCI passthrough for HVM domains with qemu in stubdomain (only - 
>> >> > without
>> >> > the other one in dom0). It looks to be this patch:
>> >> >
>> >> >>> 
>> > 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f 
>> >> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
>> >> >
>> >> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check 
>> >> > if
>> >> > the target domain is auto-translated), but xc_domain_getinfo fails 
>> >> > with
>> >> > EPERM in stubdomain.
>> >> >
>> >> > What would be the best solution for this? Allowing xc_domain_getinfo
>> >> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses 
>> >> > policy
>> >> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have 
>> >> > some
>> >> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
>> >> > removing xc_domain_getinfo call in xc_domain_memory_mapping, 
>> >> > possibly
>> >> > implementing the logic from that commit solely in libxl?
>> >> 
>> >>  Once we fixed the quirky behavior of the current implementation
>> >>  (allowing information to be returned for other than the requested
>> >>  domain), I see no reason why this couldn't become XSM_DM_PRIV.
>> >> >>>
>> >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
>> >> >>
>> >> >> Which fix? I talked of one to be made.
>> >> >>
>> >>  But let's ask Daniel explicitly. And in that context I then also 
>> >>  wonder
>> >>  whether the xsm_getdomaininfo() invocation shouldn't be limited to
>> >>  the respective sysctl.
>> >> >>>
>> >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
>> >> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
>> >> >>>  - xsm_domctl (which enforce actual policy)
>> >> >>>
>> >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
>> >> >>> useful to be able to just check if given domain is alive, without
>> >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I 
>> >> >>> find
>> >> >>> this useful also for any other inter-domain communication (for example
>> >> >>> libxenvchan connection).
>> >> >>>
>> >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
>> >> >>> device-model domain is asking about its target domain, or calling 
>> >> >>> domain
>> >> >>> is xenstore domain/privileged domain. Right?
>> >> >>
>> >> >> Yes, that's what I think too.
>> >> >>
>> >> >>>  How to combine those
>> >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
>> >> >>> usage of XSM_XS_PRIV)?
>> >> > 
>> >> > Changing the definition of XSM_XS_PRIV seems like the best solution, 
>> >> > since
>> >> > this is the only use.  I don't think it matters if the constant is 
>> >> > renamed
>> >> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used 
>> >> > by 
> a
>> >> > caller, it could be removed and the actual logic placed in the switch
>> >> > statement - that way it's clear this is a special case for getdomaininfo
>> >> > instead of attempting to make this generic.
>> >> > 
>> >> > Either method works, and I agree allowing DM to invoke this domctl is 
>> >> > both
>> >> > useful and not going to introduce problems.  The getdomaininfo 
>> >> > permission
>> >> > will also need to be added to the device_model macro in xen.if.
>> >> 
>> >> What exactly this last sentence means I need to add I'm not sure
>> >> about. Apart from that, how about the change below?
>> >> 
>> >> Jan
>> >> 
>> >> domctl: relax getdomaininfo permissions
>> >> 
>> >> Qemu needs access to this for the domain it controls, both due to it
>> >> being used by xc_domain_memory_mapping() (which qemu calls) and the
>> >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
>> >> 
>> >> This at once avoids a for_each_domain() loop when the ID of an
>> >> existing domain gets passed in.
>> >>
>> >> Reported-by: Marek Marczykowski-Górecki 
>> >> ---
>> >> I wonder what good the duplication of the returned domain ID does: I'm
>> >> tempted to remove the one in the command-specific structure. Does
>> >> anyone

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

2016-06-23 Thread Andrew Cooper 
On 23/06/16 09:32, Jan Beulich wrote:
 On 22.06.16 at 20:24,  wrote:
>> On 06/22/2016 11:23 AM, Jan Beulich wrote:
>> On 22.06.16 at 16:13,  wrote:
 On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
 On 22.06.16 at 15:03,  wrote:
>> I've finally found what was causing long standing issue of not working
>> PCI passthrough for HVM domains with qemu in stubdomain (only - without
>> the other one in dom0). It looks to be this patch:
>>
 http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
  
>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
>>
>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
>> the target domain is auto-translated), but xc_domain_getinfo fails with
>> EPERM in stubdomain.
>>
>> What would be the best solution for this? Allowing xc_domain_getinfo
>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
>> implementing the logic from that commit solely in libxl?
> Once we fixed the quirky behavior of the current implementation
> (allowing information to be returned for other than the requested
> domain), I see no reason why this couldn't become XSM_DM_PRIV.
 Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
>>> Which fix? I talked of one to be made.
>>>
> But let's ask Daniel explicitly. And in that context I then also wonder
> whether the xsm_getdomaininfo() invocation shouldn't be limited to
> the respective sysctl.
 Actually getdomaininfo is handled in two places in xsm/dummy.h:
  - xsm_getdomaininfo (which does nothing when XSM is disabled)
  - xsm_domctl (which enforce actual policy)

 Also reading commit message of XSM_XS_PRIV introduction, it may be
 useful to be able to just check if given domain is alive, without
 getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
 this useful also for any other inter-domain communication (for example
 libxenvchan connection).

 But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
 device-model domain is asking about its target domain, or calling domain
 is xenstore domain/privileged domain. Right?
>>> Yes, that's what I think too.
>>>
  How to combine those
 types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
 usage of XSM_XS_PRIV)?
>> Changing the definition of XSM_XS_PRIV seems like the best solution, since
>> this is the only use.  I don't think it matters if the constant is renamed
>> to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
>> caller, it could be removed and the actual logic placed in the switch
>> statement - that way it's clear this is a special case for getdomaininfo
>> instead of attempting to make this generic.
>>
>> Either method works, and I agree allowing DM to invoke this domctl is both
>> useful and not going to introduce problems.  The getdomaininfo permission
>> will also need to be added to the device_model macro in xen.if.
> What exactly this last sentence means I need to add I'm not sure
> about. Apart from that, how about the change below?
>
> Jan
>
> domctl: relax getdomaininfo permissions
>
> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
>
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
>
> Reported-by: Marek Marczykowski-Górecki 
> ---
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?

This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy
semantics, which go out of their way to make it easy to get wrong.

It is important that you always check the returned domid, as it may not
be the domain you asked for.  In particular, if a domain you are looking
after dies, the adjacent domain's information will be returnned.

Also, noone has yet addressed the issue that, strictly speaking,
xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the
SYSCTL interface version.  This in particular makes things interesting
for valgrind support.

~Andrew

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

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
> > >>> On 23.06.16 at 10:57,  wrote:
> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> > >> >>> On 22.06.16 at 20:24,  wrote:
> > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> > >> > On 22.06.16 at 16:13,  wrote:
> > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> > >> >>> On 22.06.16 at 15:03,  wrote:
> > >> > I've finally found what was causing long standing issue of not 
> > >> > working
> > >> > PCI passthrough for HVM domains with qemu in stubdomain (only - 
> > >> > without
> > >> > the other one in dom0). It looks to be this patch:
> > >> >
> > >> >>> 
> > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
> > >  
> > >> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> > >> >
> > >> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check 
> > >> > if
> > >> > the target domain is auto-translated), but xc_domain_getinfo fails 
> > >> > with
> > >> > EPERM in stubdomain.
> > >> >
> > >> > What would be the best solution for this? Allowing 
> > >> > xc_domain_getinfo
> > >> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses 
> > >> > policy
> > >> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have 
> > >> > some
> > >> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> > >> > removing xc_domain_getinfo call in xc_domain_memory_mapping, 
> > >> > possibly
> > >> > implementing the logic from that commit solely in libxl?
> > >> 
> > >>  Once we fixed the quirky behavior of the current implementation
> > >>  (allowing information to be returned for other than the requested
> > >>  domain), I see no reason why this couldn't become XSM_DM_PRIV.
> > >> >>>
> > >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
> > >> >>
> > >> >> Which fix? I talked of one to be made.
> > >> >>
> > >>  But let's ask Daniel explicitly. And in that context I then also 
> > >>  wonder
> > >>  whether the xsm_getdomaininfo() invocation shouldn't be limited to
> > >>  the respective sysctl.
> > >> >>>
> > >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
> > >> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
> > >> >>>  - xsm_domctl (which enforce actual policy)
> > >> >>>
> > >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
> > >> >>> useful to be able to just check if given domain is alive, without
> > >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I 
> > >> >>> find
> > >> >>> this useful also for any other inter-domain communication (for 
> > >> >>> example
> > >> >>> libxenvchan connection).
> > >> >>>
> > >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
> > >> >>> device-model domain is asking about its target domain, or calling 
> > >> >>> domain
> > >> >>> is xenstore domain/privileged domain. Right?
> > >> >>
> > >> >> Yes, that's what I think too.
> > >> >>
> > >> >>>  How to combine those
> > >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
> > >> >>> usage of XSM_XS_PRIV)?
> > >> > 
> > >> > Changing the definition of XSM_XS_PRIV seems like the best solution, 
> > >> > since
> > >> > this is the only use.  I don't think it matters if the constant is 
> > >> > renamed
> > >> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used 
> > >> > by a
> > >> > caller, it could be removed and the actual logic placed in the switch
> > >> > statement - that way it's clear this is a special case for 
> > >> > getdomaininfo
> > >> > instead of attempting to make this generic.
> > >> > 
> > >> > Either method works, and I agree allowing DM to invoke this domctl is 
> > >> > both
> > >> > useful and not going to introduce problems.  The getdomaininfo 
> > >> > permission
> > >> > will also need to be added to the device_model macro in xen.if.
> > >> 
> > >> What exactly this last sentence means I need to add I'm not sure
> > >> about. Apart from that, how about the change below?
> > >> 
> > >> Jan
> > >> 
> > >> domctl: relax getdomaininfo permissions
> > >> 
> > >> Qemu needs access to this for the domain it controls, both due to it
> > >> being used by xc_domain_memory_mapping() (which qemu calls) and the
> > >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
> > >> 
> > >> This at once avoids a for_each_domain() loop when the ID of an
> > >> existing domain gets passed in.
> > >>
> > >> Reported-by: Marek Marczykowski-Górecki 
> > >> ---
> > >> I wonder

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 10:57,  wrote:
> > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 20:24,  wrote:
> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> >> > On 22.06.16 at 16:13,  wrote:
> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 15:03,  wrote:
> >> > I've finally found what was causing long standing issue of not 
> >> > working
> >> > PCI passthrough for HVM domains with qemu in stubdomain (only - 
> >> > without
> >> > the other one in dom0). It looks to be this patch:
> >> >
> >> >>> 
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
> >  
> >> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> >> >
> >> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> >> > the target domain is auto-translated), but xc_domain_getinfo fails 
> >> > with
> >> > EPERM in stubdomain.
> >> >
> >> > What would be the best solution for this? Allowing xc_domain_getinfo
> >> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses 
> >> > policy
> >> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have 
> >> > some
> >> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> >> > removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> >> > implementing the logic from that commit solely in libxl?
> >> 
> >>  Once we fixed the quirky behavior of the current implementation
> >>  (allowing information to be returned for other than the requested
> >>  domain), I see no reason why this couldn't become XSM_DM_PRIV.
> >> >>>
> >> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
> >> >>
> >> >> Which fix? I talked of one to be made.
> >> >>
> >>  But let's ask Daniel explicitly. And in that context I then also 
> >>  wonder
> >>  whether the xsm_getdomaininfo() invocation shouldn't be limited to
> >>  the respective sysctl.
> >> >>>
> >> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
> >> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
> >> >>>  - xsm_domctl (which enforce actual policy)
> >> >>>
> >> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
> >> >>> useful to be able to just check if given domain is alive, without
> >> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I 
> >> >>> find
> >> >>> this useful also for any other inter-domain communication (for example
> >> >>> libxenvchan connection).
> >> >>>
> >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
> >> >>> device-model domain is asking about its target domain, or calling 
> >> >>> domain
> >> >>> is xenstore domain/privileged domain. Right?
> >> >>
> >> >> Yes, that's what I think too.
> >> >>
> >> >>>  How to combine those
> >> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
> >> >>> usage of XSM_XS_PRIV)?
> >> > 
> >> > Changing the definition of XSM_XS_PRIV seems like the best solution, 
> >> > since
> >> > this is the only use.  I don't think it matters if the constant is 
> >> > renamed
> >> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used 
> >> > by a
> >> > caller, it could be removed and the actual logic placed in the switch
> >> > statement - that way it's clear this is a special case for getdomaininfo
> >> > instead of attempting to make this generic.
> >> > 
> >> > Either method works, and I agree allowing DM to invoke this domctl is 
> >> > both
> >> > useful and not going to introduce problems.  The getdomaininfo permission
> >> > will also need to be added to the device_model macro in xen.if.
> >> 
> >> What exactly this last sentence means I need to add I'm not sure
> >> about. Apart from that, how about the change below?
> >> 
> >> Jan
> >> 
> >> domctl: relax getdomaininfo permissions
> >> 
> >> Qemu needs access to this for the domain it controls, both due to it
> >> being used by xc_domain_memory_mapping() (which qemu calls) and the
> >> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
> >> 
> >> This at once avoids a for_each_domain() loop when the ID of an
> >> existing domain gets passed in.
> >>
> >> Reported-by: Marek Marczykowski-Górecki 
> >> ---
> >> I wonder what good the duplication of the returned domain ID does: I'm
> >> tempted to remove the one in the command-specific structure. Does
> >> anyone have insight into why it was done that way?
> > 
> > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
> > existing domain with ID >= passed one? Reading various comments in code
> > it looks

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 10:57,  wrote:
> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 20:24,  wrote:
>> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
>> > On 22.06.16 at 16:13,  wrote:
>> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 15:03,  wrote:
>> > I've finally found what was causing long standing issue of not working
>> > PCI passthrough for HVM domains with qemu in stubdomain (only - without
>> > the other one in dom0). It looks to be this patch:
>> >
>> >>> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f 
>> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
>> >
>> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
>> > the target domain is auto-translated), but xc_domain_getinfo fails with
>> > EPERM in stubdomain.
>> >
>> > What would be the best solution for this? Allowing xc_domain_getinfo
>> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
>> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
>> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
>> > removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
>> > implementing the logic from that commit solely in libxl?
>> 
>>  Once we fixed the quirky behavior of the current implementation
>>  (allowing information to be returned for other than the requested
>>  domain), I see no reason why this couldn't become XSM_DM_PRIV.
>> >>>
>> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
>> >>
>> >> Which fix? I talked of one to be made.
>> >>
>>  But let's ask Daniel explicitly. And in that context I then also wonder
>>  whether the xsm_getdomaininfo() invocation shouldn't be limited to
>>  the respective sysctl.
>> >>>
>> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
>> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
>> >>>  - xsm_domctl (which enforce actual policy)
>> >>>
>> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
>> >>> useful to be able to just check if given domain is alive, without
>> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
>> >>> this useful also for any other inter-domain communication (for example
>> >>> libxenvchan connection).
>> >>>
>> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
>> >>> device-model domain is asking about its target domain, or calling domain
>> >>> is xenstore domain/privileged domain. Right?
>> >>
>> >> Yes, that's what I think too.
>> >>
>> >>>  How to combine those
>> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
>> >>> usage of XSM_XS_PRIV)?
>> > 
>> > Changing the definition of XSM_XS_PRIV seems like the best solution, since
>> > this is the only use.  I don't think it matters if the constant is renamed
>> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
>> > caller, it could be removed and the actual logic placed in the switch
>> > statement - that way it's clear this is a special case for getdomaininfo
>> > instead of attempting to make this generic.
>> > 
>> > Either method works, and I agree allowing DM to invoke this domctl is both
>> > useful and not going to introduce problems.  The getdomaininfo permission
>> > will also need to be added to the device_model macro in xen.if.
>> 
>> What exactly this last sentence means I need to add I'm not sure
>> about. Apart from that, how about the change below?
>> 
>> Jan
>> 
>> domctl: relax getdomaininfo permissions
>> 
>> Qemu needs access to this for the domain it controls, both due to it
>> being used by xc_domain_memory_mapping() (which qemu calls) and the
>> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
>> 
>> This at once avoids a for_each_domain() loop when the ID of an
>> existing domain gets passed in.
>>
>> Reported-by: Marek Marczykowski-Górecki 
>> ---
>> I wonder what good the duplication of the returned domain ID does: I'm
>> tempted to remove the one in the command-specific structure. Does
>> anyone have insight into why it was done that way?
> 
> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
> existing domain with ID >= passed one? Reading various comments in code
> it looks to be used to domain enumeration. This patch changes this
> behaviour.

No, it doesn't. It adjusts the behavior only for the DM case (which
isn't supposed to get information on other than the target domain,
i.e. in this one specific case the very domain ID needs to be passed
in).

Also, how is this comment of yours related to the remark above?

Jan

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

2016-06-23 Thread Marek Marczykowski-Górecki 
On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 20:24,  wrote:
> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> > On 22.06.16 at 16:13,  wrote:
> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 15:03,  wrote:
> > I've finally found what was causing long standing issue of not working
> > PCI passthrough for HVM domains with qemu in stubdomain (only - without
> > the other one in dom0). It looks to be this patch:
> >
> >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
> >>>  
> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> >
> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> > the target domain is auto-translated), but xc_domain_getinfo fails with
> > EPERM in stubdomain.
> >
> > What would be the best solution for this? Allowing xc_domain_getinfo
> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> > removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> > implementing the logic from that commit solely in libxl?
> 
>  Once we fixed the quirky behavior of the current implementation
>  (allowing information to be returned for other than the requested
>  domain), I see no reason why this couldn't become XSM_DM_PRIV.
> >>>
> >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
> >>
> >> Which fix? I talked of one to be made.
> >>
>  But let's ask Daniel explicitly. And in that context I then also wonder
>  whether the xsm_getdomaininfo() invocation shouldn't be limited to
>  the respective sysctl.
> >>>
> >>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
> >>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
> >>>  - xsm_domctl (which enforce actual policy)
> >>>
> >>> Also reading commit message of XSM_XS_PRIV introduction, it may be
> >>> useful to be able to just check if given domain is alive, without
> >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
> >>> this useful also for any other inter-domain communication (for example
> >>> libxenvchan connection).
> >>>
> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
> >>> device-model domain is asking about its target domain, or calling domain
> >>> is xenstore domain/privileged domain. Right?
> >>
> >> Yes, that's what I think too.
> >>
> >>>  How to combine those
> >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
> >>> usage of XSM_XS_PRIV)?
> > 
> > Changing the definition of XSM_XS_PRIV seems like the best solution, since
> > this is the only use.  I don't think it matters if the constant is renamed
> > to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
> > caller, it could be removed and the actual logic placed in the switch
> > statement - that way it's clear this is a special case for getdomaininfo
> > instead of attempting to make this generic.
> > 
> > Either method works, and I agree allowing DM to invoke this domctl is both
> > useful and not going to introduce problems.  The getdomaininfo permission
> > will also need to be added to the device_model macro in xen.if.
> 
> What exactly this last sentence means I need to add I'm not sure
> about. Apart from that, how about the change below?
> 
> Jan
> 
> domctl: relax getdomaininfo permissions
> 
> Qemu needs access to this for the domain it controls, both due to it
> being used by xc_domain_memory_mapping() (which qemu calls) and the
> explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().
> 
> This at once avoids a for_each_domain() loop when the ID of an
> existing domain gets passed in.
>
> Reported-by: Marek Marczykowski-Górecki 
> ---
> I wonder what good the duplication of the returned domain ID does: I'm
> tempted to remove the one in the command-specific structure. Does
> anyone have insight into why it was done that way?

Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
existing domain with ID >= passed one? Reading various comments in code
it looks to be used to domain enumeration. This patch changes this
behaviour.

> I further wonder why we have XSM_OTHER: The respective conversion into
> other XSM_* values in xsm/dummy.h could as well move into the callers,
> making intentions more obvious when looking at the actual code.
> 
> --- unstable.orig/xen/common/domctl.c
> +++ unstable/xen/common/domctl.c
> @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  switch ( op->cmd )
>  {
>  case XEN_DOMCTL_createdomain:
> -case

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

2016-06-23 Thread Jan Beulich 
>>> On 23.06.16 at 10:32,  wrote:
 On 22.06.16 at 20:24,  wrote:
>> Either method works, and I agree allowing DM to invoke this domctl is both
>> useful and not going to introduce problems.  The getdomaininfo permission
>> will also need to be added to the device_model macro in xen.if.
> 
> What exactly this last sentence means I need to add I'm not sure
> about.

Perhaps this?

--- unstable.orig/tools/flask/policy/policy/modules/xen/xen.if
+++ unstable/tools/flask/policy/policy/modules/xen/xen.if
@@ -148,7 +148,7 @@ define(`device_model', `
create_channel($2, $1, $2_channel)
allow $1 $2_channel:event create;
 
-   allow $1 $2_target:domain shutdown;
+   allow $1 $2_target:domain { getdomaininfo shutdown };
allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack 
};
allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl 
irqlevel pciroute pcilevel cacheattr send_irq };
 ')

Jan


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

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

2016-06-23 Thread Jan Beulich 
>>> On 22.06.16 at 20:24,  wrote:
> On 06/22/2016 11:23 AM, Jan Beulich wrote:
> On 22.06.16 at 16:13,  wrote:
>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>> On 22.06.16 at 15:03,  wrote:
> I've finally found what was causing long standing issue of not working
> PCI passthrough for HVM domains with qemu in stubdomain (only - without
> the other one in dom0). It looks to be this patch:
>
>>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
>>>  
> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
>
> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> the target domain is auto-translated), but xc_domain_getinfo fails with
> EPERM in stubdomain.
>
> What would be the best solution for this? Allowing xc_domain_getinfo
> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> implementing the logic from that commit solely in libxl?

 Once we fixed the quirky behavior of the current implementation
 (allowing information to be returned for other than the requested
 domain), I see no reason why this couldn't become XSM_DM_PRIV.
>>>
>>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?
>>
>> Which fix? I talked of one to be made.
>>
 But let's ask Daniel explicitly. And in that context I then also wonder
 whether the xsm_getdomaininfo() invocation shouldn't be limited to
 the respective sysctl.
>>>
>>> Actually getdomaininfo is handled in two places in xsm/dummy.h:
>>>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
>>>  - xsm_domctl (which enforce actual policy)
>>>
>>> Also reading commit message of XSM_XS_PRIV introduction, it may be
>>> useful to be able to just check if given domain is alive, without
>>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
>>> this useful also for any other inter-domain communication (for example
>>> libxenvchan connection).
>>>
>>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
>>> device-model domain is asking about its target domain, or calling domain
>>> is xenstore domain/privileged domain. Right?
>>
>> Yes, that's what I think too.
>>
>>>  How to combine those
>>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
>>> usage of XSM_XS_PRIV)?
> 
> Changing the definition of XSM_XS_PRIV seems like the best solution, since
> this is the only use.  I don't think it matters if the constant is renamed
> to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
> caller, it could be removed and the actual logic placed in the switch
> statement - that way it's clear this is a special case for getdomaininfo
> instead of attempting to make this generic.
> 
> Either method works, and I agree allowing DM to invoke this domctl is both
> useful and not going to introduce problems.  The getdomaininfo permission
> will also need to be added to the device_model macro in xen.if.

What exactly this last sentence means I need to add I'm not sure
about. Apart from that, how about the change below?

Jan

domctl: relax getdomaininfo permissions

Qemu needs access to this for the domain it controls, both due to it
being used by xc_domain_memory_mapping() (which qemu calls) and the
explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll().

This at once avoids a for_each_domain() loop when the ID of an
existing domain gets passed in.

Reported-by: Marek Marczykowski-Górecki 
---
I wonder what good the duplication of the returned domain ID does: I'm
tempted to remove the one in the command-specific structure. Does
anyone have insight into why it was done that way?

I further wonder why we have XSM_OTHER: The respective conversion into
other XSM_* values in xsm/dummy.h could as well move into the callers,
making intentions more obvious when looking at the actual code.

--- unstable.orig/xen/common/domctl.c
+++ unstable/xen/common/domctl.c
@@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 switch ( op->cmd )
 {
 case XEN_DOMCTL_createdomain:
-case XEN_DOMCTL_getdomaininfo:
 case XEN_DOMCTL_test_assign_device:
 case XEN_DOMCTL_gdbsx_guestmemio:
 d = NULL;
 break;
 default:
 d = rcu_lock_domain_by_id(op->domain);
-if ( d == NULL )
+if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
 return -ESRCH;
 }
 
@@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
 case XEN_DOMCTL_getdomaininfo:
 {
-domid_t dom = op->domain;
-
-

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

2016-06-22 Thread Daniel De Graaf 

On 06/22/2016 11:23 AM, Jan Beulich wrote:

On 22.06.16 at 16:13,  wrote:

On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:

On 22.06.16 at 15:03,  wrote:

I've finally found what was causing long standing issue of not working
PCI passthrough for HVM domains with qemu in stubdomain (only - without
the other one in dom0). It looks to be this patch:


http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f

ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257

It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
the target domain is auto-translated), but xc_domain_getinfo fails with
EPERM in stubdomain.

What would be the best solution for this? Allowing xc_domain_getinfo
from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
implementing the logic from that commit solely in libxl?


Once we fixed the quirky behavior of the current implementation
(allowing information to be returned for other than the requested
domain), I see no reason why this couldn't become XSM_DM_PRIV.


Can you explain this more? Is this fix backported to 4.6 and/or 4.4?


Which fix? I talked of one to be made.


But let's ask Daniel explicitly. And in that context I then also wonder
whether the xsm_getdomaininfo() invocation shouldn't be limited to
the respective sysctl.


Actually getdomaininfo is handled in two places in xsm/dummy.h:
 - xsm_getdomaininfo (which does nothing when XSM is disabled)
 - xsm_domctl (which enforce actual policy)

Also reading commit message of XSM_XS_PRIV introduction, it may be
useful to be able to just check if given domain is alive, without
getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
this useful also for any other inter-domain communication (for example
libxenvchan connection).

But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
device-model domain is asking about its target domain, or calling domain
is xenstore domain/privileged domain. Right?


Yes, that's what I think too.


 How to combine those
types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
usage of XSM_XS_PRIV)?


Daniel?

Jan


Changing the definition of XSM_XS_PRIV seems like the best solution, since
this is the only use.  I don't think it matters if the constant is renamed
to XSM_XS_DM_PRIV or not.  In fact, since the constant isn't ever used by a
caller, it could be removed and the actual logic placed in the switch
statement - that way it's clear this is a special case for getdomaininfo
instead of attempting to make this generic.

Either method works, and I agree allowing DM to invoke this domctl is both
useful and not going to introduce problems.  The getdomaininfo permission
will also need to be added to the device_model macro in xen.if.

--
Daniel De Graaf
National Security Agency

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

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

2016-06-22 Thread Jan Beulich 
>>> On 22.06.16 at 16:13,  wrote:
> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 15:03,  wrote:
>> > I've finally found what was causing long standing issue of not working
>> > PCI passthrough for HVM domains with qemu in stubdomain (only - without
>> > the other one in dom0). It looks to be this patch:
>> > 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f 
>> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
>> > 
>> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
>> > the target domain is auto-translated), but xc_domain_getinfo fails with
>> > EPERM in stubdomain.
>> > 
>> > What would be the best solution for this? Allowing xc_domain_getinfo
>> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
>> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
>> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
>> > removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
>> > implementing the logic from that commit solely in libxl?
>> 
>> Once we fixed the quirky behavior of the current implementation
>> (allowing information to be returned for other than the requested
>> domain), I see no reason why this couldn't become XSM_DM_PRIV.
> 
> Can you explain this more? Is this fix backported to 4.6 and/or 4.4?

Which fix? I talked of one to be made.

>> But let's ask Daniel explicitly. And in that context I then also wonder
>> whether the xsm_getdomaininfo() invocation shouldn't be limited to
>> the respective sysctl.
> 
> Actually getdomaininfo is handled in two places in xsm/dummy.h:
>  - xsm_getdomaininfo (which does nothing when XSM is disabled)
>  - xsm_domctl (which enforce actual policy)
> 
> Also reading commit message of XSM_XS_PRIV introduction, it may be
> useful to be able to just check if given domain is alive, without
> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
> this useful also for any other inter-domain communication (for example
> libxenvchan connection).
> 
> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
> device-model domain is asking about its target domain, or calling domain
> is xenstore domain/privileged domain. Right?

Yes, that's what I think too.

>  How to combine those
> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
> usage of XSM_XS_PRIV)?

Daniel?

Jan


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

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

2016-06-22 Thread Marek Marczykowski-Górecki 
On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 15:03,  wrote:
> > I've finally found what was causing long standing issue of not working
> > PCI passthrough for HVM domains with qemu in stubdomain (only - without
> > the other one in dom0). It looks to be this patch:
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f
> >  
> > ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> > 
> > It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> > the target domain is auto-translated), but xc_domain_getinfo fails with
> > EPERM in stubdomain.
> > 
> > What would be the best solution for this? Allowing xc_domain_getinfo
> > from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
> > XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
> > combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> > removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> > implementing the logic from that commit solely in libxl?
> 
> Once we fixed the quirky behavior of the current implementation
> (allowing information to be returned for other than the requested
> domain), I see no reason why this couldn't become XSM_DM_PRIV.

Can you explain this more? Is this fix backported to 4.6 and/or 4.4?

> But let's ask Daniel explicitly. And in that context I then also wonder
> whether the xsm_getdomaininfo() invocation shouldn't be limited to
> the respective sysctl.

Actually getdomaininfo is handled in two places in xsm/dummy.h:
 - xsm_getdomaininfo (which does nothing when XSM is disabled)
 - xsm_domctl (which enforce actual policy)

Also reading commit message of XSM_XS_PRIV introduction, it may be
useful to be able to just check if given domain is alive, without
getting all the information returned by XEN_DOMCTL_getdomaininfo. I find
this useful also for any other inter-domain communication (for example
libxenvchan connection).

But for now, XEN_DOMCTL_getdomaininfo should be allowed either when
device-model domain is asking about its target domain, or calling domain
is xenstore domain/privileged domain. Right?  How to combine those
types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only
usage of XSM_XS_PRIV)?

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


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2016-06-22 Thread Jan Beulich 
>>> On 22.06.16 at 15:03,  wrote:
> I've finally found what was causing long standing issue of not working
> PCI passthrough for HVM domains with qemu in stubdomain (only - without
> the other one in dom0). It looks to be this patch:
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f 
> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257
> 
> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
> the target domain is auto-translated), but xc_domain_getinfo fails with
> EPERM in stubdomain.
> 
> What would be the best solution for this? Allowing xc_domain_getinfo
> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
> implementing the logic from that commit solely in libxl?

Once we fixed the quirky behavior of the current implementation
(allowing information to be returned for other than the requested
domain), I see no reason why this couldn't become XSM_DM_PRIV.
But let's ask Daniel explicitly. And in that context I then also wonder
whether the xsm_getdomaininfo() invocation shouldn't be limited to
the respective sysctl.

Jan


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

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

2016-06-22 Thread Marek Marczykowski-Górecki 
Hi,

I've finally found what was causing long standing issue of not working
PCI passthrough for HVM domains with qemu in stubdomain (only - without
the other one in dom0). It looks to be this patch:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473fab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257

It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if
the target domain is auto-translated), but xc_domain_getinfo fails with
EPERM in stubdomain.

What would be the best solution for this? Allowing xc_domain_getinfo
from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy
XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some
combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by
removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly
implementing the logic from that commit solely in libxl?

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


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel