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 <address@hidden> wrote:
>
> > On Thu, 6 Dec 2018 09:28:34 +0100
> > David Hildenbrand <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
> > > > >   
> > > >    
> > >
> > >