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

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

[Halil Pasic]

Unlike most virtio features ACCESS_PATFORM is considered mandatory, 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.

If ACCESS_PLATFORM is offered not 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")

---
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;
     }
 }
 

base-commit: da89f242b4b774a25eaa16be125cf3e17299c127
-- 
2.32.0

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

[Daniel Henrique Barboza]

On 2/3/22 13:45, Halil Pasic wrote:
> Unlike most virtio features ACCESS_PATFORM is considered mandatory, 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


I believe a link to the virtio specification where this is being mentioned would
be good to have in the commit message.


> 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.
> 
> If ACCESS_PLATFORM is offered not we want the device to utilize an

I think you meant "If ACCESS_PLATFORM is offered".


> 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")
> 
> ---
> 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;

At this point you can also do:

    if (!has_iommu) {
        return;
    }

and the rest of the code will have one less indentation level.


Thanks,


Daniel



> +    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;
>       }
>   }
>   
> 
> base-commit: da89f242b4b774a25eaa16be125cf3e17299c127

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

[Cornelia Huck]

On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 2/3/22 13:45, Halil Pasic wrote:
>> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.

s/ACCESS_PATFORM/ACCESS_PLATFORM/

>> 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
>
>
> I believe a link to the virtio specification where this is being mentioned would
> be good to have in the commit message.

It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."

That said, I'm not sure the wording in the spec translates to
"mandatory"... if the driver fails to accept the bit, the device can
choose to not work with the driver, but it's not forced to. There are
other instances where the device may reject FEATURES_OK (e.g. when the
driver does not accept a feature that is a pre-req for another feature),
I'd say it is up to the device whether something is mandatory or not. If
the device/setup cannot work without it, it certainly is mandatory, but
the driver only knows when FEATURES_OK is rejected without the feature.

OTOH, the decision to make it mandatory is certainly sound, and covered
by the spec. As the driver must be prepared for the device failing to
accept FEATURES_OK, we can make it mandatory here -- we should just not
say that it is considered mandatory from a spec standpoint. The spec
allows to make it mandatory, and we make it mandatory in our
implementation.

>
>
>> 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.
>> 
>> If ACCESS_PLATFORM is offered not we want the device to utilize an
>
> I think you meant "If ACCESS_PLATFORM is offered".

I thought it should be "If ACCESS_PLATFORM is offered not because..." ?

>
>
>> 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")
>> 
>> ---
>> 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;
>
> At this point you can also do:
>
>     if (!has_iommu) {
>         return;
>     }
>
> and the rest of the code will have one less indentation level.

It might make it harder to add code at the tail end of the function in
the future, though.

>
>
> Thanks,
>
>
> Daniel
>
>
>
>> +    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;
>>       }
>>   }
>>   
>> 
>> base-commit: da89f242b4b774a25eaa16be125cf3e17299c127

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

[Daniel Henrique Barboza]

On 2/7/22 10:41, Cornelia Huck wrote:
> On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
>> On 2/3/22 13:45, Halil Pasic wrote:
>>> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.
> 
> s/ACCESS_PATFORM/ACCESS_PLATFORM/
> 
>>> 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
>>
>>
>> I believe a link to the virtio specification where this is being mentioned would
>> be good to have in the commit message.
> 
> It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
> driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
> section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
> fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."

If we provide this info in the commit message we can skip adding a doc link.

> 
> That said, I'm not sure the wording in the spec translates to
> "mandatory"... if the driver fails to accept the bit, the device can
> choose to not work with the driver, but it's not forced to. There are
> other instances where the device may reject FEATURES_OK (e.g. when the
> driver does not accept a feature that is a pre-req for another feature),
> I'd say it is up to the device whether something is mandatory or not. If
> the device/setup cannot work without it, it certainly is mandatory, but
> the driver only knows when FEATURES_OK is rejected without the feature.
> 
> OTOH, the decision to make it mandatory is certainly sound, and covered
> by the spec. As the driver must be prepared for the device failing to
> accept FEATURES_OK, we can make it mandatory here -- we should just not
> say that it is considered mandatory from a spec standpoint. The spec
> allows to make it mandatory, and we make it mandatory in our
> implementation.

Fair point.

> 
>>
>>
>>> 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.
>>>
>>> If ACCESS_PLATFORM is offered not we want the device to utilize an
>>
>> I think you meant "If ACCESS_PLATFORM is offered".
> 
> I thought it should be "If ACCESS_PLATFORM is offered not because..." ?
> 
>>
>>
>>> 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")
>>>
>>> ---
>>> 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;
>>
>> At this point you can also do:
>>
>>      if (!has_iommu) {
>>          return;
>>      }
>>
>> and the rest of the code will have one less indentation level.
> 
> It might make it harder to add code at the tail end of the function in
> the future, though.

True. I suggested that based on an assumption that the "!has_iommu" case is something
that is already covered and we don't need to bother about it. IMO it's fine to keep
the existing if/else code just in case we change our minds later on.


Thanks,


Daniel





> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>
>>> +    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;
>>>        }
>>>    }
>>>    
>>>
>>> base-commit: da89f242b4b774a25eaa16be125cf3e17299c127
> 

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

[Halil Pasic]

On Mon, 07 Feb 2022 14:41:58 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
> > On 2/3/22 13:45, Halil Pasic wrote:  
> >> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.  
> 
> s/ACCESS_PATFORM/ACCESS_PLATFORM/

Will fix.

> 
> >> 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  
> >
> >
> > I believe a link to the virtio specification where this is being mentioned would
> > be good to have in the commit message.  
> 
> It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
> driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
> section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
> fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."
> 
> That said, I'm not sure the wording in the spec translates to
> "mandatory"... if the driver fails to accept the bit, the device can
> choose to not work with the driver, but it's not forced to.

I didn't mean to claim that the spec makes this feature "mandatory", and
this is why I paraphrased the spec. IMHO it is QEMU that considers it
mandatory.

> There are
> other instances where the device may reject FEATURES_OK (e.g. when the
> driver does not accept a feature that is a pre-req for another feature),
> I'd say it is up to the device whether something is mandatory or not. If
> the device/setup cannot work without it, it certainly is mandatory, but
> the driver only knows when FEATURES_OK is rejected without the feature.

Right but for the guys that write the drivers it is of interest to know
what level of interoperability can  one can keep if certain
	features are
not implemented. Usually it is safe to fence delay implementing
features, as long as the support for the features is implemented in the
order mandated by the spec.

> 
> OTOH, the decision to make it mandatory is certainly sound, and covered
> by the spec. As the driver must be prepared for the device failing to
> accept FEATURES_OK, we can make it mandatory here -- we should just not
> say that it is considered mandatory from a spec standpoint. The spec
> allows to make it mandatory, and we make it mandatory in our
> implementation.

Right. Was never my intention to say that it is considered mandatory
by the spec. I guess the spec considers it less optional than the
run of the mill features.

Should I change the first sentence to something like "Unlike most virtio
features ACCESS_PATFORM is considered mandatory by QEMU, i.e. the driver
must accept it if offered by the device."

[..]

Regards,
Halil

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

[Cornelia Huck]

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

> On Mon, 07 Feb 2022 14:41:58 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:

>> OTOH, the decision to make it mandatory is certainly sound, and covered
>> by the spec. As the driver must be prepared for the device failing to
>> accept FEATURES_OK, we can make it mandatory here -- we should just not
>> say that it is considered mandatory from a spec standpoint. The spec
>> allows to make it mandatory, and we make it mandatory in our
>> implementation.
>
> Right. Was never my intention to say that it is considered mandatory
> by the spec. I guess the spec considers it less optional than the
> run of the mill features.
>
> Should I change the first sentence to something like "Unlike most virtio
> features ACCESS_PATFORM is considered mandatory by QEMU, i.e. the driver
> must accept it if offered by the device."

If you do s/PATFORM/PLATFORM/ :), yes. That's a much shorter way of
expressing what I had been trying to argue in my reply :)

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

[Halil Pasic]

On Mon, 07 Feb 2022 16:21:31 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Feb 07 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 07 Feb 2022 14:41:58 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:  
> 
> >> OTOH, the decision to make it mandatory is certainly sound, and covered
> >> by the spec. As the driver must be prepared for the device failing to
> >> accept FEATURES_OK, we can make it mandatory here -- we should just not
> >> say that it is considered mandatory from a spec standpoint. The spec
> >> allows to make it mandatory, and we make it mandatory in our
> >> implementation.  
> >
> > Right. Was never my intention to say that it is considered mandatory
> > by the spec. I guess the spec considers it less optional than the
> > run of the mill features.
> >
> > Should I change the first sentence to something like "Unlike most virtio
> > features ACCESS_PATFORM is considered mandatory by QEMU, i.e. the driver
> > must accept it if offered by the device."  
> 
> If you do s/PATFORM/PLATFORM/ :), yes. That's a much shorter way of
> expressing what I had been trying to argue in my reply :)
> 

Will do! I'm going to wait a little more before spinning a v1 to give
people a little more time to complain about the objective of this patch.

Regards,
Halil

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

[Michael S. Tsirkin]

On Mon, Feb 07, 2022 at 04:05:16PM +0100, Halil Pasic wrote:
> On Mon, 07 Feb 2022 14:41:58 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > 
> > > On 2/3/22 13:45, Halil Pasic wrote:  
> > >> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.  
> > 
> > s/ACCESS_PATFORM/ACCESS_PLATFORM/
> 
> Will fix.
> 
> > 
> > >> 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  
> > >
> > >
> > > I believe a link to the virtio specification where this is being mentioned would
> > > be good to have in the commit message.  
> > 
> > It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
> > driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
> > section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
> > fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."
> > 
> > That said, I'm not sure the wording in the spec translates to
> > "mandatory"... if the driver fails to accept the bit, the device can
> > choose to not work with the driver, but it's not forced to.
> 
> I didn't mean to claim that the spec makes this feature "mandatory", and
> this is why I paraphrased the spec. IMHO it is QEMU that considers it
> mandatory.

this:
A device MAY
fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted

is the hint here.

> > There are
> > other instances where the device may reject FEATURES_OK (e.g. when the
> > driver does not accept a feature that is a pre-req for another feature),
> > I'd say it is up to the device whether something is mandatory or not. If
> > the device/setup cannot work without it, it certainly is mandatory, but
> > the driver only knows when FEATURES_OK is rejected without the feature.
> 
> Right but for the guys that write the drivers it is of interest to know
> what level of interoperability can  one can keep if certain
> 	features are
> not implemented. Usually it is safe to fence delay implementing
> features, as long as the support for the features is implemented in the
> order mandated by the spec.
> 
> > 
> > OTOH, the decision to make it mandatory is certainly sound, and covered
> > by the spec. As the driver must be prepared for the device failing to
> > accept FEATURES_OK, we can make it mandatory here -- we should just not
> > say that it is considered mandatory from a spec standpoint. The spec
> > allows to make it mandatory, and we make it mandatory in our
> > implementation.
> 
> Right. Was never my intention to say that it is considered mandatory
> by the spec. I guess the spec considers it less optional than the
> run of the mill features.

It would be nice to have a security considerations section.

The point is that within guest, with ACCESS_PLATFORM it should be safe
to assume that device can be passed through to nested guests or
userspace.



> Should I change the first sentence to something like "Unlike most virtio
> features ACCESS_PATFORM is considered mandatory by QEMU, i.e. the driver
> must accept it if offered by the device."
> 
> [..]
> 
> Regards,
> Halil

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

[Halil Pasic]

On Mon, 7 Feb 2022 08:46:34 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 2/3/22 13:45, Halil Pasic wrote:
> > Unlike most virtio features ACCESS_PATFORM is considered mandatory, 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  
> 
> 
> I believe a link to the virtio specification where this is being mentioned would
> be good to have in the commit message.

I can add that if Michael agrees, and if the patch is deemed worthy.
> 
> 
> > 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.
> > 
> > If ACCESS_PLATFORM is offered not we want the device to utilize an  
> 
> I think you meant "If ACCESS_PLATFORM is offered".

I'm missing because. I.e. s/not/not becasue/
> 
> 
> > 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")
> > 
> > ---
> > 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;  
> 
> At this point you can also do:
> 
>     if (!has_iommu) {
>         return;
>     }
> 
> and the rest of the code will have one less indentation level.

I have considered this and decided against it. The reason why is
if that approach is taken, we can't really add more code to the
end of the function. An early return is good if we want to
abort the function with an error. My point is !has_iommu does
not necessarily mean we are done: after a block that handles
the has_iommu situation, in future, there could be a block that
handles something different.

Would this patch work for power? Or are there valid scenarios that
it breaks? I'm asking, because you voiced concern regarding this before.

Thanks for your feedback!

Halil

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

[Daniel Henrique Barboza]

On 2/7/22 11:46, Halil Pasic wrote:
> On Mon, 7 Feb 2022 08:46:34 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
>> On 2/3/22 13:45, Halil Pasic wrote:
>>> Unlike most virtio features ACCESS_PATFORM is considered mandatory, 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
>>
>>
>> I believe a link to the virtio specification where this is being mentioned would
>> be good to have in the commit message.
> 
> I can add that if Michael agrees, and if the patch is deemed worthy.
>>
>>
>>> 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.
>>>
>>> If ACCESS_PLATFORM is offered not we want the device to utilize an
>>
>> I think you meant "If ACCESS_PLATFORM is offered".
> 
> I'm missing because. I.e. s/not/not becasue/
>>
>>
>>> 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")
>>>
>>> ---
>>> 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;
>>
>> At this point you can also do:
>>
>>      if (!has_iommu) {
>>          return;
>>      }
>>
>> and the rest of the code will have one less indentation level.
> 
> I have considered this and decided against it. The reason why is
> if that approach is taken, we can't really add more code to the
> end of the function. An early return is good if we want to
> abort the function with an error. My point is !has_iommu does
> not necessarily mean we are done: after a block that handles
> the has_iommu situation, in future, there could be a block that
> handles something different.

And that's fine, but the way this patch is changing it I'm not sure it's better
than what we already have. Today we have:

if (has_iommu) {
   (... assign vdev->dma_as in some cases ...)
} else {
    vdev->dma_as = &address_space_memory;
}


Your patch is doing:

vdev->dma_as = &address_space_memory;

if (has_iommu) {
   (... assign vdev->dma_as in some cases ...)
}


You got rid of an 'else', but ended up adding a double "vdev->dma_as =" assignment
depending on the case (has_iommu = true and klass->get_dma_as != NULL). This is why
I proposed the early exit.

If we're worried about adding more code in the future might as well leave the existing
if/else as is.
        


> 
> Would this patch work for power? Or are there valid scenarios that
> it breaks? I'm asking, because you voiced concern regarding this before.


I'll test it when I have an opportunity and let you know.


Thanks,


Daniel

> 
> Thanks for your feedback!
> 
> Halil

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

[Halil Pasic]

On Mon, 7 Feb 2022 16:46:04 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> On 2/7/22 11:46, Halil Pasic wrote:
> > On Mon, 7 Feb 2022 08:46:34 -0300
> > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> >   
> > I have considered this and decided against it. The reason why is
> > if that approach is taken, we can't really add more code to the
> > end of the function. An early return is good if we want to
> > abort the function with an error. My point is !has_iommu does
> > not necessarily mean we are done: after a block that handles
> > the has_iommu situation, in future, there could be a block that
> > handles something different.  
> 
> And that's fine, but the way this patch is changing it I'm not sure it's better
> than what we already have. Today we have:
> 
> if (has_iommu) {

To be exact today we have :
if (klass->get_dma_as != NULL && has_iommu) {


>    (... assign vdev->dma_as in some cases ...)

Today not in some case but unconditionally. WE already checked for
!!klass->get_dma_as and that is important.

Because if you rewrite current to what you have just described here,
then in this branch of the if-else you have to handle !klass->get_dma_as.

So you would have to do
    if (klass->get_dma_as) {
        vdev->dma_as = klass->get_dma_as();
        if (cond) {
            do_error();
        }
    } else {
	vdev->dma_as = &address_space_memory;
    }

> } else {
>     vdev->dma_as = &address_space_memory;
> }
> 
> 
> Your patch is doing:
> 
> vdev->dma_as = &address_space_memory;
> 
> if (has_iommu) {
>    (... assign vdev->dma_as in some cases ...)
> }
> 
> 
> You got rid of an 'else', but ended up adding a double "vdev->dma_as =" assignment
> depending on the case (has_iommu = true and klass->get_dma_as != NULL). 

And why is that bad?

The solution I wrote is very clear about vdev->dma_as != NULL and that
vdev->dma_as conceptually defaults to &address_space_memory, and may
deviate from that only if both has_iommu and klass->get_dma_as != NULL
in which case get_dma_as() may override it to something different.

The compile can still generate branches and stores as it pleases
as long as the behavior is the same AFAIK. 

> This is why
> I proposed the early exit.
> 
> If we're worried about adding more code in the future might as well leave the existing
> if/else as is.
> 

Not really, we would end up having two extra else branches instead of
none (with 3 if-s in both cases) and 3 places where we might assign
->dma_as (although mutually exclusive) instead of just two.

For me my version is easier to read.

        
> 
> 
> > 
> > Would this patch work for power? Or are there valid scenarios that
> > it breaks? I'm asking, because you voiced concern regarding this before.  
> 
> 
> I'll test it when I have an opportunity and let you know.
> 
> 

Thank you!

Regards,
Halil