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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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

Sorry, didn't finish this:  -ENODEV if it's the wrong bus type (DT or
PCI), -EINVAL if there are any other problems assigning the device
(device doesn't exist, or device already assigned to another domain),
and 0 if everything is OK.

 -George


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