vfio-ap: flag as compatible with balloon
RFC PATCH 1/1 vfio-ap: flag as compatible with balloon
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.
---
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(+)
hw/vfio/ap.c +8 -0
@@ -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;
Signature · 1 line
2.17.2
Quoted 11 lines · 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 <[email protected]>---
As briefly discussed on IRC. RFC as I do not have easy access to hardware I can test this with.
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.
Quoted 25 lines
--- hw/vfio/ap.c | 8 ++++++++ 1 file changed, 8 insertions(+)hw/vfio/ap.c +8
-0@@ -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;
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/*
hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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 :)
Signature · 3 lines
Thanks, David / dhildenb
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 1 line
"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?)
Quoted 44 lines · Cornelia Huck
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 1 line
Quoted 1 line
"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.
Quoted 6 lines
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.
Quoted 1 line
Quoted 47 lines · Christian Borntraeger
Quoted 44 lines · Cornelia Huck
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 7 lines
Quoted 1 line
Quoted 1 line
"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.
Quoted 11 lines
Quoted 7 lines
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.
Quoted 1 line
Signature · 3 lines
Thanks, David / dhildenb
Quoted 56 lines · On Thu, 6 Dec 2018 09:28:34 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 47 lines · Christian Borntraeger
Quoted 44 lines · Cornelia Huck
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 7 lines
Quoted 1 line
Quoted 1 line
"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.
Quoted 4 lines
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?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
It's the same for ccw :)
As a matter of fact, I don't like that comment.
Regards,
Halil
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 line
Quoted 59 lines · On Thu, 6 Dec 2018 13:32:39 +0100 Halil Pasic <[email protected]> wrote:
Quoted 56 lines · On Thu, 6 Dec 2018 09:28:34 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 54 lines · Christian Borntraeger
Quoted 44 lines · Cornelia Huck
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 1 line
Quoted 1 line
"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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 3 lines
@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 :)
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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?
Quoted 15 lines
Regards,
HalilQuoted 10 lines
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 line
Quoted 71 lines · Cornelia Huck
Quoted 59 lines · On Thu, 6 Dec 2018 13:32:39 +0100 Halil Pasic <[email protected]> wrote:
Quoted 56 lines · On Thu, 6 Dec 2018 09:28:34 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 54 lines · Christian Borntraeger
Quoted 44 lines · Cornelia Huck
Quoted 41 lines · On Wed, 5 Dec 2018 17:38:22 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 26 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/* hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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?
Quoted 1 line
Quoted 1 line
"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.
Quoted 6 lines
Quoted 4 lines
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/?
Quoted 7 lines
The second part of the comment is still useful, I believe.
Quoted 3 lines
@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.
Quoted 26 lines
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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?
Quoted 15 lines
Regards,
HalilQuoted 10 lines
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 line
Quoted 25 lines · 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 <[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(+)hw/vfio/ap.c +1
-0@@ -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; +/*
hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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;
Quoted 1 line
Quoted 7 lines · 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 <[email protected]>
Does it make sense to add cc stable for 3.1?
Quoted 29 lines
---
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(+)hw/vfio/ap.c +8
-0@@ -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;
Quoted 10 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 7 lines · 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 <[email protected]>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.
Quoted 10 lines · Cornelia Huck
Quoted 7 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 6 lines · 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.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Cornelia Huck <[email protected]>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.
Quoted 18 lines · On Fri, 7 Dec 2018 13:32:14 +0100 Christian Borntraeger…
Quoted 10 lines · Cornelia Huck
Quoted 7 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 6 lines · 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.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Cornelia Huck <[email protected]>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.
Quoted 8 lines
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 :)
Halil Pasic
Reviewed-byQuoted 11 lines · On Fri, 7 Dec 2018 13:29:46 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 8 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 7 lines · 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 <[email protected]>
With the comment stuff sorted out:
@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?
Quoted 6 lines
Quoted 2 lines
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
Quoted 10 lines · On Fri, 7 Dec 2018 13:52:53 +0100 Halil Pasic <[email protected]> wrote:
Quoted 7 lines · On Fri, 7 Dec 2018 13:29:46 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 4 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 3 lines · Cornelia Huck
vfio-ap devices do not pin any pages in the host. Therefore, they are belived to be compatible with memory ballooning.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Flag them as compatible, so both vfio-ap and a balloon can be used simultaneously.
Signed-off-by Cornelia Huck <[email protected]>With the comment stuff sorted out:
Reviewed-by Halil Pasic <[email protected]>
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. + */
Quoted 5 lines
@Connie: Just had a look at the
MAINTAINERSfile andhw/vfio/ap.cis 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.]
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 23 lines · On Fri, 7 Dec 2018 14:17:20 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 10 lines · On Fri, 7 Dec 2018 13:52:53 +0100 Halil Pasic <[email protected]> wrote:
Quoted 7 lines · On Fri, 7 Dec 2018 13:29:46 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 4 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 3 lines · Cornelia Huck
vfio-ap devices do not pin any pages in the host. Therefore, they are belived to be compatible with memory ballooning.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Flag them as compatible, so both vfio-ap and a balloon can be used simultaneously.
Signed-off-by Cornelia Huck <[email protected]>With the comment stuff sorted out:
Reviewed-by Halil Pasic <[email protected]>So, do you agree with the comment change I suggested?
hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 112
Quoted 6 lines
hw/vfio/ap.c +6
-0+/* +* 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.
Quoted 13 lines
Quoted 6 lines
@Connie: Just had a look at the
MAINTAINERSfile andhw/vfio/ap.cis 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 :)
Quoted 4 lines
[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
Quoted 12 lines
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 40 lines · On Fri, 7 Dec 2018 15:04:14 +0100 Halil Pasic <[email protected]> wrote:
Quoted 23 lines · On Fri, 7 Dec 2018 14:17:20 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 10 lines · On Fri, 7 Dec 2018 13:52:53 +0100 Halil Pasic <[email protected]> wrote:
Quoted 7 lines · On Fri, 7 Dec 2018 13:29:46 +0100 Cornelia Huck <[email protected]> wrote:
Quoted 4 lines · On Fri, 7 Dec 2018 13:17:02 +0100 Christian Borntraeger…
Quoted 3 lines · Cornelia Huck
vfio-ap devices do not pin any pages in the host. Therefore, they are belived to be compatible with memory ballooning.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Flag them as compatible, so both vfio-ap and a balloon can be used simultaneously.
Signed-off-by Cornelia Huck <[email protected]>With the comment stuff sorted out:
Reviewed-by Halil Pasic <[email protected]>So, do you agree with the comment change I suggested?
hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 112
Quoted 6 lines
hw/vfio/ap.c +6
-0+/* +* 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 thatvfio_get_group()basically maps it's address space argument. But for s390x this pinning does not happen.
Quoted 2 lines
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 :)
Quoted 6 lines
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.
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
@Connie: Just had a look at the
MAINTAINERSfile andhw/vfio/ap.cis 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 :)
Quoted 6 lines
Quoted 4 lines
[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.
Cornelia Huck
AppliedQuoted 25 lines · On Wed, 5 Dec 2018 15:51:31 +0100 Cornelia Huck <[email protected]> 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(+)hw/vfio/ap.c +1
-0@@ -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; +/*
hw/vfio/ap.c static void vfio_ap_realize(DeviceState *dev, Error **errp) · line 117
Quoted 10 lines
hw/vfio/ap.c +7
-0+* 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.