Hi Jean,
On Tue, Jul 09, 2024 at 02:00:04PM +0100, Jean-Philippe Brucker wrote:
> Hi Mostafa,
>
> On Tue, Jul 09, 2024 at 07:12:59AM +0000, Mostafa Saleh wrote:
> > > In this case I think we're reporting InputAddr as the CD address, but it
> > > should be the IOVA
> >
> > As Eric mentioned this would require some rework to propagate the iova,
> > but what I am more worried about is the readability in that case, may be we
> > can just fixup the event after smmuv3_get_config() in case of errors,
> > something like:
> >
> > /*
> > * smmuv3_get_config() Only return translation faults in case of
> > * nested translation, otherwise it can only return C_BAD_CD,
> > * C_BAD_STE, C_BAD_STREAMID or F_STE_FETCH.
> > * But in case of translation fault, we need to fixup the
> > * InputAddr to be the IOVA of the translation as the decode
> > * functions don't know about it.
> > */
> > static void smmuv3_config_fixup_event(SMMUEventInfo *event, hwaddr iova)
> > {
> > switch (event->type) {
> > case SMMU_EVT_F_WALK_EABT:
> > case SMMU_EVT_F_TRANSLATION:
> > case SMMU_EVT_F_ADDR_SIZE:
> > case SMMU_EVT_F_ACCESS:
> > case SMMU_EVT_F_PERMISSION:
> > event->u.f_walk_eabt.addr = iova;
> > break;
> > default:
> > break;
> > }
> > }
> >
> > What do you think?
>
> Yes, I think that's also what I came up with. Maybe it would be simpler to
> unconditionally do the fixup at the end of smmuv3_translate() and remove
> .addr write from smmuv3_do_translate()?
I wanted to make it clear what case causes the IOVA to be missing, but I
guess if we unify the setup for the InputAddr it would be easier to
read and just add a comment instead.
>
> A separate union field "f_common" rather than f_walk_eabt may be clearer.
>
Makes sense, but I will not do it in this patch to avoid making it larger
and harder to review, and this can be a separate cleanup as I see in other
places we use eabt already (smmuv3_record_event and for s2 population).
Thanks,
Mostafa
> Thanks,
> Jean
