Re: [RFC PATCH v3 05/10] hw/arm/smmuv3: Parse STE config for stage-2

[Mostafa Saleh <smostafa@google.com>]

Hi Eric,

Thanks a lot for taking the time to review the patches.

On Mon, May 15, 2023 at 03:03:28PM +0200, Eric Auger wrote:
> >  
> > +/* If stage-1 fault enabled and ptw event targets it. */
> > +#define PTW_FAULT_S1(cfg, event)            ((cfg)->record_faults && \
> > +                                             !(event).u.f_walk_eabt.s2)
> > +/* If stage-2 fault enabled and ptw event targets it. */
> > +#define PTW_FAULT_S2(cfg, event)            ((cfg)->s2cfg.record_faults && \
> > +                                             (event).u.f_walk_eabt.s2)
> > +
> > +#define PTW_FAULT_ALLOWED(cfg, event)       (PTW_FAULT_S1(cfg, event) || \
> > +                                             PTW_FAULT_S2(cfg, event))
> The name of the macro does not really reflect what it tests. I would
> suggest PTW_RECORD_FAULT and PTW_RECORD_S1|S2_FAULT
> I would also suggest (cfg->stage == 1) ?  PTW_RECORD_S1_FAULT(cfg,
> event) :  PTW_RECORD_S2_FAULT(cfg, event)
> 
> PTW_FAULT_S1() and PTW_FAULT_S2() are not used anywhere else.
> 
> I would simplify PTW_RECORD_FAULT(cfg)  (cfg->stage == 1) ?
> (cfg)->record_faults:  (cfg)->s2cfg.record_faults

Yes, this is much simpler.
I am wondering as stage-2 only SMMUs can have stage-1 faults as described in
(IHI 0070.E.a) "3.4 Address sizes".
If the input address of a transaction exceeds the size of the IAS.
I guess this means that the fault record in this case is still controlled by S2R
although it is stage-1 fault as there is no CD or stage-1 config.

Thanks,
Mostafa