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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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?"

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