On 10/1/24 7:14 PM, Tomasz Jeznach wrote:
On Sun, Sep 29, 2024 at 8:46 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
On Sat, 28 Sept 2024 at 22:01, Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
On 9/28/24 5:22 PM, Peter Maydell wrote:
On Tue, 24 Sept 2024 at 23:19, Alistair Francis <alistai...@gmail.com> wrote:
+/* Register helper functions */
+static inline uint32_t riscv_iommu_reg_mod32(RISCVIOMMUState *s,
+ unsigned idx, uint32_t set, uint32_t clr)
+{
+ uint32_t val;
+ qemu_spin_lock(&s->regs_lock);
+ val = ldl_le_p(s->regs_rw + idx);
+ stl_le_p(s->regs_rw + idx, (val & ~clr) | set);
+ qemu_spin_unlock(&s->regs_lock);
+ return val;
+}
This looks very weird. Nobody else's IOMMU implementation
grabs a spinlock while it is accessing guest register data.
Why is riscv special? Why a spinlock? (We use spinlocks
only very very sparingly in general.)
The first versions of the IOMMU used qemu threads. I believe this is where
the locks come from (both from registers and from the cache).
I'm not sure if we're ever going to hit a race condition without the locks
in the current code (i.e. using mmio ops only). I think I'll make an attempt
to remove the locks and see if something breaks.
Generally access to the backing for guest registers in a
device model is protected by the fact that the iothread lock
(BQL) is held while the guest is accessing a device. I think for
iommu devices there may be some extra complication because they
get called as part of the translate-this-address codepath
where I think the BQL is not held, and so they typically
have some qemu_mutex to protect themselves[*]. But they
don't generally need/want to do the handling at this very
fine-grained per-read/write level.
[*] This is just what I've gathered from a quick read through
our other iommu implementations; don't take it as gospel.
If you do need to do something complicated with locking it would
be good to have a comment somewhere explaining why and what
the locking principles are.
Locking for the iot and ctx cache is still required as the translation
requests going through IOMMUMemoryRegionClass.translate callback
(riscv_iommu_memory_region_translate) are unlikely protected by BQL.
With ATS translation requests and eventually PRI notifications coming
to the iommu implementation, cache structures will be modified without
any external locking guarantees.
To add some background to register access spin locks.
Original implementation runs all IOMMU command processing logic as a
separate thread, avoiding holding BQL while executing cache
invalidations and other commands. This was implemented to mimic real
hardware, where IOMMU operations are not blocking CPU execution.
During the review process it was suggested and modified to execute all
commands directly as part of the MMIO callback. Please note, that this
change has consequences of potentially holding CPU/BQL for unspecified
time as the command processing flow might be calling registered IOMMU
memory notifiers for ATS invalidations and page group responses.
Anyone implementing memory notifier callback will now execute with BQL
held. Also, updates to register state might happen during translation
fault reporting and or/pri. At least queue CSR/head/tail registers
updates will need some sort of protection.
Probably adding a comment in the code and maybe restoring the original
threading model would make this implementation more readable and less
problematic in the future.
I agree that the locks makes more sense in a threading model like it was
the case in v2. At this point it doesn't make much sense to have both
BQL and manual locks.
For context, we moved away from it back in v2, after this comment from
Frank Chang:
https://lore.kernel.org/qemu-riscv/CANzO1D35eYan8axod37tAg88r=qg4jt0cvtvo+0aiwrlbbv...@mail.gmail.com/
--------------
In our experience, using QEMU thread increases the latency of command
queue processing,
which leads to the potential IOMMU fence timeout in the Linux driver
when using IOMMU with KVM,
e.g. booting the guest Linux.
Is it possible to remove the thread from the IOMMU just like ARM, AMD,
and Intel IOMMU models?
--------------
Frank then elaborated further about IOFENCE and how they were hitting asserts
that were solved after moving the IOMMU away from a thread model.
For now I removed all locks, like Peter suggested, and in my testing with
riscv-iommu-pci and riscv-iommu-sys I didn't see anything bad happening.
BQL is granting enough race protection it seems.
I believe we should stick with this approach, instead of rolling back
to a threading model that Frank said back in v2 that could be problematic.
Nothing is stopping us from moving back to a threading model later if we
start hitting race conditions or even if we have performance improvements
in doing so. At that point we'll be more versed in how the IOMMU is going
to be used by everyone else too.
Thanks,
Daniel
Thanks for the review,
- Tomasz
thanks
-- PMM
