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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/06/17 08:05, Jan Beulich wrote:
>>>> On 21.06.17 at 18:36, <george.dunlap@xxxxxxxxxx> wrote:
>> On 21/06/17 16:59, Jan Beulich wrote:
>>>>>> On 21.06.17 at 16:38, <george.dunlap@xxxxxxxxxx> 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 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.

So the libxc library interface is not meant to be stable.  Before I
looked at how libxl was using it, I was going to say that we should just
remove the domid argument from that function entirely, rather than
labelling it "unused".

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

At the moment, xc_test_assign_device() accepts both a domid and a device
identifier.  It will return -ENOSYS if there is no IOMMU, and

The domid, however, is ignored by Xen, and there is no possibility even
of XSM/Flask paying any attention to it because the domid is not passed
to the XSM hook.

Jan thinks the color of this shed is ugly and wants to repaint it.  I
agree that the color of this shed is ugly, but think a different color
would be more suitable.  Since the function is ultimately mainly for the
benefit of toolstacks like libxl, we'd like your input:

Option 1: Make xc_test_assign_device() explicitly take only a device
identifier.  Add another xc function which takes a domain argument,
which will return true if the BDF is assigned to that particular domain,
and false otherwise.
  1a: Leave the domid argument, but add a comment specifying it as ignored
  1b: Remove the domid argument.

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

NB that neither of us has a particular requirement for the proposed
additional functionality ("Device X is assigned to domid Y" in the case
of Option 1, or "Flask policy can allow or forbid specific devices to
specific domains" in the case of Option 2).

Any preferences?

 -George

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