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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Oleksandr,

>>> +
>>> +static void ipmmu_domain_backup_context(struct ipmmu_vmsa_domain *domain)
>>> +{
>>> +    struct ipmmu_vmsa_device *mmu = domain->mmu->root;
>>> +    struct ipmmu_reg_ctx *regs = mmu->reg_backup[domain->context_id];
>>> +
>>> +    dev_dbg(mmu->dev, "Handle domain context %u backup\n", 
>>> domain->context_id);
>>> +
>>> +    regs->imttlbr0 = ipmmu_ctx_read_root(domain, IMTTLBR0);
>>> +    regs->imttubr0 = ipmmu_ctx_read_root(domain, IMTTUBR0);
>>> +    regs->imttbcr  = ipmmu_ctx_read_root(domain, IMTTBCR);
>>> +    regs->imctr    = ipmmu_ctx_read_root(domain, IMCTR);
>>> +}
>>> +
>>> +static void ipmmu_domain_restore_context(struct ipmmu_vmsa_domain *domain)
>>> +{
>>> +    struct ipmmu_vmsa_device *mmu = domain->mmu->root;
>>> +    struct ipmmu_reg_ctx *regs  = mmu->reg_backup[domain->context_id];
>> NIT: There is a double space before the `=`
>>> +
>>> +    dev_dbg(mmu->dev, "Handle domain context %u restore\n", 
>>> domain->context_id);
>>> +
>>> +    ipmmu_ctx_write_root(domain, IMTTLBR0, regs->imttlbr0);
>>> +    ipmmu_ctx_write_root(domain, IMTTUBR0, regs->imttubr0);
>>> +    ipmmu_ctx_write_root(domain, IMTTBCR,  regs->imttbcr);
>>> +    ipmmu_ctx_write_all(domain,  IMCTR,    regs->imctr | IMCTR_FLUSH);
>> I see in ipmmu_tlb_invalidate() we do:
>> dsb(sy);
>> ipmmu_tlb_sync(domain);
>> Is it safe to omit them here?
> 
> Luca, good question, thanks. Below my understanding (which might be wrong):
> 
> The IMCTR_FLUSH bit here is not an explicit TLB invalidation request — it is 
> required by the HW whenever context registers are modified (regardless of 
> whether an actual TLB flush is the intent). For example, 
> ipmmu_domain_init_context() similarly writes:
> 
> ipmmu_ctx_write_root(domain, IMCTR,
>                     IMCTR_VA64 | IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
> 
> and does not follow it with dsb(sy) / ipmmu_tlb_sync().
> 
> In contrast, ipmmu_tlb_invalidate() does include the sync because it is an 
> explicit flush request from the P2M framework, and we need a guarantee that 
> the invalidation has completed before proceeding.
> 
> Here, we are simply restoring context registers after resume, there is no 
> caller waiting on flush completion, so the additional synchronization is not 
> necessary from my PoV.
> 

yes I had a closer look into Linux and for restore only there is no 
dsb/tlb_sync:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iommu/ipmmu-vmsa.c?h=v7.1-rc4#n1119

So I think we are safe to omit them here

Cheers,
Luca