On Tue, Oct 1, 2024 at 4:00 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
>
>
> 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.
>
Agree, it doesn't make sense to have both. My only concern now is
related to riscv_iommu_fault() and riscv_iommu_pri() updating related
queue CSR and interrupt status register (REG_IPSR). Other than that it
should be ok to rely on BQL.
> 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 for bringing up this comment. Yes, definitely makes sense, it
will add latency (what I've found beneficial for driver side testing).
Both solutions have downsides. I'll have a closer look if the
consequences for ATS/PRI processing are severe enough to move back to
the threading model. Let's leave as is for now.
Thanks!
- Tomasz
> Thanks,
>
> Daniel
>
>
> >
> > Thanks for the review,
> > - Tomasz
> >
> >
> >> thanks
> >> -- PMM
