Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

From: Oleksandr <olekstysh@gmail.com>
To: Julien Grall <julien.grall@arm.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Subject: Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support
Date: Thu, 8 Aug 2019 18:04:54 +0300	[thread overview]
Message-ID: <e2af5e18-2a95-0880-4acc-492848800b19@gmail.com> (raw)
In-Reply-To: <03b1bac9-40ca-3bd4-d3fa-a4acc4e9e958@arm.com>

Hi

>>>
>>>> Sorry for the possible format issues.
>>>>
>>>>
>>>>      > No, it is not disabled. But, ipmmu_irq() uses another 
>>>> mmu->lock. So, I
>>>>      > think, there won't be a deadlock.
>>>>      >
>>>>      > Or I really missed something?
>>>>      >
>>>>      > If we worry about ipmmu_tlb_invalidate() which is called 
>>>> here (to
>>>>      > perform a flush by request from P2M code, which manages a 
>>>> page table)
>>>>      > and from the irq handler (to perform a flush to resume address
>>>>      > translation), I could use a tasklet to schedule 
>>>> ipmmu_tlb_invalidate()
>>>>      > from the irq handler then. This way we would get this 
>>>> serialized. What
>>>>      > do you think?
>>>>
>>>>     I am afraid a tasklet is not an option. You need to perform the 
>>>> TLB
>>>>     flush when requested otherwise you are introducing a security 
>>>> issue.
>>>>
>>>>     This is because as soon as a region is unmapped in the page 
>>>> table, we
>>>>     remove the drop the reference on any page backing that region. 
>>>> When the
>>>>     reference is dropped to zero, the page can be reallocated to 
>>>> another
>>>>     domain or even Xen. If the TLB flush happen after, then the 
>>>> guest may
>>>>     still be able to access the page for a short time if the 
>>>> translation has
>>>>     been cached by the any TLB (IOMMU, Processor).
>>>>
>>>>
>>>>
>>>> I understand this. I am not proposing to delay a requested by P2M 
>>>> code TLB flush in any case. I just propose to issue TLB flush 
>>>> (which we have to perform in case of page faults, to resolve error 
>>>> condition and resume translations) from a tasklet rather than from 
>>>> interrupt handler directly. This is the TLB flush I am speaking about:
>>>>
>>>> https://github.com/otyshchenko1/xen/blob/ipmmu_upstream2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L598 
>>>>
>>>>
>>>> Sorry if I was unclear.
>>>
>>> My mistake, I misread what you wrote.
>>>
>>> I found the flush in the renesas-bsp and not Linux upstream but it 
>>> is not clear why this is actually required. You are not fixing any 
>>> translation error. So what this flush will do?
>>>
>>> Regarding the placement of the flush, then if you execute in a 
>>> tasklet it will likely be done later on when the IRQ has been 
>>> acknowledge. What's the implication to delay it?
>>
>>
>> Looks like, there is no need to put this flush into a tasklet. As I 
>> understand from Shimoda-san's answer it is OK to call flush here.
>>
>> So, my worry about calling ipmmu_tlb_invalidate() directly from the 
>> interrupt handler is not actual anymore.
>> ----------
>> This is my understanding regarding the flush purpose here. This code, 
>> just follows the TRM, no more no less,
>> which mentions about a need to flush TLB after clearing error status 
>> register and updating a page table entries (which, I assume, means to 
>> resolve a reason why translation/page fault error actually have 
>> happened) to resume address translation request.
>
> Well, I don't have the TRM... so my point of reference is Linux. Why 
> does upstream not do the TLB flush?

I have no idea regarding that.

>
>
> I have been told this is an errata on the IPMMU. Is it related to the 
> series posted on linux-iommu [1]?

I don't think, the TLB flush we are speaking about, is related to that 
series [1] somehow. This TLB flush, I think, is just the last step in a 
sequence of actions which should be performed when the error occurs, no 
more no less. This is how I understand this.

>
>>
>> But, with one remark, as you have already noted, we are not trying to 
>> handle/fix this fault (update page table entries), driver doesn't 
>> manage page table and is not aware what the page table is. What is 
>> more, it is unclear what actually need to be fixed in the page table 
>> which is a CPU page table as the same time.
>>
>> I have heard there is a break-before-make sequence when updating the 
>> page table. So, if device in a domain is issuing DMA somewhere in the 
>> middle of updating a page table, theoretically, we might hit into 
>> this fault. In this case the page table is correct and we don't need 
>> to fix anything...   Being honest, I have never seen a fault caused 
>> by break-before-make sequence.
>
> Ok, so it looks like you are trying to fix [1]. My first concern here 
> is there are no ground for someone without access to the TRM why this 
> is done.

No, I am definitely not trying to fix [1]. I just follow the BSP driver 
I am based on, which in turn follows the TRM. I can extend a comment in 
the code before calling ipmmu_tlb_invalidate().

>
> Furthermore, AFAICT, the patch series never reached upstream. So is it 
> present on all revision of GEN3?

I think, that the newest SoCs revisions (ES 3.0) this driver is supposed 
to support only, are *not* affected by that errata. And *not* require 
such workaround.

-- 
Regards,

Oleksandr Tyshchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel