PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1111 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2545 bytes --]

On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Daniel De Graaf]

On 06/22/2016 11:23 AM, Jan Beulich wrote:
>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Jan Beulich]

>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
> On 06/22/2016 11:23 AM, Jan Beulich wrote:
>>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
---
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;
-
-        rcu_read_lock(&domlist_read_lock);
+        domid_t dom = DOMID_INVALID;
 
-        for_each_domain ( d )
-            if ( d->domain_id >= dom )
+        if ( !d )
+        {
+            ret = -EINVAL;
+            if ( op->domain >= DOMID_FIRST_RESERVED )
                 break;
 
+            rcu_read_lock(&domlist_read_lock);
+
+            dom = op->domain;
+            for_each_domain ( d )
+                if ( d->domain_id >= dom )
+                    break;
+        }
+
         ret = -ESRCH;
         if ( d == NULL )
             goto getdomaininfo_out;
@@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         copyback = 1;
 
     getdomaininfo_out:
+        if ( dom == DOMID_INVALID )
+            break;
+
         rcu_read_unlock(&domlist_read_lock);
         d = NULL;
         break;
--- unstable.orig/xen/include/xsm/dummy.h
+++ unstable/xen/include/xsm/dummy.h
@@ -61,7 +61,12 @@ static always_inline int xsm_default_act
         return 0;
     case XSM_TARGET:
         if ( src == target )
+        {
             return 0;
+    case XSM_XS_PRIV:
+            if ( src->is_xenstore )
+                return 0;
+        }
         /* fall through */
     case XSM_DM_PRIV:
         if ( target && src->target == target )
@@ -71,10 +76,6 @@ static always_inline int xsm_default_act
         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;



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

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

[Jan Beulich]

>>> On 23.06.16 at 10:32, <JBeulich@suse.com> wrote:
>>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Daniel De Graaf]

On 06/23/2016 04:39 AM, Jan Beulich wrote:
>>>> On 23.06.16 at 10:32, <JBeulich@suse.com> wrote:
>>>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 7565 bytes --]

On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
> ---
> 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 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;
> -
> -        rcu_read_lock(&domlist_read_lock);
> +        domid_t dom = DOMID_INVALID;
>  
> -        for_each_domain ( d )
> -            if ( d->domain_id >= dom )
> +        if ( !d )
> +        {
> +            ret = -EINVAL;
> +            if ( op->domain >= DOMID_FIRST_RESERVED )
>                  break;
>  
> +            rcu_read_lock(&domlist_read_lock);
> +
> +            dom = op->domain;
> +            for_each_domain ( d )
> +                if ( d->domain_id >= dom )
> +                    break;
> +        }
> +
>          ret = -ESRCH;
>          if ( d == NULL )
>              goto getdomaininfo_out;
> @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          copyback = 1;
>  
>      getdomaininfo_out:
> +        if ( dom == DOMID_INVALID )
> +            break;
> +
>          rcu_read_unlock(&domlist_read_lock);
>          d = NULL;
>          break;
> --- unstable.orig/xen/include/xsm/dummy.h
> +++ unstable/xen/include/xsm/dummy.h
> @@ -61,7 +61,12 @@ static always_inline int xsm_default_act
>          return 0;
>      case XSM_TARGET:
>          if ( src == target )
> +        {
>              return 0;
> +    case XSM_XS_PRIV:
> +            if ( src->is_xenstore )
> +                return 0;
> +        }
>          /* fall through */
>      case XSM_DM_PRIV:
>          if ( target && src->target == target )
> @@ -71,10 +76,6 @@ static always_inline int xsm_default_act
>          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;
> 
> 

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote:
> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
>> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
>> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
>> ---
>> 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

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 6407 bytes --]

On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote:
> > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
> >> ---
> >> 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, &domctl)) < 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.

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

Because this is why domid is needed in returned structure - to know about which
domain you've got the info.

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 6969 bytes --]

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, <marmarek@invisiblethingslab.com> wrote:
> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
> > >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
> > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
> > >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
> > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
> > >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
> > >> ---
> > >> 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, &domctl)) < 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(&domlist_read_lock) is gets called only when
looping over domains, but rcu_read_unlock is called in any case. Is it
correct?

> > Also, how is this comment of yours related to the remark above?
> 
> Because this is why domid is needed in returned structure - to know about which
> domain you've got the info.
> 



-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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(&domlist_read_lock);
         d = NULL;
         break;

Jan

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 7195 bytes --]

On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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 <marmarek@invisiblethingslab.com>
---
 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(&domlist_read_lock);
+
+            for_each_domain ( d )
+                if ( d->domain_id >= op->domain )
+                {
+                    rcu_lock_domain(d);
+                    break;
+                }
+
+            rcu_read_unlock(&domlist_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(&domlist_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;
+            break;
 
         getdomaininfo(d, &op->u.getdomaininfo);
-
         op->domain = op->u.getdomaininfo.domain;
         copyback = 1;
-
-    getdomaininfo_out:
-        rcu_read_unlock(&domlist_read_lock);
-        d = NULL;
         break;
-    }
 
     case XEN_DOMCTL_getvcpucontext:
     {
-- 
2.5.5

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@invisiblethingslab.com>
---
 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);
     }
-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[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, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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 <marmarek@invisiblethingslab.com>

FWIW, I prefer this solution to the issue.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, 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(&domlist_read_lock);
> +
> +            for_each_domain ( d )
> +                if ( d->domain_id >= op->domain )
> +                {
> +                    rcu_lock_domain(d);
> +                    break;
> +                }
> +
> +            rcu_read_unlock(&domlist_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 = op->domain;
> -
> -        rcu_read_lock(&domlist_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;
> +            break;
>  
>          getdomaininfo(d, &op->u.getdomaininfo);
> -
>          op->domain = op->u.getdomaininfo.domain;
>          copyback = 1;
> -
> -    getdomaininfo_out:
> -        rcu_read_unlock(&domlist_read_lock);
> -        d = NULL;
>          break;
> -    }

As far as I can tell, this is purely cleanup, and independent of XSM change.

~Andrew

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 6627 bytes --]

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, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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 <marmarek@invisiblethingslab.com>
> 
> FWIW, I prefer this solution to the issue.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, 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 <marmarek@invisiblethingslab.com>
---
 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(&domlist_read_lock);
+
+            for_each_domain ( d )
+                if ( d->domain_id >= op->domain )
+                {
+                    rcu_lock_domain(d);
+                    break;
+                }
+
+            rcu_read_unlock(&domlist_read_lock);
+            if ( d == NULL )
+                return -ESRCH;
+        }
+        break;
+
     default:
         d = rcu_lock_domain_by_id(op->domain);
         if ( d == NULL )
@@ -862,33 +883,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(&domlist_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;
+            break;
 
         getdomaininfo(d, &op->u.getdomaininfo);
-
         op->domain = op->u.getdomaininfo.domain;
         copyback = 1;
-
-    getdomaininfo_out:
-        rcu_read_unlock(&domlist_read_lock);
-        d = NULL;
         break;
-    }
 
     case XEN_DOMCTL_getvcpucontext:
     {
-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[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, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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 <marmarek@invisiblethingslab.com>
>> FWIW, I prefer this solution to the issue.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, 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 <marmarek@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  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(&domlist_read_lock);
> +
> +            for_each_domain ( d )
> +                if ( d->domain_id >= op->domain )
> +                {
> +                    rcu_lock_domain(d);
> +                    break;
> +                }
> +
> +            rcu_read_unlock(&domlist_read_lock);
> +            if ( d == NULL )
> +                return -ESRCH;
> +        }
> +        break;
> +
>      default:
>          d = rcu_lock_domain_by_id(op->domain);
>          if ( d == NULL )
> @@ -862,33 +883,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(&domlist_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;
> +            break;
>  
>          getdomaininfo(d, &op->u.getdomaininfo);
> -
>          op->domain = op->u.getdomaininfo.domain;
>          copyback = 1;
> -
> -    getdomaininfo_out:
> -        rcu_read_unlock(&domlist_read_lock);
> -        d = NULL;
>          break;
> -    }
>  
>      case XEN_DOMCTL_getvcpucontext:
>      {


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

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

[Jan Beulich]

>>> On 23.06.16 at 15:25, <marmarek@invisiblethingslab.com> wrote:
> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
>> >>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> 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, <marmarek@invisiblethingslab.com> 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, &domctl)) < 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(&domlist_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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[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 <marmarek@invisiblethingslab.com>
> ---
>  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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 5825 bytes --]

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@invisiblethingslab.com>
> > ---
> >  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@invisiblethingslab.com>
---
 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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[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 <marmarek@invisiblethingslab.com>
>>> ---
>>>  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 <marmarek@invisiblethingslab.com>

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 <dgdegra@tycho.nsa.gov>

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


-- 
Daniel De Graaf
National Security Agency

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

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

[Jan Beulich]

>>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1531 bytes --]

On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote:
> >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> 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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1767 bytes --]

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, <marmarek@invisiblethingslab.com> 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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 23.06.16 at 17:45, <marmarek@invisiblethingslab.com> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Jan Beulich]

>>> On 23.06.16 at 11:18, <marmarek@invisiblethingslab.com> wrote:
> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>> >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote:
>> > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>> >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
>> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote:
>> >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>> >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
>> >> ---
>> >> 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, &domctl)) < 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.

Please consider the case of max_doms == 1, which is the only one
of interest here (and the only one getting tweaked).

>> Also, how is this comment of yours related to the remark above?
> 
> Because this is why domid is needed in returned structure - to know about 
> which
> domain you've got the info.

I'm sorry, but no - this does not explain the duplication. Just look
at the code you quoted: It uses domctl.domain, but completely
ignores domctl.u.getdomaininfo.domain.

Jan

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

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

[Andrew Cooper]

On 23/06/16 09:32, Jan Beulich wrote:
>>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote:
>> On 06/22/2016 11:23 AM, Jan Beulich wrote:
>>>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote:
>>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote:
>>>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com>
> ---
> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Jan Beulich]

>>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> 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: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Andrew Cooper]

On 23/06/16 10:50, Jan Beulich wrote:
>>>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> 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