diff mbox series

[v4,16/19] hw/arm/smmuv3: Handle translation faults according to SMMUPTWEventInfo

Message ID 20240701110241.2005222-17-smostafa@google.com
State New
Headers show
Series SMMUv3 nested translation support | expand

Commit Message

Mostafa Saleh July 1, 2024, 11:02 a.m. UTC 
Previously, to check if faults are enabled, it was sufficient to check
the current stage of translation and check the corresponding
record_faults flag.

However, with nesting, it is possible for stage-1 (nested) translation
to trigger a stage-2 fault, so we check SMMUPTWEventInfo as it would
have the correct stage set from the page table walk.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 hw/arm/smmuv3.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Jean-Philippe Brucker July 4, 2024, 6:36 p.m. UTC | #1 
On Mon, Jul 01, 2024 at 11:02:38AM +0000, Mostafa Saleh wrote:
> Previously, to check if faults are enabled, it was sufficient to check
> the current stage of translation and check the corresponding
> record_faults flag.
> 
> However, with nesting, it is possible for stage-1 (nested) translation
> to trigger a stage-2 fault, so we check SMMUPTWEventInfo as it would
> have the correct stage set from the page table walk.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  hw/arm/smmuv3.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 36eb6f514a..6c18dc0acf 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -34,9 +34,10 @@
>  #include "smmuv3-internal.h"
>  #include "smmu-internal.h"
>  
> -#define PTW_RECORD_FAULT(cfg)   (((cfg)->stage == SMMU_STAGE_1) ? \
> -                                 (cfg)->record_faults : \
> -                                 (cfg)->s2cfg.record_faults)
> +#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
> +                                        (cfg)->record_faults) || \
> +                                        ((ptw_info).stage == SMMU_STAGE_2 && \
> +                                        (cfg)->s2cfg.record_faults))

I guess this could be simplified as "(info.stage == STAGE_1) ? s1cfg : s2cfg"
Anyway:

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

>  
>  /**
>   * smmuv3_trigger_irq - pulse @irq if enabled and update
> @@ -919,7 +920,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>              event->u.f_walk_eabt.addr2 = ptw_info.addr;
>              break;
>          case SMMU_PTW_ERR_TRANSLATION:
> -            if (PTW_RECORD_FAULT(cfg)) {
> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>                  event->type = SMMU_EVT_F_TRANSLATION;
>                  event->u.f_translation.addr = addr;
>                  event->u.f_translation.addr2 = ptw_info.addr;
> @@ -928,7 +929,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>              }
>              break;
>          case SMMU_PTW_ERR_ADDR_SIZE:
> -            if (PTW_RECORD_FAULT(cfg)) {
> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>                  event->type = SMMU_EVT_F_ADDR_SIZE;
>                  event->u.f_addr_size.addr = addr;
>                  event->u.f_addr_size.addr2 = ptw_info.addr;
> @@ -937,7 +938,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>              }
>              break;
>          case SMMU_PTW_ERR_ACCESS:
> -            if (PTW_RECORD_FAULT(cfg)) {
> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>                  event->type = SMMU_EVT_F_ACCESS;
>                  event->u.f_access.addr = addr;
>                  event->u.f_access.addr2 = ptw_info.addr;
> @@ -946,7 +947,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>              }
>              break;
>          case SMMU_PTW_ERR_PERMISSION:
> -            if (PTW_RECORD_FAULT(cfg)) {
> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>                  event->type = SMMU_EVT_F_PERMISSION;
>                  event->u.f_permission.addr = addr;
>                  event->u.f_permission.addr2 = ptw_info.addr;
> -- 
> 2.45.2.803.g4e1b14247a-goog
>
Eric Auger July 8, 2024, 4:30 p.m. UTC | #2 
On 7/4/24 20:36, Jean-Philippe Brucker wrote:
> On Mon, Jul 01, 2024 at 11:02:38AM +0000, Mostafa Saleh wrote:
>> Previously, to check if faults are enabled, it was sufficient to check
>> the current stage of translation and check the corresponding
>> record_faults flag.
>>
>> However, with nesting, it is possible for stage-1 (nested) translation
>> to trigger a stage-2 fault, so we check SMMUPTWEventInfo as it would
>> have the correct stage set from the page table walk.
>>
>> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>> ---
>>  hw/arm/smmuv3.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 36eb6f514a..6c18dc0acf 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -34,9 +34,10 @@
>>  #include "smmuv3-internal.h"
>>  #include "smmu-internal.h"
>>  
>> -#define PTW_RECORD_FAULT(cfg)   (((cfg)->stage == SMMU_STAGE_1) ? \
>> -                                 (cfg)->record_faults : \
>> -                                 (cfg)->s2cfg.record_faults)
>> +#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
>> +                                        (cfg)->record_faults) || \
>> +                                        ((ptw_info).stage == SMMU_STAGE_2 && \
>> +                                        (cfg)->s2cfg.record_faults))
> I guess this could be simplified as "(info.stage == STAGE_1) ? s1cfg : s2cfg"
> Anyway:
>
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
>
>>  
>>  /**
>>   * smmuv3_trigger_irq - pulse @irq if enabled and update
>> @@ -919,7 +920,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>>              event->u.f_walk_eabt.addr2 = ptw_info.addr;
>>              break;
>>          case SMMU_PTW_ERR_TRANSLATION:
>> -            if (PTW_RECORD_FAULT(cfg)) {
>> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>>                  event->type = SMMU_EVT_F_TRANSLATION;
>>                  event->u.f_translation.addr = addr;
>>                  event->u.f_translation.addr2 = ptw_info.addr;
>> @@ -928,7 +929,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>>              }
>>              break;
>>          case SMMU_PTW_ERR_ADDR_SIZE:
>> -            if (PTW_RECORD_FAULT(cfg)) {
>> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>>                  event->type = SMMU_EVT_F_ADDR_SIZE;
>>                  event->u.f_addr_size.addr = addr;
>>                  event->u.f_addr_size.addr2 = ptw_info.addr;
>> @@ -937,7 +938,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>>              }
>>              break;
>>          case SMMU_PTW_ERR_ACCESS:
>> -            if (PTW_RECORD_FAULT(cfg)) {
>> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>>                  event->type = SMMU_EVT_F_ACCESS;
>>                  event->u.f_access.addr = addr;
>>                  event->u.f_access.addr2 = ptw_info.addr;
>> @@ -946,7 +947,7 @@ static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
>>              }
>>              break;
>>          case SMMU_PTW_ERR_PERMISSION:
>> -            if (PTW_RECORD_FAULT(cfg)) {
>> +            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
>>                  event->type = SMMU_EVT_F_PERMISSION;
>>                  event->u.f_permission.addr = addr;
>>                  event->u.f_permission.addr2 = ptw_info.addr;
>> -- 
>> 2.45.2.803.g4e1b14247a-goog
>>
diff mbox series

Patch

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 36eb6f514a..6c18dc0acf 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -34,9 +34,10 @@ 
 #include "smmuv3-internal.h"
 #include "smmu-internal.h"
 
-#define PTW_RECORD_FAULT(cfg)   (((cfg)->stage == SMMU_STAGE_1) ? \
-                                 (cfg)->record_faults : \
-                                 (cfg)->s2cfg.record_faults)
+#define PTW_RECORD_FAULT(ptw_info, cfg) (((ptw_info).stage == SMMU_STAGE_1 && \
+                                        (cfg)->record_faults) || \
+                                        ((ptw_info).stage == SMMU_STAGE_2 && \
+                                        (cfg)->s2cfg.record_faults))
 
 /**
  * smmuv3_trigger_irq - pulse @irq if enabled and update
@@ -919,7 +920,7 @@  static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
             event->u.f_walk_eabt.addr2 = ptw_info.addr;
             break;
         case SMMU_PTW_ERR_TRANSLATION:
-            if (PTW_RECORD_FAULT(cfg)) {
+            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
                 event->type = SMMU_EVT_F_TRANSLATION;
                 event->u.f_translation.addr = addr;
                 event->u.f_translation.addr2 = ptw_info.addr;
@@ -928,7 +929,7 @@  static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
             }
             break;
         case SMMU_PTW_ERR_ADDR_SIZE:
-            if (PTW_RECORD_FAULT(cfg)) {
+            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
                 event->type = SMMU_EVT_F_ADDR_SIZE;
                 event->u.f_addr_size.addr = addr;
                 event->u.f_addr_size.addr2 = ptw_info.addr;
@@ -937,7 +938,7 @@  static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
             }
             break;
         case SMMU_PTW_ERR_ACCESS:
-            if (PTW_RECORD_FAULT(cfg)) {
+            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
                 event->type = SMMU_EVT_F_ACCESS;
                 event->u.f_access.addr = addr;
                 event->u.f_access.addr2 = ptw_info.addr;
@@ -946,7 +947,7 @@  static SMMUTranslationStatus smmuv3_do_translate(SMMUv3State *s, hwaddr addr,
             }
             break;
         case SMMU_PTW_ERR_PERMISSION:
-            if (PTW_RECORD_FAULT(cfg)) {
+            if (PTW_RECORD_FAULT(ptw_info, cfg)) {
                 event->type = SMMU_EVT_F_PERMISSION;
                 event->u.f_permission.addr = addr;
                 event->u.f_permission.addr2 = ptw_info.addr;