2024-02-20 07:30:46

by Ankit Agrawal

[permalink] [raw]
Subject: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

From: Ankit Agrawal <[email protected]>

The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64, allowing KVM
stage 2 device mapping attributes to use NormalNC rather than
DEVICE_nGnRE, which allows guest mappings supporting combining
attributes (WC). ARM does not architecturally guarantee this is safe,
and indeed some MMIO regions like the GICv2 VCPU interface can trigger
uncontained faults if NormalNC is used.

Even worse we expect there are platforms where even DEVICE_nGnRE can
allow uncontained faults in corner cases. Unfortunately existing ARM IP
requires platform integration to take responsibility to prevent this.

To safely use VFIO in KVM the platform must guarantee full safety in the
guest where no action taken against a MMIO mapping can trigger an
uncontained failure. We belive that most VFIO PCI platforms support this
for both mapping types, at least in common flows, based on some
expectations of how PCI IP is integrated. This can be enabled more broadly,
for instance into vfio-platform drivers, but only after the platform
vendor completes auditing for safety.

The VMA flag VM_ALLOW_ANY_UNCACHED was found to be the simplest and
cleanest way to communicate the information from VFIO to KVM that
mapping the region in S2 as NormalNC is safe. KVM consumes it to
activate the code that does the S2 mapping as NormalNC.

Suggested-by: Catalin Marinas <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Acked-by: David Hildenbrand <[email protected]>
Signed-off-by: Ankit Agrawal <[email protected]>
---
include/linux/mm.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index f5a97dec5169..59576e56c58b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -391,6 +391,20 @@ extern unsigned int kobjsize(const void *objp);
# define VM_UFFD_MINOR VM_NONE
#endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */

+/*
+ * This flag is used to connect VFIO to arch specific KVM code. It
+ * indicates that the memory under this VMA is safe for use with any
+ * non-cachable memory type inside KVM. Some VFIO devices, on some
+ * platforms, are thought to be unsafe and can cause machine crashes
+ * if KVM does not lock down the memory type.
+ */
+#ifdef CONFIG_64BIT
+#define VM_ALLOW_ANY_UNCACHED_BIT 39
+#define VM_ALLOW_ANY_UNCACHED BIT(VM_ALLOW_ANY_UNCACHED_BIT)
+#else
+#define VM_ALLOW_ANY_UNCACHED VM_NONE
+#endif
+
/* Bits set in the VMA until the stack is in its final location */
#define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ | VM_STACK_EARLY)

--
2.34.1



2024-02-20 08:27:28

by Zhi Wang

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

On 2/20/24 07:29, Ankit Agrawal wrote:
> From: Ankit Agrawal <[email protected]>
>
> The VM_ALLOW_ANY_UNCACHED flag is implemented for ARM64, allowing KVM
> stage 2 device mapping attributes to use NormalNC rather than
> DEVICE_nGnRE, which allows guest mappings supporting combining
> attributes (WC). ARM does not architecturally guarantee this is safe,
> and indeed some MMIO regions like the GICv2 VCPU interface can trigger
> uncontained faults if NormalNC is used.
>
> Even worse we expect there are platforms where even DEVICE_nGnRE can
> allow uncontained faults in corner cases. Unfortunately existing ARM IP
> requires platform integration to take responsibility to prevent this.
>
> To safely use VFIO in KVM the platform must guarantee full safety in the
> guest where no action taken against a MMIO mapping can trigger an
> uncontained failure. We belive that most VFIO PCI platforms support this

A nit, let's use passive voice in the patch comment. Also belive is mostly
a typo.

> for both mapping types, at least in common flows, based on some
> expectations of how PCI IP is integrated. This can be enabled more broadly,
> for instance into vfio-platform drivers, but only after the platform
> vendor completes auditing for safety.
>
> The VMA flag VM_ALLOW_ANY_UNCACHED was found to be the simplest and
> cleanest way to communicate the information from VFIO to KVM that
> mapping the region in S2 as NormalNC is safe. KVM consumes it to
> activate the code that does the S2 mapping as NormalNC.
>
> Suggested-by: Catalin Marinas <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Acked-by: David Hildenbrand <[email protected]>
> Signed-off-by: Ankit Agrawal <[email protected]>
> ---
> include/linux/mm.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index f5a97dec5169..59576e56c58b 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -391,6 +391,20 @@ extern unsigned int kobjsize(const void *objp);
> # define VM_UFFD_MINOR VM_NONE
> #endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */
>
> +/*
> + * This flag is used to connect VFIO to arch specific KVM code. It
> + * indicates that the memory under this VMA is safe for use with any
> + * non-cachable memory type inside KVM. Some VFIO devices, on some
> + * platforms, are thought to be unsafe and can cause machine crashes
> + * if KVM does not lock down the memory type.
> + */
> +#ifdef CONFIG_64BIT
> +#define VM_ALLOW_ANY_UNCACHED_BIT 39
> +#define VM_ALLOW_ANY_UNCACHED BIT(VM_ALLOW_ANY_UNCACHED_BIT)
> +#else
> +#define VM_ALLOW_ANY_UNCACHED VM_NONE
> +#endif
> +
> /* Bits set in the VMA until the stack is in its final location */
> #define VM_STACK_INCOMPLETE_SETUP (VM_RAND_READ | VM_SEQ_READ | VM_STACK_EARLY)
>

2024-02-20 08:54:08

by Ankit Agrawal

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

>> To safely use VFIO in KVM the platform must guarantee full safety in the
>> guest where no action taken against a MMIO mapping can trigger an
>> uncontained failure. We belive that most VFIO PCI platforms support this
>
> A nit, let's use passive voice in the patch comment. Also belive is mostly
> a typo.

Sure, will do.

2024-02-20 09:12:08

by Zhi Wang

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

On 2/20/24 08:51, Ankit Agrawal wrote:
>>> To safely use VFIO in KVM the platform must guarantee full safety in the
>>> guest where no action taken against a MMIO mapping can trigger an
>>> uncontained failure. We belive that most VFIO PCI platforms support this
>>
>> A nit, let's use passive voice in the patch comment. Also belive is mostly
>> a typo.
>
> Sure, will do.
Also patch 4 has the same nit. It should be fixed as well.

2024-02-20 09:12:27

by Ankit Agrawal

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

>>>> To safely use VFIO in KVM the platform must guarantee full safety in the
>>>> guest where no action taken against a MMIO mapping can trigger an
>>>> uncontained failure. We belive that most VFIO PCI platforms support this
>>>
>>> A nit, let's use passive voice in the patch comment. Also belive is mostly
>>> a typo.
>>
>> Sure, will do.
>Also patch 4 has the same nit. It should be fixed as well.

Yes.

2024-02-20 09:13:17

by David Hildenbrand

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

On 20.02.24 09:51, Ankit Agrawal wrote:
>>> To safely use VFIO in KVM the platform must guarantee full safety in the
>>> guest where no action taken against a MMIO mapping can trigger an
>>> uncontained failure. We belive that most VFIO PCI platforms support this
>>
>> A nit, let's use passive voice in the patch comment. Also belive is mostly
>> a typo.
>
> Sure, will do.
>

s/we expect/the expectation is that/
s/We belive/The assumption is/

If it's just that, likely no need to resend. Maintainers usually can fix
that up when applying (otherwise, they'll let you know :) ).

--
Cheers,

David / dhildenb


2024-02-20 09:34:02

by Zhi Wang

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

On 2/20/24 09:09, David Hildenbrand wrote:
> External email: Use caution opening links or attachments
>
>
> On 20.02.24 09:51, Ankit Agrawal wrote:
>>>> To safely use VFIO in KVM the platform must guarantee full safety in
>>>> the
>>>> guest where no action taken against a MMIO mapping can trigger an
>>>> uncontained failure. We belive that most VFIO PCI platforms support
>>>> this
>>>
>>> A nit, let's use passive voice in the patch comment. Also belive is
>>> mostly
>>> a typo.
>>
>> Sure, will do.
>>
>
> s/we expect/the expectation is that/
> s/We belive/The assumption is/
>
> If it's just that, likely no need to resend. Maintainers usually can fix
> that up when applying (otherwise, they'll let you know :) ).
>
Many thanks! :)
> --
> Cheers,
>
> David / dhildenb
>

2024-02-20 10:03:16

by Ankit Agrawal

[permalink] [raw]
Subject: Re: [PATCH v8 2/4] mm: introduce new flag to indicate wc safe

>>>>> To safely use VFIO in KVM the platform must guarantee full safety in
>>>>> the
>>>>> guest where no action taken against a MMIO mapping can trigger an
>>>>> uncontained failure. We belive that most VFIO PCI platforms support
>>>>> this
>>>>
>>>> A nit, let's use passive voice in the patch comment. Also belive is
>>>> mostly
>>>> a typo.
>>>
>>> Sure, will do.
>>>
>>
>> s/we expect/the expectation is that/
>> s/We belive/The assumption is/
>>
>> If it's just that, likely no need to resend. Maintainers usually can fix
>> that up when applying (otherwise, they'll let you know :) ).
>>
> Many thanks! :)

Good to know, thanks David.