Re: [PATCH v8 08/13] iommu/ipmmu-vmsa: Implement suspend/resume callbacks

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Luca,

Thank you for the review.

On Fri, Apr 24, 2026 at 4:36 PM Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On 2 Apr 2026, at 11:45, Mykola Kvach <xakep.amatop@xxxxxxxxx> wrote:
> >
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > Store and restore active context and micro-TLB registers.
> >
> > Tested on R-Car H3 Starter Kit.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in V7:
> > - moved suspend context allocation before pci stuff
> > ---
> > xen/drivers/passthrough/arm/ipmmu-vmsa.c | 305 ++++++++++++++++++++++-
> > 1 file changed, 298 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> > b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > index ea9fa9ddf3..6765bd3083 100644
> > --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > @@ -71,6 +71,8 @@
> > })
> > #endif
> >
> > +#define dev_dbg(dev, fmt, ...)    \
> > +    dev_print(dev, XENLOG_DEBUG, fmt, ## __VA_ARGS__)
> > #define dev_info(dev, fmt, ...)    \
> >     dev_print(dev, XENLOG_INFO, fmt, ## __VA_ARGS__)
> > #define dev_warn(dev, fmt, ...)    \
> > @@ -130,6 +132,24 @@ struct ipmmu_features {
> >     unsigned int imuctr_ttsel_mask;
> > };
> >
>
>
> > […]
>
>
> >
> > @@ -1340,10 +1608,11 @@ static int ipmmu_add_device(u8 devfn, struct device 
> > *dev)
> >     struct iommu_fwspec *fwspec;
> >
> > #ifdef CONFIG_HAS_PCI
> > +    int ret;
> > +
> >     if ( dev_is_pci(dev) )
> >     {
> >         struct pci_dev *pdev = dev_to_pci(dev);
> > -        int ret;
> >
> >         if ( devfn != pdev->devfn )
> >             return 0;
> > @@ -1371,6 +1640,15 @@ static int ipmmu_add_device(u8 devfn, struct device 
> > *dev)
> >         /* Let Xen know that the master device is protected by an IOMMU. */
> >         dt_device_set_protected(dev_to_dt(dev));
> >     }
> > +
> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +    if ( ipmmu_alloc_ctx_suspend(dev) )
> > +    {
> > +        dev_err(dev, "Failed to allocate context for suspend\n");
> > +        return -ENOMEM;
> > +    }
> > +#endif
>
> If this fails the device will remain protected, I suggest we move this one 
> before `if ( !dev_is_pci(dev) ) { … }`
> block

Good point, thanks.

Yes, this should be fixed. In the original ordering, a failure in
ipmmu_alloc_ctx_suspend() could leave a non-PCI DT device marked as
protected even though ipmmu_add_device() returned an error.

I'll reorder the code so dt_device_set_protected() is done only
after ipmmu_alloc_ctx_suspend() succeeds. This keeps the successful path
unchanged and avoids leaving stale protected state on failure.

Best regards,
Mykola

>
> The rest looks ok to me, but I’m not an expert of this part.
>
> Cheers,
> Luca
>