From: Mykyta Poturai <mykyta_poturai@epam.com>

These functions may be unimplemented, so check that they exist before
calling to prevent crashes.

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
Introduced in patch series V3.
---
 xen/drivers/passthrough/iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 16aad86973..55b33c9719 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -613,7 +613,7 @@ int __init iommu_setup(void)
 
 int iommu_suspend(void)
 {
-    if ( iommu_enabled )
+    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->suspend )
         return iommu_call(iommu_get_ops(), suspend);
 
     return 0;
@@ -621,7 +621,7 @@ int iommu_suspend(void)
 
 void iommu_resume(void)
 {
-    if ( iommu_enabled )
+    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->resume )
         iommu_vcall(iommu_get_ops(), resume);
 }
 
-- 
2.43.0

Hi,

On 05/03/2025 09:11, Mykola Kvach wrote:
> From: Mykyta Poturai <mykyta_poturai@epam.com>
> 
> These functions may be unimplemented, so check that they exist before
> calling to prevent crashes.

Looking at the cover letter, I see you wrote the following:

"Add suspend/resume handlers to IOMMU drivers (there aren’t any
problems with the current implementation because the domains used for
test are thin, and this patch series implements only the very basic
logic)"

which I read as this patch is a temporary hack until we implement IOMMU.
Is that correct? If so, can you tag it as HACK and move to the end to 
end up to merge it?

Cheers,

-- 
Julien Grall

Hi,

On Tue, Mar 11, 2025 at 10:28 PM Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 05/03/2025 09:11, Mykola Kvach wrote:
> > From: Mykyta Poturai <mykyta_poturai@epam.com>
> >
> > These functions may be unimplemented, so check that they exist before
> > calling to prevent crashes.
>
> Looking at the cover letter, I see you wrote the following:
>
> "Add suspend/resume handlers to IOMMU drivers (there aren’t any
> problems with the current implementation because the domains used for
> test are thin, and this patch series implements only the very basic
> logic)"
>
> which I read as this patch is a temporary hack until we implement IOMMU.
> Is that correct? If so, can you tag it as HACK and move to the end to
> end up to merge it?

Yes, you're right—if we have handlers for suspend/resume in the IOMMU driver,
we don't need this patch at all. However, if we drop the iommu_suspend/resume
calls from the system suspend, this commit becomes unnecessary, even within
this patch series.

>
> Cheers,
>
> --
> Julien Grall
>

Best regards,
Mykola

On 05.03.2025 10:11, Mykola Kvach wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -613,7 +613,7 @@ int __init iommu_setup(void)
>  
>  int iommu_suspend(void)
>  {
> -    if ( iommu_enabled )
> +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->suspend )
>          return iommu_call(iommu_get_ops(), suspend);
>  
>      return 0;
> @@ -621,7 +621,7 @@ int iommu_suspend(void)
>  
>  void iommu_resume(void)
>  {
> -    if ( iommu_enabled )
> +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->resume )
>          iommu_vcall(iommu_get_ops(), resume);
>  }

When iommu_enabled is true, surely iommu_get_ops() is required to return
non-NULL?

Jan

Hi,

On Wed, Mar 5, 2025 at 6:45 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.03.2025 10:11, Mykola Kvach wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -613,7 +613,7 @@ int __init iommu_setup(void)
> >
> >  int iommu_suspend(void)
> >  {
> > -    if ( iommu_enabled )
> > +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->suspend )
> >          return iommu_call(iommu_get_ops(), suspend);
> >
> >      return 0;
> > @@ -621,7 +621,7 @@ int iommu_suspend(void)
> >
> >  void iommu_resume(void)
> >  {
> > -    if ( iommu_enabled )
> > +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->resume )
> >          iommu_vcall(iommu_get_ops(), resume);
> >  }
>
> When iommu_enabled is true, surely iommu_get_ops() is required to return
> non-NULL?

As far as I can see, in some cases, the handler is still checked even
if iommu_enabled
is true, such as in the case of the iommu_quiesce call. However, it
might be better to drop
this patch from the current patch series or add a patch that
introduces the handlers.

>
> Jan

Best regards,
Mykola

On 19.03.2025 13:01, Mykola Kvach wrote:
> Hi,
> 
> On Wed, Mar 5, 2025 at 6:45 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -613,7 +613,7 @@ int __init iommu_setup(void)
>>>
>>>  int iommu_suspend(void)
>>>  {
>>> -    if ( iommu_enabled )
>>> +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->suspend )
>>>          return iommu_call(iommu_get_ops(), suspend);
>>>
>>>      return 0;
>>> @@ -621,7 +621,7 @@ int iommu_suspend(void)
>>>
>>>  void iommu_resume(void)
>>>  {
>>> -    if ( iommu_enabled )
>>> +    if ( iommu_enabled && iommu_get_ops() && iommu_get_ops()->resume )
>>>          iommu_vcall(iommu_get_ops(), resume);
>>>  }
>>
>> When iommu_enabled is true, surely iommu_get_ops() is required to return
>> non-NULL?
> 
> As far as I can see, in some cases, the handler is still checked even
> if iommu_enabled
> is true, such as in the case of the iommu_quiesce call.

You say "handler" and also refer to a case where the handler is checked.
My comment was about the bare iommu_get_ops() though.

> However, it
> might be better to drop
> this patch from the current patch series or add a patch that
> introduces the handlers.

Only if they're not merely stubs.

Jan