Re: [PATCH V1 4/6] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops

[Oleksandr <olekstysh@gmail.com>]

On 04.05.22 03:02, Rob Herring wrote:

Hello Rob

> On Tue, May 03, 2022 at 08:09:32PM +0300, Oleksandr wrote:
>> On 03.05.22 00:59, Rob Herring wrote:
>>
>> Hello Rob
>>
>>
>>> On Fri, Apr 22, 2022 at 07:51:01PM +0300, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> Introduce Xen specific binding for the virtualized device (e.g. virtio)
>>>> to be used by Xen grant DMA-mapping layer in the subsequent commit.
>>>>
>>>> This binding indicates that Xen grant mappings scheme needs to be
>>>> enabled for the device which DT node contains that property and specifies
>>>> the ID of Xen domain where the corresponding backend resides. The ID
>>>> (domid) is used as an argument to the grant mapping APIs.
>>>>
>>>> This is needed for the option to restrict memory access using Xen grant
>>>> mappings to work which primary goal is to enable using virtio devices
>>>> in Xen guests.
>>>>
>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>> Changes RFC -> V1:
>>>>      - update commit subject/description and text in description
>>>>      - move to devicetree/bindings/arm/
>>>> ---
>>>>    .../devicetree/bindings/arm/xen,dev-domid.yaml     | 37 ++++++++++++++++++++++
>>>>    1 file changed, 37 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>>>> new file mode 100644
>>>> index 00000000..ef0f747
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>>>> @@ -0,0 +1,37 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/arm/xen,dev-domid.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Xen specific binding for the virtualized device (e.g. virtio)
>>>> +
>>>> +maintainers:
>>>> +  - Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> +
>>>> +select: true
>>> Do we really need to support this property everywhere?
>>  From my understanding - yes.
>>
>> As, I think, any device node describing virtulized device in the guest
>> device tree can have this property.  Initially (in the RFC series) the
>> "solution to restrict memory access using Xen grant mappings" was
>> virtio-specific.
>>
>> Although the support of virtio is a primary target of this series, we
>> decided to generalize this work and expand it to any device [1]. So the Xen
>> grant mappings scheme (this property to be used for) can be theoretically
>> used for any device emulated by the Xen backend.
>>
>>
>>>> +
>>>> +description:
>>>> +  This binding indicates that Xen grant mappings scheme needs to be enabled
>>>> +  for that device and specifies the ID of Xen domain where the corresponding
>>>> +  device (backend) resides. This is needed for the option to restrict memory
>>>> +  access using Xen grant mappings to work.
>>>> +
>>>> +properties:
>>>> +  xen,dev-domid:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      The domid (domain ID) of the domain where the device (backend) is running.
>>>> +
>>>> +additionalProperties: true
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    virtio_block@3000 {
>>> virtio@3000
>> ok, will change
>>
>>
>>>> +            compatible = "virtio,mmio";
>>>> +            reg = <0x3000 0x100>;
>>>> +            interrupts = <41>;
>>>> +
>>>> +            /* The device is located in Xen domain with ID 1 */
>>>> +            xen,dev-domid = <1>;
>>> This fails validation:
>>>
>>> Documentation/devicetree/bindings/arm/xen,dev-domid.example.dtb: virtio_block@3000: xen,dev-domid: [[1]] is not of type 'object'
>>>           From schema: /home/rob/proj/git/linux-dt/Documentation/devicetree/bindings/virtio/mmio.yaml
>> Thank you for pointing this out, my fault, I haven't "properly" checked this
>> before. I think, we need to remove "compatible = "virtio,mmio"; here
> Uhh, no. That just means the example is incomplete. You need to add this
> property or reference this schema from virtio/mmio.yaml.

ok, I got it


>
>
>> diff --git a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>> b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>> index 2daa8aa..d2f2140 100644
>> --- a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>> +++ b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>> @@ -28,7 +28,7 @@ additionalProperties: true
>>   examples:
>>     - |
>>       virtio_block@3000 {
>> -            compatible = "virtio,mmio";
>> +            /* ... */
>>               reg = <0x3000 0x100>;
>>               interrupts = <41>;
>>
>>
>>
>>> The property has to be added to the virtio/mmio.yaml schema. If it is
>>> not needed elsewhere, then *just* add the property there.
>> As I described above, the property is not virtio specific and can be used
>> for any virtualized device for which Xen grant mappings scheme needs to be
>> enabled (xen-grant DMA-mapping layer).
> But that's a finite list of devices, right?

Right


> In any case, you have to
> list the property anywhere it can be used.

Agree


If I got it right, we need to add to virtio/mmio.yaml something like:


diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml 
b/Documentation/devicetree/bindings/virtio/mmio.yaml
index 10c22b5..29a0932 100644
--- a/Documentation/devicetree/bindings/virtio/mmio.yaml
+++ b/Documentation/devicetree/bindings/virtio/mmio.yaml
@@ -13,6 +13,9 @@ description:
    See 
https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio for
    more details.

+allOf:
+  - $ref: /schemas/arm/xen,dev-domid.yaml#
+
  properties:
    compatible:
      const: virtio,mmio
@@ -33,6 +36,10 @@ properties:
      description: Required for devices making accesses thru an IOMMU.
      maxItems: 1

+  xen,dev-domid:
+    description: Required when Xen grant mappings need to be enabled 
for device.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
  required:
    - compatible
    - reg


This passed validation.


Could you please clarify, is my understanding correct?


>
> Rob

-- 
Regards,

Oleksandr Tyshchenko