Re: [PATCH v4 08/19] hw/arm/smmuv3: Translate CD and TT using stage-2 table

[Mostafa Saleh <smostafa@google.com>]

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