[Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

vfio-ap devices do not pin any pages in the host. Therefore, they
are belived to be compatible with memory ballooning.

Flag them as compatible, so both vfio-ap and a balloon can be
used simultaneously.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

As briefly discussed on IRC. RFC as I do not have easy access to
hardware I can test this with.

---
 hw/vfio/ap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 65de952f44..3bf48eed28 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
     vapdev->vdev.name = g_strdup_printf("%s", mdevid);
     vapdev->vdev.dev = dev;
 
+    /*
+     * vfio-ap devices are believed to operate in a way compatible with
+     * memory ballooning, as no pages are pinned in the host.
+     * This needs to be set before vfio_get_device() for vfio common to
+     * handle the balloon inhibitor.
+     */
+    vapdev->vdev.balloon_allowed = true;
+
     ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
     if (ret) {
         goto out_get_dev_err;
-- 
2.17.2

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Christian Borntraeger]

On 05.12.2018 15:51, Cornelia Huck wrote:
> vfio-ap devices do not pin any pages in the host. Therefore, they
> are belived to be compatible with memory ballooning.
> 
> Flag them as compatible, so both vfio-ap and a balloon can be
> used simultaneously.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> As briefly discussed on IRC. RFC as I do not have easy access to
> hardware I can test this with.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

vfio-ap is based on the SIE support for AP, which transparently handles host paging.
S390_AP_IOMMU has an empty iommu_ops set and the hardware can tolerate invalid page
table entries.

So this makes sense to me.

> 
> ---
>  hw/vfio/ap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952f44..3bf48eed28 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>      vapdev->vdev.dev = dev;
>  
> +    /*
> +     * vfio-ap devices are believed to operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */
> +    vapdev->vdev.balloon_allowed = true;
> +
>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>      if (ret) {
>          goto out_get_dev_err;
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[David Hildenbrand]

On 05.12.18 15:51, Cornelia Huck wrote:
> vfio-ap devices do not pin any pages in the host. Therefore, they
> are belived to be compatible with memory ballooning.
> 
> Flag them as compatible, so both vfio-ap and a balloon can be
> used simultaneously.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> As briefly discussed on IRC. RFC as I do not have easy access to
> hardware I can test this with.
> 
> ---
>  hw/vfio/ap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952f44..3bf48eed28 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>      vapdev->vdev.dev = dev;
>  
> +    /*
> +     * vfio-ap devices are believed to operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */
> +    vapdev->vdev.balloon_allowed = true;
> +
>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>      if (ret) {
>          goto out_get_dev_err;
> 

What happens if this ever changes? Shouldn't we have an API to at least
check what the vfio device can guarantee?

"are believed to operate" doesn't sound like guarantees to me :)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Wed, 5 Dec 2018 17:38:22 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 05.12.18 15:51, Cornelia Huck wrote:
> > vfio-ap devices do not pin any pages in the host. Therefore, they
> > are belived to be compatible with memory ballooning.
> > 
> > Flag them as compatible, so both vfio-ap and a balloon can be
> > used simultaneously.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > As briefly discussed on IRC. RFC as I do not have easy access to
> > hardware I can test this with.
> > 
> > ---
> >  hw/vfio/ap.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> > index 65de952f44..3bf48eed28 100644
> > --- a/hw/vfio/ap.c
> > +++ b/hw/vfio/ap.c
> > @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
> >      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> >      vapdev->vdev.dev = dev;
> >  
> > +    /*
> > +     * vfio-ap devices are believed to operate in a way compatible with
> > +     * memory ballooning, as no pages are pinned in the host.
> > +     * This needs to be set before vfio_get_device() for vfio common to
> > +     * handle the balloon inhibitor.
> > +     */
> > +    vapdev->vdev.balloon_allowed = true;
> > +
> >      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
> >      if (ret) {
> >          goto out_get_dev_err;
> >   
> 
> What happens if this ever changes? Shouldn't we have an API to at least
> check what the vfio device can guarantee?
> 
> "are believed to operate" doesn't sound like guarantees to me :)

It's the same for ccw :)

While such an API definitely sounds like a good idea, it is probably
overkill to introduce it for this case (do we envision changing the way
vfio-ap operates in the future to make that statement non-true?)

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Christian Borntraeger]

On 05.12.2018 17:45, Cornelia Huck wrote:
> On Wed, 5 Dec 2018 17:38:22 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 05.12.18 15:51, Cornelia Huck wrote:
>>> vfio-ap devices do not pin any pages in the host. Therefore, they
>>> are belived to be compatible with memory ballooning.
>>>
>>> Flag them as compatible, so both vfio-ap and a balloon can be
>>> used simultaneously.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> As briefly discussed on IRC. RFC as I do not have easy access to
>>> hardware I can test this with.
>>>
>>> ---
>>>  hw/vfio/ap.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>> index 65de952f44..3bf48eed28 100644
>>> --- a/hw/vfio/ap.c
>>> +++ b/hw/vfio/ap.c
>>> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>>>      vapdev->vdev.dev = dev;
>>>  
>>> +    /*
>>> +     * vfio-ap devices are believed to operate in a way compatible with
>>> +     * memory ballooning, as no pages are pinned in the host.
>>> +     * This needs to be set before vfio_get_device() for vfio common to
>>> +     * handle the balloon inhibitor.
>>> +     */
>>> +    vapdev->vdev.balloon_allowed = true;
>>> +
>>>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>>>      if (ret) {
>>>          goto out_get_dev_err;
>>>   
>>
>> What happens if this ever changes? Shouldn't we have an API to at least
>> check what the vfio device can guarantee?
>>
>> "are believed to operate" doesn't sound like guarantees to me :)

I would actually remove that comment or fix it. We either know or we dont.
In the way vfio-works I see no reason to disallow balloon. Even if the guest does
something wrong (e.g. crypto I/O on freed pages) the host would handle that the
same as it would for normal page accesses. From a host point of view the crypto
instructions are just CISC instructions with load/store semantics.

> 
> It's the same for ccw :)
> 
> While such an API definitely sounds like a good idea, it is probably
> overkill to introduce it for this case (do we envision changing the way
> vfio-ap operates in the future to make that statement non-true?)

agreed. 
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[David Hildenbrand]

On 05.12.18 18:25, Christian Borntraeger wrote:
> 
> 
> On 05.12.2018 17:45, Cornelia Huck wrote:
>> On Wed, 5 Dec 2018 17:38:22 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 05.12.18 15:51, Cornelia Huck wrote:
>>>> vfio-ap devices do not pin any pages in the host. Therefore, they
>>>> are belived to be compatible with memory ballooning.
>>>>
>>>> Flag them as compatible, so both vfio-ap and a balloon can be
>>>> used simultaneously.
>>>>
>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>> ---
>>>>
>>>> As briefly discussed on IRC. RFC as I do not have easy access to
>>>> hardware I can test this with.
>>>>
>>>> ---
>>>>  hw/vfio/ap.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>> index 65de952f44..3bf48eed28 100644
>>>> --- a/hw/vfio/ap.c
>>>> +++ b/hw/vfio/ap.c
>>>> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>>>>      vapdev->vdev.dev = dev;
>>>>  
>>>> +    /*
>>>> +     * vfio-ap devices are believed to operate in a way compatible with
>>>> +     * memory ballooning, as no pages are pinned in the host.
>>>> +     * This needs to be set before vfio_get_device() for vfio common to
>>>> +     * handle the balloon inhibitor.
>>>> +     */
>>>> +    vapdev->vdev.balloon_allowed = true;
>>>> +
>>>>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>>>>      if (ret) {
>>>>          goto out_get_dev_err;
>>>>   
>>>
>>> What happens if this ever changes? Shouldn't we have an API to at least
>>> check what the vfio device can guarantee?
>>>
>>> "are believed to operate" doesn't sound like guarantees to me :)
> 
> I would actually remove that comment or fix it. We either know or we dont.
> In the way vfio-works I see no reason to disallow balloon. Even if the guest does
> something wrong (e.g. crypto I/O on freed pages) the host would handle that the
> same as it would for normal page accesses. From a host point of view the crypto
> instructions are just CISC instructions with load/store semantics.

As long as vfio-ap does not and will never pin pages (and keep them
pinned), we are fine. I don't know about the details, but if vfio-ap
really just issues a synchronous instruction for us, we are fine.

> 
>>
>> It's the same for ccw :)
>>
>> While such an API definitely sounds like a good idea, it is probably
>> overkill to introduce it for this case (do we envision changing the way
>> vfio-ap operates in the future to make that statement non-true?)
> 
> agreed. 
>>
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Halil Pasic]

On Thu, 6 Dec 2018 09:28:34 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 05.12.18 18:25, Christian Borntraeger wrote:
> > 
> > 
> > On 05.12.2018 17:45, Cornelia Huck wrote:
> >> On Wed, 5 Dec 2018 17:38:22 +0100
> >> David Hildenbrand <david@redhat.com> wrote:
> >>
> >>> On 05.12.18 15:51, Cornelia Huck wrote:
> >>>> vfio-ap devices do not pin any pages in the host. Therefore, they
> >>>> are belived to be compatible with memory ballooning.
> >>>>
> >>>> Flag them as compatible, so both vfio-ap and a balloon can be
> >>>> used simultaneously.
> >>>>
> >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>>> ---
> >>>>
> >>>> As briefly discussed on IRC. RFC as I do not have easy access to
> >>>> hardware I can test this with.
> >>>>
> >>>> ---
> >>>>  hw/vfio/ap.c | 8 ++++++++
> >>>>  1 file changed, 8 insertions(+)
> >>>>
> >>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> >>>> index 65de952f44..3bf48eed28 100644
> >>>> --- a/hw/vfio/ap.c
> >>>> +++ b/hw/vfio/ap.c
> >>>> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
> >>>>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> >>>>      vapdev->vdev.dev = dev;
> >>>>  
> >>>> +    /*
> >>>> +     * vfio-ap devices are believed to operate in a way compatible with
> >>>> +     * memory ballooning, as no pages are pinned in the host.
> >>>> +     * This needs to be set before vfio_get_device() for vfio common to
> >>>> +     * handle the balloon inhibitor.
> >>>> +     */
> >>>> +    vapdev->vdev.balloon_allowed = true;
> >>>> +
> >>>>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
> >>>>      if (ret) {
> >>>>          goto out_get_dev_err;
> >>>>   
> >>>
> >>> What happens if this ever changes? Shouldn't we have an API to at least
> >>> check what the vfio device can guarantee?
> >>>
> >>> "are believed to operate" doesn't sound like guarantees to me :)
> > 
> > I would actually remove that comment or fix it. We either know or we dont.
> > In the way vfio-works I see no reason to disallow balloon. Even if the guest does
> > something wrong (e.g. crypto I/O on freed pages) the host would handle that the
> > same as it would for normal page accesses. From a host point of view the crypto
> > instructions are just CISC instructions with load/store semantics.
> 
> As long as vfio-ap does not and will never pin pages (and keep them
> pinned), we are fine. I don't know about the details, but if vfio-ap
> really just issues a synchronous instruction for us, we are fine.
> 

I agree with Christian. That comment is best removed.

@Tony, I guess you should have the most elaborate test setup. Can you give
this some testing just in case?

> > 
> >>
> >> It's the same for ccw :)

As a matter of fact, I don't like that comment.

Regards,
Halil

> >>
> >> While such an API definitely sounds like a good idea, it is probably
> >> overkill to introduce it for this case (do we envision changing the way
> >> vfio-ap operates in the future to make that statement non-true?)
> > 
> > agreed. 
> >>
> > 
> 
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Thu, 6 Dec 2018 13:32:39 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 6 Dec 2018 09:28:34 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
> > On 05.12.18 18:25, Christian Borntraeger wrote:  
> > > 
> > > 
> > > On 05.12.2018 17:45, Cornelia Huck wrote:  
> > >> On Wed, 5 Dec 2018 17:38:22 +0100
> > >> David Hildenbrand <david@redhat.com> wrote:
> > >>  
> > >>> On 05.12.18 15:51, Cornelia Huck wrote:  
> > >>>> vfio-ap devices do not pin any pages in the host. Therefore, they
> > >>>> are belived to be compatible with memory ballooning.
> > >>>>
> > >>>> Flag them as compatible, so both vfio-ap and a balloon can be
> > >>>> used simultaneously.
> > >>>>
> > >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > >>>> ---
> > >>>>
> > >>>> As briefly discussed on IRC. RFC as I do not have easy access to
> > >>>> hardware I can test this with.
> > >>>>
> > >>>> ---
> > >>>>  hw/vfio/ap.c | 8 ++++++++
> > >>>>  1 file changed, 8 insertions(+)
> > >>>>
> > >>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> > >>>> index 65de952f44..3bf48eed28 100644
> > >>>> --- a/hw/vfio/ap.c
> > >>>> +++ b/hw/vfio/ap.c
> > >>>> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
> > >>>>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
> > >>>>      vapdev->vdev.dev = dev;
> > >>>>  
> > >>>> +    /*
> > >>>> +     * vfio-ap devices are believed to operate in a way compatible with
> > >>>> +     * memory ballooning, as no pages are pinned in the host.
> > >>>> +     * This needs to be set before vfio_get_device() for vfio common to
> > >>>> +     * handle the balloon inhibitor.
> > >>>> +     */
> > >>>> +    vapdev->vdev.balloon_allowed = true;
> > >>>> +
> > >>>>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
> > >>>>      if (ret) {
> > >>>>          goto out_get_dev_err;
> > >>>>     
> > >>>
> > >>> What happens if this ever changes? Shouldn't we have an API to at least
> > >>> check what the vfio device can guarantee?
> > >>>
> > >>> "are believed to operate" doesn't sound like guarantees to me :)  
> > > 
> > > I would actually remove that comment or fix it. We either know or we dont.
> > > In the way vfio-works I see no reason to disallow balloon. Even if the guest does
> > > something wrong (e.g. crypto I/O on freed pages) the host would handle that the
> > > same as it would for normal page accesses. From a host point of view the crypto
> > > instructions are just CISC instructions with load/store semantics.  
> > 
> > As long as vfio-ap does not and will never pin pages (and keep them
> > pinned), we are fine. I don't know about the details, but if vfio-ap
> > really just issues a synchronous instruction for us, we are fine.
> >   
> 
> I agree with Christian. That comment is best removed.

What about s/believed to operate/operate/?

The second part of the comment is still useful, I believe.

> 
> @Tony, I guess you should have the most elaborate test setup. Can you give
> this some testing just in case?

Actual testing would be great :)

> 
> > >   
> > >>
> > >> It's the same for ccw :)  
> 
> As a matter of fact, I don't like that comment.

Do you have a suggestion for rewording it?

> 
> Regards,
> Halil
> 
> > >>
> > >> While such an API definitely sounds like a good idea, it is probably
> > >> overkill to introduce it for this case (do we envision changing the way
> > >> vfio-ap operates in the future to make that statement non-true?)  
> > > 
> > > agreed.   
> > >>  
> > >   
> > 
> >   
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Tony Krowiak]

On 12/6/18 7:48 AM, Cornelia Huck wrote:
> On Thu, 6 Dec 2018 13:32:39 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
>> On Thu, 6 Dec 2018 09:28:34 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 05.12.18 18:25, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 05.12.2018 17:45, Cornelia Huck wrote:
>>>>> On Wed, 5 Dec 2018 17:38:22 +0100
>>>>> David Hildenbrand <david@redhat.com> wrote:
>>>>>   
>>>>>> On 05.12.18 15:51, Cornelia Huck wrote:
>>>>>>> vfio-ap devices do not pin any pages in the host. Therefore, they
>>>>>>> are belived to be compatible with memory ballooning.
>>>>>>>
>>>>>>> Flag them as compatible, so both vfio-ap and a balloon can be
>>>>>>> used simultaneously.
>>>>>>>
>>>>>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>>>>>> ---
>>>>>>>
>>>>>>> As briefly discussed on IRC. RFC as I do not have easy access to
>>>>>>> hardware I can test this with.
>>>>>>>
>>>>>>> ---
>>>>>>>   hw/vfio/ap.c | 8 ++++++++
>>>>>>>   1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
>>>>>>> index 65de952f44..3bf48eed28 100644
>>>>>>> --- a/hw/vfio/ap.c
>>>>>>> +++ b/hw/vfio/ap.c
>>>>>>> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>>>>>>>       vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>>>>>>>       vapdev->vdev.dev = dev;
>>>>>>>   
>>>>>>> +    /*
>>>>>>> +     * vfio-ap devices are believed to operate in a way compatible with
>>>>>>> +     * memory ballooning, as no pages are pinned in the host.
>>>>>>> +     * This needs to be set before vfio_get_device() for vfio common to
>>>>>>> +     * handle the balloon inhibitor.
>>>>>>> +     */
>>>>>>> +    vapdev->vdev.balloon_allowed = true;
>>>>>>> +
>>>>>>>       ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>>>>>>>       if (ret) {
>>>>>>>           goto out_get_dev_err;
>>>>>>>      
>>>>>>
>>>>>> What happens if this ever changes? Shouldn't we have an API to at least
>>>>>> check what the vfio device can guarantee?
>>>>>>
>>>>>> "are believed to operate" doesn't sound like guarantees to me :)
>>>>
>>>> I would actually remove that comment or fix it. We either know or we dont.
>>>> In the way vfio-works I see no reason to disallow balloon. Even if the guest does
>>>> something wrong (e.g. crypto I/O on freed pages) the host would handle that the
>>>> same as it would for normal page accesses. From a host point of view the crypto
>>>> instructions are just CISC instructions with load/store semantics.
>>>
>>> As long as vfio-ap does not and will never pin pages (and keep them
>>> pinned), we are fine. I don't know about the details, but if vfio-ap
>>> really just issues a synchronous instruction for us, we are fine.
>>>    
>>
>> I agree with Christian. That comment is best removed.
> 
> What about s/believed to operate/operate/?
> 
> The second part of the comment is still useful, I believe.
> 
>>
>> @Tony, I guess you should have the most elaborate test setup. Can you give
>> this some testing just in case?
> 
> Actual testing would be great :)

Will do.

> 
>>
>>>>    
>>>>>
>>>>> It's the same for ccw :)
>>
>> As a matter of fact, I don't like that comment.
> 
> Do you have a suggestion for rewording it?
> 
>>
>> Regards,
>> Halil
>>
>>>>>
>>>>> While such an API definitely sounds like a good idea, it is probably
>>>>> overkill to introduce it for this case (do we envision changing the way
>>>>> vfio-ap operates in the future to make that statement non-true?)
>>>>
>>>> agreed.
>>>>>   
>>>>    
>>>
>>>    
>>
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Tony Krowiak]

On 12/5/18 9:51 AM, Cornelia Huck wrote:
> vfio-ap devices do not pin any pages in the host. Therefore, they
> are belived to be compatible with memory ballooning.
> 
> Flag them as compatible, so both vfio-ap and a balloon can be
> used simultaneously.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> As briefly discussed on IRC. RFC as I do not have easy access to
> hardware I can test this with.
> 
> ---
>   hw/vfio/ap.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952f44..3bf48eed28 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>       vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>       vapdev->vdev.dev = dev;
>   
> +    /*
> +     * vfio-ap devices are believed to operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */
> +    vapdev->vdev.balloon_allowed = true;
> +
>       ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>       if (ret) {
>           goto out_get_dev_err;

Tested-by: Tony Krowiak <akrowiak@linux.ibm.com>

> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Christian Borntraeger]

On 05.12.2018 15:51, Cornelia Huck wrote:
> vfio-ap devices do not pin any pages in the host. Therefore, they
> are belived to be compatible with memory ballooning.
> 
> Flag them as compatible, so both vfio-ap and a balloon can be
> used simultaneously.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Does it make sense to add cc stable for 3.1?


> ---
> 
> As briefly discussed on IRC. RFC as I do not have easy access to
> hardware I can test this with.
> 
> ---
>  hw/vfio/ap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952f44..3bf48eed28 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>      vapdev->vdev.dev = dev;
>  
> +    /*
> +     * vfio-ap devices are believed to operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */
> +    vapdev->vdev.balloon_allowed = true;
> +
>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>      if (ret) {
>          goto out_get_dev_err;
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Fri, 7 Dec 2018 13:17:02 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 05.12.2018 15:51, Cornelia Huck wrote:
> > vfio-ap devices do not pin any pages in the host. Therefore, they
> > are belived to be compatible with memory ballooning.
> > 
> > Flag them as compatible, so both vfio-ap and a balloon can be
> > used simultaneously.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> 
> Does it make sense to add cc stable for 3.1?

Can do that, given that s390x systems really rely on the ballooner in
general.

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Christian Borntraeger]

On 07.12.2018 13:29, Cornelia Huck wrote:
> On Fri, 7 Dec 2018 13:17:02 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 05.12.2018 15:51, Cornelia Huck wrote:
>>> vfio-ap devices do not pin any pages in the host. Therefore, they
>>> are belived to be compatible with memory ballooning.
>>>
>>> Flag them as compatible, so both vfio-ap and a balloon can be
>>> used simultaneously.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
>>
>> Does it make sense to add cc stable for 3.1?
> 
> Can do that, given that s390x systems really rely on the ballooner in
> general.

Well, not relying is the wrong word. It is just strange that things like

  <memory unit='GiB'>2</memory>
  <currentMemory unit='GiB'>1</currentMemory>

do work 
but suddenly no longer work when an AP device is added.

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Fri, 7 Dec 2018 13:32:14 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07.12.2018 13:29, Cornelia Huck wrote:
> > On Fri, 7 Dec 2018 13:17:02 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 05.12.2018 15:51, Cornelia Huck wrote:  
> >>> vfio-ap devices do not pin any pages in the host. Therefore, they
> >>> are belived to be compatible with memory ballooning.
> >>>
> >>> Flag them as compatible, so both vfio-ap and a balloon can be
> >>> used simultaneously.
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>    
> >>
> >> Does it make sense to add cc stable for 3.1?  
> > 
> > Can do that, given that s390x systems really rely on the ballooner in
> > general.  
> 
> Well, not relying is the wrong word. It is just strange that things like
> 
>   <memory unit='GiB'>2</memory>
>   <currentMemory unit='GiB'>1</currentMemory>
> 
> do work 
> but suddenly no longer work when an AP device is added.
> 

Let me rephrase that: Current setups rely on the functionality, and we
don't want to break them by adding ap :)

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Halil Pasic]

On Fri, 7 Dec 2018 13:29:46 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 7 Dec 2018 13:17:02 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > On 05.12.2018 15:51, Cornelia Huck wrote:
> > > vfio-ap devices do not pin any pages in the host. Therefore, they
> > > are belived to be compatible with memory ballooning.
> > > 
> > > Flag them as compatible, so both vfio-ap and a balloon can be
> > > used simultaneously.
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  

With the comment stuff sorted out:
Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 

@Connie: Just had a look at the MAINTAINERS file and hw/vfio/ap.c
is listed under Arch. support S90 with you as a maintainer, and under
vfio-ap with 4 maintainers listed one of them being me. The question
is who is going to post a PULL request for this?

> > 
> > Does it make sense to add cc stable for 3.1?
> 
> Can do that, given that s390x systems really rely on the ballooner in
> general.
> 

I agree with cc stable.

Halil

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Fri, 7 Dec 2018 13:52:53 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 7 Dec 2018 13:29:46 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 7 Dec 2018 13:17:02 +0100
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> > > On 05.12.2018 15:51, Cornelia Huck wrote:  
> > > > vfio-ap devices do not pin any pages in the host. Therefore, they
> > > > are belived to be compatible with memory ballooning.
> > > > 
> > > > Flag them as compatible, so both vfio-ap and a balloon can be
> > > > used simultaneously.
> > > > 
> > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>    
> 
> With the comment stuff sorted out:
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 

So, do you agree with the comment change I suggested?

+    /*
+     * vfio-ap devices operate in a way compatible with
+     * memory ballooning, as no pages are pinned in the host.
+     * This needs to be set before vfio_get_device() for vfio common to
+     * handle the balloon inhibitor.
+     */

> 
> @Connie: Just had a look at the MAINTAINERS file and hw/vfio/ap.c
> is listed under Arch. support S90 with you as a maintainer, and under
> vfio-ap with 4 maintainers listed one of them being me. The question
> is who is going to post a PULL request for this?

General practice has been that I'm collecting everything s390x related.
I have also pulled from others before (e.g. some bios changes from
Thomas). While you could apply the patch, send it to me, and then I'd
queue it to s390-next, I can also simply queue it directly with your
ack :)

[Longer term, if you want to collect ap patches and then send me a pull
request, I would also be happy to do that. For this single patch, it
seems overkill.]

> 
> > > 
> > > Does it make sense to add cc stable for 3.1?  
> > 
> > Can do that, given that s390x systems really rely on the ballooner in
> > general.
> >   
> 
> I agree with cc stable.

Will add when applying.

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Halil Pasic]

On Fri, 7 Dec 2018 14:17:20 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, 7 Dec 2018 13:52:53 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Fri, 7 Dec 2018 13:29:46 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Fri, 7 Dec 2018 13:17:02 +0100
> > > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > >   
> > > > On 05.12.2018 15:51, Cornelia Huck wrote:  
> > > > > vfio-ap devices do not pin any pages in the host. Therefore, they
> > > > > are belived to be compatible with memory ballooning.
> > > > > 
> > > > > Flag them as compatible, so both vfio-ap and a balloon can be
> > > > > used simultaneously.
> > > > > 
> > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>    
> > 
> > With the comment stuff sorted out:
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 
> 
> So, do you agree with the comment change I suggested?
> 
> +    /*
> +     * vfio-ap devices operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */

I did some digging because my understanding of the problem was
completely insufficient -- now it is just plain insufficient. If I
understood it correctly the crux here seems to be that under certain
circumstances (IOMMU type, presence/absence of domain) vfio locks on
VFIO_IOMMU_MAP, and that vfio_get_group() basically maps it's address
space argument. But for s390x this pinning does not happen.

I mean vfio-ccw does pin pages in the host and is still safe. BTW do
we want to change the message for vfio-cccw?

I intend do some more digging and should I come to some remotely
satisfactory result, I intend to post a short write-up of my
findings here.

I agree to this patch with that commit message despite not having the
clarity, because not having it seems way worse than having it.


> 
> > 
> > @Connie: Just had a look at the MAINTAINERS file and hw/vfio/ap.c
> > is listed under Arch. support S90 with you as a maintainer, and under
> > vfio-ap with 4 maintainers listed one of them being me. The question
> > is who is going to post a PULL request for this?
> 
> General practice has been that I'm collecting everything s390x related.
> I have also pulled from others before (e.g. some bios changes from
> Thomas). While you could apply the patch, send it to me, and then I'd
> queue it to s390-next, I can also simply queue it directly with your
> ack :)
> 
> [Longer term, if you want to collect ap patches and then send me a pull
> request, I would also be happy to do that. For this single patch, it
> seems overkill.]
> 

I agree, it would be an overkill. I guess r-b qualifies as ack.

Regards,
Halil

> > 
> > > > 
> > > > Does it make sense to add cc stable for 3.1?  
> > > 
> > > Can do that, given that s390x systems really rely on the ballooner in
> > > general.
> > >   
> > 
> > I agree with cc stable.
> 
> Will add when applying.
> 

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Fri, 7 Dec 2018 15:04:14 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 7 Dec 2018 14:17:20 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 7 Dec 2018 13:52:53 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Fri, 7 Dec 2018 13:29:46 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >   
> > > > On Fri, 7 Dec 2018 13:17:02 +0100
> > > > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > > >     
> > > > > On 05.12.2018 15:51, Cornelia Huck wrote:    
> > > > > > vfio-ap devices do not pin any pages in the host. Therefore, they
> > > > > > are belived to be compatible with memory ballooning.
> > > > > > 
> > > > > > Flag them as compatible, so both vfio-ap and a balloon can be
> > > > > > used simultaneously.
> > > > > > 
> > > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>      
> > > 
> > > With the comment stuff sorted out:
> > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>   
> > 
> > So, do you agree with the comment change I suggested?
> > 
> > +    /*
> > +     * vfio-ap devices operate in a way compatible with
> > +     * memory ballooning, as no pages are pinned in the host.
> > +     * This needs to be set before vfio_get_device() for vfio common to
> > +     * handle the balloon inhibitor.
> > +     */  
> 
> I did some digging because my understanding of the problem was
> completely insufficient -- now it is just plain insufficient. If I
> understood it correctly the crux here seems to be that under certain
> circumstances (IOMMU type, presence/absence of domain) vfio locks on
> VFIO_IOMMU_MAP, and that vfio_get_group() basically maps it's address
> space argument. But for s390x this pinning does not happen.
> 
> I mean vfio-ccw does pin pages in the host and is still safe. BTW do
> we want to change the message for vfio-cccw?

If you have a better wording, feel free to do so :)

> I intend do some more digging and should I come to some remotely
> satisfactory result, I intend to post a short write-up of my
> findings here.
> 
> I agree to this patch with that commit message despite not having the
> clarity, because not having it seems way worse than having it.

Yes.

> > > @Connie: Just had a look at the MAINTAINERS file and hw/vfio/ap.c
> > > is listed under Arch. support S90 with you as a maintainer, and under
> > > vfio-ap with 4 maintainers listed one of them being me. The question
> > > is who is going to post a PULL request for this?  
> > 
> > General practice has been that I'm collecting everything s390x related.
> > I have also pulled from others before (e.g. some bios changes from
> > Thomas). While you could apply the patch, send it to me, and then I'd
> > queue it to s390-next, I can also simply queue it directly with your
> > ack :)
> > 
> > [Longer term, if you want to collect ap patches and then send me a pull
> > request, I would also be happy to do that. For this single patch, it
> > seems overkill.]
> >   
> 
> I agree, it would be an overkill. I guess r-b qualifies as ack.

I've always seen r-b as a superset of a-b.

Re: [Qemu-devel] [PATCH RFC] vfio-ap: flag as compatible with balloon

[Cornelia Huck]

On Wed,  5 Dec 2018 15:51:31 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> vfio-ap devices do not pin any pages in the host. Therefore, they
> are belived to be compatible with memory ballooning.
> 
> Flag them as compatible, so both vfio-ap and a balloon can be
> used simultaneously.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> As briefly discussed on IRC. RFC as I do not have easy access to
> hardware I can test this with.
> 
> ---
>  hw/vfio/ap.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 65de952f44..3bf48eed28 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -104,6 +104,14 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
>      vapdev->vdev.name = g_strdup_printf("%s", mdevid);
>      vapdev->vdev.dev = dev;
>  
> +    /*
> +     * vfio-ap devices are believed to operate in a way compatible with
> +     * memory ballooning, as no pages are pinned in the host.
> +     * This needs to be set before vfio_get_device() for vfio common to
> +     * handle the balloon inhibitor.
> +     */
> +    vapdev->vdev.balloon_allowed = true;
> +
>      ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
>      if (ret) {
>          goto out_get_dev_err;

Queued (with tweaked wording) to s390-next.