On Thu, 19 Jun 2025, Oleksii Moisieiev wrote:
> On 18/06/2025 03:04, Stefano Stabellini wrote:
> > On Thu, 12 Jun 2025, Oleksii Moisieiev wrote:
> >> Hi Stefano,
> >>
> >> I'm very sorry for a long silence. Please see my answers below:
> >>
> >> On 22/05/2025 03:25, Stefano Stabellini wrote:
> >>> On Mon, 19 May 2025, Oleksii Moisieiev wrote:
> >>>> From: Grygorii Strashko<grygorii_stras...@epam.com>
> >>>>
> >>>> Add chained handling of assigned DT devices to support access-controller
> >>>> functionality through SCI framework, so DT device assign request can be
> >>>> passed to FW for processing and enabling VM access to requested device
> >>>> (for example, device power management through FW interface like SCMI).
> >>>>
> >>>> The SCI access-controller DT device processing is chained after IOMMU
> >>>> processing and expected to be executed for any DT device regardless of
> >>>> its
> >>>> protection by IOMMU (or if IOMMU is disabled).
> >>>>
> >>>> This allows to pass not only IOMMU protected DT device through
> >>>> xl.cfg:"dtdev" property for processing:
> >>>>
> >>>> dtdev = [
> >>>> "/soc/video@e6ef0000", <- IOMMU protected device
> >>>> "/soc/i2c@e6508000", <- not IOMMU protected device
> >>>> ]
> >>>>
> >>>> The change is done in two parts:
> >>>> 1) update iommu_do_dt_domctl() to check for dt_device_is_protected() and
> >>>> not fail if DT device is not protected by IOMMU
> >>>> 2) add chained call to sci_do_domctl() in do_domctl()
> >>>>
> >>>> Signed-off-by: Grygorii Strashko<grygorii_stras...@epam.com>
> >>>> Signed-off-by: Oleksii Moisieiev<oleksii_moisie...@epam.com>
> >>>> ---
> >>>>
> >>>>
> >>>>
> >>>> xen/arch/arm/firmware/sci.c | 37 +++++++++++++++++++++++++
> >>>> xen/arch/arm/include/asm/firmware/sci.h | 14 ++++++++++
> >>>> xen/common/domctl.c | 19 +++++++++++++
> >>>> xen/drivers/passthrough/device_tree.c | 6 ++++
> >>>> 4 files changed, 76 insertions(+)
> >>>>
> >>>> diff --git a/xen/arch/arm/firmware/sci.c b/xen/arch/arm/firmware/sci.c
> >>>> index e1522e10e2..8efd541c4f 100644
> >>>> --- a/xen/arch/arm/firmware/sci.c
> >>>> +++ b/xen/arch/arm/firmware/sci.c
> >>>> @@ -126,6 +126,43 @@ int sci_assign_dt_device(struct domain *d, struct
> >>>> dt_device_node *dev)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +int sci_do_domctl(struct xen_domctl *domctl, struct domain *d,
> >>>> + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >>>> +{
> >>>> + struct dt_device_node *dev;
> >>>> + int ret = 0;
> >>>> +
> >>>> + switch ( domctl->cmd )
> >>>> + {
> >>>> + case XEN_DOMCTL_assign_device:
> >>>> + ret = -EOPNOTSUPP;
> >>> Are you sure -EOPNOTSUPP is the right error code for the 3 checks below?
> >> The -EOPNOTSUPP code is used because this is part of a chained call after
> >> iommu_do_domctl, as stated in xen/common/domctl.c:859. The
> >> XEN_DOMCTL_assign_device
> >> call is expected to handle any DT device, regardless of whether the DT
> >> device is
> >> protected by an IOMMU or if the IOMMU is disabled.
> >> The following cases are considered:
> >>
> >> 1. IOMMU Protected Device (Success)
> >>
> >> If the device is protected by the IOMMU and iommu_do_domctl returns 0,
> >> we continue
> >> processing the DT device by calling sci_do_domctl.
> >>
> >> 2. IOMMU Disabled (-EOPNOTSUPP from iommu_do_domctl)
> >>
> >> If iommu_do_domctl returns -EOPNOTSUPP, indicating that the IOMMU is
> >> disabled,
> >> we still proceed to call sci_do_domctl.
> > OK this makes sense. I think it is OK to have a special error code to
> > say "the IOMMU is disabled" but I don't know if it is a good idea to try
> > to use -EOPNOTSUPP for that. -EOPNOTSUPP could mean a hypervisor
> > configuration with domctl disabled, for instance.
> >
> > It might be wiser to use a different error code. Maybe ENOENT?
> >
> I see that in the following commit:
>
> 71e617a6b8 (use is_iommu_enabled() where appropriate..., 2019-09-17)
>
> -ENOSYS return code was changed to -EOPNOTSUPP in iommu_do_domctl.
>
> It's not clear to me why this was done from the commit description.
>
> Maybe we should add commit author?
Roger and Jan might know
