[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH-for-4.2 v3 5/5] hw/arm/smmuv3: Remove spurious e
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH-for-4.2 v3 5/5] hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations |
Date: |
Thu, 22 Aug 2019 17:14:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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
>