Re: [Qemu-devel] [PATCH-for-4.2 v3 5/5] hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations

[Auger Eric]

Hi Peter,

On 8/5/19 4:39 PM, Peter Maydell wrote:
> On Thu, 11 Jul 2019 at 07:19, Eric Auger <address@hidden> wrote:
>>
>> An IOVA/ASID invalidation is notified to all IOMMU Memory Regions
>> through smmuv3_inv_notifiers_iova/smmuv3_notify_iova.
>>
>> When the notification occurs it is possible that some of the
>> PCIe devices associated to the notified regions do not have a
>> valid stream table entry. In that case we output a LOG_GUEST_ERROR
>> message, for example:
>>
>> invalid sid=<SID> (L1STD span=0)
>> "smmuv3_notify_iova error decoding the configuration for iommu mr=<MR>
>>
>> This is unfortunate as the user gets the impression that there
>> are some translation decoding errors whereas there are not.
>>
>> This patch adds a new field in SMMUEventInfo that tells whether
>> the detection of an invalid STE must lead to an error report.
>> invalid_ste_allowed is set before doing the invalidations and
>> kept unset on actual translation.
>>
>> The other configuration decoding error messages are kept since if the
>> STE is valid then the rest of the config must be correct.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>> ---
>>  hw/arm/smmuv3-internal.h |  1 +
>>  hw/arm/smmuv3.c          | 15 ++++++++-------
>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index b160289cd1..d190181ef1 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -381,6 +381,7 @@ typedef struct SMMUEventInfo {
>>      uint32_t sid;
>>      bool recorded;
>>      bool record_trans_faults;
>> +    bool inval_ste_allowed;
>>      union {
>>          struct {
>>              uint32_t ssid;
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 2e270a0f07..517755aed5 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -320,7 +320,9 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>>      uint32_t config;
>>
>>      if (!STE_VALID(ste)) {
>> -        qemu_log_mask(LOG_GUEST_ERROR, "invalid STE\n");
>> +        if (!event->inval_ste_allowed) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "invalid STE\n");
>> +        }
> 
> Here the new flag check just suppresses the error log...
> 
>>          goto bad_ste;
>>      }
>>
>> @@ -405,7 +407,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, 
>> STE *ste,
>>
>>          span = L1STD_SPAN(&l1std);
>>
>> -        if (!span) {
>> +        if (!span && !event->inval_ste_allowed) {
>>              /* l2ptr is not valid */
>>              qemu_log_mask(LOG_GUEST_ERROR,
>>                            "invalid sid=%d (L1STD span=0)\n", sid);
> 
> ...but here it suppresses both the error logging and also
> the setting of event->type and returning -EINVAL. Instead
> we'll fall through to the following code. Is that really OK?
Sorry for the delay, I was off.

Yes it's wrong I should only suppress the log. Curiously it had no other
impact on the execution while testing.

Thanks

Eric
> 
> thanks
> -- PMM
>