All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] xen/iommu: cleanup iommu related domctl handling
@ 2022-04-20  5:57 Juergen Gross
  2022-04-20  6:11 ` Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Juergen Gross @ 2022-04-20  5:57 UTC (permalink / raw)
  To: xen-devel
  Cc: Juergen Gross, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Roger Pau Monné,
	Paul Durrant

Today iommu_do_domctl() is being called from arch_do_domctl() in the
"default:" case of a switch statement. This has led already to crashes
due to unvalidated parameters.

Fix that by moving the call of iommu_do_domctl() to the main switch
statement of do_domctl().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
V4:
- add iommu_do_domctl() stub for !CONFIG_HAS_PASSTHROUGH (Andrew Cooper,
  Jan Beulich)
---
 xen/arch/arm/domctl.c   | 11 +----------
 xen/arch/x86/domctl.c   |  2 +-
 xen/common/domctl.c     |  7 +++++++
 xen/include/xen/iommu.h | 12 +++++++++---
 4 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 6245af6d0b..1baf25c3d9 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -176,16 +176,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         return rc;
     }
     default:
-    {
-        int rc;
-
-        rc = subarch_do_domctl(domctl, d, u_domctl);
-
-        if ( rc == -ENOSYS )
-            rc = iommu_do_domctl(domctl, d, u_domctl);
-
-        return rc;
-    }
+        return subarch_do_domctl(domctl, d, u_domctl);
     }
 }
 
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a6aae500a3..c9699bb868 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1380,7 +1380,7 @@ long arch_do_domctl(
         break;
 
     default:
-        ret = iommu_do_domctl(domctl, d, u_domctl);
+        ret = -ENOSYS;
         break;
     }
 
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 5879117580..0a866e3132 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             copyback = 1;
         break;
 
+    case XEN_DOMCTL_assign_device:
+    case XEN_DOMCTL_test_assign_device:
+    case XEN_DOMCTL_deassign_device:
+    case XEN_DOMCTL_get_device_group:
+        ret = iommu_do_domctl(op, d, u_domctl);
+        break;
+
     default:
         ret = arch_do_domctl(op, d, u_domctl);
         break;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 3a83981464..c6bbb65bbf 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -341,8 +341,17 @@ struct domain_iommu {
 /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
 #ifdef CONFIG_HAS_PASSTHROUGH
 #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
+
+int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
 #else
 #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
+
+static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
+                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+    return -ENOSYS;
+}
 #endif
 
 int __must_check iommu_suspend(void);
@@ -356,9 +365,6 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 #endif
 
-int iommu_do_domctl(struct xen_domctl *, struct domain *d,
-                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
-
 void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
 
 /*
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  5:57 [PATCH v4] xen/iommu: cleanup iommu related domctl handling Juergen Gross
@ 2022-04-20  6:11 ` Jan Beulich
  2022-04-20  6:22   ` Juergen Gross
  2022-04-21 17:51 ` Andrew Cooper
  2022-04-22 23:41 ` Stefano Stabellini
  2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-04-20  6:11 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Roger Pau Monné,
	Paul Durrant, xen-devel

On 20.04.2022 07:57, Juergen Gross wrote:
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -341,8 +341,17 @@ struct domain_iommu {
>  /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>  #ifdef CONFIG_HAS_PASSTHROUGH
>  #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
> +
> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
> +
> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    return -ENOSYS;
> +}

As said in reply to Andrew as well as in a number or earlier occasions,
I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
here may differ of course, but in the absence of objections I consider
this easy enough to adjust while committing. If, of course, the
approach finds a majority in the first place - as indicated before I
don't view it as very desirable to enumerate all the IOMMU related
domctl-s in the common handler.

FTAOD I can accept x86'es arch_do_domctl() returning -ENOSYS, but only
for the purpose of not altering the pre-existing error code which would
be returned if making it there.

Jan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  6:11 ` Jan Beulich
@ 2022-04-20  6:22   ` Juergen Gross
  2022-04-20  6:27     ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2022-04-20  6:22 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Roger Pau Monné,
	Paul Durrant, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1118 bytes --]

On 20.04.22 08:11, Jan Beulich wrote:
> On 20.04.2022 07:57, Juergen Gross wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>   #ifdef CONFIG_HAS_PASSTHROUGH
>>   #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>> +
>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>   #else
>>   #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>> +
>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>> +{
>> +    return -ENOSYS;
>> +}
> 
> As said in reply to Andrew as well as in a number or earlier occasions,
> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views

In libxl there is an explicit check for ENOSYS being returned for
assigning/deassigning a device, same in the xc python bindings.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  6:22   ` Juergen Gross
@ 2022-04-20  6:27     ` Jan Beulich
  2022-04-21 17:47       ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-04-20  6:27 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Roger Pau Monné,
	Paul Durrant, xen-devel

On 20.04.2022 08:22, Juergen Gross wrote:
> On 20.04.22 08:11, Jan Beulich wrote:
>> On 20.04.2022 07:57, Juergen Gross wrote:
>>> --- a/xen/include/xen/iommu.h
>>> +++ b/xen/include/xen/iommu.h
>>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>>   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>>   #ifdef CONFIG_HAS_PASSTHROUGH
>>>   #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>>> +
>>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>   #else
>>>   #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>>> +
>>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> +{
>>> +    return -ENOSYS;
>>> +}
>>
>> As said in reply to Andrew as well as in a number or earlier occasions,
>> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
> 
> In libxl there is an explicit check for ENOSYS being returned for
> assigning/deassigning a device, same in the xc python bindings.

Urgh.

Jan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  6:27     ` Jan Beulich
@ 2022-04-21 17:47       ` Andrew Cooper
  2022-04-22  7:09         ` Jan Beulich
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2022-04-21 17:47 UTC (permalink / raw)
  To: Jan Beulich, Juergen Gross
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monne,
	Paul Durrant, xen-devel

On 20/04/2022 07:27, Jan Beulich wrote:
> On 20.04.2022 08:22, Juergen Gross wrote:
>> On 20.04.22 08:11, Jan Beulich wrote:
>>> On 20.04.2022 07:57, Juergen Gross wrote:
>>>> --- a/xen/include/xen/iommu.h
>>>> +++ b/xen/include/xen/iommu.h
>>>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>>>   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>>>   #ifdef CONFIG_HAS_PASSTHROUGH
>>>>   #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>>>> +
>>>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>>   #else
>>>>   #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>>>> +
>>>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>> +{
>>>> +    return -ENOSYS;
>>>> +}
>>> As said in reply to Andrew as well as in a number or earlier occasions,
>>> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
>> In libxl there is an explicit check for ENOSYS being returned for
>> assigning/deassigning a device, same in the xc python bindings.
> Urgh.

Honestly, I wasn't particularly happy with your push to swap ENOSYS out
for EOPNOTSUPP.  This shows plainly why it's a bad move.

An end user doesn't give two hoots about the distinction between
hypercall not supported and subops not supported; they care about
whether Xen can perform the requested action or not.  ENOSYS is the more
common way of signalling this, and having only one errno value to check
is better for everyone involved.

~Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  5:57 [PATCH v4] xen/iommu: cleanup iommu related domctl handling Juergen Gross
  2022-04-20  6:11 ` Jan Beulich
@ 2022-04-21 17:51 ` Andrew Cooper
  2022-04-22 23:41 ` Stefano Stabellini
  2 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2022-04-21 17:51 UTC (permalink / raw)
  To: Juergen Gross, xen-devel
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Jan Beulich, Wei Liu,
	Roger Pau Monne, Paul Durrant

On 20/04/2022 06:57, Juergen Gross wrote:
> Today iommu_do_domctl() is being called from arch_do_domctl() in the
> "default:" case of a switch statement. This has led already to crashes
> due to unvalidated parameters.
>
> Fix that by moving the call of iommu_do_domctl() to the main switch
> statement of do_domctl().
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-21 17:47       ` Andrew Cooper
@ 2022-04-22  7:09         ` Jan Beulich
  2022-04-22 19:01           ` Andrew Cooper
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2022-04-22  7:09 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monne,
	Paul Durrant, xen-devel, Juergen Gross

On 21.04.2022 19:47, Andrew Cooper wrote:
> On 20/04/2022 07:27, Jan Beulich wrote:
>> On 20.04.2022 08:22, Juergen Gross wrote:
>>> On 20.04.22 08:11, Jan Beulich wrote:
>>>> On 20.04.2022 07:57, Juergen Gross wrote:
>>>>> --- a/xen/include/xen/iommu.h
>>>>> +++ b/xen/include/xen/iommu.h
>>>>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>>>>   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>>>>   #ifdef CONFIG_HAS_PASSTHROUGH
>>>>>   #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>>>>> +
>>>>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>>>   #else
>>>>>   #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>>>>> +
>>>>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>> +{
>>>>> +    return -ENOSYS;
>>>>> +}
>>>> As said in reply to Andrew as well as in a number or earlier occasions,
>>>> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
>>> In libxl there is an explicit check for ENOSYS being returned for
>>> assigning/deassigning a device, same in the xc python bindings.
>> Urgh.
> 
> Honestly, I wasn't particularly happy with your push to swap ENOSYS out
> for EOPNOTSUPP.  This shows plainly why it's a bad move.
> 
> An end user doesn't give two hoots about the distinction between
> hypercall not supported and subops not supported; they care about
> whether Xen can perform the requested action or not.  ENOSYS is the more
> common way of signalling this, and having only one errno value to check
> is better for everyone involved.

End users are of little interest here. Code looking for ENOSYS is what
is of interest, when the meaning of ENOSYS is quite well defined as
"system call not implemented" (which we merely extend to "hypercall").
Anything smaller scope than a major hypercall cannot possibly be
"hypercall not implemented". Code caring about knowing one vs the
other should not be misguided, and code looking for one when the other
is meant is simply flawed.

Jan



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-22  7:09         ` Jan Beulich
@ 2022-04-22 19:01           ` Andrew Cooper
  2022-04-23  5:55             ` Juergen Gross
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2022-04-22 19:01 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monne,
	Paul Durrant, xen-devel, Juergen Gross

On 22/04/2022 08:09, Jan Beulich wrote:
> On 21.04.2022 19:47, Andrew Cooper wrote:
>> On 20/04/2022 07:27, Jan Beulich wrote:
>>> On 20.04.2022 08:22, Juergen Gross wrote:
>>>> On 20.04.22 08:11, Jan Beulich wrote:
>>>>> On 20.04.2022 07:57, Juergen Gross wrote:
>>>>>> --- a/xen/include/xen/iommu.h
>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>>>>>   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>>>>>   #ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>   #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>>>>>> +
>>>>>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>>>>   #else
>>>>>>   #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>>>>>> +
>>>>>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>> +{
>>>>>> +    return -ENOSYS;
>>>>>> +}
>>>>> As said in reply to Andrew as well as in a number or earlier occasions,
>>>>> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
>>>> In libxl there is an explicit check for ENOSYS being returned for
>>>> assigning/deassigning a device, same in the xc python bindings.
>>> Urgh.
>> Honestly, I wasn't particularly happy with your push to swap ENOSYS out
>> for EOPNOTSUPP.  This shows plainly why it's a bad move.
>>
>> An end user doesn't give two hoots about the distinction between
>> hypercall not supported and subops not supported; they care about
>> whether Xen can perform the requested action or not.  ENOSYS is the more
>> common way of signalling this, and having only one errno value to check
>> is better for everyone involved.
> End users are of little interest here. Code looking for ENOSYS is what
> is of interest, when the meaning of ENOSYS is quite well defined as
> "system call not implemented"

POSIX specifies it as "Functionality not supported" which is why other
compliment systems use it for smaller granularity than a system call.

~Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-20  5:57 [PATCH v4] xen/iommu: cleanup iommu related domctl handling Juergen Gross
  2022-04-20  6:11 ` Jan Beulich
  2022-04-21 17:51 ` Andrew Cooper
@ 2022-04-22 23:41 ` Stefano Stabellini
  2 siblings, 0 replies; 10+ messages in thread
From: Stefano Stabellini @ 2022-04-22 23:41 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
	Wei Liu, Roger Pau Monné,
	Paul Durrant

On Wed, 20 Apr 2022, Juergen Gross wrote:
> Today iommu_do_domctl() is being called from arch_do_domctl() in the
> "default:" case of a switch statement. This has led already to crashes
> due to unvalidated parameters.
> 
> Fix that by moving the call of iommu_do_domctl() to the main switch
> statement of do_domctl().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

For the ARM side:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


I have no opinion on the ENOSYS vs EOPNOTSUPP discussion.



> ---
> V3:
> - new patch
> V4:
> - add iommu_do_domctl() stub for !CONFIG_HAS_PASSTHROUGH (Andrew Cooper,
>   Jan Beulich)
> ---
>  xen/arch/arm/domctl.c   | 11 +----------
>  xen/arch/x86/domctl.c   |  2 +-
>  xen/common/domctl.c     |  7 +++++++
>  xen/include/xen/iommu.h | 12 +++++++++---
>  4 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 6245af6d0b..1baf25c3d9 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -176,16 +176,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          return rc;
>      }
>      default:
> -    {
> -        int rc;
> -
> -        rc = subarch_do_domctl(domctl, d, u_domctl);
> -
> -        if ( rc == -ENOSYS )
> -            rc = iommu_do_domctl(domctl, d, u_domctl);
> -
> -        return rc;
> -    }
> +        return subarch_do_domctl(domctl, d, u_domctl);
>      }
>  }
>  
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index a6aae500a3..c9699bb868 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1380,7 +1380,7 @@ long arch_do_domctl(
>          break;
>  
>      default:
> -        ret = iommu_do_domctl(domctl, d, u_domctl);
> +        ret = -ENOSYS;
>          break;
>      }
>  
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 5879117580..0a866e3132 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -871,6 +871,13 @@ long cf_check do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              copyback = 1;
>          break;
>  
> +    case XEN_DOMCTL_assign_device:
> +    case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_deassign_device:
> +    case XEN_DOMCTL_get_device_group:
> +        ret = iommu_do_domctl(op, d, u_domctl);
> +        break;
> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 3a83981464..c6bbb65bbf 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -341,8 +341,17 @@ struct domain_iommu {
>  /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>  #ifdef CONFIG_HAS_PASSTHROUGH
>  #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
> +
> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>  #else
>  #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
> +
> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> +{
> +    return -ENOSYS;
> +}
>  #endif
>  
>  int __must_check iommu_suspend(void);
> @@ -356,9 +365,6 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>  #endif
>  
> -int iommu_do_domctl(struct xen_domctl *, struct domain *d,
> -                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
> -
>  void iommu_dev_iotlb_flush_timeout(struct domain *d, struct pci_dev *pdev);
>  
>  /*
> -- 
> 2.34.1
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling
  2022-04-22 19:01           ` Andrew Cooper
@ 2022-04-23  5:55             ` Juergen Gross
  0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2022-04-23  5:55 UTC (permalink / raw)
  To: Andrew Cooper, Jan Beulich
  Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, George Dunlap, Wei Liu, Roger Pau Monne,
	Paul Durrant, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 3060 bytes --]

On 22.04.22 21:01, Andrew Cooper wrote:
> On 22/04/2022 08:09, Jan Beulich wrote:
>> On 21.04.2022 19:47, Andrew Cooper wrote:
>>> On 20/04/2022 07:27, Jan Beulich wrote:
>>>> On 20.04.2022 08:22, Juergen Gross wrote:
>>>>> On 20.04.22 08:11, Jan Beulich wrote:
>>>>>> On 20.04.2022 07:57, Juergen Gross wrote:
>>>>>>> --- a/xen/include/xen/iommu.h
>>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>>> @@ -341,8 +341,17 @@ struct domain_iommu {
>>>>>>>    /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
>>>>>>>    #ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>    #define need_iommu_pt_sync(d)     (dom_iommu(d)->need_sync)
>>>>>>> +
>>>>>>> +int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>> +                    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
>>>>>>>    #else
>>>>>>>    #define need_iommu_pt_sync(d)     ({ (void)(d); false; })
>>>>>>> +
>>>>>>> +static inline int iommu_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>>>>>> +                                  XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>>>>> +{
>>>>>>> +    return -ENOSYS;
>>>>>>> +}
>>>>>> As said in reply to Andrew as well as in a number or earlier occasions,
>>>>>> I firmly think that this wants to be -EOPNOTSUPP, not -ENOSYS. Views
>>>>> In libxl there is an explicit check for ENOSYS being returned for
>>>>> assigning/deassigning a device, same in the xc python bindings.
>>>> Urgh.
>>> Honestly, I wasn't particularly happy with your push to swap ENOSYS out
>>> for EOPNOTSUPP.  This shows plainly why it's a bad move.
>>>
>>> An end user doesn't give two hoots about the distinction between
>>> hypercall not supported and subops not supported; they care about
>>> whether Xen can perform the requested action or not.  ENOSYS is the more
>>> common way of signalling this, and having only one errno value to check
>>> is better for everyone involved.
>> End users are of little interest here. Code looking for ENOSYS is what
>> is of interest, when the meaning of ENOSYS is quite well defined as
>> "system call not implemented"
> 
> POSIX specifies it as "Functionality not supported" which is why other
> compliment systems use it for smaller granularity than a system call.

Thinking a little bit more about this topic, I'd like to return ENOSYS.

EOPNOTSUPP should be returned in case an operation is not suitable
for the given parameter combination (e.g. an IOMMU related operation
for a guest not being subject to IOMMU handling). The same operation
can be successful on the given system with other parameter combinations.

ENOSYS should be returned if the current hypervisor doesn't support the
requested operation (hypercall or hypercall + sub-op) on the given system
at all. The high level caller (e.g. a user of libxc) doesn't care, whether
a requested operation is a dedicated hypercall or a sub-op of a hypercall.
The only interesting aspect is whether the operation is unavailable, or
just failing due to the current situation.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-04-23  5:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20  5:57 [PATCH v4] xen/iommu: cleanup iommu related domctl handling Juergen Gross
2022-04-20  6:11 ` Jan Beulich
2022-04-20  6:22   ` Juergen Gross
2022-04-20  6:27     ` Jan Beulich
2022-04-21 17:47       ` Andrew Cooper
2022-04-22  7:09         ` Jan Beulich
2022-04-22 19:01           ` Andrew Cooper
2022-04-23  5:55             ` Juergen Gross
2022-04-21 17:51 ` Andrew Cooper
2022-04-22 23:41 ` Stefano Stabellini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.