by Stefano Stabellini

[permalink] [raw]

Subject: Re: [PATCH V1 6/6] arm/xen: Assign xen-grant DMA ops for xen-grant DMA devices

On Fri, 22 Apr 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[email protected]>
>
> As the main (and single at the moment) purpose of xen-grant
> DMA devices is to enable using virtio devices in Xen guests
> in a safe manner, assign xen-grant DMA ops only if restricted
> access to the guest memory is enabled.
>
> Signed-off-by: Oleksandr Tyshchenko <[email protected]>

Reviewed-by: Stefano Stabellini <[email protected]>

> ---
> Changes RFC -> V1:
> - update commit subject/description
> - remove #ifdef CONFIG_XEN_VIRTIO
> - re-organize the check taking into the account that
> swiotlb and virtio cases are mutually exclusive
> - update according to the new naming scheme:
> s/virtio/grant_dma
> ---
> include/xen/arm/xen-ops.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/xen/arm/xen-ops.h b/include/xen/arm/xen-ops.h
> index 288deb1..26954e5 100644
> --- a/include/xen/arm/xen-ops.h
> +++ b/include/xen/arm/xen-ops.h
> @@ -2,12 +2,17 @@
> #ifndef _ASM_ARM_XEN_OPS_H
> #define _ASM_ARM_XEN_OPS_H
>
> +#include <linux/virtio_config.h>
> #include <xen/swiotlb-xen.h>
> +#include <xen/xen-ops.h>
>
> static inline void xen_setup_dma_ops(struct device *dev)
> {
> #ifdef CONFIG_XEN
> - if (xen_swiotlb_detect())
> + if (arch_has_restricted_virtio_memory_access() &&
> + xen_is_grant_dma_device(dev))
> + xen_grant_setup_dma_ops(dev);
> + else if (xen_swiotlb_detect())
> dev->dma_ops = &xen_swiotlb_dma_ops;
> #endif
> }
> --
> 2.7.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2022-04-23 09:55:34

by Oleksandr Tyshchenko

[permalink] [raw]

Subject: Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen

On 23.04.22 02:00, Stefano Stabellini wrote:

Hello Stefano

> On Fri, 22 Apr 2022, Oleksandr Tyshchenko wrote:
>> From: Juergen Gross <[email protected]>
>>
>> Introduce Xen grant DMA-mapping layer which contains special DMA-mapping
>> routines for providing grant references as DMA addresses to be used by
>> frontends (e.g. virtio) in Xen guests.
>>
>> In order to support virtio in Xen guests add a config option XEN_VIRTIO
>> enabling the user to specify whether in all Xen guests virtio should
>> be able to access memory via Xen grant mappings only on the host side.
>>
>> As this also requires providing arch_has_restricted_virtio_memory_access
>> implementation, switch from a pure stub to a real function on Arm
>> and combine with existing implementation for the SEV guests on x86.
>>
>> Add the needed functionality by providing a special set of DMA ops
>> handling the needed grant operations for the I/O pages.
>>
>> Signed-off-by: Juergen Gross <[email protected]>
>> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
> There are a couple of minor things that checkpatch.pl reports,

Thank you for pointing this out, my fault.

> but aside
> from those the patch looks fine to me.

good

The attached diff to be squashed for the new version. One thing remains:

checkpatch.pl says regarding drivers/xen/grant-dma-ops.c:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#151:
new file mode 100644

Which, I assume, this is not an issue as new file falls under XEN
HYPERVISOR INTERFACE maintainership?

scripts/get_maintainer.pl -f drivers/xen/grant-dma-ops.c
Boris Ostrovsky <[email protected]> (supporter:XEN HYPERVISOR
INTERFACE)
Juergen Gross <[email protected]> (supporter:XEN HYPERVISOR INTERFACE)
Stefano Stabellini <[email protected]> (reviewer:XEN HYPERVISOR
INTERFACE)
[email protected] (moderated list:XEN HYPERVISOR INTERFACE)
[email protected] (open list)

[snip]

--
Regards,

Oleksandr Tyshchenko

Attachments:

0001-fixup.patch (3.15 kB)

2022-04-23 14:12:52

On 24.04.22 18:53, Oleksandr wrote:
>
> On 23.04.22 19:40, Christoph Hellwig wrote:
>
>
> Hello Christoph
>
>> Please split this into one patch that creates grant-dma-ops, and another
>> that sets up the virtio restricted access helpers.
>
>
> Sounds reasonable, will do:
>
> 1. grant-dma-ops.c with config XEN_GRANT_DMA_OPS
>
> 2. arch_has_restricted_virtio_memory_access() with config XEN_VIRTIO
>
>
>>
>>> +
>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>> +int arch_has_restricted_virtio_memory_access(void)
>>> +{
>>> + return (xen_has_restricted_virtio_memory_access() ||
>>> + cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>>> +}
>> So instead of hardcoding Xen here, this seems like a candidate for
>> another cc_platform_has flag.
>
>
> I have a limited knowledge of x86 and Xen on x86.
>
> Would the Xen specific bits fit into Confidential Computing Platform checks? I
> will let Juergen/Boris comment on this.

I don't think cc_platform_has would be correct here. Xen certainly
provides more isolation between guests and dom0, but "Confidential
Computing" is basically orthogonal to that feature.

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2022-04-25 20:22:30

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen

On Mon, Apr 25, 2022 at 09:47:49AM +0200, Juergen Gross wrote:
> > Would the Xen specific bits fit into Confidential Computing Platform
> > checks? I will let Juergen/Boris comment on this.
>
> I don't think cc_platform_has would be correct here. Xen certainly
> provides more isolation between guests and dom0, but "Confidential
> Computing" is basically orthogonal to that feature.

The point of cc_platform_has is to remove all these open code checks.
If a Xen hypervisor / dom0 can't access arbitrary guest memory for
virtual I/O and we need special APIs for that it certainly false
into the scope of cc_platform_has, even if the confientiality is
rather limited.

2022-04-26 03:27:57

by Juergen Gross

[permalink] [raw]

Subject: Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen

On 24.04.22 20:08, Boris Ostrovsky wrote:
>
> On 4/24/22 12:53 PM, Oleksandr wrote:
>>
>> On 23.04.22 19:40, Christoph Hellwig wrote:
>>
>>
>>
>>
>>>
>>>> +
>>>> +#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
>>>> +int arch_has_restricted_virtio_memory_access(void)
>>>> +{
>>>> + return (xen_has_restricted_virtio_memory_access() ||
>>>> + cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT));
>>>> +}
>>> So instead of hardcoding Xen here, this seems like a candidate for
>>> another cc_platform_has flag.
>>
>>
>> I have a limited knowledge of x86 and Xen on x86.
>>
>> Would the Xen specific bits fit into Confidential Computing Platform checks? I
>> will let Juergen/Boris comment on this.
>>
>
> This is unrelated to confidential so I don't think we can add another CC_ flag.
>
>
> Would arch/x86/kernel/cpu/hypervisor.c be a better home for this?

Or a callback in struct struct x86_hyper_runtime maybe?

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2022-04-26 05:20:30

On 25.04.22 23:25, Borislav Petkov wrote:
> On Mon, Apr 25, 2022 at 11:38:36PM +0300, Oleksandr wrote:
>> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
>> index efd8205..d06bc7a 100644
>> --- a/include/linux/cc_platform.h
>> +++ b/include/linux/cc_platform.h
>> @@ -72,6 +72,19 @@ enum cc_attr {
>>          * Examples include TDX guest & SEV.
>>          */
>>         CC_ATTR_GUEST_UNROLL_STRING_IO,
>> +
>> +       /**
>> +        * @CC_ATTR_GUEST_MEM_ACCESS_RESTRICTED: Restricted memory access to
>> +        *                                       Guest memory is active
>> +        *
>> +        * The platform/OS is running as a guest/virtual machine and uses
>> +        * the restricted access to its memory. This attribute is set if
>> either
>> +        * Guest memory encryption or restricted memory access using Xen
>> grant
>> +        * mappings is active.
>> +        *
>> +        * Examples include Xen guest and SEV.
>
> Wait, whaaat?
>
> The cc_platform* stuff is for *confidential computing* guests to check
> different platform aspects.
>
> From quickly skimming over this, this looks like a misuse to me.

Christoph suggested (rather firmly) this would be the way to go.

>
> Why can't you query this from the hypervisor just like you do your other
> querying about what is supported, etc? Hypercalls, CPUID, whatever...

This is needed on guest side at a rather hypervisor independent place.

So a capability of some sort seems appropriate.

Another suggestion of mine was to have a callback (or flag) in
struct x86_hyper_runtime for that purpose.

Juergen

Attachments:

OpenPGP_0xB0DE9DD628BF132F.asc (3.08 kB)
OpenPGP public key OpenPGP_signature (505.00 B)
OpenPGP digital signature Download all attachments

2022-04-26 18:20:05

by Borislav Petkov

[permalink] [raw]

Subject: Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen

On Tue, Apr 26, 2022 at 07:16:16AM +0200, Juergen Gross wrote:
> Christoph suggested (rather firmly) this would be the way to go.

Yeah, I saw it but I don't think it is the right way to go.

What happens the next time a guest needs to query the platform
underneath? Misuse these interfaces again?

Because people will see the Xen use and say, hey, look, I will use this
for my funky HV too.

Even worse: what happens if Xen decides to implement SEV/TDX? Then
you're in for a world of fun.

Now, if we want to *extend* the interfaces to have something as generic
as, say, platform_has() and that should be the way for generic kernel
code running in the guest to query the platform capabilities, then sure,
by all means.

> This is needed on guest side at a rather hypervisor independent place.
>
> So a capability of some sort seems appropriate.
>
> Another suggestion of mine was to have a callback (or flag) in
> struct x86_hyper_runtime for that purpose.

This becomes an issue if the HV is not x86 - then you need a different
method of querying it, which then underneath will call the arch-specific
interface.

I don't know how much of querying guests need to do and how they've been
doing that so far. Depending on the requirements, we probably should
think about a clean design from the get-go instead of homegrown things.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2022-04-26 20:46:42

by Borislav Petkov

[permalink] [raw]

Subject: Re: [PATCH V1 3/6] xen/virtio: Add option to restrict memory access under Xen

On Tue, Apr 26, 2022 at 11:36:40AM +0200, Juergen Gross wrote:
> As the suggestion was to add another flag this wouldn't be a problem IMO.

We had a problem already with adding one flag would break the same flag
on the other guest type. That's why we added cc_vendor too. So it can be
tricky.

> platform_has() doesn't seem too bad IMO.
>
> I will write a patch for starting the discussion.

Yeah, I guess such a proposal would need a wider audience - maybe CC
linux-arch...

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2022-04-27 11:26:31

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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
>>>> +
>>>> +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