| Message ID | 20220420055736.27901-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] xen/iommu: cleanup iommu related domctl handling | expand |
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
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
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
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
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>
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
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
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 >
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
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); /*
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(-)