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

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 11 Jul 2019 at 07:19, Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
> ---
>  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?

thanks
-- PMM