* [PATCH] domctl: improve device assignment structure layout and use
@ 2017-06-09 13:47 Jan Beulich
2017-06-12 14:15 ` Julien Grall
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jan Beulich @ 2017-06-09 13:47 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall
[-- Attachment #1: Type: text/plain, Size: 7836 bytes --]
Avoid needless gaps. Make flags field mandatory for all three
operations (and rename it to fit the intended future purpose of
possibly holding more than just one flag).
Also correct a typo in a related domctl.h comment.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The domain ID treatment of test_assign_device is at least strange:
Callers pass in a value, but do_domctl() as well as the actual handler
code ignore them. I think we should increase flexibility here by making
e.g. DOMID_INVALID a wild card to obtain current behavior, while
allowing individual domain IDs to be passed in to inquire whether the
device is assigned to that specific domain.
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1488,7 +1488,7 @@ int xc_assign_device(
xc_interface *xch,
uint32_t domid,
uint32_t machine_sbdf,
- uint32_t flag)
+ uint32_t flags)
{
DECLARE_DOMCTL;
@@ -1496,7 +1496,7 @@ int xc_assign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
- domctl.u.assign_device.flag = flag;
+ domctl.u.assign_device.flags = flags;
return do_domctl(xch, &domctl);
}
@@ -1547,6 +1547,7 @@ int xc_test_assign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.flags = 0;
return do_domctl(xch, &domctl);
}
@@ -1562,6 +1563,7 @@ int xc_deassign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.flags = 0;
return do_domctl(xch, &domctl);
}
@@ -1588,7 +1590,7 @@ int xc_assign_dt_device(
* DT doesn't own any RDM so actually DT has nothing to do
* for any flag and here just fix that as 0.
*/
- domctl.u.assign_device.flag = 0;
+ domctl.u.assign_device.flags = 0;
set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
rc = do_domctl(xch, &domctl);
@@ -1617,6 +1619,7 @@ int xc_test_assign_dt_device(
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);
+ domctl.u.assign_device.flags = 0;
rc = do_domctl(xch, &domctl);
@@ -1644,6 +1646,7 @@ int xc_deassign_dt_device(
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);
+ domctl.u.assign_device.flags = 0;
rc = do_domctl(xch, &domctl);
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -147,11 +147,9 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
- if ( unlikely(d->is_dying) )
- {
- ret = -EINVAL;
+ ret = -EINVAL;
+ if ( d->is_dying || domctl->u.assign_device.flags )
break;
- }
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
@@ -176,6 +174,10 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
&dev);
@@ -197,6 +199,10 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
&dev);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1540,12 +1540,13 @@ int iommu_do_pci_domctl(
{
u16 seg;
u8 bus, devfn;
- u32 flag;
int ret = 0;
uint32_t machine_sbdf;
switch ( domctl->cmd )
{
+ unsigned int flags;
+
case XEN_DOMCTL_get_device_group:
{
u32 max_sdevs;
@@ -1583,6 +1584,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
@@ -1614,11 +1619,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
- if ( unlikely(d->is_dying) )
- {
- ret = -EINVAL;
+ ret = -EINVAL;
+ flags = domctl->u.assign_device.flags;
+ if ( d->is_dying || (flags & ~XEN_DOMCTL_DEV_RDM_RELAXED) )
break;
- }
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
@@ -1629,15 +1633,9 @@ int iommu_do_pci_domctl(
seg = machine_sbdf >> 16;
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN2(machine_sbdf);
- flag = domctl->u.assign_device.flag;
- if ( flag & ~XEN_DOMCTL_DEV_RDM_RELAXED )
- {
- ret = -EINVAL;
- break;
- }
ret = device_assigned(seg, bus, devfn) ?:
- assign_device(d, seg, bus, devfn, flag);
+ assign_device(d, seg, bus, devfn, flags);
if ( ret == -ERESTART )
ret = hypercall_create_continuation(__HYPERVISOR_domctl,
"h", u_domctl);
@@ -1661,6 +1659,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
ret = xsm_deassign_device(XSM_HOOK, d, machine_sbdf);
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -511,14 +511,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendt
* XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
* between the different type of device:
* - PCI device (XEN_DOMCTL_DEV_PCI) will be reassigned to DOM0
- * - DT device (XEN_DOMCTL_DT_PCI) will left unassigned. DOM0
+ * - DT device (XEN_DOMCTL_DEV_DT) will left unassigned. DOM0
* will have to call XEN_DOMCTL_assign_device in order to use the
* device.
*/
#define XEN_DOMCTL_DEV_PCI 0
#define XEN_DOMCTL_DEV_DT 1
struct xen_domctl_assign_device {
+ /* IN */
uint32_t dev; /* XEN_DOMCTL_DEV_* */
+ uint32_t flags;
+#define XEN_DOMCTL_DEV_RDM_RELAXED 1 /* assign only */
union {
struct {
uint32_t machine_sbdf; /* machine PCI ID of assigned device */
@@ -528,9 +531,6 @@ struct xen_domctl_assign_device {
XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node */
} dt;
} u;
- /* IN */
-#define XEN_DOMCTL_DEV_RDM_RELAXED 1
- uint32_t flag; /* flag of assigned device */
};
typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t);
[-- Attachment #2: domctl-assign-dev-layout.patch --]
[-- Type: text/plain, Size: 7894 bytes --]
domctl: improve device assignment structure layout and use
Avoid needless gaps. Make flags field mandatory for all three
operations (and rename it to fit the intended future purpose of
possibly holding more than just one flag).
Also correct a typo in a related domctl.h comment.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The domain ID treatment of test_assign_device is at least strange:
Callers pass in a value, but do_domctl() as well as the actual handler
code ignore them. I think we should increase flexibility here by making
e.g. DOMID_INVALID a wild card to obtain current behavior, while
allowing individual domain IDs to be passed in to inquire whether the
device is assigned to that specific domain.
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1488,7 +1488,7 @@ int xc_assign_device(
xc_interface *xch,
uint32_t domid,
uint32_t machine_sbdf,
- uint32_t flag)
+ uint32_t flags)
{
DECLARE_DOMCTL;
@@ -1496,7 +1496,7 @@ int xc_assign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
- domctl.u.assign_device.flag = flag;
+ domctl.u.assign_device.flags = flags;
return do_domctl(xch, &domctl);
}
@@ -1547,6 +1547,7 @@ int xc_test_assign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.flags = 0;
return do_domctl(xch, &domctl);
}
@@ -1562,6 +1563,7 @@ int xc_deassign_device(
domctl.domain = domid;
domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.flags = 0;
return do_domctl(xch, &domctl);
}
@@ -1588,7 +1590,7 @@ int xc_assign_dt_device(
* DT doesn't own any RDM so actually DT has nothing to do
* for any flag and here just fix that as 0.
*/
- domctl.u.assign_device.flag = 0;
+ domctl.u.assign_device.flags = 0;
set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
rc = do_domctl(xch, &domctl);
@@ -1617,6 +1619,7 @@ int xc_test_assign_dt_device(
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);
+ domctl.u.assign_device.flags = 0;
rc = do_domctl(xch, &domctl);
@@ -1644,6 +1646,7 @@ int xc_deassign_dt_device(
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);
+ domctl.u.assign_device.flags = 0;
rc = do_domctl(xch, &domctl);
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -147,11 +147,9 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
- if ( unlikely(d->is_dying) )
- {
- ret = -EINVAL;
+ ret = -EINVAL;
+ if ( d->is_dying || domctl->u.assign_device.flags )
break;
- }
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
@@ -176,6 +174,10 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
&dev);
@@ -197,6 +199,10 @@ int iommu_do_dt_domctl(struct xen_domctl
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
domctl->u.assign_device.u.dt.size,
&dev);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1540,12 +1540,13 @@ int iommu_do_pci_domctl(
{
u16 seg;
u8 bus, devfn;
- u32 flag;
int ret = 0;
uint32_t machine_sbdf;
switch ( domctl->cmd )
{
+ unsigned int flags;
+
case XEN_DOMCTL_get_device_group:
{
u32 max_sdevs;
@@ -1583,6 +1584,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
@@ -1614,11 +1619,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
- if ( unlikely(d->is_dying) )
- {
- ret = -EINVAL;
+ ret = -EINVAL;
+ flags = domctl->u.assign_device.flags;
+ if ( d->is_dying || (flags & ~XEN_DOMCTL_DEV_RDM_RELAXED) )
break;
- }
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
@@ -1629,15 +1633,9 @@ int iommu_do_pci_domctl(
seg = machine_sbdf >> 16;
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN2(machine_sbdf);
- flag = domctl->u.assign_device.flag;
- if ( flag & ~XEN_DOMCTL_DEV_RDM_RELAXED )
- {
- ret = -EINVAL;
- break;
- }
ret = device_assigned(seg, bus, devfn) ?:
- assign_device(d, seg, bus, devfn, flag);
+ assign_device(d, seg, bus, devfn, flags);
if ( ret == -ERESTART )
ret = hypercall_create_continuation(__HYPERVISOR_domctl,
"h", u_domctl);
@@ -1661,6 +1659,10 @@ int iommu_do_pci_domctl(
if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
break;
+ ret = -EINVAL;
+ if ( domctl->u.assign_device.flags )
+ break;
+
machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
ret = xsm_deassign_device(XSM_HOOK, d, machine_sbdf);
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -511,14 +511,17 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendt
* XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
* between the different type of device:
* - PCI device (XEN_DOMCTL_DEV_PCI) will be reassigned to DOM0
- * - DT device (XEN_DOMCTL_DT_PCI) will left unassigned. DOM0
+ * - DT device (XEN_DOMCTL_DEV_DT) will left unassigned. DOM0
* will have to call XEN_DOMCTL_assign_device in order to use the
* device.
*/
#define XEN_DOMCTL_DEV_PCI 0
#define XEN_DOMCTL_DEV_DT 1
struct xen_domctl_assign_device {
+ /* IN */
uint32_t dev; /* XEN_DOMCTL_DEV_* */
+ uint32_t flags;
+#define XEN_DOMCTL_DEV_RDM_RELAXED 1 /* assign only */
union {
struct {
uint32_t machine_sbdf; /* machine PCI ID of assigned device */
@@ -528,9 +531,6 @@ struct xen_domctl_assign_device {
XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node */
} dt;
} u;
- /* IN */
-#define XEN_DOMCTL_DEV_RDM_RELAXED 1
- uint32_t flag; /* flag of assigned device */
};
typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t);
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] domctl: improve device assignment structure layout and use
2017-06-09 13:47 [PATCH] domctl: improve device assignment structure layout and use Jan Beulich
@ 2017-06-12 14:15 ` Julien Grall
2017-06-12 14:23 ` Julien Grall
2017-06-12 15:52 ` Wei Liu
2 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2017-06-12 14:15 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
Hi Jan,
On 09/06/17 14:47, Jan Beulich wrote:
> Avoid needless gaps. Make flags field mandatory for all three
> operations (and rename it to fit the intended future purpose of
> possibly holding more than just one flag).
>
> Also correct a typo in a related domctl.h comment.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
For the device_tree part:
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] domctl: improve device assignment structure layout and use
2017-06-09 13:47 [PATCH] domctl: improve device assignment structure layout and use Jan Beulich
2017-06-12 14:15 ` Julien Grall
@ 2017-06-12 14:23 ` Julien Grall
2017-06-12 15:52 ` Wei Liu
2 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2017-06-12 14:23 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan
On 09/06/17 14:47, Jan Beulich wrote:
> Avoid needless gaps. Make flags field mandatory for all three
> operations (and rename it to fit the intended future purpose of
> possibly holding more than just one flag).
>
> Also correct a typo in a related domctl.h comment.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The domain ID treatment of test_assign_device is at least strange:
> Callers pass in a value, but do_domctl() as well as the actual handler
> code ignore them. I think we should increase flexibility here by making
> e.g. DOMID_INVALID a wild card to obtain current behavior, while
> allowing individual domain IDs to be passed in to inquire whether the
> device is assigned to that specific domain.
I forgot to answer on this bit. I agree that the current behavior is
strange. I would be in favor of your suggestion.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] domctl: improve device assignment structure layout and use
2017-06-09 13:47 [PATCH] domctl: improve device assignment structure layout and use Jan Beulich
2017-06-12 14:15 ` Julien Grall
2017-06-12 14:23 ` Julien Grall
@ 2017-06-12 15:52 ` Wei Liu
2 siblings, 0 replies; 4+ messages in thread
From: Wei Liu @ 2017-06-12 15:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, George Dunlap, Andrew Cooper,
Ian Jackson, Tim Deegan, Julien Grall, xen-devel
On Fri, Jun 09, 2017 at 07:47:12AM -0600, Jan Beulich wrote:
> Avoid needless gaps. Make flags field mandatory for all three
> operations (and rename it to fit the intended future purpose of
> possibly holding more than just one flag).
>
> Also correct a typo in a related domctl.h comment.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-12 15:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-09 13:47 [PATCH] domctl: improve device assignment structure layout and use Jan Beulich
2017-06-12 14:15 ` Julien Grall
2017-06-12 14:23 ` Julien Grall
2017-06-12 15:52 ` Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).