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.
Thanks for the review,
- Tomasz
> thanks
> -- PMM
