Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Daniel De Graaf]

On 06/22/2017 05:40 AM, George Dunlap wrote:

On 22/06/17 08:05, Jan Beulich wrote:

On 21.06.17 at 18:36,  wrote:

On 21/06/17 16:59, Jan Beulich wrote:

On 21.06.17 at 16:38,  wrote:

On 21/06/17 11:08, Jan Beulich wrote:

So far callers of the libxc interface passed in a domain ID which was
then ignored in the hypervisor. Instead, make the hypervisor honor it
(accepting DOMID_INVALID to obtain original behavior), allowing to
query whether a device is assigned to a particular domain. Ignore the
passed in domain ID at the libxc layer instead, in order to not break
existing callers. New libxc functions would need to be added if callers
wanted to leverage the new functionality.


I don't think your modified description matches the name of the call at all.

It looks like the callers expect "test_assign_device" to answer the
question: "Can I assign a device to this domain"?


I don't think so - the question being answered by the original
operation is "Is this device assigned to any domain?" with the
implied inverse "Is this device available to be assigned to some
domain (i.e. it is currently unassigned or owned by Dom0)?"


If the question were "Is this device assigned to any domain?", then I
would expect:
1. The return value to be a boolean
2. It would always return, "No it's not assigned" in the case where
there is no IOMMU.

However, that's not what happens:
1. It returns "success" if there is an IOMMU and the device is *not*
assigned, and returns an error if the device is assigned
2. It returns an error if there is no IOMMU.

The only place in the code this is called 'for real' in the tree is in
libxl_pci.c:libxl__device_pci_add()

 if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
 rc = xc_test_assign_device(ctx->xch, domid,
pcidev_encode_bdf(pcidev));
 if (rc) {
 LOGD(ERROR, domid,
  "PCI device %04x:%02x:%02x.%u %s?",
  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
  errno == ENOSYS ? "cannot be assigned - no IOMMU"
  : "already assigned to a different guest");
 goto out;
 }
 }

Here 'domid' is the domain to which libxl wants to assign the device.
So libxl is now asking Xen, "Am I allowed to assign device $bdf to
domain $domain?"

Your description provides the *algorithm* by which Xen normally provides
an answer: that is, normally the only thing Xen cares about is that it
hasn't already been assigned to a domain.  But it still remains the case
that what libxl is asking is, "Can I assign X to Y?"


Taking the log message into account that you quote, I do not
view the code's intention to be what you describe.


Well, I'm not sure what to say, because in my view the log message
supports my view. :-)  Note that there are two errors, both explaining
why the domain cannot be assigned -- one is "no IOMMU", one is "already
assigned to a different guest".

Yes, at the moment it doesn't have a separate message for -EPERM (which
is presumably what XSM would return if there were some other problem).
But it also doesn't correctly report other potential errors: -ENODEV if
you try to assign a DT device on a PCI-based system, or a PCI device on
a DT-based system.  (Apparently we also retirn -EINVAL if you included
inappropriate flags, *or* if the device didn't exist, *or* if the device
was already assigned somehwere else.  As long as we're re-painting
things we should probably change this as well.)

But to make test_assign_device answer the question, "Is this assigned to
a domU?", you'd have to have it return SUCCESS when there is no IOMMU
(since the device is not, in fact, assigned to a domU); and thus libxl
would have to make a separate call to find out if an IOMMU was present.


It looks like it's meant to be used in XSM environments, to allow a
policy to permit or forbid specific guests to have access to specific
devices.  On a default (non-XSM) system, the answer to that question
doesn't depend on the domain it's being assigned to, but only whether
the device is already assigned to another domain; but on XSM systems the
logic can presumably be more complicated.

That sounds like a perfectly sane semantic to me, and this patch removes
that ability.


And again I don't think so: Prior to the patch, do_domctl() at its
very top makes sure to entirely ignore the passed in domain ID.
This code sits ahead of the XSM check, so XSM has no way of
knowing which domain has been specified by the caller.


Right, I see that now.

Still, I assert that the original hypercall semantics is a very useful
one, and what you're doing is changing the hypercall such that the
question can no longer be asked.  It would be better to extend things so
that XSM can actually deny device assignment based on both the bdf and
the domain.

Do you have a particular use case in mind for your alternate hypercall?


No - I'm open to any change to it which 

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 11:58, Jan Beulich wrote:
>>> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
>>> policies that can forbid specific devices from being assigned to
>>> specific guests.
>>
>> Is there any possible downside to this ?
> 
> As soon as flask wouldn't ignore it anymore, there would be the risk
> of things currently working with a given XSM policy to stop working.
> Much depends on how consistent the test-assign and assign checks
> are (going to be) performed.
> 
>>> Any preferences?
>>
>> See above.  George's arguments make much more sense to me than Jan's,
>> in this thread.
> 
> Fine with me. Now as well as on past instances of looking at this,
> it simply didn't occur to me that the operation could be intended
> to work in the way George described. And in the end the patch
> will end up smaller with that alternative model. One last question
> then is whether retaining the original semantics with some special
> domain ID (DOMID_INVALID at present) is of any use.

I think test_assign_device should behave exactly as assign_device,
except that on success the device is not actually assigned.

The commit that introduced test_assign_device btw is 239ad70, "vt-d:
Test device assignability in xend, but defer actual assignment to
qemu-dm."  The obvious purpose was to be able to fail early if there was
going to be a failure.

In fact, there's probably an argument that the "test" functionality
should have been implemented by adding an extra flag, rather than an
extra hypercall.  Another approach would actually be to have the
test_assign_device and assign_device take exactly the same codepath,
except not finally do the assignment.  That would avoid any risk that
the two paths might get out of sync.

 -George

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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 22.06.17 at 11:58,  wrote:
> George Dunlap writes ("Re: [PATCH] passthrough: give 
> XEN_DOMCTL_test_assign_device more sane semantics"):
>> I suggest we ask the toolstack maintainers what kind of a function they
>> think would be most useful, and then we can implement that.
>> 
>> So, Wei and Ian (and Daniel if you're around):
> 
> After having reread the thread I still don't understand why Jan thinks
> the ignored argument is a problem per so.  Ignored arguments are often
> provided to ease future expansion (whether there is an ABI stability
> guarantee or not).
> 
> In this case I think that the domid is not passed to the XSM check is
> simply a bug.  I don't know if that can be fixed easily.

Well, the patch does that already, just that now the argument is
being ignored in flask. I'd leave that to someone else (Daniel?) to
implement.

>> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
>> policies that can forbid specific devices from being assigned to
>> specific guests.
> 
> Is there any possible downside to this ?

As soon as flask wouldn't ignore it anymore, there would be the risk
of things currently working with a given XSM policy to stop working.
Much depends on how consistent the test-assign and assign checks
are (going to be) performed.

>> Any preferences?
> 
> See above.  George's arguments make much more sense to me than Jan's,
> in this thread.

Fine with me. Now as well as on past instances of looking at this,
it simply didn't occur to me that the operation could be intended
to work in the way George described. And in the end the patch
will end up smaller with that alternative model. One last question
then is whether retaining the original semantics with some special
domain ID (DOMID_INVALID at present) is of any use.

Jan


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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 22.06.17 at 11:56,  wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
>> No - I'm open to any change to it which makes the currently ignored
>> argument no longer ignored, without breaking existing (known and
>> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
>> make it work the way you think it was originally meant to work; it is
>> just that given its current use I've come to a different conclusion as
>> to what the original intention may have been.
> 
> Actually, I think the clincher is this:
> 
> test_assign_device, assign_device, and deassign_device all use the same
> structure.
> 
> That makes it pretty obvious that "test_assign_device" was meant to ask
> the question, "If I call this hypercall with assign_device instead, will
> it succeed or fail?"

That's again one possible interpretation, yes, but that would again
collide with the domain left unused in the present implementation.
Anyway, let's see what others would prefer (and in the worst case
we can leave it in the strange state it's in now).

Jan


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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Ian Jackson]

George Dunlap writes ("Re: [PATCH] passthrough: give 
XEN_DOMCTL_test_assign_device more sane semantics"):
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.

The way I would put this is: The log message generation logic is
wrong, in that it sometimes lies.

>  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)

This is quite unhelpful, indeed.

> I suggest we ask the toolstack maintainers what kind of a function they
> think would be most useful, and then we can implement that.
> 
> So, Wei and Ian (and Daniel if you're around):

After having reread the thread I still don't understand why Jan thinks
the ignored argument is a problem per so.  Ignored arguments are often
provided to ease future expansion (whether there is an ABI stability
guarantee or not).

In this case I think that the domid is not passed to the XSM check is
simply a bug.  I don't know if that can be fixed easily.

> Option 2: Pass the domain to the XSM callback, enabling XSM / Flask
> policies that can forbid specific devices from being assigned to
> specific guests.

Is there any possible downside to this ?

> Any preferences?

See above.  George's arguments make much more sense to me than Jan's,
in this thread.

Ian.

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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 08:05, Jan Beulich wrote:
> No - I'm open to any change to it which makes the currently ignored
> argument no longer ignored, without breaking existing (known and
> unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
> make it work the way you think it was originally meant to work; it is
> just that given its current use I've come to a different conclusion as
> to what the original intention may have been.

Actually, I think the clincher is this:

test_assign_device, assign_device, and deassign_device all use the same
structure.

That makes it pretty obvious that "test_assign_device" was meant to ask
the question, "If I call this hypercall with assign_device instead, will
it succeed or fail?"

 -George

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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 10:40, George Dunlap wrote:
> On 22/06/17 08:05, Jan Beulich wrote:
> On 21.06.17 at 18:36,  wrote:
>>> On 21/06/17 16:59, Jan Beulich wrote:
>>> On 21.06.17 at 16:38,  wrote:
> On 21/06/17 11:08, Jan Beulich wrote:
>> So far callers of the libxc interface passed in a domain ID which was
>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>> query whether a device is assigned to a particular domain. Ignore the
>> passed in domain ID at the libxc layer instead, in order to not break
>> existing callers. New libxc functions would need to be added if callers
>> wanted to leverage the new functionality.
>
> I don't think your modified description matches the name of the call at 
> all.
>
> It looks like the callers expect "test_assign_device" to answer the
> question: "Can I assign a device to this domain"?

 I don't think so - the question being answered by the original
 operation is "Is this device assigned to any domain?" with the
 implied inverse "Is this device available to be assigned to some
 domain (i.e. it is currently unassigned or owned by Dom0)?"
>>>
>>> If the question were "Is this device assigned to any domain?", then I
>>> would expect:
>>> 1. The return value to be a boolean
>>> 2. It would always return, "No it's not assigned" in the case where
>>> there is no IOMMU.
>>>
>>> However, that's not what happens:
>>> 1. It returns "success" if there is an IOMMU and the device is *not*
>>> assigned, and returns an error if the device is assigned
>>> 2. It returns an error if there is no IOMMU.
>>>
>>> The only place in the code this is called 'for real' in the tree is in
>>> libxl_pci.c:libxl__device_pci_add()
>>>
>>> if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>>> rc = xc_test_assign_device(ctx->xch, domid,
>>> pcidev_encode_bdf(pcidev));
>>> if (rc) {
>>> LOGD(ERROR, domid,
>>>  "PCI device %04x:%02x:%02x.%u %s?",
>>>  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>>>  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>>>  : "already assigned to a different guest");
>>> goto out;
>>> }
>>> }
>>>
>>> Here 'domid' is the domain to which libxl wants to assign the device.
>>> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
>>> domain $domain?"
>>>
>>> Your description provides the *algorithm* by which Xen normally provides
>>> an answer: that is, normally the only thing Xen cares about is that it
>>> hasn't already been assigned to a domain.  But it still remains the case
>>> that what libxl is asking is, "Can I assign X to Y?"
>>
>> Taking the log message into account that you quote, I do not
>> view the code's intention to be what you describe.
> 
> Well, I'm not sure what to say, because in my view the log message
> supports my view. :-)  Note that there are two errors, both explaining
> why the domain cannot be assigned -- one is "no IOMMU", one is "already
> assigned to a different guest".
> 
> Yes, at the moment it doesn't have a separate message for -EPERM (which
> is presumably what XSM would return if there were some other problem).
> But it also doesn't correctly report other potential errors: -ENODEV if
> you try to assign a DT device on a PCI-based system, or a PCI device on
> a DT-based system.  (Apparently we also retirn -EINVAL if you included
> inappropriate flags, *or* if the device didn't exist, *or* if the device
> was already assigned somehwere else.  As long as we're re-painting
> things we should probably change this as well.)
> 
> But to make test_assign_device answer the question, "Is this assigned to
> a domU?", you'd have to have it return SUCCESS when there is no IOMMU
> (since the device is not, in fact, assigned to a domU); and thus libxl
> would have to make a separate call to find out if an IOMMU was present.
> 
> It looks like it's meant to be used in XSM environments, to allow a
> policy to permit or forbid specific guests to have access to specific
> devices.  On a default (non-XSM) system, the answer to that question
> doesn't depend on the domain it's being assigned to, but only whether
> the device is already assigned to another domain; but on XSM systems the
> logic can presumably be more complicated.
>
> That sounds like a perfectly sane semantic to me, and this patch removes
> that ability.

 And again I don't think so: Prior to the patch, do_domctl() at its
 very top makes sure to entirely ignore the passed in domain ID.
 This code sits ahead of the XSM check, so XSM has no way of
 knowing which domain has been specified by the caller.
>>>
>>> Right, I see that now.
>>>
>>> Still, I assert that the 

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 22/06/17 08:05, Jan Beulich wrote:
 On 21.06.17 at 18:36,  wrote:
>> On 21/06/17 16:59, Jan Beulich wrote:
>> On 21.06.17 at 16:38,  wrote:
 On 21/06/17 11:08, Jan Beulich wrote:
> So far callers of the libxc interface passed in a domain ID which was
> then ignored in the hypervisor. Instead, make the hypervisor honor it
> (accepting DOMID_INVALID to obtain original behavior), allowing to
> query whether a device is assigned to a particular domain. Ignore the
> passed in domain ID at the libxc layer instead, in order to not break
> existing callers. New libxc functions would need to be added if callers
> wanted to leverage the new functionality.

 I don't think your modified description matches the name of the call at 
 all.

 It looks like the callers expect "test_assign_device" to answer the
 question: "Can I assign a device to this domain"?
>>>
>>> I don't think so - the question being answered by the original
>>> operation is "Is this device assigned to any domain?" with the
>>> implied inverse "Is this device available to be assigned to some
>>> domain (i.e. it is currently unassigned or owned by Dom0)?"
>>
>> If the question were "Is this device assigned to any domain?", then I
>> would expect:
>> 1. The return value to be a boolean
>> 2. It would always return, "No it's not assigned" in the case where
>> there is no IOMMU.
>>
>> However, that's not what happens:
>> 1. It returns "success" if there is an IOMMU and the device is *not*
>> assigned, and returns an error if the device is assigned
>> 2. It returns an error if there is no IOMMU.
>>
>> The only place in the code this is called 'for real' in the tree is in
>> libxl_pci.c:libxl__device_pci_add()
>>
>> if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
>> rc = xc_test_assign_device(ctx->xch, domid,
>> pcidev_encode_bdf(pcidev));
>> if (rc) {
>> LOGD(ERROR, domid,
>>  "PCI device %04x:%02x:%02x.%u %s?",
>>  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>>  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>>  : "already assigned to a different guest");
>> goto out;
>> }
>> }
>>
>> Here 'domid' is the domain to which libxl wants to assign the device.
>> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
>> domain $domain?"
>>
>> Your description provides the *algorithm* by which Xen normally provides
>> an answer: that is, normally the only thing Xen cares about is that it
>> hasn't already been assigned to a domain.  But it still remains the case
>> that what libxl is asking is, "Can I assign X to Y?"
> 
> Taking the log message into account that you quote, I do not
> view the code's intention to be what you describe.

Well, I'm not sure what to say, because in my view the log message
supports my view. :-)  Note that there are two errors, both explaining
why the domain cannot be assigned -- one is "no IOMMU", one is "already
assigned to a different guest".

Yes, at the moment it doesn't have a separate message for -EPERM (which
is presumably what XSM would return if there were some other problem).
But it also doesn't correctly report other potential errors: -ENODEV if
you try to assign a DT device on a PCI-based system, or a PCI device on
a DT-based system.  (Apparently we also retirn -EINVAL if you included
inappropriate flags, *or* if the device didn't exist, *or* if the device
was already assigned somehwere else.  As long as we're re-painting
things we should probably change this as well.)

But to make test_assign_device answer the question, "Is this assigned to
a domU?", you'd have to have it return SUCCESS when there is no IOMMU
(since the device is not, in fact, assigned to a domU); and thus libxl
would have to make a separate call to find out if an IOMMU was present.

 It looks like it's meant to be used in XSM environments, to allow a
 policy to permit or forbid specific guests to have access to specific
 devices.  On a default (non-XSM) system, the answer to that question
 doesn't depend on the domain it's being assigned to, but only whether
 the device is already assigned to another domain; but on XSM systems the
 logic can presumably be more complicated.

 That sounds like a perfectly sane semantic to me, and this patch removes
 that ability.
>>>
>>> And again I don't think so: Prior to the patch, do_domctl() at its
>>> very top makes sure to entirely ignore the passed in domain ID.
>>> This code sits ahead of the XSM check, so XSM has no way of
>>> knowing which domain has been specified by the caller.
>>
>> Right, I see that now.
>>
>> Still, I assert that the original hypercall semantics is a very useful
>> one, and what you're doing is changing the hypercall such that the
>> question can no longer be asked.  It would be 

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 21.06.17 at 18:36,  wrote:
> On 21/06/17 16:59, Jan Beulich wrote:
> On 21.06.17 at 16:38,  wrote:
>>> On 21/06/17 11:08, Jan Beulich wrote:
 So far callers of the libxc interface passed in a domain ID which was
 then ignored in the hypervisor. Instead, make the hypervisor honor it
 (accepting DOMID_INVALID to obtain original behavior), allowing to
 query whether a device is assigned to a particular domain. Ignore the
 passed in domain ID at the libxc layer instead, in order to not break
 existing callers. New libxc functions would need to be added if callers
 wanted to leverage the new functionality.
>>>
>>> I don't think your modified description matches the name of the call at all.
>>>
>>> It looks like the callers expect "test_assign_device" to answer the
>>> question: "Can I assign a device to this domain"?
>> 
>> I don't think so - the question being answered by the original
>> operation is "Is this device assigned to any domain?" with the
>> implied inverse "Is this device available to be assigned to some
>> domain (i.e. it is currently unassigned or owned by Dom0)?"
> 
> If the question were "Is this device assigned to any domain?", then I
> would expect:
> 1. The return value to be a boolean
> 2. It would always return, "No it's not assigned" in the case where
> there is no IOMMU.
> 
> However, that's not what happens:
> 1. It returns "success" if there is an IOMMU and the device is *not*
> assigned, and returns an error if the device is assigned
> 2. It returns an error if there is no IOMMU.
> 
> The only place in the code this is called 'for real' in the tree is in
> libxl_pci.c:libxl__device_pci_add()
> 
> if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
> rc = xc_test_assign_device(ctx->xch, domid,
> pcidev_encode_bdf(pcidev));
> if (rc) {
> LOGD(ERROR, domid,
>  "PCI device %04x:%02x:%02x.%u %s?",
>  pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
>  errno == ENOSYS ? "cannot be assigned - no IOMMU"
>  : "already assigned to a different guest");
> goto out;
> }
> }
> 
> Here 'domid' is the domain to which libxl wants to assign the device.
> So libxl is now asking Xen, "Am I allowed to assign device $bdf to
> domain $domain?"
> 
> Your description provides the *algorithm* by which Xen normally provides
> an answer: that is, normally the only thing Xen cares about is that it
> hasn't already been assigned to a domain.  But it still remains the case
> that what libxl is asking is, "Can I assign X to Y?"

Taking the log message into account that you quote, I do not
view the code's intention to be what you describe.

>>> It looks like it's meant to be used in XSM environments, to allow a
>>> policy to permit or forbid specific guests to have access to specific
>>> devices.  On a default (non-XSM) system, the answer to that question
>>> doesn't depend on the domain it's being assigned to, but only whether
>>> the device is already assigned to another domain; but on XSM systems the
>>> logic can presumably be more complicated.
>>>
>>> That sounds like a perfectly sane semantic to me, and this patch removes
>>> that ability.
>> 
>> And again I don't think so: Prior to the patch, do_domctl() at its
>> very top makes sure to entirely ignore the passed in domain ID.
>> This code sits ahead of the XSM check, so XSM has no way of
>> knowing which domain has been specified by the caller.
> 
> Right, I see that now.
> 
> Still, I assert that the original hypercall semantics is a very useful
> one, and what you're doing is changing the hypercall such that the
> question can no longer be asked.  It would be better to extend things so
> that XSM can actually deny device assignment based on both the bdf and
> the domain.
> 
> Do you have a particular use case in mind for your alternate hypercall?

No - I'm open to any change to it which makes the currently ignored
argument no longer ignored, without breaking existing (known and
unknown) callers of the libxc wrapper. I.e. I'm in no way opposed to
make it work the way you think it was originally meant to work; it is
just that given its current use I've come to a different conclusion as
to what the original intention may have been.

Jan

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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 21/06/17 16:59, Jan Beulich wrote:
 On 21.06.17 at 16:38,  wrote:
>> On 21/06/17 11:08, Jan Beulich wrote:
>>> So far callers of the libxc interface passed in a domain ID which was
>>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>>> query whether a device is assigned to a particular domain. Ignore the
>>> passed in domain ID at the libxc layer instead, in order to not break
>>> existing callers. New libxc functions would need to be added if callers
>>> wanted to leverage the new functionality.
>>
>> I don't think your modified description matches the name of the call at all.
>>
>> It looks like the callers expect "test_assign_device" to answer the
>> question: "Can I assign a device to this domain"?
> 
> I don't think so - the question being answered by the original
> operation is "Is this device assigned to any domain?" with the
> implied inverse "Is this device available to be assigned to some
> domain (i.e. it is currently unassigned or owned by Dom0)?"

If the question were "Is this device assigned to any domain?", then I
would expect:
1. The return value to be a boolean
2. It would always return, "No it's not assigned" in the case where
there is no IOMMU.

However, that's not what happens:
1. It returns "success" if there is an IOMMU and the device is *not*
assigned, and returns an error if the device is assigned
2. It returns an error if there is no IOMMU.

The only place in the code this is called 'for real' in the tree is in
libxl_pci.c:libxl__device_pci_add()

if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) {
rc = xc_test_assign_device(ctx->xch, domid,
pcidev_encode_bdf(pcidev));
if (rc) {
LOGD(ERROR, domid,
 "PCI device %04x:%02x:%02x.%u %s?",
 pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func,
 errno == ENOSYS ? "cannot be assigned - no IOMMU"
 : "already assigned to a different guest");
goto out;
}
}

Here 'domid' is the domain to which libxl wants to assign the device.
So libxl is now asking Xen, "Am I allowed to assign device $bdf to
domain $domain?"

Your description provides the *algorithm* by which Xen normally provides
an answer: that is, normally the only thing Xen cares about is that it
hasn't already been assigned to a domain.  But it still remains the case
that what libxl is asking is, "Can I assign X to Y?"

>> It looks like it's meant to be used in XSM environments, to allow a
>> policy to permit or forbid specific guests to have access to specific
>> devices.  On a default (non-XSM) system, the answer to that question
>> doesn't depend on the domain it's being assigned to, but only whether
>> the device is already assigned to another domain; but on XSM systems the
>> logic can presumably be more complicated.
>>
>> That sounds like a perfectly sane semantic to me, and this patch removes
>> that ability.
> 
> And again I don't think so: Prior to the patch, do_domctl() at its
> very top makes sure to entirely ignore the passed in domain ID.
> This code sits ahead of the XSM check, so XSM has no way of
> knowing which domain has been specified by the caller.

Right, I see that now.

Still, I assert that the original hypercall semantics is a very useful
one, and what you're doing is changing the hypercall such that the
question can no longer be asked.  It would be better to extend things so
that XSM can actually deny device assignment based on both the bdf and
the domain.

Do you have a particular use case in mind for your alternate hypercall?

 -George


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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

>>> On 21.06.17 at 16:38,  wrote:
> On 21/06/17 11:08, Jan Beulich wrote:
>> So far callers of the libxc interface passed in a domain ID which was
>> then ignored in the hypervisor. Instead, make the hypervisor honor it
>> (accepting DOMID_INVALID to obtain original behavior), allowing to
>> query whether a device is assigned to a particular domain. Ignore the
>> passed in domain ID at the libxc layer instead, in order to not break
>> existing callers. New libxc functions would need to be added if callers
>> wanted to leverage the new functionality.
> 
> I don't think your modified description matches the name of the call at all.
> 
> It looks like the callers expect "test_assign_device" to answer the
> question: "Can I assign a device to this domain"?

I don't think so - the question being answered by the original
operation is "Is this device assigned to any domain?" with the
implied inverse "Is this device available to be assigned to some
domain (i.e. it is currently unassigned or owned by Dom0)?"

> It looks like it's meant to be used in XSM environments, to allow a
> policy to permit or forbid specific guests to have access to specific
> devices.  On a default (non-XSM) system, the answer to that question
> doesn't depend on the domain it's being assigned to, but only whether
> the device is already assigned to another domain; but on XSM systems the
> logic can presumably be more complicated.
> 
> That sounds like a perfectly sane semantic to me, and this patch removes
> that ability.

And again I don't think so: Prior to the patch, do_domctl() at its
very top makes sure to entirely ignore the passed in domain ID.
This code sits ahead of the XSM check, so XSM has no way of
knowing which domain has been specified by the caller.

Jan


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

Re: [Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[George Dunlap]

On 21/06/17 11:08, Jan Beulich wrote:
> So far callers of the libxc interface passed in a domain ID which was
> then ignored in the hypervisor. Instead, make the hypervisor honor it
> (accepting DOMID_INVALID to obtain original behavior), allowing to
> query whether a device is assigned to a particular domain. Ignore the
> passed in domain ID at the libxc layer instead, in order to not break
> existing callers. New libxc functions would need to be added if callers
> wanted to leverage the new functionality.

I don't think your modified description matches the name of the call at all.

It looks like the callers expect "test_assign_device" to answer the
question: "Can I assign a device to this domain"?

It looks like it's meant to be used in XSM environments, to allow a
policy to permit or forbid specific guests to have access to specific
devices.  On a default (non-XSM) system, the answer to that question
doesn't depend on the domain it's being assigned to, but only whether
the device is already assigned to another domain; but on XSM systems the
logic can presumably be more complicated.

That sounds like a perfectly sane semantic to me, and this patch removes
that ability.

I think if you want a hypercall that answers the question, "Is device X
assigned to domain Y", I think you should add a new one with a name more
like, "test_device_is_assigned_to" or something.

 -George


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

[Xen-devel] [PATCH] passthrough: give XEN_DOMCTL_test_assign_device more sane semantics

[Jan Beulich]

So far callers of the libxc interface passed in a domain ID which was
then ignored in the hypervisor. Instead, make the hypervisor honor it
(accepting DOMID_INVALID to obtain original behavior), allowing to
query whether a device is assigned to a particular domain. Ignore the
passed in domain ID at the libxc layer instead, in order to not break
existing callers. New libxc functions would need to be added if callers
wanted to leverage the new functionality.

Signed-off-by: Jan Beulich 
---
TBD: Would DOMID_IO be a better fit than DOMID_INVALID here?

--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1539,13 +1539,13 @@ int xc_get_device_group(
 
 int xc_test_assign_device(
 xc_interface *xch,
-uint32_t domid,
+uint32_t domid, /* ignored */
 uint32_t machine_sbdf)
 {
 DECLARE_DOMCTL;
 
 domctl.cmd = XEN_DOMCTL_test_assign_device;
-domctl.domain = domid;
+domctl.domain = DOMID_INVALID;
 domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
 domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
 domctl.u.assign_device.flags = 0;
@@ -1603,7 +1603,7 @@ int xc_assign_dt_device(
 
 int xc_test_assign_dt_device(
 xc_interface *xch,
-uint32_t domid,
+uint32_t domid, /* ignored */
 char *path)
 {
 int rc;
@@ -1615,7 +1615,7 @@ int xc_test_assign_dt_device(
 return -1;
 
 domctl.cmd = XEN_DOMCTL_test_assign_device;
-domctl.domain = (domid_t)domid;
+domctl.domain = DOMID_INVALID;
 
 domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
 domctl.u.assign_device.u.dt.size = size;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -391,11 +391,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
 
 switch ( op->cmd )
 {
-case XEN_DOMCTL_createdomain:
 case XEN_DOMCTL_test_assign_device:
+if ( op->domain == DOMID_INVALID )
+{
+case XEN_DOMCTL_createdomain:
 case XEN_DOMCTL_gdbsx_guestmemio:
-d = NULL;
-break;
+d = NULL;
+break;
+}
+/* fall through */
 default:
 d = rcu_lock_domain_by_id(op->domain);
 if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo )
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -93,7 +93,8 @@ fail:
 return rc;
 }
 
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+static bool_t iommu_dt_device_is_assigned(const struct domain *d,
+  const struct dt_device_node *dev)
 {
 bool_t assigned = 0;
 
@@ -101,7 +102,8 @@ static bool_t iommu_dt_device_is_assigne
 return 0;
 
 spin_lock(_lock);
-assigned = !list_empty(>domain_list);
+assigned = d ? dt_device_used_by(dev) == d->domain_id
+ : !list_empty(>domain_list);
 spin_unlock(_lock);
 
 return assigned;
@@ -209,11 +211,11 @@ int iommu_do_dt_domctl(struct xen_domctl
 if ( ret )
 break;
 
-ret = xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
+ret = xsm_test_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
 if ( ret )
 break;
 
-if ( iommu_dt_device_is_assigned(dev) )
+if ( iommu_dt_device_is_assigned(d, dev) )
 {
 printk(XENLOG_G_ERR "%s already assigned.\n",
dt_node_full_name(dev));
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -522,7 +522,7 @@ struct pci_dev *pci_get_real_pdev(int se
 }
 
 struct pci_dev *pci_get_pdev_by_domain(
-struct domain *d, int seg, int bus, int devfn)
+const struct domain *d, int seg, int bus, int devfn)
 {
 struct pci_seg *pseg = get_pseg(seg);
 struct pci_dev *pdev = NULL;
@@ -1337,12 +1337,12 @@ int iommu_remove_device(struct pci_dev *
  * If the device isn't owned by the hardware domain, it means it already
  * has been assigned to other domain, or it doesn't exist.
  */
-static int device_assigned(u16 seg, u8 bus, u8 devfn)
+static int device_assigned(const struct domain *d, u16 seg, u8 bus, u8 devfn)
 {
-struct pci_dev *pdev;
+const struct pci_dev *pdev;
 
 pcidevs_lock();
-pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+pdev = pci_get_pdev_by_domain(d ?: hardware_domain, seg, bus, devfn);
 pcidevs_unlock();
 
 return pdev ? 0 : -EBUSY;
@@ -1590,7 +1590,7 @@ int iommu_do_pci_domctl(
 
 machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
 
-ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
+ret = xsm_test_assign_device(XSM_HOOK, d, machine_sbdf);
 if ( ret )
 break;
 
@@ -1598,13 +1598,12 @@ int iommu_do_pci_domctl(
 bus = PCI_BUS(machine_sbdf);
 devfn = PCI_DEVFN2(machine_sbdf);
 
-if ( device_assigned(seg, bus, devfn) )
-{
+ret = device_assigned(d, seg, bus, devfn);
+if ( ret && !d )
 printk(XENLOG_G_INFO