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

[Oleksandr <olekstysh@gmail.com>]

> Hi,

Hi, Julien.


>
>> 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.

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.

>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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