[Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix spurious notification errors and stall with vfio-pci

[Qemu-devel] [PATCH 0/2] ARM SMMUv3: Fix spurious notification errors and stall with vfio-pci

[Eric Auger]

The first patch fixes spurious translation configuration decoding errors
rarely happening on guest IOVA invalidation notifications.

The second patch fixes the guest stall observed when attempting to run
a guest exposed with a SMMUv3 and a VFIO-PCI device. As a reminder
SMMUv3 is not integrated with VFIO (the device will not work properly)
but this shouldn't prevent the guest from booting.

Best Regards

Eric

Eric Auger (2):
  hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations
  hw/arm/smmuv3: Implement dummy replay

 hw/arm/smmuv3-internal.h |  1 +
 hw/arm/smmuv3.c          | 17 +++++++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations

[Eric Auger]

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.

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 detction of an invalid STE msut lead to an error report.
invalid_ste_allowed is set before doing the invalidations and
kept unset on actual translation.

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

---

I also experimented to pass Error handles to all the subfunctions
and handle the Error at top level but that's intricate to sort
out the various kinds of errors, whether they need to be logged,
and if so if they match LOG_GUEST_ERRoR mask or unimplemented
mask. So I think just passing this boolean has a lesser impact on
the code base.
---
 hw/arm/smmuv3-internal.h |  1 +
 hw/arm/smmuv3.c          | 11 +++++------
 2 files changed, 6 insertions(+), 6 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 fd8ec7860e..e2f07d2864 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -404,7 +404,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);
@@ -602,7 +602,9 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
     SMMUv3State *s = sdev->smmu;
     uint32_t sid = smmu_get_sid(sdev);
-    SMMUEventInfo event = {.type = SMMU_EVT_NONE, .sid = sid};
+    SMMUEventInfo event = {.type = SMMU_EVT_NONE,
+                           .sid = sid,
+                           .inval_ste_allowed = false};
     SMMUPTWEventInfo ptw_info = {};
     SMMUTranslationStatus status;
     SMMUState *bs = ARM_SMMU(s);
@@ -795,16 +797,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
                                dma_addr_t iova)
 {
     SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
-    SMMUEventInfo event = {};
+    SMMUEventInfo event = {.inval_ste_allowed = true};
     SMMUTransTableInfo *tt;
     SMMUTransCfg *cfg;
     IOMMUTLBEntry entry;
 
     cfg = smmuv3_get_config(sdev, &event);
     if (!cfg) {
-        qemu_log_mask(LOG_GUEST_ERROR,
-                      "%s error decoding the configuration for iommu mr=%s\n",
-                      __func__, mr->parent_obj.name);
         return;
     }
 
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Eric Auger]

On ARM we currently do not support VFIO-PCI devices protected
by the IOMMU. Any attempt to run such use case results in this
kind of warning:

"-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
warning: SMMUv3 does not support notification on MAP: device vfio-pci
will not function properly".

However this is just a warning and this should not prevent the
guest from booting in a reasonable amount of time. This does not
happen currently.

This is due to the fact the VFIO vfio_listener_region_add() calls
memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
currently does not implement the replay() callback, the default
memory_region_iommu_replay() implementation is used. This latter
loops on the whole notifier's range (48b address space), translates
each page and call the notifier on the resulting entry. This totally
freezes the guest.

The Intel IOMMU implements the replay() function which only
notifies valid page table entries.

In the looming SMMUv3 nested stage VFIO integration, there will be
no need to replay() anything as there will not be any shadow page
tables: the stage 1 page tables are owned by the guest.

So let's implement a void replay() which satisfies both cases.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index e2f07d2864..1f578365ef 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1489,6 +1489,11 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
     }
 }
 
+static inline void
+smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
+{
+}
+
 static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
                                                   void *data)
 {
@@ -1496,6 +1501,7 @@ static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
 
     imrc->translate = smmuv3_translate;
     imrc->notify_flag_changed = smmuv3_notify_flag_changed;
+    imrc->replay = smmuv3_replay;
 }
 
 static const TypeInfo smmuv3_type_info = {
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations

[Peter Maydell]

On Tue, 11 Jun 2019 at 15:29, 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.
>
> 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 detction of an invalid STE msut lead to an error report.
> invalid_ste_allowed is set before doing the invalidations and
> kept unset on actual translation.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> I also experimented to pass Error handles to all the subfunctions
> and handle the Error at top level but that's intricate to sort
> out the various kinds of errors, whether they need to be logged,
> and if so if they match LOG_GUEST_ERRoR mask or unimplemented
> mask. So I think just passing this boolean has a lesser impact on
> the code base.
> ---
>  hw/arm/smmuv3-internal.h |  1 +
>  hw/arm/smmuv3.c          | 11 +++++------
>  2 files changed, 6 insertions(+), 6 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 fd8ec7860e..e2f07d2864 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -404,7 +404,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);

Why is this specific qemu_log_mask() the only one we need
to suppress ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Peter Maydell]

On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>
> On ARM we currently do not support VFIO-PCI devices protected
> by the IOMMU. Any attempt to run such use case results in this
> kind of warning:
>
> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
> warning: SMMUv3 does not support notification on MAP: device vfio-pci
> will not function properly".
>
> However this is just a warning and this should not prevent the
> guest from booting in a reasonable amount of time. This does not
> happen currently.
>
> This is due to the fact the VFIO vfio_listener_region_add() calls
> memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
> currently does not implement the replay() callback, the default
> memory_region_iommu_replay() implementation is used. This latter
> loops on the whole notifier's range (48b address space), translates
> each page and call the notifier on the resulting entry. This totally
> freezes the guest.
>
> The Intel IOMMU implements the replay() function which only
> notifies valid page table entries.
>
> In the looming SMMUv3 nested stage VFIO integration, there will be
> no need to replay() anything as there will not be any shadow page
> tables: the stage 1 page tables are owned by the guest.
>
> So let's implement a void replay() which satisfies both cases.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e2f07d2864..1f578365ef 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1489,6 +1489,11 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>      }
>  }
>
> +static inline void
> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> +{
> +}

This doesn't seem like a valid implementation of the replay
method to me. The API doc comment says
     * The default implementation of memory_region_iommu_replay() is to
     * call the IOMMU translate method for every page in the address space
     * with flag == IOMMU_NONE and then call the notifier if translate
     * returns a valid mapping. If this method is implemented then it
     * overrides the default behaviour, and must provide the full semantics
     * of memory_region_iommu_replay(), by calling @notifier for every
     * translation present in the IOMMU.

This empty function is definitely not going to call the notifier
for every IOMMU translation...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Auger Eric]

Hi Peter,

On 6/14/19 3:26 PM, Peter Maydell wrote:
> On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> On ARM we currently do not support VFIO-PCI devices protected
>> by the IOMMU. Any attempt to run such use case results in this
>> kind of warning:
>>
>> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
>> warning: SMMUv3 does not support notification on MAP: device vfio-pci
>> will not function properly".
>>
>> However this is just a warning and this should not prevent the
>> guest from booting in a reasonable amount of time. This does not
>> happen currently.
>>
>> This is due to the fact the VFIO vfio_listener_region_add() calls
>> memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
>> currently does not implement the replay() callback, the default
>> memory_region_iommu_replay() implementation is used. This latter
>> loops on the whole notifier's range (48b address space), translates
>> each page and call the notifier on the resulting entry. This totally
>> freezes the guest.
>>
>> The Intel IOMMU implements the replay() function which only
>> notifies valid page table entries.
>>
>> In the looming SMMUv3 nested stage VFIO integration, there will be
>> no need to replay() anything as there will not be any shadow page
>> tables: the stage 1 page tables are owned by the guest.
>>
>> So let's implement a void replay() which satisfies both cases.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index e2f07d2864..1f578365ef 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -1489,6 +1489,11 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>>      }
>>  }
>>
>> +static inline void
>> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>> +{
>> +}
> 
> This doesn't seem like a valid implementation of the replay
> method to me. The API doc comment says
>      * The default implementation of memory_region_iommu_replay() is to
>      * call the IOMMU translate method for every page in the address space
>      * with flag == IOMMU_NONE and then call the notifier if translate
>      * returns a valid mapping. If this method is implemented then it
>      * overrides the default behaviour, and must provide the full semantics
>      * of memory_region_iommu_replay(), by calling @notifier for every
>      * translation present in the IOMMU.
> 
> This empty function is definitely not going to call the notifier
> for every IOMMU translation...
The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
devices will not work anyway (we are not able to notify on MAP). There
is a warning already reporting the issue. However the default
implementation of memory_region_iommu_replay() prevents the guest from
booting. So what would you advise?

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Peter Maydell]

On Fri, 14 Jun 2019 at 14:40, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 6/14/19 3:26 PM, Peter Maydell wrote:
> > On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
> >>
> >> On ARM we currently do not support VFIO-PCI devices protected
> >> by the IOMMU. Any attempt to run such use case results in this
> >> kind of warning:
> >>
> >> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
> >> warning: SMMUv3 does not support notification on MAP: device vfio-pci
> >> will not function properly".

> >> +static inline void
> >> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> >> +{
> >> +}
> >
> > This doesn't seem like a valid implementation of the replay
> > method to me. The API doc comment says
> >      * The default implementation of memory_region_iommu_replay() is to
> >      * call the IOMMU translate method for every page in the address space
> >      * with flag == IOMMU_NONE and then call the notifier if translate
> >      * returns a valid mapping. If this method is implemented then it
> >      * overrides the default behaviour, and must provide the full semantics
> >      * of memory_region_iommu_replay(), by calling @notifier for every
> >      * translation present in the IOMMU.
> >
> > This empty function is definitely not going to call the notifier
> > for every IOMMU translation...
> The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
> devices will not work anyway (we are not able to notify on MAP). There
> is a warning already reporting the issue. However the default
> implementation of memory_region_iommu_replay() prevents the guest from
> booting. So what would you advise?

I dunno, but if the API isn't supposed to behave the way we've
documented it to, we should fix the documentation... Since
the only user of memory_region_iommu_replay() is the vfio code
I guess we can define it however is most convenient for vfio,
but we should document what the method has to do to make things
work.

PS: we have a memory_region_iommu_replay_all() which currently
appears to be not used by anybody, could we get rid of it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] hw/arm/smmuv3: Remove spurious error messages on IOVA invalidations

[Auger Eric]

Hi Peter,

On 6/14/19 3:23 PM, Peter Maydell wrote:
> On Tue, 11 Jun 2019 at 15:29, 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.
>>
>> 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 detction of an invalid STE msut lead to an error report.
>> invalid_ste_allowed is set before doing the invalidations and
>> kept unset on actual translation.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> I also experimented to pass Error handles to all the subfunctions
>> and handle the Error at top level but that's intricate to sort
>> out the various kinds of errors, whether they need to be logged,
>> and if so if they match LOG_GUEST_ERRoR mask or unimplemented
>> mask. So I think just passing this boolean has a lesser impact on
>> the code base.
>> ---
>>  hw/arm/smmuv3-internal.h |  1 +
>>  hw/arm/smmuv3.c          | 11 +++++------
>>  2 files changed, 6 insertions(+), 6 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 fd8ec7860e..e2f07d2864 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -404,7 +404,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);
> 
> Why is this specific qemu_log_mask() the only one we need
> to suppress ?

I focused on messages related to STE invalidity. This one corresponds to
the 2-level stream table case where the top-level entry is invalid.

In linear stream table case, the STE is fetched in smmu_get_ste(): this
shouldn't fail as the STE table was allocated.  Then its validity is
checked in decode_ste() but up to now there is no tracing if the STE in
not valid. I should revise this in the future though.

Then obviously the config decoding is not over but I considered that if
the STE is valid, the rest of the config should be valid as well. If we
were to detect any error afterwards, those would deserve to be reported.

Thanks

Eric

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/smmuv3: Implement dummy replay

[Auger Eric]

Hi Peter,

On 6/14/19 3:45 PM, Peter Maydell wrote:
> On Fri, 14 Jun 2019 at 14:40, Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 6/14/19 3:26 PM, Peter Maydell wrote:
>>> On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>>>>
>>>> On ARM we currently do not support VFIO-PCI devices protected
>>>> by the IOMMU. Any attempt to run such use case results in this
>>>> kind of warning:
>>>>
>>>> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
>>>> warning: SMMUv3 does not support notification on MAP: device vfio-pci
>>>> will not function properly".
> 
>>>> +static inline void
>>>> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>>>> +{
>>>> +}
>>>
>>> This doesn't seem like a valid implementation of the replay
>>> method to me. The API doc comment says
>>>      * The default implementation of memory_region_iommu_replay() is to
>>>      * call the IOMMU translate method for every page in the address space
>>>      * with flag == IOMMU_NONE and then call the notifier if translate
>>>      * returns a valid mapping. If this method is implemented then it
>>>      * overrides the default behaviour, and must provide the full semantics
>>>      * of memory_region_iommu_replay(), by calling @notifier for every
>>>      * translation present in the IOMMU.
>>>
>>> This empty function is definitely not going to call the notifier
>>> for every IOMMU translation...
>> The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
>> devices will not work anyway (we are not able to notify on MAP). There
>> is a warning already reporting the issue. However the default
>> implementation of memory_region_iommu_replay() prevents the guest from
>> booting. So what would you advise?
> 
> I dunno, but if the API isn't supposed to behave the way we've
> documented it to, we should fix the documentation...

fair enough

 Since
> the only user of memory_region_iommu_replay() is the vfio code
> I guess we can define it however is most convenient for vfio,
> but we should document what the method has to do to make things
> work.

OK I need to think about it. Maybe an alternative is to call
memory_region_iommu_replay() only when it makes sense.
> 
> PS: we have a memory_region_iommu_replay_all() which currently
> appears to be not used by anybody, could we get rid of it?

I will do that

Thanks

Eric
> 
> thanks
> -- PMM
>