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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>>> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel