Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi,
On 11/03/2015 13:50, Julien Grall wrote:
On 11/03/2015 12:37, Ian Campbell wrote:
On Tue, 2015-03-10 at 16:33 +, Julien Grall wrote:
Hi Ian,
On 20/02/15 17:17, Ian Campbell wrote:
+/* TODO: Do we need to check is_dying? Mostly to protect against
+ * hypercall trying to passthrough a device while we are
+ * dying.
FWIW the PCI case appears not to care...
There is one place in XEN_DOMCTL_assign_device...
Although I don't understand much the usage of is_dying.
+ */
+
+switch ( domctl->cmd )
+{
+case XEN_DOMCTL_assign_device:
+ret = -ENOSYS;
+if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+break;
You added something similar to iommu_do_pci_domctl, would it not be
preferable for the caller to switch on domctl->u.assign_device.dev and
call the correct iommu_do_*_domctl?
I though about it. It would require to stub iommu_do_*_domctl. So I
preferred to chose the current solution.
You mean iommu_do_{pci,dt}_domctl?
IIRC you already added suitable #defines for when these features are
present, so reusing them at the call sites wouldn't be too bad, or else
stubs aren't the worst thing either.
Hmmm I think I got you point now. Do you mean have something like:
switch (domctl->u.assign_device.dev)
{
#ifdef HAS_PCI
case XEN_DOMCTL_DEV_PCI:
ret = iommu_do_pci_domctl();
break;
#endif
#ifdef HAS_DEVICE_TREE
case XEN_DOMCTL_DEV_DT:
ret = iommu_do_dt_domctl()
break;
#endif
default:
ret = -ENOSYS;
}
I though more about it and it won't be possible to do it unless we
decode DOMCTL too.
This is because the DOMCTL_get_device_group use a different structure
and is only for PCI.
So unless we decide to consolidate the 2 DOMCTL structures in a single
one. I would prefer to keep the current solution with the suggestion
made by Jan earlier.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Jan,
On 11/03/2015 14:03, Jan Beulich wrote:
On 11.03.15 at 14:55, wrote:
On Wed, 2015-03-11 at 13:50 +, Julien Grall wrote:
Hmmm I think I got you point now. Do you mean have something like:
That's one option I'd be happy with, yes. Jan might disagree.
Looks quite reasonable to except for ...
switch (domctl->u.assign_device.dev)
{
#ifdef HAS_PCI
case XEN_DOMCTL_DEV_PCI:
ret = iommu_do_pci_domctl();
break;
#endif
#ifdef HAS_DEVICE_TREE
case XEN_DOMCTL_DEV_DT:
ret = iommu_do_dt_domctl()
break;
#endif
default:
ret = -ENOSYS;
... the -ENOSYS here: We commonly use e.g. -EOPNOTSUPP in
such situations.
Ok I will use it.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 11.03.15 at 14:55, wrote:
> On Wed, 2015-03-11 at 13:50 +, Julien Grall wrote:
>> Hi Ian,
>>
>> On 11/03/2015 12:37, Ian Campbell wrote:
>> > On Tue, 2015-03-10 at 16:33 +, Julien Grall wrote:
>> >> Hi Ian,
>> >>
>> >> On 20/02/15 17:17, Ian Campbell wrote:
>> +/* TODO: Do we need to check is_dying? Mostly to protect against
>> + * hypercall trying to passthrough a device while we are
>> + * dying.
>> >>>
>> >>> FWIW the PCI case appears not to care...
>> >>
>> >> There is one place in XEN_DOMCTL_assign_device...
>> >>
>> >> Although I don't understand much the usage of is_dying.
>> >>
>> + */
>> +
>> +switch ( domctl->cmd )
>> +{
>> +case XEN_DOMCTL_assign_device:
>> +ret = -ENOSYS;
>> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>> +break;
>> >>>
>> >>> You added something similar to iommu_do_pci_domctl, would it not be
>> >>> preferable for the caller to switch on domctl->u.assign_device.dev and
>> >>> call the correct iommu_do_*_domctl?
>> >>
>> >> I though about it. It would require to stub iommu_do_*_domctl. So I
>> >> preferred to chose the current solution.
>> >
>> > You mean iommu_do_{pci,dt}_domctl?
>> >
>> > IIRC you already added suitable #defines for when these features are
>> > present, so reusing them at the call sites wouldn't be too bad, or else
>> > stubs aren't the worst thing either.
>>
>> Hmmm I think I got you point now. Do you mean have something like:
>
> That's one option I'd be happy with, yes. Jan might disagree.
Looks quite reasonable to except for ...
>> switch (domctl->u.assign_device.dev)
>> {
>> #ifdef HAS_PCI
>> case XEN_DOMCTL_DEV_PCI:
>> ret = iommu_do_pci_domctl();
>> break;
>> #endif
>> #ifdef HAS_DEVICE_TREE
>> case XEN_DOMCTL_DEV_DT:
>> ret = iommu_do_dt_domctl()
>> break;
>> #endif
>>
>> default:
>> ret = -ENOSYS;
... the -ENOSYS here: We commonly use e.g. -EOPNOTSUPP in
such situations.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Ian,
On 20/02/15 17:17, Ian Campbell wrote:
>> +/* TODO: Do we need to check is_dying? Mostly to protect against
>> + * hypercall trying to passthrough a device while we are
>> + * dying.
>
> FWIW the PCI case appears not to care...
There is one place in XEN_DOMCTL_assign_device...
Although I don't understand much the usage of is_dying.
>> + */
>> +
>> +switch ( domctl->cmd )
>> +{
>> +case XEN_DOMCTL_assign_device:
>> +ret = -ENOSYS;
>> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
>> +break;
>
> You added something similar to iommu_do_pci_domctl, would it not be
> preferable for the caller to switch on domctl->u.assign_device.dev and
> call the correct iommu_do_*_domctl?
I though about it. It would require to stub iommu_do_*_domctl. So I
preferred to chose the current solution.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Wed, 2015-03-11 at 13:50 +, Julien Grall wrote:
> Hi Ian,
>
> On 11/03/2015 12:37, Ian Campbell wrote:
> > On Tue, 2015-03-10 at 16:33 +, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 20/02/15 17:17, Ian Campbell wrote:
> +/* TODO: Do we need to check is_dying? Mostly to protect against
> + * hypercall trying to passthrough a device while we are
> + * dying.
> >>>
> >>> FWIW the PCI case appears not to care...
> >>
> >> There is one place in XEN_DOMCTL_assign_device...
> >>
> >> Although I don't understand much the usage of is_dying.
> >>
> + */
> +
> +switch ( domctl->cmd )
> +{
> +case XEN_DOMCTL_assign_device:
> +ret = -ENOSYS;
> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +break;
> >>>
> >>> You added something similar to iommu_do_pci_domctl, would it not be
> >>> preferable for the caller to switch on domctl->u.assign_device.dev and
> >>> call the correct iommu_do_*_domctl?
> >>
> >> I though about it. It would require to stub iommu_do_*_domctl. So I
> >> preferred to chose the current solution.
> >
> > You mean iommu_do_{pci,dt}_domctl?
> >
> > IIRC you already added suitable #defines for when these features are
> > present, so reusing them at the call sites wouldn't be too bad, or else
> > stubs aren't the worst thing either.
>
> Hmmm I think I got you point now. Do you mean have something like:
That's one option I'd be happy with, yes. Jan might disagree.
>
> switch (domctl->u.assign_device.dev)
> {
> #ifdef HAS_PCI
> case XEN_DOMCTL_DEV_PCI:
> ret = iommu_do_pci_domctl();
> break;
> #endif
> #ifdef HAS_DEVICE_TREE
> case XEN_DOMCTL_DEV_DT:
> ret = iommu_do_dt_domctl()
> break;
> #endif
>
> default:
> ret = -ENOSYS;
> }
>
> Regards,
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Ian,
On 11/03/2015 12:37, Ian Campbell wrote:
On Tue, 2015-03-10 at 16:33 +, Julien Grall wrote:
Hi Ian,
On 20/02/15 17:17, Ian Campbell wrote:
+/* TODO: Do we need to check is_dying? Mostly to protect against
+ * hypercall trying to passthrough a device while we are
+ * dying.
FWIW the PCI case appears not to care...
There is one place in XEN_DOMCTL_assign_device...
Although I don't understand much the usage of is_dying.
+ */
+
+switch ( domctl->cmd )
+{
+case XEN_DOMCTL_assign_device:
+ret = -ENOSYS;
+if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+break;
You added something similar to iommu_do_pci_domctl, would it not be
preferable for the caller to switch on domctl->u.assign_device.dev and
call the correct iommu_do_*_domctl?
I though about it. It would require to stub iommu_do_*_domctl. So I
preferred to chose the current solution.
You mean iommu_do_{pci,dt}_domctl?
IIRC you already added suitable #defines for when these features are
present, so reusing them at the call sites wouldn't be too bad, or else
stubs aren't the worst thing either.
Hmmm I think I got you point now. Do you mean have something like:
switch (domctl->u.assign_device.dev)
{
#ifdef HAS_PCI
case XEN_DOMCTL_DEV_PCI:
ret = iommu_do_pci_domctl();
break;
#endif
#ifdef HAS_DEVICE_TREE
case XEN_DOMCTL_DEV_DT:
ret = iommu_do_dt_domctl()
break;
#endif
default:
ret = -ENOSYS;
}
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 11.03.15 at 13:15, wrote: > Hi Jan, > > On 11/03/2015 08:53, Jan Beulich wrote: > On 10.03.15 at 23:45, wrote: >>> In order to do static labeling for device passthrough, the nodes in a >>> device tree need a 32-bit numeric identifier. IO memory uses the MFN, >>> PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. >> >> A 32-bit number can't uniquely identify an MMIO MFN. > > Isn't 32-bit enough to describe an MFN? No, that only covers up to 1Tb. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Tue, 2015-03-10 at 16:33 +, Julien Grall wrote:
> Hi Ian,
>
> On 20/02/15 17:17, Ian Campbell wrote:
> >> +/* TODO: Do we need to check is_dying? Mostly to protect against
> >> + * hypercall trying to passthrough a device while we are
> >> + * dying.
> >
> > FWIW the PCI case appears not to care...
>
> There is one place in XEN_DOMCTL_assign_device...
>
> Although I don't understand much the usage of is_dying.
>
> >> + */
> >> +
> >> +switch ( domctl->cmd )
> >> +{
> >> +case XEN_DOMCTL_assign_device:
> >> +ret = -ENOSYS;
> >> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> >> +break;
> >
> > You added something similar to iommu_do_pci_domctl, would it not be
> > preferable for the caller to switch on domctl->u.assign_device.dev and
> > call the correct iommu_do_*_domctl?
>
> I though about it. It would require to stub iommu_do_*_domctl. So I
> preferred to chose the current solution.
You mean iommu_do_{pci,dt}_domctl?
IIRC you already added suitable #defines for when these features are
present, so reusing them at the call sites wouldn't be too bad, or else
stubs aren't the worst thing either.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 11/03/2015 12:23, Ian Campbell wrote: On Wed, 2015-03-11 at 12:15 +, Julien Grall wrote: Hi Jan, On 11/03/2015 08:53, Jan Beulich wrote: On 10.03.15 at 23:45, wrote: In order to do static labeling for device passthrough, the nodes in a device tree need a 32-bit numeric identifier. IO memory uses the MFN, PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. A 32-bit number can't uniquely identify an MMIO MFN. Isn't 32-bit enough to describe an MFN? Up to 32+12== 44 bits, yes, but we know there are ARM systems with 48 bit PA out there. Hmmm... checkpolicy and flask will have to be fixed then. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Daniel, On 10/03/2015 23:39, Daniel De Graaf wrote: On 03/10/2015 07:07 PM, Julien Grall wrote: Hi Daniel, On 10/03/2015 22:45, Daniel De Graaf wrote: BTW, do you have any pointer on how to write a policy for device/IRQ passthrough? There is a bit of documentation in xsm-flask.txt about device labeling, which is the hard part of making passthrough work. Labels can be set either statically in the security policy (as documented in the section "Device Labeling") or dynamically using a tool like flask-label-pci as documented in "Resource Policy". Once that is done, then rules to allow the passthrough operation can be added, similar to the example resource nic_dev_t in xen.te. I tried to follow xsm-flask.txt and uncomment one of the pirqcon line in the xsm policy. But I got the following error: policy/modules/xen/xen.te:199:ERROR 'syntax error' at token 'pirqcon' on line 1986: pirqcon 33 system_u:object_r:nic_dev_t Did I miss anything? No, this is an error in either the policy or the parser in checkpolicy. The parser in checkpolicy is rather inflexible, and it currently requires that the device labels be specified at the end of the policy.conf instead of in the middle (as they are now). You should add the commented out lines to the end of tools/flask/policy/policy/initial_sids for now; I will be sending a patch to move them to another file tomorrow. It's working now. Thanks! In order to do static labeling for device passthrough, the nodes in a device tree need a 32-bit numeric identifier. IO memory uses the MFN, PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. Why it's restricted to an integer? Would it be possible to use a string as it's done for the sid? The sid is not actually represented as a string internally: it is mapped into a set of integers for the user/role/type and MLS range. I gave a look to the checkpolicy code and the ocontext can store pretty much anything. Currently for IOMEM, 2 32-bit number is stored to describe the range. AFAIU, in order to support DT device, I will have to update checkpolicy for adding a new token (smth like dtdevicecon). So I don't see why I couldn't use a string for purpose. Or maybe you had in mind to reuse pcidevicecon? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Wed, 2015-03-11 at 12:15 +, Julien Grall wrote: > Hi Jan, > > On 11/03/2015 08:53, Jan Beulich wrote: > On 10.03.15 at 23:45, wrote: > >> In order to do static labeling for device passthrough, the nodes in a > >> device tree need a 32-bit numeric identifier. IO memory uses the MFN, > >> PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. > > > > A 32-bit number can't uniquely identify an MMIO MFN. > > Isn't 32-bit enough to describe an MFN? Up to 32+12== 44 bits, yes, but we know there are ARM systems with 48 bit PA out there. > > Although, MMIO are described with an MFN range. > > Regards, > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Jan, On 11/03/2015 08:53, Jan Beulich wrote: On 10.03.15 at 23:45, wrote: In order to do static labeling for device passthrough, the nodes in a device tree need a 32-bit numeric identifier. IO memory uses the MFN, PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. A 32-bit number can't uniquely identify an MMIO MFN. Isn't 32-bit enough to describe an MFN? Although, MMIO are described with an MFN range. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 10.03.15 at 23:45, wrote: > In order to do static labeling for device passthrough, the nodes in a > device tree need a 32-bit numeric identifier. IO memory uses the MFN, > PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. A 32-bit number can't uniquely identify an MMIO MFN. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 03/10/2015 07:07 PM, Julien Grall wrote: Hi Daniel, On 10/03/2015 22:45, Daniel De Graaf wrote: BTW, do you have any pointer on how to write a policy for device/IRQ passthrough? There is a bit of documentation in xsm-flask.txt about device labeling, which is the hard part of making passthrough work. Labels can be set either statically in the security policy (as documented in the section "Device Labeling") or dynamically using a tool like flask-label-pci as documented in "Resource Policy". Once that is done, then rules to allow the passthrough operation can be added, similar to the example resource nic_dev_t in xen.te. I tried to follow xsm-flask.txt and uncomment one of the pirqcon line in the xsm policy. But I got the following error: policy/modules/xen/xen.te:199:ERROR 'syntax error' at token 'pirqcon' on line 1986: pirqcon 33 system_u:object_r:nic_dev_t Did I miss anything? No, this is an error in either the policy or the parser in checkpolicy. The parser in checkpolicy is rather inflexible, and it currently requires that the device labels be specified at the end of the policy.conf instead of in the middle (as they are now). You should add the commented out lines to the end of tools/flask/policy/policy/initial_sids for now; I will be sending a patch to move them to another file tomorrow. In order to do static labeling for device passthrough, the nodes in a device tree need a 32-bit numeric identifier. IO memory uses the MFN, PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. Why it's restricted to an integer? Would it be possible to use a string as it's done for the sid? The sid is not actually represented as a string internally: it is mapped into a set of integers for the user/role/type and MLS range. -- Daniel De Graaf National Security Agency ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Daniel, On 10/03/2015 22:45, Daniel De Graaf wrote: BTW, do you have any pointer on how to write a policy for device/IRQ passthrough? There is a bit of documentation in xsm-flask.txt about device labeling, which is the hard part of making passthrough work. Labels can be set either statically in the security policy (as documented in the section "Device Labeling") or dynamically using a tool like flask-label-pci as documented in "Resource Policy". Once that is done, then rules to allow the passthrough operation can be added, similar to the example resource nic_dev_t in xen.te. I tried to follow xsm-flask.txt and uncomment one of the pirqcon line in the xsm policy. But I got the following error: policy/modules/xen/xen.te:199:ERROR 'syntax error' at token 'pirqcon' on line 1986: pirqcon 33 system_u:object_r:nic_dev_t Did I miss anything? In order to do static labeling for device passthrough, the nodes in a device tree need a 32-bit numeric identifier. IO memory uses the MFN, PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number. Why it's restricted to an integer? Would it be possible to use a string as it's done for the sid? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 03/10/2015 12:52 PM, Julien Grall wrote:
Hi Daniel,
On 23/02/15 16:25, Daniel De Graaf wrote:
On 02/20/2015 12:17 PM, Ian Campbell wrote:
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
TODO: Update the commit message
A device node is described by a path. It will be used to retrieved the
node in the device tree and assign the related device to the domain.
Only device protected by an IOMMU can be assigned to a guest.
Signed-off-by: Julien Grall
Cc: Ian Jackson
Cc: Wei Liu
Cc: Jan Beulich
---
Changes in v2:
- Use a different number for XEN_DOMCTL_assign_dt_device
---
tools/libxc/include/xenctrl.h | 10
tools/libxc/xc_domain.c | 95
--
These bits all look fine.
+int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+int ret;
+struct dt_device_node *dev;
+
+/* TODO: How to deal with XSM? */
Adding Daniel.
It seems the PCI ones are protected by
xsm_test_assign_device(XSM_HOOK,
domctl->u.assign_device.machine_sbdf);
So it seem that either this needs to become "test_assign_pci_device" and
a similar "test_assign_dt_device" needs to be added and plumbed through
or it needs to grow a type parameter and take the union for the
identifier.
Either would work, but a distinct hook seems simpler to me, especially as
the call sites are distinct and the hook would process them differently.
Sounds good.
The code to apply an XSM context to a DT node would need consideration
too I suppose?
This may require a bit more thought. At first glance, the dt_phandle
field seems to be an identifier that could be used by FLASK to identify a
device using an ocontext lookup. Labeling would then be done in the same
way as PCI devices and x86 legacy I/O ports.
We don't always have a dt_phandle in hand. They are mostly used for
referencing a node within another (such as IOMMU, interrupt
controller...). Also, the value is controlled by the compiler.
AFAICT, the only unique value we have in hand is the path of the device.
OK. I was hoping that there would be a unique numeric identifier. If
there is not, it may be necessary to either create one or to add a new
field to device nodes (like the one for event channels) so that they
can be labeled.
BTW, do you have any pointer on how to write a policy for device/IRQ
passthrough?
There is a bit of documentation in xsm-flask.txt about device labeling,
which is the hard part of making passthrough work. Labels can be set
either statically in the security policy (as documented in the section
"Device Labeling") or dynamically using a tool like flask-label-pci
as documented in "Resource Policy". Once that is done, then rules to
allow the passthrough operation can be added, similar to the example
resource nic_dev_t in xen.te.
In order to do static labeling for device passthrough, the nodes in a
device tree need a 32-bit numeric identifier. IO memory uses the MFN,
PCI devices use SBDF, and IRQs and x86 legacy IOs just use the number.
If device tree nodes can be labeled in this way, they could be added
as another resource type in the policy. If not, then the label of a
device node will need to be set at boot using the XSM hypercalls;
this label would be stored in a security field added to device tree
nodes.
--
Daniel De Graaf
National Security Agency
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Daniel,
On 23/02/15 16:25, Daniel De Graaf wrote:
> On 02/20/2015 12:17 PM, Ian Campbell wrote:
>> On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
>>> TODO: Update the commit message
>>>
>>> A device node is described by a path. It will be used to retrieved the
>>> node in the device tree and assign the related device to the domain.
>>>
>>> Only device protected by an IOMMU can be assigned to a guest.
>>>
>>> Signed-off-by: Julien Grall
>>> Cc: Ian Jackson
>>> Cc: Wei Liu
>>> Cc: Jan Beulich
>>>
>>> ---
>>> Changes in v2:
>>> - Use a different number for XEN_DOMCTL_assign_dt_device
>>> ---
>>> tools/libxc/include/xenctrl.h | 10
>>> tools/libxc/xc_domain.c | 95
>>> --
>>
>> These bits all look fine.
>>
>>> +int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>>> + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> +{
>>> +int ret;
>>> +struct dt_device_node *dev;
>>> +
>>> +/* TODO: How to deal with XSM? */
>>
>> Adding Daniel.
>>
>> It seems the PCI ones are protected by
>> xsm_test_assign_device(XSM_HOOK,
>> domctl->u.assign_device.machine_sbdf);
>>
>> So it seem that either this needs to become "test_assign_pci_device" and
>> a similar "test_assign_dt_device" needs to be added and plumbed through
>> or it needs to grow a type parameter and take the union for the
>> identifier.
>
> Either would work, but a distinct hook seems simpler to me, especially as
> the call sites are distinct and the hook would process them differently.
Sounds good.
>> The code to apply an XSM context to a DT node would need consideration
>> too I suppose?
>
> This may require a bit more thought. At first glance, the dt_phandle
> field seems to be an identifier that could be used by FLASK to identify a
> device using an ocontext lookup. Labeling would then be done in the same
> way as PCI devices and x86 legacy I/O ports.
We don't always have a dt_phandle in hand. They are mostly used for
referencing a node within another (such as IOMMU, interrupt
controller...). Also, the value is controlled by the compiler.
AFAICT, the only unique value we have in hand is the path of the device.
BTW, do you have any pointer on how to write a policy for device/IRQ
passthrough?
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 02/20/2015 12:17 PM, Ian Campbell wrote:
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
TODO: Update the commit message
A device node is described by a path. It will be used to retrieved the
node in the device tree and assign the related device to the domain.
Only device protected by an IOMMU can be assigned to a guest.
Signed-off-by: Julien Grall
Cc: Ian Jackson
Cc: Wei Liu
Cc: Jan Beulich
---
Changes in v2:
- Use a different number for XEN_DOMCTL_assign_dt_device
---
tools/libxc/include/xenctrl.h | 10
tools/libxc/xc_domain.c | 95 --
These bits all look fine.
+int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+int ret;
+struct dt_device_node *dev;
+
+/* TODO: How to deal with XSM? */
Adding Daniel.
It seems the PCI ones are protected by
xsm_test_assign_device(XSM_HOOK, domctl->u.assign_device.machine_sbdf);
So it seem that either this needs to become "test_assign_pci_device" and
a similar "test_assign_dt_device" needs to be added and plumbed through
or it needs to grow a type parameter and take the union for the
identifier.
Either would work, but a distinct hook seems simpler to me, especially as
the call sites are distinct and the hook would process them differently.
The code to apply an XSM context to a DT node would need consideration
too I suppose?
This may require a bit more thought. At first glance, the dt_phandle
field seems to be an identifier that could be used by FLASK to identify a
device using an ocontext lookup. Labeling would then be done in the same
way as PCI devices and x86 legacy I/O ports.
--
Daniel De Graaf
National Security Agency
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Tue, 2015-01-13 at 14:25 +, Julien Grall wrote:
> TODO: Update the commit message
>
> A device node is described by a path. It will be used to retrieved the
> node in the device tree and assign the related device to the domain.
>
> Only device protected by an IOMMU can be assigned to a guest.
>
> Signed-off-by: Julien Grall
> Cc: Ian Jackson
> Cc: Wei Liu
> Cc: Jan Beulich
>
> ---
> Changes in v2:
> - Use a different number for XEN_DOMCTL_assign_dt_device
> ---
> tools/libxc/include/xenctrl.h | 10
> tools/libxc/xc_domain.c | 95 --
These bits all look fine.
> +int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
> + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +int ret;
> +struct dt_device_node *dev;
> +
> +/* TODO: How to deal with XSM? */
Adding Daniel.
It seems the PCI ones are protected by
xsm_test_assign_device(XSM_HOOK, domctl->u.assign_device.machine_sbdf);
So it seem that either this needs to become "test_assign_pci_device" and
a similar "test_assign_dt_device" needs to be added and plumbed through
or it needs to grow a type parameter and take the union for the
identifier.
The code to apply an XSM context to a DT node would need consideration
too I suppose?
> +/* TODO: Do we need to check is_dying? Mostly to protect against
> + * hypercall trying to passthrough a device while we are
> + * dying.
FWIW the PCI case appears not to care...
> + */
> +
> +switch ( domctl->cmd )
> +{
> +case XEN_DOMCTL_assign_device:
> +ret = -ENOSYS;
> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
> +break;
You added something similar to iommu_do_pci_domctl, would it not be
preferable for the caller to switch on domctl->u.assign_device.dev and
call the correct iommu_do_*_domctl?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 29/01/15 15:03, Stefano Stabellini wrote:
> On Thu, 29 Jan 2015, Julien Grall wrote:
>> On 29/01/15 12:09, Stefano Stabellini wrote:
>>> On Thu, 29 Jan 2015, Julien Grall wrote:
Hi Stefano,
On 29/01/15 10:29, Stefano Stabellini wrote:
>> +static bool_t iommu_dt_device_is_assigned(const struct dt_device_node
>> *dev)
>> +{
>> +bool_t assigned = 0;
>> +
>> +if ( !dt_device_is_protected(dev) )
>> +return 1;
>
> Why return true here?
Because any device not protected cannot be assigned to another guest.
This could be used by the toolstack to know whether the device is
assigned or not.
>>>
>>> I understand that much.
>>>
>>>
IHMO, returning 0 would be a false negative.
>>>
>>> Why? Returning 0 means that the device is not assigned, that would be
>>> correct. From this statement I think that actually you are thinking as
>>> if this function actually returned whether a given device is assignable.
>>
>> 0 means the device is not assigned and therefore can be used for
>> passthrough.
>
> I disagree on the "therefore".
> 0 means (or should mean that) the device is not assigned and stop there.
>
>
>> This function is used in the DOMCTL test_assign_device. That would mean
>> we will return 0 for non-protected device. That doesn't sound right as a
>> such device can never be assigned.
>
> assigned -> passed through to a VM
> assignable -> can potentially be assigned
>
> A non-assignable device cannot be assigned by definition. It does not
> make sense to returned true for it, if the question is "is it
> assigned?".
Ok. I will return 0 in when the device is not protected.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Thu, 29 Jan 2015, Julien Grall wrote:
> On 29/01/15 12:09, Stefano Stabellini wrote:
> > On Thu, 29 Jan 2015, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 29/01/15 10:29, Stefano Stabellini wrote:
> +static bool_t iommu_dt_device_is_assigned(const struct dt_device_node
> *dev)
> +{
> +bool_t assigned = 0;
> +
> +if ( !dt_device_is_protected(dev) )
> +return 1;
> >>>
> >>> Why return true here?
> >>
> >> Because any device not protected cannot be assigned to another guest.
> >> This could be used by the toolstack to know whether the device is
> >> assigned or not.
> >
> > I understand that much.
> >
> >
> >> IHMO, returning 0 would be a false negative.
> >
> > Why? Returning 0 means that the device is not assigned, that would be
> > correct. From this statement I think that actually you are thinking as
> > if this function actually returned whether a given device is assignable.
>
> 0 means the device is not assigned and therefore can be used for
> passthrough.
I disagree on the "therefore".
0 means (or should mean that) the device is not assigned and stop there.
> This function is used in the DOMCTL test_assign_device. That would mean
> we will return 0 for non-protected device. That doesn't sound right as a
> such device can never be assigned.
assigned -> passed through to a VM
assignable -> can potentially be assigned
A non-assignable device cannot be assigned by definition. It does not
make sense to returned true for it, if the question is "is it
assigned?".
> > In that case you should rename the function to
> >
> > iommu_dt_device_is_assignable
>
> dt_device_is_assignable is not more clear.
I disagree.
> Return 0 here would mean the
> device cannot be assigned in the sense it's not protected.
>
> Moreover, this name would not be in sync with the DOMCTL test_assign_device.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 29/01/15 12:09, Stefano Stabellini wrote:
> On Thu, 29 Jan 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 29/01/15 10:29, Stefano Stabellini wrote:
+static bool_t iommu_dt_device_is_assigned(const struct dt_device_node
*dev)
+{
+bool_t assigned = 0;
+
+if ( !dt_device_is_protected(dev) )
+return 1;
>>>
>>> Why return true here?
>>
>> Because any device not protected cannot be assigned to another guest.
>> This could be used by the toolstack to know whether the device is
>> assigned or not.
>
> I understand that much.
>
>
>> IHMO, returning 0 would be a false negative.
>
> Why? Returning 0 means that the device is not assigned, that would be
> correct. From this statement I think that actually you are thinking as
> if this function actually returned whether a given device is assignable.
0 means the device is not assigned and therefore can be used for
passthrough.
This function is used in the DOMCTL test_assign_device. That would mean
we will return 0 for non-protected device. That doesn't sound right as a
such device can never be assigned.
> In that case you should rename the function to
>
> iommu_dt_device_is_assignable
dt_device_is_assignable is not more clear. Return 0 here would mean the
device cannot be assigned in the sense it's not protected.
Moreover, this name would not be in sync with the DOMCTL test_assign_device.
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Thu, 29 Jan 2015, Julien Grall wrote:
> Hi Stefano,
>
> On 29/01/15 10:29, Stefano Stabellini wrote:
> >> +static bool_t iommu_dt_device_is_assigned(const struct dt_device_node
> >> *dev)
> >> +{
> >> +bool_t assigned = 0;
> >> +
> >> +if ( !dt_device_is_protected(dev) )
> >> +return 1;
> >
> > Why return true here?
>
> Because any device not protected cannot be assigned to another guest.
> This could be used by the toolstack to know whether the device is
> assigned or not.
I understand that much.
> IHMO, returning 0 would be a false negative.
Why? Returning 0 means that the device is not assigned, that would be
correct. From this statement I think that actually you are thinking as
if this function actually returned whether a given device is assignable.
In that case you should rename the function to
iommu_dt_device_is_assignable
> Would a comment in the code suitable?
I think you should rename the function or be consistent with its name.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
Hi Stefano,
On 29/01/15 10:29, Stefano Stabellini wrote:
>> +static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
>> +{
>> +bool_t assigned = 0;
>> +
>> +if ( !dt_device_is_protected(dev) )
>> +return 1;
>
> Why return true here?
Because any device not protected cannot be assigned to another guest.
This could be used by the toolstack to know whether the device is
assigned or not. IHMO, returning 0 would be a false negative.
Would a comment in the code suitable?
Regards,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 29/01/15 10:29, Stefano Stabellini wrote: >>> -seg = domctl->u.assign_device.machine_sbdf >> 16; >>> -bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff; >>> -devfn = domctl->u.assign_device.machine_sbdf & 0xff; >>> +seg = machine_sbdf >> 16; >>> +bus = (machine_sbdf >> 8) & 0xff; >>> +devfn = machine_sbdf & 0xff; >> >> If you fiddle with these, please make them use at least PCI_BUS() >> and PCI_DEVFN2() (we don't have a matching macro for retrieving >> the segment). > > Maybe we should? I could add one. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 29.01.15 at 11:29, wrote: > On Tue, 20 Jan 2015, Jan Beulich wrote: >> >>> On 13.01.15 at 15:25, wrote: >> > -seg = domctl->u.assign_device.machine_sbdf >> 16; >> > -bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff; >> > -devfn = domctl->u.assign_device.machine_sbdf & 0xff; >> > +seg = machine_sbdf >> 16; >> > +bus = (machine_sbdf >> 8) & 0xff; >> > +devfn = machine_sbdf & 0xff; >> >> If you fiddle with these, please make them use at least PCI_BUS() >> and PCI_DEVFN2() (we don't have a matching macro for retrieving >> the segment). > > Maybe we should? Maybe, but it would be of pretty limited use, as generally we don't pass around SBDFs, but (segment, BDF) pairs, or (segment, bus, devfn) triplets, or all four values separately. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Tue, 13 Jan 2015, Julien Grall wrote:
> TODO: Update the commit message
>
> A device node is described by a path. It will be used to retrieved the
> node in the device tree and assign the related device to the domain.
>
> Only device protected by an IOMMU can be assigned to a guest.
>
> Signed-off-by: Julien Grall
> Cc: Ian Jackson
> Cc: Wei Liu
> Cc: Jan Beulich
>
> ---
> Changes in v2:
> - Use a different number for XEN_DOMCTL_assign_dt_device
> ---
> tools/libxc/include/xenctrl.h | 10
> tools/libxc/xc_domain.c | 95 --
> xen/drivers/passthrough/device_tree.c | 97
> +--
> xen/drivers/passthrough/iommu.c | 7 +++
> xen/drivers/passthrough/pci.c | 43 +++-
> xen/include/public/domctl.h | 15 +-
> xen/include/xen/iommu.h | 3 ++
> 7 files changed, 249 insertions(+), 21 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d66571f..db45475 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2055,6 +2055,16 @@ int xc_deassign_device(xc_interface *xch,
> uint32_t domid,
> uint32_t machine_bdf);
>
> +int xc_assign_dt_device(xc_interface *xch,
> +uint32_t domid,
> +char *path);
> +int xc_test_assign_dt_device(xc_interface *xch,
> + uint32_t domid,
> + char *path);
> +int xc_deassign_dt_device(xc_interface *xch,
> + uint32_t domid,
> + char *path);
> +
> int xc_domain_memory_mapping(xc_interface *xch,
> uint32_t domid,
> unsigned long first_gfn,
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index eb066cf..bca3aee 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1637,7 +1637,8 @@ int xc_assign_device(
>
> domctl.cmd = XEN_DOMCTL_assign_device;
> domctl.domain = domid;
> -domctl.u.assign_device.machine_sbdf = machine_sbdf;
> +domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
> +domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
>
> return do_domctl(xch, &domctl);
> }
> @@ -1686,7 +1687,8 @@ int xc_test_assign_device(
>
> domctl.cmd = XEN_DOMCTL_test_assign_device;
> domctl.domain = domid;
> -domctl.u.assign_device.machine_sbdf = machine_sbdf;
> +domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
> +domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
>
> return do_domctl(xch, &domctl);
> }
> @@ -1700,11 +1702,96 @@ int xc_deassign_device(
>
> domctl.cmd = XEN_DOMCTL_deassign_device;
> domctl.domain = domid;
> -domctl.u.assign_device.machine_sbdf = machine_sbdf;
> -
> +domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
> +domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
> +
> return do_domctl(xch, &domctl);
> }
>
> +int xc_assign_dt_device(
> +xc_interface *xch,
> +uint32_t domid,
> +char *path)
> +{
> +int rc;
> +size_t size = strlen(path);
> +DECLARE_DOMCTL;
> +DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +
> +if ( xc_hypercall_bounce_pre(xch, path) )
> +return -1;
> +
> +domctl.cmd = XEN_DOMCTL_assign_device;
> +domctl.domain = (domid_t)domid;
> +
> +domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
> +domctl.u.assign_device.u.dt.size = size;
> +set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
> +
> +rc = do_domctl(xch, &domctl);
> +
> +xc_hypercall_bounce_post(xch, path);
> +
> +return rc;
> +}
> +
> +int xc_test_assign_dt_device(
> +xc_interface *xch,
> +uint32_t domid,
> +char *path)
> +{
> +int rc;
> +size_t size = strlen(path);
> +DECLARE_DOMCTL;
> +DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +
> +if ( xc_hypercall_bounce_pre(xch, path) )
> +return -1;
> +
> +domctl.cmd = XEN_DOMCTL_test_assign_device;
> +domctl.domain = (domid_t)domid;
> +
> +domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
> +domctl.u.assign_device.u.dt.size = size;
> +set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
> +
> +rc = do_domctl(xch, &domctl);
> +
> +xc_hypercall_bounce_post(xch, path);
> +
> +return rc;
> +}
> +
> +int xc_deassign_dt_device(
> +xc_interface *xch,
> +uint32_t domid,
> +char *path)
> +{
> +int rc;
> +size_t size = strlen(path);
> +DECLARE_DOMCTL;
> +DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +
> +if ( xc_hypercall_bounce_pre(xch, path) )
> +return -1;
> +
> +domctl.cmd = XEN_DOMCTL_deassign_device;
> +domctl.domain = (domid_t)domid;
> +
> +domctl.u.ass
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On Tue, 20 Jan 2015, Jan Beulich wrote:
> >>> On 13.01.15 at 15:25, wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -337,6 +337,13 @@ int iommu_do_domctl(
> > ret = iommu_do_pci_domctl(domctl, d, u_domctl);
> > #endif
> >
> > +if ( ret != -ENOSYS )
> > +return ret;
> > +
> > +#ifdef HAS_DEVICE_TREE
> > +ret = iommu_do_dt_domctl(domctl, d, u_domctl);
> > +#endif
>
> Please move the (inverted) if() into the #ifdef block (but also see
> below regarding the specific error code used).
>
> > @@ -1533,13 +1534,19 @@ int iommu_do_pci_domctl(
> > break;
> >
> > case XEN_DOMCTL_test_assign_device:
> > -ret = xsm_test_assign_device(XSM_HOOK,
> > domctl->u.assign_device.machine_sbdf);
> > +ret = -ENOSYS;
> > +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
> > +break;
>
> I'm rather uncertain about the use of -ENOSYS here: The hypercall
> isn't unimplemented after all. Provided you use consistent error
> codes in the PCI and DT variants, there's no problem calling the
> other variant if that specific error code got returned from the first
> one - the second would then just return the same one again, even
> if the first one failed on something other than the device type
> check. As a result, I'd much prefer -ENODEV here.
>
> > +
> > +machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
> > +
> > +ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
> > if ( ret )
> > break;
> >
> > -seg = domctl->u.assign_device.machine_sbdf >> 16;
> > -bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
> > -devfn = domctl->u.assign_device.machine_sbdf & 0xff;
> > +seg = machine_sbdf >> 16;
> > +bus = (machine_sbdf >> 8) & 0xff;
> > +devfn = machine_sbdf & 0xff;
>
> If you fiddle with these, please make them use at least PCI_BUS()
> and PCI_DEVFN2() (we don't have a matching macro for retrieving
> the segment).
Maybe we should?
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -475,12 +475,23 @@ typedef struct xen_domctl_sendtrigger
> > xen_domctl_sendtrigger_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
> >
> >
> > -/* Assign PCI device to HVM guest. Sets up IOMMU structures. */
> > +/* Assign a device to a guest. Sets up IOMMU structures. */
> > /* XEN_DOMCTL_assign_device */
> > /* XEN_DOMCTL_test_assign_device */
> > /* XEN_DOMCTL_deassign_device */
> > +#define XEN_DOMCTL_DEV_PCI 0
> > +#define XEN_DOMCTL_DEV_DT 1
> > struct xen_domctl_assign_device {
> > -uint32_t machine_sbdf; /* machine PCI ID of assigned device */
> > +uint32_t dev; /* XEN_DOMCTL_DEV_* */
> > +union {
> > +struct {
> > +uint32_t machine_sbdf; /* machine PCI ID of assigned device
> > */
> > +} pci;
> > +struct {
> > +uint32_t size; /* Length of the path */
> > +XEN_GUEST_HANDLE_64(char) path; /* path to the device tree
> > node */
> > +} dt;
> > +} u;
> > };
>
> An incompatible change like this requires bumping
> XEN_DOMCTL_INTERFACE_VERSION when not already done during
> the current release cycle.
>
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 20.01.15 at 15:37, wrote: > On 20/01/15 09:34, Jan Beulich wrote: > On 13.01.15 at 15:25, wrote: >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -337,6 +337,13 @@ int iommu_do_domctl( >>> ret = iommu_do_pci_domctl(domctl, d, u_domctl); >>> #endif >>> >>> +if ( ret != -ENOSYS ) >>> +return ret; >>> + >>> +#ifdef HAS_DEVICE_TREE >>> +ret = iommu_do_dt_domctl(domctl, d, u_domctl); >>> +#endif >> >> Please move the (inverted) if() into the #ifdef block (but also see >> below regarding the specific error code used). > > What do you mean by the "inverted if()"? #ifdef HAS_DEVICE_TREE if ( ret == -ENOSYS ) ret = iommu_do_dt_domctl(domctl, d, u_domctl); #endif Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
On 20/01/15 09:34, Jan Beulich wrote:
On 13.01.15 at 15:25, wrote:
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -337,6 +337,13 @@ int iommu_do_domctl(
>> ret = iommu_do_pci_domctl(domctl, d, u_domctl);
>> #endif
>>
>> +if ( ret != -ENOSYS )
>> +return ret;
>> +
>> +#ifdef HAS_DEVICE_TREE
>> +ret = iommu_do_dt_domctl(domctl, d, u_domctl);
>> +#endif
>
> Please move the (inverted) if() into the #ifdef block (but also see
> below regarding the specific error code used).
What do you mean by the "inverted if()"?
>> @@ -1533,13 +1534,19 @@ int iommu_do_pci_domctl(
>> break;
>>
>> case XEN_DOMCTL_test_assign_device:
>> -ret = xsm_test_assign_device(XSM_HOOK,
>> domctl->u.assign_device.machine_sbdf);
>> +ret = -ENOSYS;
>> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
>> +break;
>
> I'm rather uncertain about the use of -ENOSYS here: The hypercall
> isn't unimplemented after all. Provided you use consistent error
> codes in the PCI and DT variants, there's no problem calling the
> other variant if that specific error code got returned from the first
> one - the second would then just return the same one again, even
> if the first one failed on something other than the device type
> check. As a result, I'd much prefer -ENODEV here.
I will use -ENODEV on the next version.
>> +
>> +machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
>> +
>> +ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
>> if ( ret )
>> break;
>>
>> -seg = domctl->u.assign_device.machine_sbdf >> 16;
>> -bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
>> -devfn = domctl->u.assign_device.machine_sbdf & 0xff;
>> +seg = machine_sbdf >> 16;
>> +bus = (machine_sbdf >> 8) & 0xff;
>> +devfn = machine_sbdf & 0xff;
>
> If you fiddle with these, please make them use at least PCI_BUS()
> and PCI_DEVFN2() (we don't have a matching macro for retrieving
> the segment).
Ok.
>
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -475,12 +475,23 @@ typedef struct xen_domctl_sendtrigger
>> xen_domctl_sendtrigger_t;
>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
>>
>>
>> -/* Assign PCI device to HVM guest. Sets up IOMMU structures. */
>> +/* Assign a device to a guest. Sets up IOMMU structures. */
>> /* XEN_DOMCTL_assign_device */
>> /* XEN_DOMCTL_test_assign_device */
>> /* XEN_DOMCTL_deassign_device */
>> +#define XEN_DOMCTL_DEV_PCI 0
>> +#define XEN_DOMCTL_DEV_DT 1
>> struct xen_domctl_assign_device {
>> -uint32_t machine_sbdf; /* machine PCI ID of assigned device */
>> +uint32_t dev; /* XEN_DOMCTL_DEV_* */
>> +union {
>> +struct {
>> +uint32_t machine_sbdf; /* machine PCI ID of assigned device */
>> +} pci;
>> +struct {
>> +uint32_t size; /* Length of the path */
>> +XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node
>> */
>> +} dt;
>> +} u;
>> };
>
> An incompatible change like this requires bumping
> XEN_DOMCTL_INTERFACE_VERSION when not already done during
> the current release cycle.
The XEN_DOMCTL_INTERFACE_VERSION will be bumped in patch #1 of this
series [1].
Regards,
[1] https://patches.linaro.org/43014/
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
>>> On 13.01.15 at 15:25, wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -337,6 +337,13 @@ int iommu_do_domctl(
> ret = iommu_do_pci_domctl(domctl, d, u_domctl);
> #endif
>
> +if ( ret != -ENOSYS )
> +return ret;
> +
> +#ifdef HAS_DEVICE_TREE
> +ret = iommu_do_dt_domctl(domctl, d, u_domctl);
> +#endif
Please move the (inverted) if() into the #ifdef block (but also see
below regarding the specific error code used).
> @@ -1533,13 +1534,19 @@ int iommu_do_pci_domctl(
> break;
>
> case XEN_DOMCTL_test_assign_device:
> -ret = xsm_test_assign_device(XSM_HOOK,
> domctl->u.assign_device.machine_sbdf);
> +ret = -ENOSYS;
> +if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
> +break;
I'm rather uncertain about the use of -ENOSYS here: The hypercall
isn't unimplemented after all. Provided you use consistent error
codes in the PCI and DT variants, there's no problem calling the
other variant if that specific error code got returned from the first
one - the second would then just return the same one again, even
if the first one failed on something other than the device type
check. As a result, I'd much prefer -ENODEV here.
> +
> +machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
> +
> +ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
> if ( ret )
> break;
>
> -seg = domctl->u.assign_device.machine_sbdf >> 16;
> -bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
> -devfn = domctl->u.assign_device.machine_sbdf & 0xff;
> +seg = machine_sbdf >> 16;
> +bus = (machine_sbdf >> 8) & 0xff;
> +devfn = machine_sbdf & 0xff;
If you fiddle with these, please make them use at least PCI_BUS()
and PCI_DEVFN2() (we don't have a matching macro for retrieving
the segment).
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -475,12 +475,23 @@ typedef struct xen_domctl_sendtrigger
> xen_domctl_sendtrigger_t;
> DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
>
>
> -/* Assign PCI device to HVM guest. Sets up IOMMU structures. */
> +/* Assign a device to a guest. Sets up IOMMU structures. */
> /* XEN_DOMCTL_assign_device */
> /* XEN_DOMCTL_test_assign_device */
> /* XEN_DOMCTL_deassign_device */
> +#define XEN_DOMCTL_DEV_PCI 0
> +#define XEN_DOMCTL_DEV_DT 1
> struct xen_domctl_assign_device {
> -uint32_t machine_sbdf; /* machine PCI ID of assigned device */
> +uint32_t dev; /* XEN_DOMCTL_DEV_* */
> +union {
> +struct {
> +uint32_t machine_sbdf; /* machine PCI ID of assigned device */
> +} pci;
> +struct {
> +uint32_t size; /* Length of the path */
> +XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node
> */
> +} dt;
> +} u;
> };
An incompatible change like this requires bumping
XEN_DOMCTL_INTERFACE_VERSION when not already done during
the current release cycle.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 20/24] xen/passthrough: Extend XEN_DOMCTL_assign_device to support DT device
TODO: Update the commit message
A device node is described by a path. It will be used to retrieved the
node in the device tree and assign the related device to the domain.
Only device protected by an IOMMU can be assigned to a guest.
Signed-off-by: Julien Grall
Cc: Ian Jackson
Cc: Wei Liu
Cc: Jan Beulich
---
Changes in v2:
- Use a different number for XEN_DOMCTL_assign_dt_device
---
tools/libxc/include/xenctrl.h | 10
tools/libxc/xc_domain.c | 95 --
xen/drivers/passthrough/device_tree.c | 97 +--
xen/drivers/passthrough/iommu.c | 7 +++
xen/drivers/passthrough/pci.c | 43 +++-
xen/include/public/domctl.h | 15 +-
xen/include/xen/iommu.h | 3 ++
7 files changed, 249 insertions(+), 21 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d66571f..db45475 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2055,6 +2055,16 @@ int xc_deassign_device(xc_interface *xch,
uint32_t domid,
uint32_t machine_bdf);
+int xc_assign_dt_device(xc_interface *xch,
+uint32_t domid,
+char *path);
+int xc_test_assign_dt_device(xc_interface *xch,
+ uint32_t domid,
+ char *path);
+int xc_deassign_dt_device(xc_interface *xch,
+ uint32_t domid,
+ char *path);
+
int xc_domain_memory_mapping(xc_interface *xch,
uint32_t domid,
unsigned long first_gfn,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index eb066cf..bca3aee 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1637,7 +1637,8 @@ int xc_assign_device(
domctl.cmd = XEN_DOMCTL_assign_device;
domctl.domain = domid;
-domctl.u.assign_device.machine_sbdf = machine_sbdf;
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
return do_domctl(xch, &domctl);
}
@@ -1686,7 +1687,8 @@ int xc_test_assign_device(
domctl.cmd = XEN_DOMCTL_test_assign_device;
domctl.domain = domid;
-domctl.u.assign_device.machine_sbdf = machine_sbdf;
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
return do_domctl(xch, &domctl);
}
@@ -1700,11 +1702,96 @@ int xc_deassign_device(
domctl.cmd = XEN_DOMCTL_deassign_device;
domctl.domain = domid;
-domctl.u.assign_device.machine_sbdf = machine_sbdf;
-
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+
return do_domctl(xch, &domctl);
}
+int xc_assign_dt_device(
+xc_interface *xch,
+uint32_t domid,
+char *path)
+{
+int rc;
+size_t size = strlen(path);
+DECLARE_DOMCTL;
+DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+if ( xc_hypercall_bounce_pre(xch, path) )
+return -1;
+
+domctl.cmd = XEN_DOMCTL_assign_device;
+domctl.domain = (domid_t)domid;
+
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+domctl.u.assign_device.u.dt.size = size;
+set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+rc = do_domctl(xch, &domctl);
+
+xc_hypercall_bounce_post(xch, path);
+
+return rc;
+}
+
+int xc_test_assign_dt_device(
+xc_interface *xch,
+uint32_t domid,
+char *path)
+{
+int rc;
+size_t size = strlen(path);
+DECLARE_DOMCTL;
+DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+if ( xc_hypercall_bounce_pre(xch, path) )
+return -1;
+
+domctl.cmd = XEN_DOMCTL_test_assign_device;
+domctl.domain = (domid_t)domid;
+
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+domctl.u.assign_device.u.dt.size = size;
+set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+rc = do_domctl(xch, &domctl);
+
+xc_hypercall_bounce_post(xch, path);
+
+return rc;
+}
+
+int xc_deassign_dt_device(
+xc_interface *xch,
+uint32_t domid,
+char *path)
+{
+int rc;
+size_t size = strlen(path);
+DECLARE_DOMCTL;
+DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+if ( xc_hypercall_bounce_pre(xch, path) )
+return -1;
+
+domctl.cmd = XEN_DOMCTL_deassign_device;
+domctl.domain = (domid_t)domid;
+
+domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+domctl.u.assign_device.u.dt.size = size;
+set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+rc = do_domctl(xch, &domctl);
+
+xc_hypercall_bounce_post(xch, path);
+
+return rc;
+}
+
+
+
+
int xc_domain_update_msi_irq(
xc_interface *xch,
uint32_t domid,
diff --git a/xe
