[4/4] s390x/pci: add iommu replay callback

Commit Message

Yi Min Zhao Aug. 28, 2017, 8:04 a.m. UTC

Let's introduce iommu replay callback for s390 pci iommu memory region.
Currently we don't need any dma mapping replay. So let it return
directly. This implementation will avoid meaningless loops calling
translation callback.

Reviewed-by: Pierre Morel <[e-mail]>
Reviewed-by: Halil Pasic <[e-mail]>
Signed-off-by: Yi Min Zhao <[e-mail]>
---
 hw/s390x/s390-pci-bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Cornelia Huck Aug. 28, 2017, 3:57 p.m. UTC | #1

On Mon, 28 Aug 2017 10:04:47 +0200
Yi Min Zhao <[e-mail]> wrote:

> Let's introduce iommu replay callback for s390 pci iommu memory region.
> Currently we don't need any dma mapping replay. So let it return
> directly. This implementation will avoid meaningless loops calling
> translation callback.
> 
> Reviewed-by: Pierre Morel <[e-mail]>
> Reviewed-by: Halil Pasic <[e-mail]>
> Signed-off-by: Yi Min Zhao <[e-mail]>
> ---
>  hw/s390x/s390-pci-bus.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 9e1f7ff5c5..359509ccea 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>      return ret;
>  }
>  
> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
> +                                  IOMMUNotifier *notifier)
> +{
> +    /* we don't need iommu replay currently */

This really needs to be heavier on the _why_. My guess is that anything
which would require replay goes through the rpcit instruction?

> +    return;
> +}
> +
>  static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
>                                          int devfn)
>  {
> @@ -1055,6 +1062,7 @@ static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>      IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>  
>      imrc->translate = s390_translate_iommu;
> +    imrc->replay = s390_pci_iommu_replay;
>  }
>  
>  static const TypeInfo s390_iommu_memory_region_info = {

Yi Min Zhao Aug. 29, 2017, 4:46 a.m. UTC | #2

在 2017/8/28 下午11:57, Cornelia Huck 写道:
> On Mon, 28 Aug 2017 10:04:47 +0200
> Yi Min Zhao <[e-mail]> wrote:
>
>> Let's introduce iommu replay callback for s390 pci iommu memory region.
>> Currently we don't need any dma mapping replay. So let it return
>> directly. This implementation will avoid meaningless loops calling
>> translation callback.
>>
>> Reviewed-by: Pierre Morel <[e-mail]>
>> Reviewed-by: Halil Pasic <[e-mail]>
>> Signed-off-by: Yi Min Zhao <[e-mail]>
>> ---
>>   hw/s390x/s390-pci-bus.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 9e1f7ff5c5..359509ccea 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>>       return ret;
>>   }
>>   
>> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
>> +                                  IOMMUNotifier *notifier)
>> +{
>> +    /* we don't need iommu replay currently */
> This really needs to be heavier on the _why_. My guess is that anything
> which would require replay goes through the rpcit instruction?
My understanding is:
Our arch is different from others. Each pci device has its own iommu, not
like other archs' implementation. So currently there must be no existing
mappings belonging to any newly plugged pci device whose iommu doesn't
have any mapping at the time.
In addition, it's also due to vfio blocking migration. If vfio-pci supports
migration in future, we could implement iommu replay by that time.
>
>> +    return;
>> +}
>> +
>>   static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
>>                                           int devfn)
>>   {
>> @@ -1055,6 +1062,7 @@ static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
>>       IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>>   
>>       imrc->translate = s390_translate_iommu;
>> +    imrc->replay = s390_pci_iommu_replay;
>>   }
>>   
>>   static const TypeInfo s390_iommu_memory_region_info = {
>
>

Cornelia Huck Aug. 29, 2017, 8:07 a.m. UTC | #3

[Restored cc:s. Please remember to do reply-all.]

On Tue, 29 Aug 2017 12:46:51 +0800
Yi Min Zhao <[e-mail]> wrote:

> 在 2017/8/28 下午11:57, Cornelia Huck 写道:
> > On Mon, 28 Aug 2017 10:04:47 +0200
> > Yi Min Zhao <[e-mail]> wrote:
> >
> >> Let's introduce iommu replay callback for s390 pci iommu memory region.
> >> Currently we don't need any dma mapping replay. So let it return
> >> directly. This implementation will avoid meaningless loops calling
> >> translation callback.
> >>
> >> Reviewed-by: Pierre Morel <[e-mail]>
> >> Reviewed-by: Halil Pasic <[e-mail]>
> >> Signed-off-by: Yi Min Zhao <[e-mail]>
> >> ---
> >>   hw/s390x/s390-pci-bus.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >> index 9e1f7ff5c5..359509ccea 100644
> >> --- a/hw/s390x/s390-pci-bus.c
> >> +++ b/hw/s390x/s390-pci-bus.c
> >> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
> >>       return ret;
> >>   }
> >>   
> >> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
> >> +                                  IOMMUNotifier *notifier)
> >> +{
> >> +    /* we don't need iommu replay currently */
> > This really needs to be heavier on the _why_. My guess is that anything
> > which would require replay goes through the rpcit instruction?
> My understanding is:
> Our arch is different from others. Each pci device has its own iommu, not
> like other archs' implementation. So currently there must be no existing
> mappings belonging to any newly plugged pci device whose iommu doesn't
> have any mapping at the time.

So please put that explanation into the function. (Also, "currently"?
Are we expecting it to change?)

> In addition, it's also due to vfio blocking migration. If vfio-pci supports
> migration in future, we could implement iommu replay by that time.

That's not an argument: This is the base zpci support, it should not
depend on the supported devices and what they do. (What's the status of
virtio-pci, btw? Does it work with your patches applied, or is there
still more to be done?)

Yi Min Zhao Aug. 29, 2017, 8:26 a.m. UTC | #4

在 2017/8/29 下午4:07, Cornelia Huck 写道:
> [Restored cc:s. Please remember to do reply-all.]
>
> On Tue, 29 Aug 2017 12:46:51 +0800
> Yi Min Zhao <[e-mail]> wrote:
>
>> 在 2017/8/28 下午11:57, Cornelia Huck 写道:
>>> On Mon, 28 Aug 2017 10:04:47 +0200
>>> Yi Min Zhao <[e-mail]> wrote:
>>>
>>>> Let's introduce iommu replay callback for s390 pci iommu memory region.
>>>> Currently we don't need any dma mapping replay. So let it return
>>>> directly. This implementation will avoid meaningless loops calling
>>>> translation callback.
>>>>
>>>> Reviewed-by: Pierre Morel <[e-mail]>
>>>> Reviewed-by: Halil Pasic <[e-mail]>
>>>> Signed-off-by: Yi Min Zhao <[e-mail]>
>>>> ---
>>>>    hw/s390x/s390-pci-bus.c | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>> index 9e1f7ff5c5..359509ccea 100644
>>>> --- a/hw/s390x/s390-pci-bus.c
>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>>>>        return ret;
>>>>    }
>>>>    
>>>> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
>>>> +                                  IOMMUNotifier *notifier)
>>>> +{
>>>> +    /* we don't need iommu replay currently */
>>> This really needs to be heavier on the _why_. My guess is that anything
>>> which would require replay goes through the rpcit instruction?
>> My understanding is:
>> Our arch is different from others. Each pci device has its own iommu, not
>> like other archs' implementation. So currently there must be no existing
>> mappings belonging to any newly plugged pci device whose iommu doesn't
>> have any mapping at the time.
> So please put that explanation into the function. (Also, "currently"?
> Are we expecting it to change?)
The iommu replay function is originally introduced for vfio. I think 
they want to re-build
the existing mappings because vfio has a copy of mappings in kernel. For 
our case,
the mappings would be cleanup when a pci device unplugged, and new mappings
would be created when a pci device plugged. I think replay only can 
happen during
vfio-pci device migration.
>
>> In addition, it's also due to vfio blocking migration. If vfio-pci supports
>> migration in future, we could implement iommu replay by that time.
> That's not an argument: This is the base zpci support, it should not
> depend on the supported devices and what they do. (What's the status of
> virtio-pci, btw? Does it work with your patches applied, or is there
> still more to be done?)
>
>
My understanding is virtio-pci doesn't need replay. All mappings of any 
pci device are existing in
guest memory. Once the mappings is built, all of them could be migrated 
along with the guest
system. But I might misunderstand it. Please correct me.

Cornelia Huck Aug. 29, 2017, 9:33 a.m. UTC | #5

On Tue, 29 Aug 2017 16:26:10 +0800
Yi Min Zhao <[e-mail]> wrote:

> 在 2017/8/29 下午4:07, Cornelia Huck 写道:
> > [Restored cc:s. Please remember to do reply-all.]
> >
> > On Tue, 29 Aug 2017 12:46:51 +0800
> > Yi Min Zhao <[e-mail]> wrote:
> >  
> >> 在 2017/8/28 下午11:57, Cornelia Huck 写道:  
> >>> On Mon, 28 Aug 2017 10:04:47 +0200
> >>> Yi Min Zhao <[e-mail]> wrote:
> >>>  
> >>>> Let's introduce iommu replay callback for s390 pci iommu memory region.
> >>>> Currently we don't need any dma mapping replay. So let it return
> >>>> directly. This implementation will avoid meaningless loops calling
> >>>> translation callback.
> >>>>
> >>>> Reviewed-by: Pierre Morel <[e-mail]>
> >>>> Reviewed-by: Halil Pasic <[e-mail]>
> >>>> Signed-off-by: Yi Min Zhao <[e-mail]>
> >>>> ---
> >>>>    hw/s390x/s390-pci-bus.c | 8 ++++++++
> >>>>    1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >>>> index 9e1f7ff5c5..359509ccea 100644
> >>>> --- a/hw/s390x/s390-pci-bus.c
> >>>> +++ b/hw/s390x/s390-pci-bus.c
> >>>> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
> >>>>        return ret;
> >>>>    }
> >>>>    
> >>>> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
> >>>> +                                  IOMMUNotifier *notifier)
> >>>> +{
> >>>> +    /* we don't need iommu replay currently */  
> >>> This really needs to be heavier on the _why_. My guess is that anything
> >>> which would require replay goes through the rpcit instruction?  
> >> My understanding is:
> >> Our arch is different from others. Each pci device has its own iommu, not
> >> like other archs' implementation. So currently there must be no existing
> >> mappings belonging to any newly plugged pci device whose iommu doesn't
> >> have any mapping at the time.  
> > So please put that explanation into the function. (Also, "currently"?
> > Are we expecting it to change?)  
> The iommu replay function is originally introduced for vfio. I think 
> they want to re-build
> the existing mappings because vfio has a copy of mappings in kernel. For 
> our case,
> the mappings would be cleanup when a pci device unplugged, and new mappings
> would be created when a pci device plugged. I think replay only can 
> happen during
> vfio-pci device migration.

So, the base reason is that it is impossible to plug a pci device on
s390x that already has iommu mappings which need to be replayed, which
is due to the "one iommu per zpci device" construct (and independent of
which devices need replay on other architectures)?

Then this should go into the explanation above. (And I'd still like to
know what "currently" refers to. I don't like to rely on some kind of
implicit assumptions - are we expecting this to break?)

> >  
> >> In addition, it's also due to vfio blocking migration. If vfio-pci supports
> >> migration in future, we could implement iommu replay by that time.  
> > That's not an argument: This is the base zpci support, it should not
> > depend on the supported devices and what they do. (What's the status of
> > virtio-pci, btw? Does it work with your patches applied, or is there
> > still more to be done?)
> >
> >  
> My understanding is virtio-pci doesn't need replay. All mappings of any 
> pci device are existing in
> guest memory. Once the mappings is built, all of them could be migrated 
> along with the guest
> system. But I might misunderstand it. Please correct me.

My question was whether virtio-pci works with your patches on top at
all - last time I checked on master, virtio-pci devices failed to
realize with a "msi-x is mandatory" message.

Cornelia Huck Aug. 29, 2017, 9:49 a.m. UTC | #6

On Tue, 29 Aug 2017 11:33:53 +0200
Cornelia Huck <[e-mail]> wrote:

> My question was whether virtio-pci works with your patches on top at
> all - last time I checked on master, virtio-pci devices failed to
> realize with a "msi-x is mandatory" message.

Just checked again, I still get

qemu-system-s390x: -device virtio-rng-pci: MSI-X support is mandatory in the S390 architecture

Any clue to what is missing?

Yi Min Zhao Aug. 29, 2017, 9:51 a.m. UTC | #7

在 2017/8/29 下午5:33, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 16:26:10 +0800
> Yi Min Zhao <[e-mail]> wrote:
>
>> 在 2017/8/29 下午4:07, Cornelia Huck 写道:
>>> [Restored cc:s. Please remember to do reply-all.]
>>>
>>> On Tue, 29 Aug 2017 12:46:51 +0800
>>> Yi Min Zhao <[e-mail]> wrote:
>>>   
>>>> 在 2017/8/28 下午11:57, Cornelia Huck 写道:
>>>>> On Mon, 28 Aug 2017 10:04:47 +0200
>>>>> Yi Min Zhao <[e-mail]> wrote:
>>>>>   
>>>>>> Let's introduce iommu replay callback for s390 pci iommu memory region.
>>>>>> Currently we don't need any dma mapping replay. So let it return
>>>>>> directly. This implementation will avoid meaningless loops calling
>>>>>> translation callback.
>>>>>>
>>>>>> Reviewed-by: Pierre Morel <[e-mail]>
>>>>>> Reviewed-by: Halil Pasic <[e-mail]>
>>>>>> Signed-off-by: Yi Min Zhao <[e-mail]>
>>>>>> ---
>>>>>>     hw/s390x/s390-pci-bus.c | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>>>>> index 9e1f7ff5c5..359509ccea 100644
>>>>>> --- a/hw/s390x/s390-pci-bus.c
>>>>>> +++ b/hw/s390x/s390-pci-bus.c
>>>>>> @@ -407,6 +407,13 @@ static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
>>>>>>         return ret;
>>>>>>     }
>>>>>>     
>>>>>> +static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
>>>>>> +                                  IOMMUNotifier *notifier)
>>>>>> +{
>>>>>> +    /* we don't need iommu replay currently */
>>>>> This really needs to be heavier on the _why_. My guess is that anything
>>>>> which would require replay goes through the rpcit instruction?
>>>> My understanding is:
>>>> Our arch is different from others. Each pci device has its own iommu, not
>>>> like other archs' implementation. So currently there must be no existing
>>>> mappings belonging to any newly plugged pci device whose iommu doesn't
>>>> have any mapping at the time.
>>> So please put that explanation into the function. (Also, "currently"?
>>> Are we expecting it to change?)
>> The iommu replay function is originally introduced for vfio. I think
>> they want to re-build
>> the existing mappings because vfio has a copy of mappings in kernel. For
>> our case,
>> the mappings would be cleanup when a pci device unplugged, and new mappings
>> would be created when a pci device plugged. I think replay only can
>> happen during
>> vfio-pci device migration.
> So, the base reason is that it is impossible to plug a pci device on
> s390x that already has iommu mappings which need to be replayed, which
> is due to the "one iommu per zpci device" construct (and independent of
> which devices need replay on other architectures)?
Yes.
>
> Then this should go into the explanation above. (And I'd still like to
> know what "currently" refers to. I don't like to rely on some kind of
> implicit assumptions - are we expecting this to break?)
As our discussion during internal review, we don't need to replay 
currently because vfio-pci device
doesn't support migration. 'currently' refers to the time of vfio-pci 
device migration block.
Only when vfio-pci supports migration in future, we should fill the 
replaying code.
>
>>>   
>>>> In addition, it's also due to vfio blocking migration. If vfio-pci supports
>>>> migration in future, we could implement iommu replay by that time.
>>> That's not an argument: This is the base zpci support, it should not
>>> depend on the supported devices and what they do. (What's the status of
>>> virtio-pci, btw? Does it work with your patches applied, or is there
>>> still more to be done?)
>>>
>>>   
>> My understanding is virtio-pci doesn't need replay. All mappings of any
>> pci device are existing in
>> guest memory. Once the mappings is built, all of them could be migrated
>> along with the guest
>> system. But I might misunderstand it. Please correct me.
> My question was whether virtio-pci works with your patches on top at
> all - last time I checked on master, virtio-pci devices failed to
> realize with a "msi-x is mandatory" message.
>
>
I tested. virtio-pci works fine. The message "msix is mandatory" means 
we only support
msix interrupt. If virtio-pci device (like virtio-rng) doesn't support 
msix, we don't allow it
to plug. I thinik this is not related to iommu replay.

Yi Min Zhao Aug. 29, 2017, 9:53 a.m. UTC | #8

在 2017/8/29 下午5:49, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 11:33:53 +0200
> Cornelia Huck <[e-mail]> wrote:
>
>> My question was whether virtio-pci works with your patches on top at
>> all - last time I checked on master, virtio-pci devices failed to
>> realize with a "msi-x is mandatory" message.
> Just checked again, I still get
>
> qemu-system-s390x: -device virtio-rng-pci: MSI-X support is mandatory in the S390 architecture
>
> Any clue to what is missing?
>
>
virtio-rng-pci doesn't support msix. But msix is required on s390 arch.
So we avoid plugging any pci device without msix support.

Cornelia Huck Aug. 29, 2017, 9:57 a.m. UTC | #9

On Tue, 29 Aug 2017 17:51:43 +0800
Yi Min Zhao <[e-mail]> wrote:

> 在 2017/8/29 下午5:33, Cornelia Huck 写道:

> > My question was whether virtio-pci works with your patches on top at
> > all - last time I checked on master, virtio-pci devices failed to
> > realize with a "msi-x is mandatory" message.
> >
> >  
> I tested. virtio-pci works fine. The message "msix is mandatory" means 
> we only support
> msix interrupt. If virtio-pci device (like virtio-rng) doesn't support 
> msix, we don't allow it
> to plug.

Ah, that's it (it's a bit strange that not all virtio-pci devices
support msi-x). I can add a virtio-net-pci device just fine.

(Maybe we can enhance the message so that it is clear that it refers to
that particular device?)

> I thinik this is not related to iommu replay.

This question was unrelated to this particular patch, more to the whole
series :)

Yi Min Zhao Aug. 29, 2017, 10 a.m. UTC | #10

在 2017/8/29 下午5:57, Cornelia Huck 写道:
> On Tue, 29 Aug 2017 17:51:43 +0800
> Yi Min Zhao <[e-mail]> wrote:
>
>> 在 2017/8/29 下午5:33, Cornelia Huck 写道:
>>> My question was whether virtio-pci works with your patches on top at
>>> all - last time I checked on master, virtio-pci devices failed to
>>> realize with a "msi-x is mandatory" message.
>>>
>>>   
>> I tested. virtio-pci works fine. The message "msix is mandatory" means
>> we only support
>> msix interrupt. If virtio-pci device (like virtio-rng) doesn't support
>> msix, we don't allow it
>> to plug.
> Ah, that's it (it's a bit strange that not all virtio-pci devices
> support msi-x). I can add a virtio-net-pci device just fine.
>
> (Maybe we can enhance the message so that it is clear that it refers to
> that particular device?)
Hum, I think so. But it's not urgent. I could post another patch for 
message enhancement
after this series review. Do you agree?
>
>> I thinik this is not related to iommu replay.
> This question was unrelated to this particular patch, more to the whole
> series :)
>
>
Yup.

Patch

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 9e1f7ff5c5..359509ccea 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -407,6 +407,13 @@  static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
     return ret;
 }
 
+static void s390_pci_iommu_replay(IOMMUMemoryRegion *iommu,
+                                  IOMMUNotifier *notifier)
+{
+    /* we don't need iommu replay currently */
+    return;
+}
+
 static S390PCIIOMMU *s390_pci_get_iommu(S390pciState *s, PCIBus *bus,
                                         int devfn)
 {
@@ -1055,6 +1062,7 @@  static void s390_iommu_memory_region_class_init(ObjectClass *klass, void *data)
     IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
 
     imrc->translate = s390_translate_iommu;
+    imrc->replay = s390_pci_iommu_replay;
 }
 
 static const TypeInfo s390_iommu_memory_region_info = {