On Thu, 6 Dec 2018 13:32:39 +0100 Halil Pasic <[email protected]> wrote:
> On Thu, 6 Dec 2018 09:28:34 +0100 > David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> > > >>>> --- > > >>>> > > >>>> 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. > > >> > > > > > > > >
