[PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
QEMU, i.e. the driver must accept it if offered by the device. The
virtio specification says that the driver SHOULD accept the
ACCESS_PLATFORM feature if offered, and that the device MAY fail to
operate if ACCESS_PLATFORM was offered but not negotiated.

While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
the device when the driver fences ACCESS_PLATFORM. With commit
2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
decision to do so whenever the get_dma_as() callback is implemented (by
the bus), which in practice means for the entirety of virtio-pci.

That means, if the device needs to translate I/O addresses, then
ACCESS_PLATFORM is mandatory. The aforementioned commit tells us in the
commit message that this is for security reasons. More precisely if we
were to allow a less then trusted driver (e.g. an user-space driver, or
a nested guest) to make the device bypass the IOMMU by not negotiating
ACCESS_PLATFORM, then the guest kernel would have no ability to
control/police (by programming the IOMMU) what pieces of guest memory
the driver may manipulate using the device. Which would break security
assumptions within the guest.

If ACCESS_PLATFORM is offered not because we want the device to utilize
an IOMMU and do address translation, but because the device does not
have access to the entire guest RAM, and needs the driver to grant
access to the bits it needs access to (e.g. confidential guest support),
we still require the guest to have the corresponding logic and to accept
ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
things are bound to go wrong, and we may see failures much less graceful
than failing the device because the driver didn't negotiate
ACCESS_PLATFORM.

So let us make ACCESS_PLATFORM mandatory for the driver regardless
of whether the get_dma_as() callback is implemented or not.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")

---

RFC -> v1:
* Tweaked the commit message and fixed typos (Connie)
* Added two sentences discussing the security implications (Michael)

This patch is based on:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg866199.html

During the review of "virtio: fix the condition for iommu_platform not
supported" Daniel raised the question why do we "force IOMMU_PLATFORM"
iff has_iommu && !!klass->get_dma_as. My answer to that was, that
this logic ain't right.

While at it I used the opportunity to re-organize the code a little
and provide an explanatory comment.
---
 hw/virtio/virtio-bus.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index fbf0dd14b8..359430eb1c 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
         return;
     }
 
-    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
-    if (klass->get_dma_as != NULL && has_iommu) {
+    vdev->dma_as = &address_space_memory;
+    if (has_iommu) {
+        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
+        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
         virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
-        vdev->dma_as = klass->get_dma_as(qbus->parent);
-        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
-            error_setg(errp,
+        if (klass->get_dma_as) {
+            vdev->dma_as = klass->get_dma_as(qbus->parent);
+            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
+                error_setg(errp,
                        "iommu_platform=true is not supported by the device");
+                return;
+            }
         }
-    } else {
-        vdev->dma_as = &address_space_memory;
     }
 }
 
-- 
2.32.0

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Cornelia Huck]

On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
> QEMU, i.e. the driver must accept it if offered by the device. The
> virtio specification says that the driver SHOULD accept the
> ACCESS_PLATFORM feature if offered, and that the device MAY fail to
> operate if ACCESS_PLATFORM was offered but not negotiated.

Maybe add

(see the "{Driver,Device} Requirements: Reserved Feature Bits" sections
in the virtio spec)

?

>
> While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
> the device when the driver fences ACCESS_PLATFORM. With commit
> 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
> decision to do so whenever the get_dma_as() callback is implemented (by
> the bus), which in practice means for the entirety of virtio-pci.
>
> That means, if the device needs to translate I/O addresses, then
> ACCESS_PLATFORM is mandatory. The aforementioned commit tells us in the
> commit message that this is for security reasons. More precisely if we
> were to allow a less then trusted driver (e.g. an user-space driver, or
> a nested guest) to make the device bypass the IOMMU by not negotiating
> ACCESS_PLATFORM, then the guest kernel would have no ability to
> control/police (by programming the IOMMU) what pieces of guest memory
> the driver may manipulate using the device. Which would break security
> assumptions within the guest.
>
> If ACCESS_PLATFORM is offered not because we want the device to utilize
> an IOMMU and do address translation, but because the device does not
> have access to the entire guest RAM, and needs the driver to grant
> access to the bits it needs access to (e.g. confidential guest support),
> we still require the guest to have the corresponding logic and to accept
> ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
> things are bound to go wrong, and we may see failures much less graceful
> than failing the device because the driver didn't negotiate
> ACCESS_PLATFORM.
>
> So let us make ACCESS_PLATFORM mandatory for the driver regardless
> of whether the get_dma_as() callback is implemented or not.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")
>
> ---
>
> RFC -> v1:
> * Tweaked the commit message and fixed typos (Connie)
> * Added two sentences discussing the security implications (Michael)
>
> This patch is based on:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg866199.html
>
> During the review of "virtio: fix the condition for iommu_platform not
> supported" Daniel raised the question why do we "force IOMMU_PLATFORM"
> iff has_iommu && !!klass->get_dma_as. My answer to that was, that
> this logic ain't right.
>
> While at it I used the opportunity to re-organize the code a little
> and provide an explanatory comment.
> ---
>  hw/virtio/virtio-bus.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index fbf0dd14b8..359430eb1c 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>          return;
>      }
>  
> -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> -    if (klass->get_dma_as != NULL && has_iommu) {
> +    vdev->dma_as = &address_space_memory;
> +    if (has_iommu) {
> +        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */

I must admit that the more I stare at this code, the more confused I
get. We run this function during device realization, and the reason that
the feature bit might have gotten lost is that the ->get_features()
device callback dropped it. This happens before the driver is actually
involved; the check whether the *driver* dropped the feature is done
during feature validation, which is another code path. So what we do
here is failing device realization if a backend doesn't support
IOMMU_PLATFORM, isn't it?

>          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> -        vdev->dma_as = klass->get_dma_as(qbus->parent);
> -        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> -            error_setg(errp,
> +        if (klass->get_dma_as) {
> +            vdev->dma_as = klass->get_dma_as(qbus->parent);
> +            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> +                error_setg(errp,
>                         "iommu_platform=true is not supported by the device");
> +                return;
> +            }
>          }
> -    } else {
> -        vdev->dma_as = &address_space_memory;
>      }
>  }
>  

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Wed, 09 Feb 2022 18:24:56 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
> > QEMU, i.e. the driver must accept it if offered by the device. The
> > virtio specification says that the driver SHOULD accept the
> > ACCESS_PLATFORM feature if offered, and that the device MAY fail to
> > operate if ACCESS_PLATFORM was offered but not negotiated.  
> 
> Maybe add
> 
> (see the "{Driver,Device} Requirements: Reserved Feature Bits" sections
> in the virtio spec)
> 
> ?

I can add that, but I doubt people will have trouble finding it anyway.
There are 6 mentions of ACCESS_PLATFORM in the spec, so unless somebody
is using the dead tree version...
[..]
> > @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >          return;
> >      }
> >  
> > -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> > -    if (klass->get_dma_as != NULL && has_iommu) {
> > +    vdev->dma_as = &address_space_memory;
> > +    if (has_iommu) {
> > +        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> > +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */  
> 
> I must admit that the more I stare at this code, the more confused I
> get. We run this function during device realization, and the reason that
> the feature bit might have gotten lost is that the ->get_features()
> device callback dropped it. This happens before the driver is actually
> involved; the check whether the *driver* dropped the feature is done
> during feature validation, which is another code path. 
[moved text from here]
> 
> >          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); [Mark 1]


Let us have a look at 
static int virtio_validate_features(VirtIODevice *vdev)                         
{                                                                               
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);                       
                                                                                
    if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&               
        !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {              
        return -EFAULT;                                                         [Mark 2]                  
    }                                                                           
[..]

So were it not of the [Mark 1] we could not hit [Mark 2] if the feature
bit was lost because the ->get_features() callback dropped it. Yes,
feature negotiation is another code path, but the two are interdependent
in a non-trivial way. That is why I added that comment.

[moved here]
> So what we do
> here is failing device realization if a backend doesn't support
> IOMMU_PLATFORM, isn't it?

Not really. We fail the device realization if !vdev_has_iommu &&
vdev->dma_as != &address_space_memory, that is the device does not
support address translation, but we need it to support address
translation because ->dma_as != &address_space memory. If however
->dma_as == &address_space memory we carry on happily even if ->get_features() dropped
IOMMU_PLATFORM, because we don't actually need an iova -> gpa
translation. This is the case with virtiofs confidential guests for
example.

But we still don't want the guest dropping ACCESS_PLATFORM, because it is
still mandatory, because the device won't operate correctly unless the
driver grants access to the pieces of memory that the device needs to
access. The underlying mechanism of granting access may not have
anything to do with an IOMMU though.

Does it make sense now?

> > -        vdev->dma_as = klass->get_dma_as(qbus->parent);
> > -        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> > -            error_setg(errp,
> > +        if (klass->get_dma_as) {
> > +            vdev->dma_as = klass->get_dma_as(qbus->parent);
> > +            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> > +                error_setg(errp,
> >                         "iommu_platform=true is not supported by the device");
> > +                return;
> > +            }
> >          }
> > -    } else {
> > -        vdev->dma_as = &address_space_memory;
> >      }
> >  }
> >    
> 
> 

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Cornelia Huck]

On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 09 Feb 2022 18:24:56 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
>> > @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>> >          return;
>> >      }
>> >  
>> > -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>> > -    if (klass->get_dma_as != NULL && has_iommu) {
>> > +    vdev->dma_as = &address_space_memory;
>> > +    if (has_iommu) {
>> > +        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>> > +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */  
>> 
>> I must admit that the more I stare at this code, the more confused I
>> get. We run this function during device realization, and the reason that
>> the feature bit might have gotten lost is that the ->get_features()
>> device callback dropped it. This happens before the driver is actually
>> involved; the check whether the *driver* dropped the feature is done
>> during feature validation, which is another code path. 
> [moved text from here]
>> 
>> >          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); [Mark 1]
>
>
> Let us have a look at 
> static int virtio_validate_features(VirtIODevice *vdev)                         
> {                                                                               
>     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);                       
>                                                                                 
>     if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&               
>         !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {              
>         return -EFAULT;                                                         [Mark 2]                  
>     }                                                                           
> [..]
>
> So were it not of the [Mark 1] we could not hit [Mark 2] if the feature
> bit was lost because the ->get_features() callback dropped it. Yes,
> feature negotiation is another code path, but the two are interdependent
> in a non-trivial way. That is why I added that comment.

Yes, of course we need to offer the bit to the driver in the first
place. My point is that the code here is not what makes us fail
FEATURES_OK; we won't even get to that point because the device will
fail realization.

>
> [moved here]
>> So what we do
>> here is failing device realization if a backend doesn't support
>> IOMMU_PLATFORM, isn't it?
>
> Not really. We fail the device realization if !vdev_has_iommu &&
> vdev->dma_as != &address_space_memory, that is the device does not
> support address translation, but we need it to support address
> translation because ->dma_as != &address_space memory. If however
> ->dma_as == &address_space memory we carry on happily even if ->get_features() dropped
> IOMMU_PLATFORM, because we don't actually need an iova -> gpa
> translation. This is the case with virtiofs confidential guests for
> example.
>

Well yes, that's what I meant, I just did not spell out all of the
conditions...

> But we still don't want the guest dropping ACCESS_PLATFORM, because it is
> still mandatory, because the device won't operate correctly unless the
> driver grants access to the pieces of memory that the device needs to
> access. The underlying mechanism of granting access may not have
> anything to do with an IOMMU though.
>
> Does it make sense now?

The code yes, the comment no. What we are actually doing is failing
realization so we don't end up offering a device without IOMMU_PLATFORM
that would need it. The code that fails FEATURES_OK if the driver
dropped it is already in place.

I'd suggest a comment like

/* make sure that the device did not drop a required IOMMU_PLATFORM */

or so.


>
>> > -        vdev->dma_as = klass->get_dma_as(qbus->parent);
>> > -        if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
>> > -            error_setg(errp,
>> > +        if (klass->get_dma_as) {
>> > +            vdev->dma_as = klass->get_dma_as(qbus->parent);
>> > +            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
>> > +                error_setg(errp,
>> >                         "iommu_platform=true is not supported by the device");
>> > +                return;
>> > +            }
>> >          }
>> > -    } else {
>> > -        vdev->dma_as = &address_space_memory;
>> >      }
>> >  }
>> >    
>> 
>> 

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Thu, 10 Feb 2022 10:55:13 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 09 Feb 2022 18:24:56 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:  
> >> > @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
> >> >          return;
> >> >      }
> >> >  
> >> > -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> >> > -    if (klass->get_dma_as != NULL && has_iommu) {
> >> > +    vdev->dma_as = &address_space_memory;
> >> > +    if (has_iommu) {
> >> > +        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> >> > +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */    
> >> 
> >> I must admit that the more I stare at this code, the more confused I
> >> get. We run this function during device realization, and the reason that
> >> the feature bit might have gotten lost is that the ->get_features()
> >> device callback dropped it. This happens before the driver is actually
> >> involved; the check whether the *driver* dropped the feature is done
> >> during feature validation, which is another code path.   
> > [moved text from here]  
> >>   
> >> >          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); [Mark 1]  
> >
> >
> > Let us have a look at 
> > static int virtio_validate_features(VirtIODevice *vdev)                         
> > {                                                                               
> >     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);                       
> >                                                                                 
> >     if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&               
> >         !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {              
> >         return -EFAULT;                                                         [Mark 2]                  
> >     }                                                                           
> > [..]
> >
> > So were it not of the [Mark 1] we could not hit [Mark 2] if the feature
> > bit was lost because the ->get_features() callback dropped it. Yes,
> > feature negotiation is another code path, but the two are interdependent
> > in a non-trivial way. That is why I added that comment.  
> 
> Yes, of course we need to offer the bit to the driver in the first
> place. My point is that the code here is not what makes us fail
> FEATURES_OK; we won't even get to that point because the device will
> fail realization.

I disagree! Have you tested your hypothesis? Which line of code does
cause the device realization to fail? Where is that check?

> 
> >
> > [moved here]  
> >> So what we do
> >> here is failing device realization if a backend doesn't support
> >> IOMMU_PLATFORM, isn't it?  
> >
> > Not really. We fail the device realization if !vdev_has_iommu &&
> > vdev->dma_as != &address_space_memory, that is the device does not
> > support address translation, but we need it to support address
> > translation because ->dma_as != &address_space memory. If however  
> > ->dma_as == &address_space memory we carry on happily even if ->get_features() dropped  
> > IOMMU_PLATFORM, because we don't actually need an iova -> gpa
> > translation. This is the case with virtiofs confidential guests for
> > example.
> >  
> 
> Well yes, that's what I meant, I just did not spell out all of the
> conditions...
> 
> > But we still don't want the guest dropping ACCESS_PLATFORM, because it is
> > still mandatory, because the device won't operate correctly unless the
> > driver grants access to the pieces of memory that the device needs to
> > access. The underlying mechanism of granting access may not have
> > anything to do with an IOMMU though.
> >
> > Does it make sense now?  
> 
> The code yes, the comment no. What we are actually doing is failing
> realization so we don't end up offering a device without IOMMU_PLATFORM
> that would need it. 

I don't understand. That is only one of the possible cases IMHO.

Do you mean the check
        if (klass->get_dma_as) {                                                
            vdev->dma_as = klass->get_dma_as(qbus->parent);                     
            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {     
                error_setg(errp,                                                
                       "iommu_platform=true is not supported by the device");   
                return;                                                         
            }                                                                   
        }
or something different? If yo mean that check, it does not cover all
cases where has_iommu.

Please note that the line in question is

    if (has_iommu) {                                                            
        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */        
        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);  
only conditional on has_iommu.

But we want the guest to *never* drop ACCESS_PLATFORM, regardless of 
vdev_has_iommu and ->dma_as.

Please also note that the comment 
/* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
is intended to document why do we do 
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);  
_only_ and is not intended to document the entire code that follows:

        virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);      
        if (klass->get_dma_as) {                                                
            vdev->dma_as = klass->get_dma_as(qbus->parent);                     
            if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {     
                error_setg(errp,                                                
                       "iommu_platform=true is not supported by the device");   
                return;                                                         
            }                                                                   
        }

Is that the source of the confusion? If yes, maybe I should add a blank
line after virtio_add_feature().

Regards,
Halil

> The code that fails FEATURES_OK if the driver
> dropped it is already in place.
> 
> I'd suggest a comment like
> 
> /* make sure that the device did not drop a required IOMMU_PLATFORM */
> 
> or so.
> 
> 

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Cornelia Huck]

On Thu, Feb 10 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 10 Feb 2022 10:55:13 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
>> 
>> > On Wed, 09 Feb 2022 18:24:56 +0100
>> > Cornelia Huck <cohuck@redhat.com> wrote:
>> >  
>> >> On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:  
>> >> > @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error **errp)
>> >> >          return;
>> >> >      }
>> >> >  
>> >> > -    vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>> >> > -    if (klass->get_dma_as != NULL && has_iommu) {
>> >> > +    vdev->dma_as = &address_space_memory;
>> >> > +    if (has_iommu) {
>> >> > +        vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>> >> > +        /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */    
>> >> 
>> >> I must admit that the more I stare at this code, the more confused I
>> >> get. We run this function during device realization, and the reason that
>> >> the feature bit might have gotten lost is that the ->get_features()
>> >> device callback dropped it. This happens before the driver is actually
>> >> involved; the check whether the *driver* dropped the feature is done
>> >> during feature validation, which is another code path.   
>> > [moved text from here]  
>> >>   
>> >> >          virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); [Mark 1]  
>> >
>> >
>> > Let us have a look at 
>> > static int virtio_validate_features(VirtIODevice *vdev)                         
>> > {                                                                               
>> >     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);                       
>> >                                                                                 
>> >     if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&               
>> >         !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {              
>> >         return -EFAULT;                                                         [Mark 2]                  
>> >     }                                                                           
>> > [..]
>> >
>> > So were it not of the [Mark 1] we could not hit [Mark 2] if the feature
>> > bit was lost because the ->get_features() callback dropped it. Yes,
>> > feature negotiation is another code path, but the two are interdependent
>> > in a non-trivial way. That is why I added that comment.  
>> 
>> Yes, of course we need to offer the bit to the driver in the first
>> place. My point is that the code here is not what makes us fail
>> FEATURES_OK; we won't even get to that point because the device will
>> fail realization.
>
> I disagree! Have you tested your hypothesis? Which line of code does
> cause the device realization to fail? Where is that check?

Because this function is called from the device realization function?

(I do not have time to play with this, sorry.)

>
>> 
>> >
>> > [moved here]  
>> >> So what we do
>> >> here is failing device realization if a backend doesn't support
>> >> IOMMU_PLATFORM, isn't it?  
>> >
>> > Not really. We fail the device realization if !vdev_has_iommu &&
>> > vdev->dma_as != &address_space_memory, that is the device does not
>> > support address translation, but we need it to support address
>> > translation because ->dma_as != &address_space memory. If however  
>> > ->dma_as == &address_space memory we carry on happily even if ->get_features() dropped  
>> > IOMMU_PLATFORM, because we don't actually need an iova -> gpa
>> > translation. This is the case with virtiofs confidential guests for
>> > example.
>> >  
>> 
>> Well yes, that's what I meant, I just did not spell out all of the
>> conditions...
>> 
>> > But we still don't want the guest dropping ACCESS_PLATFORM, because it is
>> > still mandatory, because the device won't operate correctly unless the
>> > driver grants access to the pieces of memory that the device needs to
>> > access. The underlying mechanism of granting access may not have
>> > anything to do with an IOMMU though.
>> >
>> > Does it make sense now?  
>> 
>> The code yes, the comment no. What we are actually doing is failing
>> realization so we don't end up offering a device without IOMMU_PLATFORM
>> that would need it. 
>
> I don't understand. That is only one of the possible cases IMHO.
>
> Do you mean the check
>         if (klass->get_dma_as) {                                                
>             vdev->dma_as = klass->get_dma_as(qbus->parent);                     
>             if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {     
>                 error_setg(errp,                                                
>                        "iommu_platform=true is not supported by the device");   
>                 return;                                                         
>             }                                                                   
>         }
> or something different? If yo mean that check, it does not cover all
> cases where has_iommu.

No.

>
> Please note that the line in question is
>
>     if (has_iommu) {                                                            
>         vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>         /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */        
>         virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);  
> only conditional on has_iommu.
>
> But we want the guest to *never* drop ACCESS_PLATFORM, regardless of 
> vdev_has_iommu and ->dma_as.

I think that's the base issue: sometimes, you talk about ACCESS,
sometimes, you talk about IOMMU. These are the same feature bit, but
used for different purposes. For one condition, the device never gets
realized, and the driver never gets to it. For the other, the driver
will see a device, and it's up to the driver whether it can support it
or not.

>
> Please also note that the comment 
> /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
> is intended to document why do we do 
> virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);  
> _only_ and is not intended to document the entire code that follows:
>
>         virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);      
>         if (klass->get_dma_as) {                                                
>             vdev->dma_as = klass->get_dma_as(qbus->parent);                     
>             if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {     
>                 error_setg(errp,                                                
>                        "iommu_platform=true is not supported by the device");   
>                 return;                                                         
>             }                                                                   
>         }
>
> Is that the source of the confusion? If yes, maybe I should add a blank
> line after virtio_add_feature().

Nope, that's not my problem. We make sure that the bit is persistent, we
fail realization if the bit got removed by the callback when required,
and we fail feature validation if the driver removes the bit, which is
in a different code path. We should not talk about FEATURES_OK in this
code.

We force-add the bit, and then still might fail realization. The
important condition is the has_iommu one, not the checks later on. I
find it very confusing to talk about what a potential driver might do in
that context.

What about moving the virtio_add_feature() after the if
(klass->get_dma_as) check, and adding a comment

/* we want to always force IOMMU_PLATFORM here */

[I'll withdraw from this discussion for now, I fear I might just add
confusion.]

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Thu, 10 Feb 2022 12:19:25 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

[..]
> 
> Nope, that's not my problem. We make sure that the bit is persistent, we
> fail realization if the bit got removed by the callback when required,
> and we fail feature validation if the driver removes the bit, which is
> in a different code path. We should not talk about FEATURES_OK in this
> code.

I agree. I changed my mind. Expanation follows...

> 
> We force-add the bit, and then still might fail realization. The
> important condition is the has_iommu one, not the checks later on. I
> find it very confusing to talk about what a potential driver might do in
> that context.
> 

I assumed stuff like virtiofs+SE regressed with commit 04ceb61a40
("virtio: Fail if iommu_platform is requested, but unsupported") but I
think, I was wrong. It didn't work before that, because we did not
present ACCESS_PLATFORM to the guest. I operated under the assumption
that virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM)
does not impact the set of features offered to the driver by the device,
but it does.

So we need both this patch and "[PATCH v5 1/1] virtio: fix the condition
for iommu_platform not supported" to get virtiofs to work with SE/SEV/Secure VM.

I have to admit I only tested for the error message, and not with a full
SE setup.

I agree my comment was inadequate. Can we use

/* make sure that the device did not drop a required IOMMU_PLATFORM */

I will think some more though. This is again about the dual nature
of ACCESS_PLATFORM...

> What about moving the virtio_add_feature() after the if
> (klass->get_dma_as) check, and adding a comment
> 
> /* we want to always force IOMMU_PLATFORM here */
> 
> [I'll withdraw from this discussion for now, I fear I might just add
> confusion.]
> 
> 

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Michael S. Tsirkin]

On Thu, Feb 10, 2022 at 02:29:29PM +0100, Halil Pasic wrote:
> On Thu, 10 Feb 2022 12:19:25 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> [..]
> > 
> > Nope, that's not my problem. We make sure that the bit is persistent, we
> > fail realization if the bit got removed by the callback when required,
> > and we fail feature validation if the driver removes the bit, which is
> > in a different code path. We should not talk about FEATURES_OK in this
> > code.
> 
> I agree. I changed my mind. Expanation follows...
> 
> > 
> > We force-add the bit, and then still might fail realization. The
> > important condition is the has_iommu one, not the checks later on. I
> > find it very confusing to talk about what a potential driver might do in
> > that context.
> > 
> 
> I assumed stuff like virtiofs+SE regressed with commit 04ceb61a40
> ("virtio: Fail if iommu_platform is requested, but unsupported") but I
> think, I was wrong. It didn't work before that, because we did not
> present ACCESS_PLATFORM to the guest. I operated under the assumption
> that virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM)
> does not impact the set of features offered to the driver by the device,
> but it does.
> 
> So we need both this patch and "[PATCH v5 1/1] virtio: fix the condition
> for iommu_platform not supported" to get virtiofs to work with SE/SEV/Secure VM.
> 
> I have to admit I only tested for the error message, and not with a full
> SE setup.
> 
> I agree my comment was inadequate. Can we use
> 
> /* make sure that the device did not drop a required IOMMU_PLATFORM */
> 
> I will think some more though. This is again about the dual nature
> of ACCESS_PLATFORM...

Were you going to post a new version of this patch?

> > What about moving the virtio_add_feature() after the if
> > (klass->get_dma_as) check, and adding a comment
> > 
> > /* we want to always force IOMMU_PLATFORM here */
> > 
> > [I'll withdraw from this discussion for now, I fear I might just add
> > confusion.]
> > 
> > 

Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Fri, 4 Mar 2022 03:12:03 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 10, 2022 at 02:29:29PM +0100, Halil Pasic wrote:
> > On Thu, 10 Feb 2022 12:19:25 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > [..]  
> > > 
> > > Nope, that's not my problem. We make sure that the bit is persistent, we
> > > fail realization if the bit got removed by the callback when required,
> > > and we fail feature validation if the driver removes the bit, which is
> > > in a different code path. We should not talk about FEATURES_OK in this
> > > code.  
> > 
> > I agree. I changed my mind. Expanation follows...
> >   
> > > 
> > > We force-add the bit, and then still might fail realization. The
> > > important condition is the has_iommu one, not the checks later on. I
> > > find it very confusing to talk about what a potential driver might do in
> > > that context.
> > >   
> > 
> > I assumed stuff like virtiofs+SE regressed with commit 04ceb61a40
> > ("virtio: Fail if iommu_platform is requested, but unsupported") but I
> > think, I was wrong. It didn't work before that, because we did not
> > present ACCESS_PLATFORM to the guest. I operated under the assumption
> > that virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM)
> > does not impact the set of features offered to the driver by the device,
> > but it does.
> > 
> > So we need both this patch and "[PATCH v5 1/1] virtio: fix the condition
> > for iommu_platform not supported" to get virtiofs to work with SE/SEV/Secure VM.
> > 
> > I have to admit I only tested for the error message, and not with a full
> > SE setup.
> > 
> > I agree my comment was inadequate. Can we use
> > 
> > /* make sure that the device did not drop a required IOMMU_PLATFORM */
> > 
> > I will think some more though. This is again about the dual nature
> > of ACCESS_PLATFORM...  
> 
> Were you going to post a new version of this patch?
> 

Sorry I got sidetracked. I will spin a new version today!

Regards,
Halil