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

[Eric Auger <eric.auger@redhat.com>]

Hi Mostafa,

On 5/15/23 17:37, Mostafa Saleh wrote:
> 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.
Yes this sounds sensible.

Eric
>
> Thanks,
> Mostafa
>