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

[Cornelia Huck <cohuck@redhat.com>]

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.]