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
