[PATCH] virtio: Force DMA restricted devices through DMA API

[PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

If virtio devices are tagged for "restricted-dma-pool", then that
pool should be used for virtio ring setup, via the DMA API.

In particular, this fixes virtio_balloon for ARM PKVM, where the usual
workaround of setting VIRTIO_F_ACCESS_PLATFORM in the virtio device
doesn't work because the virtio_balloon driver clears the flag. This
seems a more robust fix than fiddling the flag again.

Signed-off-by: Keir Fraser <keirf@google.com>
---
 drivers/virtio/virtio_ring.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a5ec724c01d8..12be2607c648 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -12,6 +12,7 @@
 #include <linux/hrtimer.h>
 #include <linux/dma-mapping.h>
 #include <linux/spinlock.h>
+#include <linux/swiotlb.h>
 #include <xen/xen.h>
 
 #ifdef DEBUG
@@ -248,6 +249,13 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
 	if (!virtio_has_dma_quirk(vdev))
 		return true;
 
+	/* If the device is configured to use a DMA restricted pool,
+	 * we had better use it.
+	 */
+	if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
+	    is_swiotlb_for_alloc(vdev->dev.parent))
+		return true;
+
 	/* Otherwise, we are left to guess. */
 	/*
 	 * In theory, it's possible to have a buggy QEMU-supposed
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Michael S. Tsirkin]

On Tue, Jul 19, 2022 at 10:02:56AM +0000, Keir Fraser wrote:
> If virtio devices are tagged for "restricted-dma-pool", then that
> pool should be used for virtio ring setup, via the DMA API.
> 
> In particular, this fixes virtio_balloon for ARM PKVM, where the usual
> workaround of setting VIRTIO_F_ACCESS_PLATFORM in the virtio device
> doesn't work because the virtio_balloon driver clears the flag. This
> seems a more robust fix than fiddling the flag again.
> 
> Signed-off-by: Keir Fraser <keirf@google.com>


So the reason balloon disables ACCESS_PLATFORM is simply
because it passes physical addresses to device and
expects device to be able to poke at them.

I worry about modifying DMA semantics yet again - it has as much of a
chance to break some legacy configs as it has to fix some.


And I don't really know much about restricted-dma-pool but
I'd like to understand why does it make sense to set it for
the balloon since it pokes at all and any system memory.


> ---
>  drivers/virtio/virtio_ring.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a5ec724c01d8..12be2607c648 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -12,6 +12,7 @@
>  #include <linux/hrtimer.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/spinlock.h>
> +#include <linux/swiotlb.h>
>  #include <xen/xen.h>
>  
>  #ifdef DEBUG
> @@ -248,6 +249,13 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>  	if (!virtio_has_dma_quirk(vdev))
>  		return true;
>  
> +	/* If the device is configured to use a DMA restricted pool,
> +	 * we had better use it.
> +	 */
> +	if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> +	    is_swiotlb_for_alloc(vdev->dev.parent))
> +		return true;
> +
>  	/* Otherwise, we are left to guess. */
>  	/*
>  	 * In theory, it's possible to have a buggy QEMU-supposed
> -- 
> 2.37.0.170.g444d1eabd0-goog

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

On Tue, Jul 19, 2022 at 07:56:09AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2022 at 10:02:56AM +0000, Keir Fraser wrote:
> > If virtio devices are tagged for "restricted-dma-pool", then that
> > pool should be used for virtio ring setup, via the DMA API.
> > 
> > In particular, this fixes virtio_balloon for ARM PKVM, where the usual
> > workaround of setting VIRTIO_F_ACCESS_PLATFORM in the virtio device
> > doesn't work because the virtio_balloon driver clears the flag. This
> > seems a more robust fix than fiddling the flag again.
> > 
> > Signed-off-by: Keir Fraser <keirf@google.com>
> 
> 
> So the reason balloon disables ACCESS_PLATFORM is simply
> because it passes physical addresses to device and
> expects device to be able to poke at them.
> 
> I worry about modifying DMA semantics yet again - it has as much of a
> chance to break some legacy configs as it has to fix some.
> 
> 
> And I don't really know much about restricted-dma-pool but
> I'd like to understand why does it make sense to set it for
> the balloon since it pokes at all and any system memory.

So this is set in the device tree by the host, telling it to bounce all DMA
through a restricted memory window (basically swiotlb). The original reason
is simply to isolate DMA, to the extent possible, on IOMMU-less systems.

However it is also useful for PKVM because the host is not trusted to access
ordinary protected VM memory. To allow I/O via the host, restricted-dma-pool
is used to cause a bounce aperture to be allocated during VM boot, which is
then explicitly shared with the host. For correct PKVM virtio operation, all
data *and metadata* (virtio rings and descriptors) must be allocated in or
bounced through this aperture.

Insofar as virtio device accesses to virtio rings in guest memory essentially
*are* DMA (from the pov of the guest), I think it makes sense to respect the
bounce buffer for those rings, if so configured by the device tree.

> > ---
> >  drivers/virtio/virtio_ring.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index a5ec724c01d8..12be2607c648 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/hrtimer.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/swiotlb.h>
> >  #include <xen/xen.h>
> >  
> >  #ifdef DEBUG
> > @@ -248,6 +249,13 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> >  	if (!virtio_has_dma_quirk(vdev))
> >  		return true;
> >  
> > +	/* If the device is configured to use a DMA restricted pool,
> > +	 * we had better use it.
> > +	 */
> > +	if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> > +	    is_swiotlb_for_alloc(vdev->dev.parent))
> > +		return true;
> > +
> >  	/* Otherwise, we are left to guess. */
> >  	/*
> >  	 * In theory, it's possible to have a buggy QEMU-supposed
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> 
> 

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Michael S. Tsirkin]

On Tue, Jul 19, 2022 at 02:05:58PM +0000, Keir Fraser wrote:
> On Tue, Jul 19, 2022 at 07:56:09AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jul 19, 2022 at 10:02:56AM +0000, Keir Fraser wrote:
> > > If virtio devices are tagged for "restricted-dma-pool", then that
> > > pool should be used for virtio ring setup, via the DMA API.
> > > 
> > > In particular, this fixes virtio_balloon for ARM PKVM, where the usual
> > > workaround of setting VIRTIO_F_ACCESS_PLATFORM in the virtio device
> > > doesn't work because the virtio_balloon driver clears the flag. This
> > > seems a more robust fix than fiddling the flag again.
> > > 
> > > Signed-off-by: Keir Fraser <keirf@google.com>
> > 
> > 
> > So the reason balloon disables ACCESS_PLATFORM is simply
> > because it passes physical addresses to device and
> > expects device to be able to poke at them.
> > 
> > I worry about modifying DMA semantics yet again - it has as much of a
> > chance to break some legacy configs as it has to fix some.
> > 
> > 
> > And I don't really know much about restricted-dma-pool but
> > I'd like to understand why does it make sense to set it for
> > the balloon since it pokes at all and any system memory.
> 
> So this is set in the device tree by the host, telling it to bounce all DMA
> through a restricted memory window (basically swiotlb). The original reason
> is simply to isolate DMA, to the extent possible, on IOMMU-less systems.
> 
> However it is also useful for PKVM because the host is not trusted to access
> ordinary protected VM memory.

I'll have to read up on pKVM. Will get back to you.

> To allow I/O via the host, restricted-dma-pool
> is used to cause a bounce aperture to be allocated during VM boot, which is
> then explicitly shared with the host. For correct PKVM virtio operation, all
> data *and metadata* (virtio rings and descriptors) must be allocated in or
> bounced through this aperture.
>
> Insofar as virtio device accesses to virtio rings in guest memory essentially
> *are* DMA (from the pov of the guest), I think it makes sense to respect the
> bounce buffer for those rings, if so configured by the device tree.
>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index a5ec724c01d8..12be2607c648 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/hrtimer.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/spinlock.h>
> > > +#include <linux/swiotlb.h>
> > >  #include <xen/xen.h>
> > >  
> > >  #ifdef DEBUG
> > > @@ -248,6 +249,13 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > >  	if (!virtio_has_dma_quirk(vdev))
> > >  		return true;
> > >  
> > > +	/* If the device is configured to use a DMA restricted pool,
> > > +	 * we had better use it.
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_DMA_RESTRICTED_POOL) &&
> > > +	    is_swiotlb_for_alloc(vdev->dev.parent))
> > > +		return true;
> > > +
> > >  	/* Otherwise, we are left to guess. */
> > >  	/*
> > >  	 * In theory, it's possible to have a buggy QEMU-supposed
> > > -- 
> > > 2.37.0.170.g444d1eabd0-goog
> > 
> > 

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Christoph Hellwig]

On Tue, Jul 19, 2022 at 10:02:56AM +0000, Keir Fraser wrote:
> +#include <linux/swiotlb.h>

Drivers must never use this header.  We have a few pre-existing abuses
in the drm code, but they will go away.

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

On Tue, Jul 19, 2022 at 08:23:28AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 19, 2022 at 10:02:56AM +0000, Keir Fraser wrote:
> > +#include <linux/swiotlb.h>
> 
> Drivers must never use this header.  We have a few pre-existing abuses
> in the drm code, but they will go away.
> 

Ok fair enough, and I'll admit I don't like my use of
swiotlb_for_alloc() a lot either.

However, if the general idea at least is acceptable, would the
implementation be acceptable if I add an explicit API for this to the
DMA subsystem, and hide the detail there?

Or a completely different approach would be to revert the patch
e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
driver. MST: That's back in your court, as it's your patch!

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Christoph Hellwig]

On Tue, Jul 19, 2022 at 03:46:08PM +0000, Keir Fraser wrote:
> However, if the general idea at least is acceptable, would the
> implementation be acceptable if I add an explicit API for this to the
> DMA subsystem, and hide the detail there?

I don't think so.  The right thing to key off is
VIRTIO_F_ACCESS_PLATFORM, which really should be set in any modern
virtio device after all the problems we had with the lack of it.

> Or a completely different approach would be to revert the patch
> e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
> driver. MST: That's back in your court, as it's your patch!

Which also means this needs to be addresses, but I don't think a
simple revert is enough.

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

On Tue, Jul 19, 2022 at 08:51:54AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 19, 2022 at 03:46:08PM +0000, Keir Fraser wrote:
> > However, if the general idea at least is acceptable, would the
> > implementation be acceptable if I add an explicit API for this to the
> > DMA subsystem, and hide the detail there?
> 
> I don't think so.  The right thing to key off is
> VIRTIO_F_ACCESS_PLATFORM, which really should be set in any modern
> virtio device after all the problems we had with the lack of it.

Ok. Certainly the flag description in virtio spec fits the bill.

> > Or a completely different approach would be to revert the patch
> > e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
> > driver. MST: That's back in your court, as it's your patch!
> 
> Which also means this needs to be addresses, but I don't think a
> simple revert is enough.

Well here are two possible approaches:

1. Revert e41b1355508d outright. I'm not even sure what it would mean
for reported pages to go through IOMMU. And VIRTIO_F_ACCESS_PLATFORM
is no longer IOMMU-specific anyway.

2. Continue to clear the flag during virtio_balloon negotiation, but
remember that it was offered, and test for that in vring_use_dma_api()
as well as, or instead of, virtio_has_dma_quirk().

Do either of those appeal?

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Christoph Hellwig]

On Tue, Jul 19, 2022 at 04:11:50PM +0000, Keir Fraser wrote:
> Well here are two possible approaches:
> 
> 1. Revert e41b1355508d outright. I'm not even sure what it would mean
> for reported pages to go through IOMMU. And VIRTIO_F_ACCESS_PLATFORM
> is no longer IOMMU-specific anyway.
> 
> 2. Continue to clear the flag during virtio_balloon negotiation, but
> remember that it was offered, and test for that in vring_use_dma_api()
> as well as, or instead of, virtio_has_dma_quirk().
> 
> Do either of those appeal?

I'll have to defer to people that actually understand the virtio_balloon
code, because I haven't evey seriously looked at it.

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Michael S. Tsirkin]

On Tue, Jul 19, 2022 at 04:11:50PM +0000, Keir Fraser wrote:
> On Tue, Jul 19, 2022 at 08:51:54AM -0700, Christoph Hellwig wrote:
> > On Tue, Jul 19, 2022 at 03:46:08PM +0000, Keir Fraser wrote:
> > > However, if the general idea at least is acceptable, would the
> > > implementation be acceptable if I add an explicit API for this to the
> > > DMA subsystem, and hide the detail there?
> > 
> > I don't think so.  The right thing to key off is
> > VIRTIO_F_ACCESS_PLATFORM, which really should be set in any modern
> > virtio device after all the problems we had with the lack of it.
> 
> Ok. Certainly the flag description in virtio spec fits the bill.

Maybe. But balloon really isn't a normal device. Yes the rings kind of
operate more or less normally. However consider for example free page
hinting. We stick a page address in ring for purposes of telling host it
can blow it away at any later time unless we write something into the
page.  Free page reporting is similar.
Sending gigabytes of memory through swiotlb is at minimum not
a good idea.

Conversely, is it even okay security wise that host can blow away any
guest page?  Because with balloon GFNs do not go through bounce
buffering.



> > > Or a completely different approach would be to revert the patch
> > > e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
> > > driver. MST: That's back in your court, as it's your patch!
> > 
> > Which also means this needs to be addresses, but I don't think a
> > simple revert is enough.
> 
> Well here are two possible approaches:
> 
> 1. Revert e41b1355508d outright. I'm not even sure what it would mean
> for reported pages to go through IOMMU. And VIRTIO_F_ACCESS_PLATFORM
> is no longer IOMMU-specific anyway.
> 
> 2. Continue to clear the flag during virtio_balloon negotiation, but
> remember that it was offered, and test for that in vring_use_dma_api()
> as well as, or instead of, virtio_has_dma_quirk().
> 
> Do either of those appeal?

I think the use case needs to be documented better.


-- 
MST

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

On Wed, Jul 20, 2022 at 02:59:53AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2022 at 04:11:50PM +0000, Keir Fraser wrote:
> > On Tue, Jul 19, 2022 at 08:51:54AM -0700, Christoph Hellwig wrote:
> > > On Tue, Jul 19, 2022 at 03:46:08PM +0000, Keir Fraser wrote:
> > > > However, if the general idea at least is acceptable, would the
> > > > implementation be acceptable if I add an explicit API for this to the
> > > > DMA subsystem, and hide the detail there?
> > > 
> > > I don't think so.  The right thing to key off is
> > > VIRTIO_F_ACCESS_PLATFORM, which really should be set in any modern
> > > virtio device after all the problems we had with the lack of it.
> > 
> > Ok. Certainly the flag description in virtio spec fits the bill.
> 
> Maybe. But balloon really isn't a normal device. Yes the rings kind of
> operate more or less normally. However consider for example free page
> hinting. We stick a page address in ring for purposes of telling host it
> can blow it away at any later time unless we write something into the
> page.  Free page reporting is similar.
> Sending gigabytes of memory through swiotlb is at minimum not
> a good idea.

I don't think any balloon use case needs the page's guest data to be
made available to the host device. What the device side does with
reported guest-physical page addresses is somewhat VMM/Hyp specific,
but I think it's fair to say it will know how to reclaim or track
pages by guest PA, and bouncing reported/hinted page addresses through
a SWIOTLB or IOMMU would not be of any use to any use case I can think
of.

As far as I can see, the only translation that makes sense at all for
virtio balloon is in ring management.

> Conversely, is it even okay security wise that host can blow away any
> guest page?  Because with balloon GFNs do not go through bounce
> buffering.

Ok, I think this is fair and I can address that by describing the use
case more broadly.

The short answer is that there will be more patches forthcoming,
because the balloon driver will need to tell the hypervisor (EL2 Hyp
in the ARM PKVM case) that is is willingly relinquishing memory
pages. So, there will be a patch to add a new HVC to PKVM Hyp, and a
patch to detect and use the new HVC via a new API that hooks into
Linux's balloon infrastructure.

So the use case is that virtio_balloon needs to set up its rings and
descriptors in a shared memory region, as requested via
dma-restricted-pool and the VIRTIO_F_PALTFORM_ACCESS flag. This is
required because the host device has no access to regular guest
memory.

However, in PKVM, page notifications will notify both the (trusted)
Hypervisor, via hypercall, and the (untrusted) VMM, via virtio. Guest
physical addresses are fine here. The VMM understands guest PAs
perfectly well, it's just not normally allowed to access their
contents or otherwise map or use those pages itself.

> 
> > > > Or a completely different approach would be to revert the patch
> > > > e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
> > > > driver. MST: That's back in your court, as it's your patch!
> > > 
> > > Which also means this needs to be addresses, but I don't think a
> > > simple revert is enough.
> > 
> > Well here are two possible approaches:
> > 
> > 1. Revert e41b1355508d outright. I'm not even sure what it would mean
> > for reported pages to go through IOMMU. And VIRTIO_F_ACCESS_PLATFORM
> > is no longer IOMMU-specific anyway.
> > 
> > 2. Continue to clear the flag during virtio_balloon negotiation, but
> > remember that it was offered, and test for that in vring_use_dma_api()
> > as well as, or instead of, virtio_has_dma_quirk().
> > 
> > Do either of those appeal?
> 
> I think the use case needs to be documented better.

I hope the above is helpful in giving some more context.

Perhaps it would make more sense to re-submit this patch as part of
a larger series that includes the rest of the mechanism needed to
support virtio_balloon on PKVM?

Thanks,
Keir

> 
> -- 
> MST
> 
> 

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Michael S. Tsirkin]

On Wed, Jul 20, 2022 at 08:27:51AM +0000, Keir Fraser wrote:
> On Wed, Jul 20, 2022 at 02:59:53AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jul 19, 2022 at 04:11:50PM +0000, Keir Fraser wrote:
> > > On Tue, Jul 19, 2022 at 08:51:54AM -0700, Christoph Hellwig wrote:
> > > > On Tue, Jul 19, 2022 at 03:46:08PM +0000, Keir Fraser wrote:
> > > > > However, if the general idea at least is acceptable, would the
> > > > > implementation be acceptable if I add an explicit API for this to the
> > > > > DMA subsystem, and hide the detail there?
> > > > 
> > > > I don't think so.  The right thing to key off is
> > > > VIRTIO_F_ACCESS_PLATFORM, which really should be set in any modern
> > > > virtio device after all the problems we had with the lack of it.
> > > 
> > > Ok. Certainly the flag description in virtio spec fits the bill.
> > 
> > Maybe. But balloon really isn't a normal device. Yes the rings kind of
> > operate more or less normally. However consider for example free page
> > hinting. We stick a page address in ring for purposes of telling host it
> > can blow it away at any later time unless we write something into the
> > page.  Free page reporting is similar.
> > Sending gigabytes of memory through swiotlb is at minimum not
> > a good idea.
> 
> I don't think any balloon use case needs the page's guest data to be
> made available to the host device. What the device side does with
> reported guest-physical page addresses is somewhat VMM/Hyp specific,
> but I think it's fair to say it will know how to reclaim or track
> pages by guest PA, and bouncing reported/hinted page addresses through
> a SWIOTLB or IOMMU would not be of any use to any use case I can think
> of.
> 
> As far as I can see, the only translation that makes sense at all for
> virtio balloon is in ring management.
> 
> > Conversely, is it even okay security wise that host can blow away any
> > guest page?  Because with balloon GFNs do not go through bounce
> > buffering.
> 
> Ok, I think this is fair and I can address that by describing the use
> case more broadly.
> 
> The short answer is that there will be more patches forthcoming,
> because the balloon driver will need to tell the hypervisor (EL2 Hyp
> in the ARM PKVM case) that is is willingly relinquishing memory
> pages. So, there will be a patch to add a new HVC to PKVM Hyp, and a
> patch to detect and use the new HVC via a new API that hooks into
> Linux's balloon infrastructure.
> 
> So the use case is that virtio_balloon needs to set up its rings and
> descriptors in a shared memory region, as requested via
> dma-restricted-pool and the VIRTIO_F_PALTFORM_ACCESS flag. This is
> required because the host device has no access to regular guest
> memory.
> 
> However, in PKVM, page notifications will notify both the (trusted)
> Hypervisor, via hypercall, and the (untrusted) VMM, via virtio. Guest
> physical addresses are fine here. The VMM understands guest PAs
> perfectly well, it's just not normally allowed to access their
> contents or otherwise map or use those pages itself.

OK ... and I wonder whether this extends the balloon device
in some way? Is there or can there be a new feature bit for this
functionality?


> > 
> > > > > Or a completely different approach would be to revert the patch
> > > > > e41b1355508d which clears VIRTIO_F_ACCESS_PLATFORM in the balloon
> > > > > driver. MST: That's back in your court, as it's your patch!
> > > > 
> > > > Which also means this needs to be addresses, but I don't think a
> > > > simple revert is enough.
> > > 
> > > Well here are two possible approaches:
> > > 
> > > 1. Revert e41b1355508d outright. I'm not even sure what it would mean
> > > for reported pages to go through IOMMU. And VIRTIO_F_ACCESS_PLATFORM
> > > is no longer IOMMU-specific anyway.
> > > 
> > > 2. Continue to clear the flag during virtio_balloon negotiation, but
> > > remember that it was offered, and test for that in vring_use_dma_api()
> > > as well as, or instead of, virtio_has_dma_quirk().
> > > 
> > > Do either of those appeal?
> > 
> > I think the use case needs to be documented better.
> 
> I hope the above is helpful in giving some more context.
> 
> Perhaps it would make more sense to re-submit this patch as part of
> a larger series that includes the rest of the mechanism needed to
> support virtio_balloon on PKVM?
> 
> Thanks,
> Keir

I suspect so, yes.


> > 
> > -- 
> > MST
> > 
> > 

Re: [PATCH] virtio: Force DMA restricted devices through DMA API

[Keir Fraser]

On Wed, Jul 20, 2022 at 05:58:28AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 20, 2022 at 08:27:51AM +0000, Keir Fraser wrote:
> > The short answer is that there will be more patches forthcoming,
> > because the balloon driver will need to tell the hypervisor (EL2 Hyp
> > in the ARM PKVM case) that is is willingly relinquishing memory
> > pages. So, there will be a patch to add a new HVC to PKVM Hyp, and a
> > patch to detect and use the new HVC via a new API that hooks into
> > Linux's balloon infrastructure.
> > 
> > So the use case is that virtio_balloon needs to set up its rings and
> > descriptors in a shared memory region, as requested via
> > dma-restricted-pool and the VIRTIO_F_PALTFORM_ACCESS flag. This is
> > required because the host device has no access to regular guest
> > memory.
> > 
> > However, in PKVM, page notifications will notify both the (trusted)
> > Hypervisor, via hypercall, and the (untrusted) VMM, via virtio. Guest
> > physical addresses are fine here. The VMM understands guest PAs
> > perfectly well, it's just not normally allowed to access their
> > contents or otherwise map or use those pages itself.
> 
> OK ... and I wonder whether this extends the balloon device
> in some way? Is there or can there be a new feature bit for this
> functionality?

To my mind it is implementation dependent whether the balloon device
needs to be extended. In my current implementation it is not, and
probably it will continue to be entirely handled host-side by the host
kernel and hypervisor. But we should consider the possibility of
requiring knowledge/extension in the device for sure.

Currently there is no feature flag for the new Hyp-notification path
on the driver side. The notification hypercall is hidden behind a new
API, and there is an init/probe call on that API by which the driver
unilaterally decides the extended path including Hyp notification is to
be used. One downside of this is that the device cannot detect a
legacy driver that lacks knowledge of the extended PKVM path. Any
pages returned by a legacy driver will simply crash the VMM, because
the hypervisor is still protecting those pages. A rather inelegant
failure mode!

I can envision a new feature flag that:

1. Is advertised by the device (makes sense: the VMM surely knows that
it is managing a protected VM).

2. Is Ack'ed by an aware driver, and which switches on the extended
notification path.

3. Is not negotiated by a legacy driver, causing the device to clear
FEATURES_OK, and the balloon is unavailable.

A balloon-specific flag called perhaps VIRTIO_F_BALLOON_UNTRUSTED_HOST,
or VIRTIO_F_BALLOON_NOTIFY_HYP, or somesuch? In some senses it's not a
balloon-specific piece of information, but it's only a pertinent feature
for balloon (at least as of now).

My understanding is that the first step in upstreaming such a new flag
would be to get it accepted into the virtio specification? If so and
this sounds agreeable, I'll rework my private patches, and cook up an
extension to the virtio spec. If an RFC posting of the patches here is
preferred before posting to the virtio-spec list, I can do that too.

> > Perhaps it would make more sense to re-submit this patch as part of
> > a larger series that includes the rest of the mechanism needed to
> > support virtio_balloon on PKVM?
> > 
> > Thanks,
> > Keir
> 
> I suspect so, yes.

Thanks for your review feedback. I will submit a full patch series in
due course.

Regards,
Keir