Re: [PATCH v4] xen/iommu: cleanup iommu related domctl handling

[Juergen Gross <jgross@xxxxxxxx>]

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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature