[PATCH 0/2] kernel: add new infrastructure for platform_has() support

[PATCH 0/2] kernel: add new infrastructure for platform_has() support

[Juergen Gross]

In another patch series [1] the need has come up to have support for
a generic feature flag infrastructure.

This patch series is introducing that infrastructure and adds the first
use case.

I have decided to use a similar interface as the already known x86
cpu_has() function. As the new infrastructure is meant to be usable for
general and arch-specific feature flags, the flags are being spread
between a general bitmap and an arch specific one.

The bitmaps start all being zero, single features can be set or reset
at any time by using the related platform_[re]set_feature() functions.

The platform_has() function is using a simple test_bit() call for now,
further optimization might be added when needed.

[1]: https://lore.kernel.org/lkml/1650646263-22047-1-git-send-email-olekstysh@gmail.com/T/#t

Juergen Gross (2):
  kernel: add platform_has() infrastructure
  virtio: replace arch_has_restricted_virtio_memory_access()

 MAINTAINERS                            |  8 +++++++
 arch/s390/Kconfig                      |  1 -
 arch/s390/mm/init.c                    | 13 +++--------
 arch/x86/Kconfig                       |  1 -
 arch/x86/kernel/cpu/mshyperv.c         |  5 ++++-
 arch/x86/mm/mem_encrypt.c              |  6 ------
 arch/x86/mm/mem_encrypt_identity.c     |  5 +++++
 drivers/virtio/Kconfig                 |  6 ------
 drivers/virtio/virtio.c                |  5 ++---
 include/asm-generic/Kbuild             |  1 +
 include/asm-generic/platform-feature.h |  8 +++++++
 include/linux/platform-feature.h       | 30 ++++++++++++++++++++++++++
 include/linux/virtio_config.h          |  9 --------
 kernel/Makefile                        |  2 +-
 kernel/platform-feature.c              |  7 ++++++
 15 files changed, 69 insertions(+), 38 deletions(-)
 create mode 100644 include/asm-generic/platform-feature.h
 create mode 100644 include/linux/platform-feature.h
 create mode 100644 kernel/platform-feature.c

-- 
2.34.1

[PATCH 0/2] kernel: add new infrastructure for platform_has() support

[Juergen Gross via Virtualization]

In another patch series [1] the need has come up to have support for
a generic feature flag infrastructure.

This patch series is introducing that infrastructure and adds the first
use case.

I have decided to use a similar interface as the already known x86
cpu_has() function. As the new infrastructure is meant to be usable for
general and arch-specific feature flags, the flags are being spread
between a general bitmap and an arch specific one.

The bitmaps start all being zero, single features can be set or reset
at any time by using the related platform_[re]set_feature() functions.

The platform_has() function is using a simple test_bit() call for now,
further optimization might be added when needed.

[1]: https://lore.kernel.org/lkml/1650646263-22047-1-git-send-email-olekstysh@gmail.com/T/#t

Juergen Gross (2):
  kernel: add platform_has() infrastructure
  virtio: replace arch_has_restricted_virtio_memory_access()

 MAINTAINERS                            |  8 +++++++
 arch/s390/Kconfig                      |  1 -
 arch/s390/mm/init.c                    | 13 +++--------
 arch/x86/Kconfig                       |  1 -
 arch/x86/kernel/cpu/mshyperv.c         |  5 ++++-
 arch/x86/mm/mem_encrypt.c              |  6 ------
 arch/x86/mm/mem_encrypt_identity.c     |  5 +++++
 drivers/virtio/Kconfig                 |  6 ------
 drivers/virtio/virtio.c                |  5 ++---
 include/asm-generic/Kbuild             |  1 +
 include/asm-generic/platform-feature.h |  8 +++++++
 include/linux/platform-feature.h       | 30 ++++++++++++++++++++++++++
 include/linux/virtio_config.h          |  9 --------
 kernel/Makefile                        |  2 +-
 kernel/platform-feature.c              |  7 ++++++
 15 files changed, 69 insertions(+), 38 deletions(-)
 create mode 100644 include/asm-generic/platform-feature.h
 create mode 100644 include/linux/platform-feature.h
 create mode 100644 kernel/platform-feature.c

-- 
2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/2] kernel: add platform_has() infrastructure

[Juergen Gross]

Add a simple infrastructure for setting, resetting and querying
platform feature flags.

Flags can be either global or architecture specific.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 MAINTAINERS                            |  8 +++++++
 include/asm-generic/Kbuild             |  1 +
 include/asm-generic/platform-feature.h |  8 +++++++
 include/linux/platform-feature.h       | 29 ++++++++++++++++++++++++++
 kernel/Makefile                        |  2 +-
 kernel/platform-feature.c              |  7 +++++++
 6 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 include/asm-generic/platform-feature.h
 create mode 100644 include/linux/platform-feature.h
 create mode 100644 kernel/platform-feature.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e8c2f611766..eb943f089eda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15650,6 +15650,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
 F:	drivers/iio/chemical/pms7003.c
 
+PLATFORM FEATURE INFRASTRUCTURE
+M:	Juergen Gross <jgross@suse.com>
+S:	Maintained
+F:	arch/*/include/asm/platform-feature.h
+F:	include/asm-generic/platform-feature.h
+F:	include/linux/platform-feature.h
+F:	kernel/platform-feature.c
+
 PLDMFW LIBRARY
 M:	Jacob Keller <jacob.e.keller@intel.com>
 S:	Maintained
diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index 302506bbc2a4..8e47d483b524 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -44,6 +44,7 @@ mandatory-y += msi.h
 mandatory-y += pci.h
 mandatory-y += percpu.h
 mandatory-y += pgalloc.h
+mandatory-y += platform-feature.h
 mandatory-y += preempt.h
 mandatory-y += rwonce.h
 mandatory-y += sections.h
diff --git a/include/asm-generic/platform-feature.h b/include/asm-generic/platform-feature.h
new file mode 100644
index 000000000000..4b0af3d51588
--- /dev/null
+++ b/include/asm-generic/platform-feature.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_PLATFORM_FEATURE_H
+#define _ASM_GENERIC_PLATFORM_FEATURE_H
+
+/* Number of arch specific feature flags. */
+#define PLATFORM_ARCH_FEAT_N	0
+
+#endif /* _ASM_GENERIC_PLATFORM_FEATURE_H */
diff --git a/include/linux/platform-feature.h b/include/linux/platform-feature.h
new file mode 100644
index 000000000000..df393d502a4f
--- /dev/null
+++ b/include/linux/platform-feature.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _PLATFORM_FEATURE_H
+#define _PLATFORM_FEATURE_H
+
+#include <linux/bitops.h>
+#include <asm/platform-feature.h>
+
+/* The platform features are starting with the architecture specific ones. */
+#define PLATFORM_FEAT_N				(0 + PLATFORM_ARCH_FEAT_N)
+
+#define PLATFORM_FEAT_ARRAY_SZ	BITS_TO_LONGS(PLATFORM_FEAT_N)
+extern unsigned long platform_features[PLATFORM_FEAT_ARRAY_SZ];
+
+static inline void platform_set_feature(unsigned int feature)
+{
+	set_bit(feature, platform_features);
+}
+
+static inline void platform_reset_feature(unsigned int feature)
+{
+	clear_bit(feature, platform_features);
+}
+
+static inline bool platform_has(unsigned int feature)
+{
+	return test_bit(feature, platform_features);
+}
+
+#endif /* _PLATFORM_FEATURE_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index 847a82bfe0e3..2f412f80110d 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -7,7 +7,7 @@ obj-y     = fork.o exec_domain.o panic.o \
 	    cpu.o exit.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o user.o \
 	    signal.o sys.o umh.o workqueue.o pid.o task_work.o \
-	    extable.o params.o \
+	    extable.o params.o platform-feature.o \
 	    kthread.o sys_ni.o nsproxy.o \
 	    notifier.o ksysfs.o cred.o reboot.o \
 	    async.o range.o smpboot.o ucount.o regset.o
diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
new file mode 100644
index 000000000000..2d52f8442cd5
--- /dev/null
+++ b/kernel/platform-feature.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/cache.h>
+#include <linux/platform-feature.h>
+
+unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];
+EXPORT_SYMBOL_GPL(platform_features);
-- 
2.34.1

[PATCH 1/2] kernel: add platform_has() infrastructure

[Juergen Gross via Virtualization]

Add a simple infrastructure for setting, resetting and querying
platform feature flags.

Flags can be either global or architecture specific.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 MAINTAINERS                            |  8 +++++++
 include/asm-generic/Kbuild             |  1 +
 include/asm-generic/platform-feature.h |  8 +++++++
 include/linux/platform-feature.h       | 29 ++++++++++++++++++++++++++
 kernel/Makefile                        |  2 +-
 kernel/platform-feature.c              |  7 +++++++
 6 files changed, 54 insertions(+), 1 deletion(-)
 create mode 100644 include/asm-generic/platform-feature.h
 create mode 100644 include/linux/platform-feature.h
 create mode 100644 kernel/platform-feature.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e8c2f611766..eb943f089eda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15650,6 +15650,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
 F:	drivers/iio/chemical/pms7003.c
 
+PLATFORM FEATURE INFRASTRUCTURE
+M:	Juergen Gross <jgross@suse.com>
+S:	Maintained
+F:	arch/*/include/asm/platform-feature.h
+F:	include/asm-generic/platform-feature.h
+F:	include/linux/platform-feature.h
+F:	kernel/platform-feature.c
+
 PLDMFW LIBRARY
 M:	Jacob Keller <jacob.e.keller@intel.com>
 S:	Maintained
diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
index 302506bbc2a4..8e47d483b524 100644
--- a/include/asm-generic/Kbuild
+++ b/include/asm-generic/Kbuild
@@ -44,6 +44,7 @@ mandatory-y += msi.h
 mandatory-y += pci.h
 mandatory-y += percpu.h
 mandatory-y += pgalloc.h
+mandatory-y += platform-feature.h
 mandatory-y += preempt.h
 mandatory-y += rwonce.h
 mandatory-y += sections.h
diff --git a/include/asm-generic/platform-feature.h b/include/asm-generic/platform-feature.h
new file mode 100644
index 000000000000..4b0af3d51588
--- /dev/null
+++ b/include/asm-generic/platform-feature.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_GENERIC_PLATFORM_FEATURE_H
+#define _ASM_GENERIC_PLATFORM_FEATURE_H
+
+/* Number of arch specific feature flags. */
+#define PLATFORM_ARCH_FEAT_N	0
+
+#endif /* _ASM_GENERIC_PLATFORM_FEATURE_H */
diff --git a/include/linux/platform-feature.h b/include/linux/platform-feature.h
new file mode 100644
index 000000000000..df393d502a4f
--- /dev/null
+++ b/include/linux/platform-feature.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _PLATFORM_FEATURE_H
+#define _PLATFORM_FEATURE_H
+
+#include <linux/bitops.h>
+#include <asm/platform-feature.h>
+
+/* The platform features are starting with the architecture specific ones. */
+#define PLATFORM_FEAT_N				(0 + PLATFORM_ARCH_FEAT_N)
+
+#define PLATFORM_FEAT_ARRAY_SZ	BITS_TO_LONGS(PLATFORM_FEAT_N)
+extern unsigned long platform_features[PLATFORM_FEAT_ARRAY_SZ];
+
+static inline void platform_set_feature(unsigned int feature)
+{
+	set_bit(feature, platform_features);
+}
+
+static inline void platform_reset_feature(unsigned int feature)
+{
+	clear_bit(feature, platform_features);
+}
+
+static inline bool platform_has(unsigned int feature)
+{
+	return test_bit(feature, platform_features);
+}
+
+#endif /* _PLATFORM_FEATURE_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index 847a82bfe0e3..2f412f80110d 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -7,7 +7,7 @@ obj-y     = fork.o exec_domain.o panic.o \
 	    cpu.o exit.o softirq.o resource.o \
 	    sysctl.o capability.o ptrace.o user.o \
 	    signal.o sys.o umh.o workqueue.o pid.o task_work.o \
-	    extable.o params.o \
+	    extable.o params.o platform-feature.o \
 	    kthread.o sys_ni.o nsproxy.o \
 	    notifier.o ksysfs.o cred.o reboot.o \
 	    async.o range.o smpboot.o ucount.o regset.o
diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
new file mode 100644
index 000000000000..2d52f8442cd5
--- /dev/null
+++ b/kernel/platform-feature.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/cache.h>
+#include <linux/platform-feature.h>
+
+unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];
+EXPORT_SYMBOL_GPL(platform_features);
-- 
2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Borislav Petkov]

On Tue, Apr 26, 2022 at 03:40:20PM +0200, Juergen Gross wrote:
> diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
> new file mode 100644
> index 000000000000..2d52f8442cd5
> --- /dev/null
> +++ b/kernel/platform-feature.c
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/cache.h>
> +#include <linux/platform-feature.h>
> +
> +unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];

Probably __ro_after_init.

> +EXPORT_SYMBOL_GPL(platform_features);

You probably should make that thing static and use only accessors to
modify it in case you wanna change the underlying data structure in the
future.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Borislav Petkov]

On Tue, Apr 26, 2022 at 03:40:20PM +0200, Juergen Gross wrote:
> diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
> new file mode 100644
> index 000000000000..2d52f8442cd5
> --- /dev/null
> +++ b/kernel/platform-feature.c
> @@ -0,0 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/cache.h>
> +#include <linux/platform-feature.h>
> +
> +unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];

Probably __ro_after_init.

> +EXPORT_SYMBOL_GPL(platform_features);

You probably should make that thing static and use only accessors to
modify it in case you wanna change the underlying data structure in the
future.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Heiko Carstens]

On Tue, Apr 26, 2022 at 07:31:14PM +0200, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:20PM +0200, Juergen Gross wrote:
> > diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
> > new file mode 100644
> > index 000000000000..2d52f8442cd5
> > --- /dev/null
> > +++ b/kernel/platform-feature.c
> > @@ -0,0 +1,7 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/cache.h>
> > +#include <linux/platform-feature.h>
> > +
> > +unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];
> 
> Probably __ro_after_init.
> 
> > +EXPORT_SYMBOL_GPL(platform_features);
> 
> You probably should make that thing static and use only accessors to
> modify it in case you wanna change the underlying data structure in the
> future.

That would add another indirection, which at least I think is not
necessary, and would make it less likely that this infrastructure is
used.

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Borislav Petkov]

On Tue, Apr 26, 2022 at 09:53:42PM +0200, Heiko Carstens wrote:
> > You probably should make that thing static and use only accessors to
> > modify it in case you wanna change the underlying data structure in the
> > future.
>
> That would add another indirection, which at least I think is not
> necessary and would make it less likely that this infrastructure is
> used.

So if you want to have a single variable which contains platform feature
bits, you don't need any platform_<bla> accessors but use this variable
directly.

I'd prefer the accessors any day of the week, though.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Borislav Petkov]

On Tue, Apr 26, 2022 at 09:53:42PM +0200, Heiko Carstens wrote:
> > You probably should make that thing static and use only accessors to
> > modify it in case you wanna change the underlying data structure in the
> > future.
>
> That would add another indirection, which at least I think is not
> necessary and would make it less likely that this infrastructure is
> used.

So if you want to have a single variable which contains platform feature
bits, you don't need any platform_<bla> accessors but use this variable
directly.

I'd prefer the accessors any day of the week, though.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1053 bytes --]

On 26.04.22 19:31, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:20PM +0200, Juergen Gross wrote:
>> diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
>> new file mode 100644
>> index 000000000000..2d52f8442cd5
>> --- /dev/null
>> +++ b/kernel/platform-feature.c
>> @@ -0,0 +1,7 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/cache.h>
>> +#include <linux/platform-feature.h>
>> +
>> +unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];
> 
> Probably __ro_after_init.

Yes, good idea.

> 
>> +EXPORT_SYMBOL_GPL(platform_features);
> 
> You probably should make that thing static and use only accessors to
> modify it in case you wanna change the underlying data structure in the
> future.

The question is whether we think that those platform features will be used
in hot paths or not. If so the inline accessors (at least the platform_has()
one) would be preferred IMO.

OTOH really performance critical cases could use static_branch or such.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] kernel: add platform_has() infrastructure

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1053 bytes --]

On 26.04.22 19:31, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:20PM +0200, Juergen Gross wrote:
>> diff --git a/kernel/platform-feature.c b/kernel/platform-feature.c
>> new file mode 100644
>> index 000000000000..2d52f8442cd5
>> --- /dev/null
>> +++ b/kernel/platform-feature.c
>> @@ -0,0 +1,7 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/cache.h>
>> +#include <linux/platform-feature.h>
>> +
>> +unsigned long __read_mostly platform_features[PLATFORM_FEAT_ARRAY_SZ];
> 
> Probably __ro_after_init.

Yes, good idea.

> 
>> +EXPORT_SYMBOL_GPL(platform_features);
> 
> You probably should make that thing static and use only accessors to
> modify it in case you wanna change the underlying data structure in the
> future.

The question is whether we think that those platform features will be used
in hot paths or not. If so the inline accessors (at least the platform_has()
one) would be preferred IMO.

OTOH really performance critical cases could use static_branch or such.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

Instead of using arch_has_restricted_virtio_memory_access() together
with CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, replace those
with platform_has() and a new platform feature
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
I've only done a compile test on x86 for now, as I can't test these
changes easily (SEV might be doable for me, but s390 isn't).
---
 arch/s390/Kconfig                  |  1 -
 arch/s390/mm/init.c                | 13 +++----------
 arch/x86/Kconfig                   |  1 -
 arch/x86/kernel/cpu/mshyperv.c     |  5 ++++-
 arch/x86/mm/mem_encrypt.c          |  6 ------
 arch/x86/mm/mem_encrypt_identity.c |  5 +++++
 drivers/virtio/Kconfig             |  6 ------
 drivers/virtio/virtio.c            |  5 ++---
 include/linux/platform-feature.h   |  3 ++-
 include/linux/virtio_config.h      |  9 ---------
 10 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index e084c72104f8..f97a22ae69a8 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -772,7 +772,6 @@ menu "Virtualization"
 config PROTECTED_VIRTUALIZATION_GUEST
 	def_bool n
 	prompt "Protected virtualization guest support"
-	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
 	help
 	  Select this option, if you want to be able to run this
 	  kernel as a protected virtualization KVM guest.
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 86ffd0d51fd5..8e4fa10c6b12 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -31,6 +31,7 @@
 #include <linux/cma.h>
 #include <linux/gfp.h>
 #include <linux/dma-direct.h>
+#include <linux/platform-feature.h>
 #include <asm/processor.h>
 #include <linux/uaccess.h>
 #include <asm/pgalloc.h>
@@ -168,22 +169,14 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
-#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-
-int arch_has_restricted_virtio_memory_access(void)
-{
-	return is_prot_virt_guest();
-}
-EXPORT_SYMBOL(arch_has_restricted_virtio_memory_access);
-
-#endif
-
 /* protected virtualization */
 static void pv_init(void)
 {
 	if (!is_prot_virt_guest())
 		return;
 
+	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+
 	/* make sure bounce buffers are shared */
 	swiotlb_force = SWIOTLB_FORCE;
 	swiotlb_init(1);
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b0142e01002e..20ac72546ae4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1515,7 +1515,6 @@ config X86_CPA_STATISTICS
 config X86_MEM_ENCRYPT
 	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
 	select DYNAMIC_PHYSICAL_MASK
-	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
 	def_bool n
 
 config AMD_MEM_ENCRYPT
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 4b67094215bb..435611d83895 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -19,6 +19,7 @@
 #include <linux/i8253.h>
 #include <linux/random.h>
 #include <linux/swiotlb.h>
+#include <linux/platform-feature.h>
 #include <asm/processor.h>
 #include <asm/hypervisor.h>
 #include <asm/hyperv-tlfs.h>
@@ -347,8 +348,10 @@ static void __init ms_hyperv_init_platform(void)
 #endif
 		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
 		if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
-			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
+			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE) {
 				cc_set_vendor(CC_VENDOR_HYPERV);
+				platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+			}
 		}
 	}
 
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 50d209939c66..9b6a7c98b2b1 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -76,9 +76,3 @@ void __init mem_encrypt_init(void)
 
 	print_mem_encrypt_feature_info();
 }
-
-int arch_has_restricted_virtio_memory_access(void)
-{
-	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
-}
-EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index b43bc24d2bb6..6043ba6cd17d 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -40,6 +40,7 @@
 #include <linux/mm.h>
 #include <linux/mem_encrypt.h>
 #include <linux/cc_platform.h>
+#include <linux/platform-feature.h>
 
 #include <asm/setup.h>
 #include <asm/sections.h>
@@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
 	} else {
 		/* SEV state cannot be controlled by a command line option */
 		sme_me_mask = me_mask;
+
+		/* Set restricted memory access for virtio. */
+		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+
 		goto out;
 	}
 
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index b5adf6abd241..a6dc8b5846fe 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -6,12 +6,6 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
 	  or CONFIG_S390_GUEST.
 
-config ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-	bool
-	help
-	  This option is selected if the architecture may need to enforce
-	  VIRTIO_F_ACCESS_PLATFORM
-
 config VIRTIO_PCI_LIB
 	tristate
 	help
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 22f15f444f75..371e16b18381 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -5,6 +5,7 @@
 #include <linux/module.h>
 #include <linux/idr.h>
 #include <linux/of.h>
+#include <linux/platform-feature.h>
 #include <uapi/linux/virtio_ids.h>
 
 /* Unique numbering for virtio devices. */
@@ -170,12 +171,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status);
 static int virtio_features_ok(struct virtio_device *dev)
 {
 	unsigned status;
-	int ret;
 
 	might_sleep();
 
-	ret = arch_has_restricted_virtio_memory_access();
-	if (ret) {
+	if (platform_has(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS)) {
 		if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
 			dev_warn(&dev->dev,
 				 "device must provide VIRTIO_F_VERSION_1\n");
diff --git a/include/linux/platform-feature.h b/include/linux/platform-feature.h
index df393d502a4f..34b649aaa1da 100644
--- a/include/linux/platform-feature.h
+++ b/include/linux/platform-feature.h
@@ -6,7 +6,8 @@
 #include <asm/platform-feature.h>
 
 /* The platform features are starting with the architecture specific ones. */
-#define PLATFORM_FEAT_N				(0 + PLATFORM_ARCH_FEAT_N)
+#define PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS	0
+#define PLATFORM_FEAT_N				(1 + PLATFORM_ARCH_FEAT_N)
 
 #define PLATFORM_FEAT_ARRAY_SZ	BITS_TO_LONGS(PLATFORM_FEAT_N)
 extern unsigned long platform_features[PLATFORM_FEAT_ARRAY_SZ];
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index b341dd62aa4d..79498298519d 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -559,13 +559,4 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
 		_r;							\
 	})
 
-#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-int arch_has_restricted_virtio_memory_access(void);
-#else
-static inline int arch_has_restricted_virtio_memory_access(void)
-{
-	return 0;
-}
-#endif /* CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS */
-
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
2.34.1

[PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

Instead of using arch_has_restricted_virtio_memory_access() together
with CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS, replace those
with platform_has() and a new platform feature
PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
I've only done a compile test on x86 for now, as I can't test these
changes easily (SEV might be doable for me, but s390 isn't).
---
 arch/s390/Kconfig                  |  1 -
 arch/s390/mm/init.c                | 13 +++----------
 arch/x86/Kconfig                   |  1 -
 arch/x86/kernel/cpu/mshyperv.c     |  5 ++++-
 arch/x86/mm/mem_encrypt.c          |  6 ------
 arch/x86/mm/mem_encrypt_identity.c |  5 +++++
 drivers/virtio/Kconfig             |  6 ------
 drivers/virtio/virtio.c            |  5 ++---
 include/linux/platform-feature.h   |  3 ++-
 include/linux/virtio_config.h      |  9 ---------
 10 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index e084c72104f8..f97a22ae69a8 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -772,7 +772,6 @@ menu "Virtualization"
 config PROTECTED_VIRTUALIZATION_GUEST
 	def_bool n
 	prompt "Protected virtualization guest support"
-	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
 	help
 	  Select this option, if you want to be able to run this
 	  kernel as a protected virtualization KVM guest.
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 86ffd0d51fd5..8e4fa10c6b12 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -31,6 +31,7 @@
 #include <linux/cma.h>
 #include <linux/gfp.h>
 #include <linux/dma-direct.h>
+#include <linux/platform-feature.h>
 #include <asm/processor.h>
 #include <linux/uaccess.h>
 #include <asm/pgalloc.h>
@@ -168,22 +169,14 @@ bool force_dma_unencrypted(struct device *dev)
 	return is_prot_virt_guest();
 }
 
-#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-
-int arch_has_restricted_virtio_memory_access(void)
-{
-	return is_prot_virt_guest();
-}
-EXPORT_SYMBOL(arch_has_restricted_virtio_memory_access);
-
-#endif
-
 /* protected virtualization */
 static void pv_init(void)
 {
 	if (!is_prot_virt_guest())
 		return;
 
+	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+
 	/* make sure bounce buffers are shared */
 	swiotlb_force = SWIOTLB_FORCE;
 	swiotlb_init(1);
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b0142e01002e..20ac72546ae4 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1515,7 +1515,6 @@ config X86_CPA_STATISTICS
 config X86_MEM_ENCRYPT
 	select ARCH_HAS_FORCE_DMA_UNENCRYPTED
 	select DYNAMIC_PHYSICAL_MASK
-	select ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
 	def_bool n
 
 config AMD_MEM_ENCRYPT
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 4b67094215bb..435611d83895 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -19,6 +19,7 @@
 #include <linux/i8253.h>
 #include <linux/random.h>
 #include <linux/swiotlb.h>
+#include <linux/platform-feature.h>
 #include <asm/processor.h>
 #include <asm/hypervisor.h>
 #include <asm/hyperv-tlfs.h>
@@ -347,8 +348,10 @@ static void __init ms_hyperv_init_platform(void)
 #endif
 		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
 		if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
-			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE)
+			if (hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE) {
 				cc_set_vendor(CC_VENDOR_HYPERV);
+				platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+			}
 		}
 	}
 
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 50d209939c66..9b6a7c98b2b1 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -76,9 +76,3 @@ void __init mem_encrypt_init(void)
 
 	print_mem_encrypt_feature_info();
 }
-
-int arch_has_restricted_virtio_memory_access(void)
-{
-	return cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT);
-}
-EXPORT_SYMBOL_GPL(arch_has_restricted_virtio_memory_access);
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index b43bc24d2bb6..6043ba6cd17d 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -40,6 +40,7 @@
 #include <linux/mm.h>
 #include <linux/mem_encrypt.h>
 #include <linux/cc_platform.h>
+#include <linux/platform-feature.h>
 
 #include <asm/setup.h>
 #include <asm/sections.h>
@@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
 	} else {
 		/* SEV state cannot be controlled by a command line option */
 		sme_me_mask = me_mask;
+
+		/* Set restricted memory access for virtio. */
+		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+
 		goto out;
 	}
 
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index b5adf6abd241..a6dc8b5846fe 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -6,12 +6,6 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG
 	  or CONFIG_S390_GUEST.
 
-config ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-	bool
-	help
-	  This option is selected if the architecture may need to enforce
-	  VIRTIO_F_ACCESS_PLATFORM
-
 config VIRTIO_PCI_LIB
 	tristate
 	help
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 22f15f444f75..371e16b18381 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -5,6 +5,7 @@
 #include <linux/module.h>
 #include <linux/idr.h>
 #include <linux/of.h>
+#include <linux/platform-feature.h>
 #include <uapi/linux/virtio_ids.h>
 
 /* Unique numbering for virtio devices. */
@@ -170,12 +171,10 @@ EXPORT_SYMBOL_GPL(virtio_add_status);
 static int virtio_features_ok(struct virtio_device *dev)
 {
 	unsigned status;
-	int ret;
 
 	might_sleep();
 
-	ret = arch_has_restricted_virtio_memory_access();
-	if (ret) {
+	if (platform_has(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS)) {
 		if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) {
 			dev_warn(&dev->dev,
 				 "device must provide VIRTIO_F_VERSION_1\n");
diff --git a/include/linux/platform-feature.h b/include/linux/platform-feature.h
index df393d502a4f..34b649aaa1da 100644
--- a/include/linux/platform-feature.h
+++ b/include/linux/platform-feature.h
@@ -6,7 +6,8 @@
 #include <asm/platform-feature.h>
 
 /* The platform features are starting with the architecture specific ones. */
-#define PLATFORM_FEAT_N				(0 + PLATFORM_ARCH_FEAT_N)
+#define PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS	0
+#define PLATFORM_FEAT_N				(1 + PLATFORM_ARCH_FEAT_N)
 
 #define PLATFORM_FEAT_ARRAY_SZ	BITS_TO_LONGS(PLATFORM_FEAT_N)
 extern unsigned long platform_features[PLATFORM_FEAT_ARRAY_SZ];
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index b341dd62aa4d..79498298519d 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -559,13 +559,4 @@ static inline void virtio_cwrite64(struct virtio_device *vdev,
 		_r;							\
 	})
 
-#ifdef CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS
-int arch_has_restricted_virtio_memory_access(void);
-#else
-static inline int arch_has_restricted_virtio_memory_access(void)
-{
-	return 0;
-}
-#endif /* CONFIG_ARCH_HAS_RESTRICTED_VIRTIO_MEMORY_ACCESS */
-
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>  /* protected virtualization */
>  static void pv_init(void)
>  {
>  	if (!is_prot_virt_guest())
>  		return;
>  
> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

Kinda long-ish for my taste. I'll probably call it:

	platform_set()

as it is implicit that it sets a feature bit.

> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index b43bc24d2bb6..6043ba6cd17d 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -40,6 +40,7 @@
>  #include <linux/mm.h>
>  #include <linux/mem_encrypt.h>
>  #include <linux/cc_platform.h>
> +#include <linux/platform-feature.h>
>  
>  #include <asm/setup.h>
>  #include <asm/sections.h>
> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>  	} else {
>  		/* SEV state cannot be controlled by a command line option */
>  		sme_me_mask = me_mask;
> +
> +		/* Set restricted memory access for virtio. */
> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

Huh, what does that have to do with SME?

In any case, yeah, looks ok at a quick glance. It would obviously need
for more people to look at it and say whether it makes sense to them and
whether that's fine to have in generic code but so far, the experience
with cc_platform_* says that it seems to work ok in generic code.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>  /* protected virtualization */
>  static void pv_init(void)
>  {
>  	if (!is_prot_virt_guest())
>  		return;
>  
> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

Kinda long-ish for my taste. I'll probably call it:

	platform_set()

as it is implicit that it sets a feature bit.

> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> index b43bc24d2bb6..6043ba6cd17d 100644
> --- a/arch/x86/mm/mem_encrypt_identity.c
> +++ b/arch/x86/mm/mem_encrypt_identity.c
> @@ -40,6 +40,7 @@
>  #include <linux/mm.h>
>  #include <linux/mem_encrypt.h>
>  #include <linux/cc_platform.h>
> +#include <linux/platform-feature.h>
>  
>  #include <asm/setup.h>
>  #include <asm/sections.h>
> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>  	} else {
>  		/* SEV state cannot be controlled by a command line option */
>  		sme_me_mask = me_mask;
> +
> +		/* Set restricted memory access for virtio. */
> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

Huh, what does that have to do with SME?

In any case, yeah, looks ok at a quick glance. It would obviously need
for more people to look at it and say whether it makes sense to them and
whether that's fine to have in generic code but so far, the experience
with cc_platform_* says that it seems to work ok in generic code.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Heiko Carstens]

On Tue, Apr 26, 2022 at 07:35:43PM +0200, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
> >  /* protected virtualization */
> >  static void pv_init(void)
> >  {
> >  	if (!is_prot_virt_guest())
> >  		return;
> >  
> > +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> Kinda long-ish for my taste. I'll probably call it:
> 
> 	platform_set()
> 
> as it is implicit that it sets a feature bit.

...and platform_clear(), instead of platform_reset_feature() please.

> In any case, yeah, looks ok at a quick glance. It would obviously need
> for more people to look at it and say whether it makes sense to them and
> whether that's fine to have in generic code but so far, the experience
> with cc_platform_* says that it seems to work ok in generic code.

We _could_ convert s390's machine flags to this mechanism. Those flags
are historically per-cpu, but if I'm not mistaken none of them is
performance critical anymore, and those who are could/should probably
transformed to jump labels or alternatives anyway.

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1351 bytes --]

On 26.04.22 21:51, Heiko Carstens wrote:
> On Tue, Apr 26, 2022 at 07:35:43PM +0200, Borislav Petkov wrote:
>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>   /* protected virtualization */
>>>   static void pv_init(void)
>>>   {
>>>   	if (!is_prot_virt_guest())
>>>   		return;
>>>   
>>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>
>> Kinda long-ish for my taste. I'll probably call it:
>>
>> 	platform_set()
>>
>> as it is implicit that it sets a feature bit.
> 
> ...and platform_clear(), instead of platform_reset_feature() please.

Fine with me.

> 
>> In any case, yeah, looks ok at a quick glance. It would obviously need
>> for more people to look at it and say whether it makes sense to them and
>> whether that's fine to have in generic code but so far, the experience
>> with cc_platform_* says that it seems to work ok in generic code.
> 
> We _could_ convert s390's machine flags to this mechanism. Those flags
> are historically per-cpu, but if I'm not mistaken none of them is
> performance critical anymore, and those who are could/should probably
> transformed to jump labels or alternatives anyway.

I was planning to look at the x86 cpu features to see whether some of
those might be candidates to be switched to platform features instead.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1351 bytes --]

On 26.04.22 21:51, Heiko Carstens wrote:
> On Tue, Apr 26, 2022 at 07:35:43PM +0200, Borislav Petkov wrote:
>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>   /* protected virtualization */
>>>   static void pv_init(void)
>>>   {
>>>   	if (!is_prot_virt_guest())
>>>   		return;
>>>   
>>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>
>> Kinda long-ish for my taste. I'll probably call it:
>>
>> 	platform_set()
>>
>> as it is implicit that it sets a feature bit.
> 
> ...and platform_clear(), instead of platform_reset_feature() please.

Fine with me.

> 
>> In any case, yeah, looks ok at a quick glance. It would obviously need
>> for more people to look at it and say whether it makes sense to them and
>> whether that's fine to have in generic code but so far, the experience
>> with cc_platform_* says that it seems to work ok in generic code.
> 
> We _could_ convert s390's machine flags to this mechanism. Those flags
> are historically per-cpu, but if I'm not mistaken none of them is
> performance critical anymore, and those who are could/should probably
> transformed to jump labels or alternatives anyway.

I was planning to look at the x86 cpu features to see whether some of
those might be candidates to be switched to platform features instead.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Wed, Apr 27, 2022 at 08:40:08AM +0200, Juergen Gross wrote:
> I was planning to look at the x86 cpu features to see whether some of
> those might be candidates to be switched to platform features instead.

I'd say "never touch a running system" unless the platform features are
of an advantage...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Wed, Apr 27, 2022 at 08:40:08AM +0200, Juergen Gross wrote:
> I was planning to look at the x86 cpu features to see whether some of
> those might be candidates to be switched to platform features instead.

I'd say "never touch a running system" unless the platform features are
of an advantage...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 804 bytes --]

On 27.04.22 14:26, Borislav Petkov wrote:
> On Wed, Apr 27, 2022 at 08:40:08AM +0200, Juergen Gross wrote:
>> I was planning to look at the x86 cpu features to see whether some of
>> those might be candidates to be switched to platform features instead.
> 
> I'd say "never touch a running system" unless the platform features are
> of an advantage...

Depends on the use case IMHO.

All features being based on a cpuid bit are no candidates. Same applies
to all features used for alternative handling (assuming we don't want
to expand that to platform features).

I really have no idea whether this will leave any candidates. In case
it does, it might be interesting to switch those in order to save some
per-cpu bits.

Other candidates might be the x86_legacy_features.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 804 bytes --]

On 27.04.22 14:26, Borislav Petkov wrote:
> On Wed, Apr 27, 2022 at 08:40:08AM +0200, Juergen Gross wrote:
>> I was planning to look at the x86 cpu features to see whether some of
>> those might be candidates to be switched to platform features instead.
> 
> I'd say "never touch a running system" unless the platform features are
> of an advantage...

Depends on the use case IMHO.

All features being based on a cpuid bit are no candidates. Same applies
to all features used for alternative handling (assuming we don't want
to expand that to platform features).

I really have no idea whether this will leave any candidates. In case
it does, it might be interesting to switch those in order to save some
per-cpu bits.

Other candidates might be the x86_legacy_features.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1522 bytes --]

On 26.04.22 19:35, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>>   	if (!is_prot_virt_guest())
>>   		return;
>>   
>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> Kinda long-ish for my taste. I'll probably call it:
> 
> 	platform_set()
> 
> as it is implicit that it sets a feature bit.

Okay, fine with me.

> 
>> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
>> index b43bc24d2bb6..6043ba6cd17d 100644
>> --- a/arch/x86/mm/mem_encrypt_identity.c
>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>> @@ -40,6 +40,7 @@
>>   #include <linux/mm.h>
>>   #include <linux/mem_encrypt.h>
>>   #include <linux/cc_platform.h>
>> +#include <linux/platform-feature.h>
>>   
>>   #include <asm/setup.h>
>>   #include <asm/sections.h>
>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>   	} else {
>>   		/* SEV state cannot be controlled by a command line option */
>>   		sme_me_mask = me_mask;
>> +
>> +		/* Set restricted memory access for virtio. */
>> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> Huh, what does that have to do with SME?

I picked the function where sev_status is being set, as this seemed to be
the correct place to set the feature bit.

Looking at it in more detail it might be preferable to do it in
sev_setup_arch() instead.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 1522 bytes --]

On 26.04.22 19:35, Borislav Petkov wrote:
> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>   /* protected virtualization */
>>   static void pv_init(void)
>>   {
>>   	if (!is_prot_virt_guest())
>>   		return;
>>   
>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> Kinda long-ish for my taste. I'll probably call it:
> 
> 	platform_set()
> 
> as it is implicit that it sets a feature bit.

Okay, fine with me.

> 
>> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
>> index b43bc24d2bb6..6043ba6cd17d 100644
>> --- a/arch/x86/mm/mem_encrypt_identity.c
>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>> @@ -40,6 +40,7 @@
>>   #include <linux/mm.h>
>>   #include <linux/mem_encrypt.h>
>>   #include <linux/cc_platform.h>
>> +#include <linux/platform-feature.h>
>>   
>>   #include <asm/setup.h>
>>   #include <asm/sections.h>
>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>   	} else {
>>   		/* SEV state cannot be controlled by a command line option */
>>   		sme_me_mask = me_mask;
>> +
>> +		/* Set restricted memory access for virtio. */
>> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> Huh, what does that have to do with SME?

I picked the function where sev_status is being set, as this seemed to be
the correct place to set the feature bit.

Looking at it in more detail it might be preferable to do it in
sev_setup_arch() instead.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
> On 26.04.22 19:35, Borislav Petkov wrote:
> > On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
> > >   /* protected virtualization */
> > >   static void pv_init(void)
> > >   {
> > >   	if (!is_prot_virt_guest())
> > >   		return;
> > > +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> > 
> > Kinda long-ish for my taste. I'll probably call it:
> > 
> > 	platform_set()
> > 
> > as it is implicit that it sets a feature bit.
> 
> Okay, fine with me.
> 
> > 
> > > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > > index b43bc24d2bb6..6043ba6cd17d 100644
> > > --- a/arch/x86/mm/mem_encrypt_identity.c
> > > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > > @@ -40,6 +40,7 @@
> > >   #include <linux/mm.h>
> > >   #include <linux/mem_encrypt.h>
> > >   #include <linux/cc_platform.h>
> > > +#include <linux/platform-feature.h>
> > >   #include <asm/setup.h>
> > >   #include <asm/sections.h>
> > > @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
> > >   	} else {
> > >   		/* SEV state cannot be controlled by a command line option */
> > >   		sme_me_mask = me_mask;
> > > +
> > > +		/* Set restricted memory access for virtio. */
> > > +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> > 
> > Huh, what does that have to do with SME?
> 
> I picked the function where sev_status is being set, as this seemed to be
> the correct place to set the feature bit.

What I don't understand is what does restricted memory access have to do
with AMD SEV and how does play together with what you guys are trying to
do?

The big picture pls.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Borislav Petkov]

On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
> On 26.04.22 19:35, Borislav Petkov wrote:
> > On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
> > >   /* protected virtualization */
> > >   static void pv_init(void)
> > >   {
> > >   	if (!is_prot_virt_guest())
> > >   		return;
> > > +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> > 
> > Kinda long-ish for my taste. I'll probably call it:
> > 
> > 	platform_set()
> > 
> > as it is implicit that it sets a feature bit.
> 
> Okay, fine with me.
> 
> > 
> > > diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
> > > index b43bc24d2bb6..6043ba6cd17d 100644
> > > --- a/arch/x86/mm/mem_encrypt_identity.c
> > > +++ b/arch/x86/mm/mem_encrypt_identity.c
> > > @@ -40,6 +40,7 @@
> > >   #include <linux/mm.h>
> > >   #include <linux/mem_encrypt.h>
> > >   #include <linux/cc_platform.h>
> > > +#include <linux/platform-feature.h>
> > >   #include <asm/setup.h>
> > >   #include <asm/sections.h>
> > > @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
> > >   	} else {
> > >   		/* SEV state cannot be controlled by a command line option */
> > >   		sme_me_mask = me_mask;
> > > +
> > > +		/* Set restricted memory access for virtio. */
> > > +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> > 
> > Huh, what does that have to do with SME?
> 
> I picked the function where sev_status is being set, as this seemed to be
> the correct place to set the feature bit.

What I don't understand is what does restricted memory access have to do
with AMD SEV and how does play together with what you guys are trying to
do?

The big picture pls.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2515 bytes --]

On 27.04.22 14:28, Borislav Petkov wrote:
> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>> On 26.04.22 19:35, Borislav Petkov wrote:
>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>    /* protected virtualization */
>>>>    static void pv_init(void)
>>>>    {
>>>>    	if (!is_prot_virt_guest())
>>>>    		return;
>>>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>
>>> Kinda long-ish for my taste. I'll probably call it:
>>>
>>> 	platform_set()
>>>
>>> as it is implicit that it sets a feature bit.
>>
>> Okay, fine with me.
>>
>>>
>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>> @@ -40,6 +40,7 @@
>>>>    #include <linux/mm.h>
>>>>    #include <linux/mem_encrypt.h>
>>>>    #include <linux/cc_platform.h>
>>>> +#include <linux/platform-feature.h>
>>>>    #include <asm/setup.h>
>>>>    #include <asm/sections.h>
>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>    	} else {
>>>>    		/* SEV state cannot be controlled by a command line option */
>>>>    		sme_me_mask = me_mask;
>>>> +
>>>> +		/* Set restricted memory access for virtio. */
>>>> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>
>>> Huh, what does that have to do with SME?
>>
>> I picked the function where sev_status is being set, as this seemed to be
>> the correct place to set the feature bit.
> 
> What I don't understand is what does restricted memory access have to do
> with AMD SEV and how does play together with what you guys are trying to
> do?
> 
> The big picture pls.

Ah, okay.

For support of virtio with Xen we want to not only support the virtio
devices like KVM, but use grants for letting the guest decide which
pages are allowed to be mapped by the backend (dom0).

Instead of physical guest addresses the guest will use grant-ids (plus
offset). In order to be able to handle this at the basic virtio level
instead of the single virtio device drivers, we need to use dedicated
dma-ops. And those will be used by virtio only, if the "restricted
virtio memory request" flag is set, which is used by SEV, too. In order
to let virtio set this flag, we need a way to communicate to virtio
that the running system is either a SEV guest or a Xen guest.

HTH,


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2515 bytes --]

On 27.04.22 14:28, Borislav Petkov wrote:
> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>> On 26.04.22 19:35, Borislav Petkov wrote:
>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>    /* protected virtualization */
>>>>    static void pv_init(void)
>>>>    {
>>>>    	if (!is_prot_virt_guest())
>>>>    		return;
>>>> +	platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>
>>> Kinda long-ish for my taste. I'll probably call it:
>>>
>>> 	platform_set()
>>>
>>> as it is implicit that it sets a feature bit.
>>
>> Okay, fine with me.
>>
>>>
>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>> @@ -40,6 +40,7 @@
>>>>    #include <linux/mm.h>
>>>>    #include <linux/mem_encrypt.h>
>>>>    #include <linux/cc_platform.h>
>>>> +#include <linux/platform-feature.h>
>>>>    #include <asm/setup.h>
>>>>    #include <asm/sections.h>
>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>    	} else {
>>>>    		/* SEV state cannot be controlled by a command line option */
>>>>    		sme_me_mask = me_mask;
>>>> +
>>>> +		/* Set restricted memory access for virtio. */
>>>> +		platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>
>>> Huh, what does that have to do with SME?
>>
>> I picked the function where sev_status is being set, as this seemed to be
>> the correct place to set the feature bit.
> 
> What I don't understand is what does restricted memory access have to do
> with AMD SEV and how does play together with what you guys are trying to
> do?
> 
> The big picture pls.

Ah, okay.

For support of virtio with Xen we want to not only support the virtio
devices like KVM, but use grants for letting the guest decide which
pages are allowed to be mapped by the backend (dom0).

Instead of physical guest addresses the guest will use grant-ids (plus
offset). In order to be able to handle this at the basic virtio level
instead of the single virtio device drivers, we need to use dedicated
dma-ops. And those will be used by virtio only, if the "restricted
virtio memory request" flag is set, which is used by SEV, too. In order
to let virtio set this flag, we need a way to communicate to virtio
that the running system is either a SEV guest or a Xen guest.

HTH,


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Tom Lendacky]

On 4/27/22 07:37, Juergen Gross wrote:
> On 27.04.22 14:28, Borislav Petkov wrote:
>> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>>> On 26.04.22 19:35, Borislav Petkov wrote:
>>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>>    /* protected virtualization */
>>>>>    static void pv_init(void)
>>>>>    {
>>>>>        if (!is_prot_virt_guest())
>>>>>            return;
>>>>> +    platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>>
>>>> Kinda long-ish for my taste. I'll probably call it:
>>>>
>>>>     platform_set()
>>>>
>>>> as it is implicit that it sets a feature bit.
>>>
>>> Okay, fine with me.
>>>
>>>>
>>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c 
>>>>> b/arch/x86/mm/mem_encrypt_identity.c
>>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>>> @@ -40,6 +40,7 @@
>>>>>    #include <linux/mm.h>
>>>>>    #include <linux/mem_encrypt.h>
>>>>>    #include <linux/cc_platform.h>
>>>>> +#include <linux/platform-feature.h>
>>>>>    #include <asm/setup.h>
>>>>>    #include <asm/sections.h>
>>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>>        } else {
>>>>>            /* SEV state cannot be controlled by a command line option */
>>>>>            sme_me_mask = me_mask;
>>>>> +
>>>>> +        /* Set restricted memory access for virtio. */
>>>>> +        platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

This is way early in the boot, but it appears that marking the platform 
feature bitmap as __read_mostly puts this in the .data section, so avoids 
the issue of bss being cleared.

TDX support also uses the arch_has_restricted_virtio_memory_access() 
function and will need to be updated.

Seems like a lot of changes, I just wonder if the the arch_has...() 
function couldn't be updated to also include a Xen check?

Thanks,
Tom

>>>>
>>>> Huh, what does that have to do with SME?
>>>
>>> I picked the function where sev_status is being set, as this seemed to be
>>> the correct place to set the feature bit.
>>
>> What I don't understand is what does restricted memory access have to do
>> with AMD SEV and how does play together with what you guys are trying to
>> do?
>>
>> The big picture pls.
> 
> Ah, okay.
> 
> For support of virtio with Xen we want to not only support the virtio
> devices like KVM, but use grants for letting the guest decide which
> pages are allowed to be mapped by the backend (dom0).
> 
> Instead of physical guest addresses the guest will use grant-ids (plus
> offset). In order to be able to handle this at the basic virtio level
> instead of the single virtio device drivers, we need to use dedicated
> dma-ops. And those will be used by virtio only, if the "restricted
> virtio memory request" flag is set, which is used by SEV, too. In order
> to let virtio set this flag, we need a way to communicate to virtio
> that the running system is either a SEV guest or a Xen guest.
> 
> HTH,
> 
> 
> Juergen

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Tom Lendacky via Virtualization]

On 4/27/22 07:37, Juergen Gross wrote:
> On 27.04.22 14:28, Borislav Petkov wrote:
>> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>>> On 26.04.22 19:35, Borislav Petkov wrote:
>>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>>    /* protected virtualization */
>>>>>    static void pv_init(void)
>>>>>    {
>>>>>        if (!is_prot_virt_guest())
>>>>>            return;
>>>>> +    platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>>
>>>> Kinda long-ish for my taste. I'll probably call it:
>>>>
>>>>     platform_set()
>>>>
>>>> as it is implicit that it sets a feature bit.
>>>
>>> Okay, fine with me.
>>>
>>>>
>>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c 
>>>>> b/arch/x86/mm/mem_encrypt_identity.c
>>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>>> @@ -40,6 +40,7 @@
>>>>>    #include <linux/mm.h>
>>>>>    #include <linux/mem_encrypt.h>
>>>>>    #include <linux/cc_platform.h>
>>>>> +#include <linux/platform-feature.h>
>>>>>    #include <asm/setup.h>
>>>>>    #include <asm/sections.h>
>>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>>        } else {
>>>>>            /* SEV state cannot be controlled by a command line option */
>>>>>            sme_me_mask = me_mask;
>>>>> +
>>>>> +        /* Set restricted memory access for virtio. */
>>>>> +        platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);

This is way early in the boot, but it appears that marking the platform 
feature bitmap as __read_mostly puts this in the .data section, so avoids 
the issue of bss being cleared.

TDX support also uses the arch_has_restricted_virtio_memory_access() 
function and will need to be updated.

Seems like a lot of changes, I just wonder if the the arch_has...() 
function couldn't be updated to also include a Xen check?

Thanks,
Tom

>>>>
>>>> Huh, what does that have to do with SME?
>>>
>>> I picked the function where sev_status is being set, as this seemed to be
>>> the correct place to set the feature bit.
>>
>> What I don't understand is what does restricted memory access have to do
>> with AMD SEV and how does play together with what you guys are trying to
>> do?
>>
>> The big picture pls.
> 
> Ah, okay.
> 
> For support of virtio with Xen we want to not only support the virtio
> devices like KVM, but use grants for letting the guest decide which
> pages are allowed to be mapped by the backend (dom0).
> 
> Instead of physical guest addresses the guest will use grant-ids (plus
> offset). In order to be able to handle this at the basic virtio level
> instead of the single virtio device drivers, we need to use dedicated
> dma-ops. And those will be used by virtio only, if the "restricted
> virtio memory request" flag is set, which is used by SEV, too. In order
> to let virtio set this flag, we need a way to communicate to virtio
> that the running system is either a SEV guest or a Xen guest.
> 
> HTH,
> 
> 
> Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2398 bytes --]

On 27.04.22 16:09, Tom Lendacky wrote:
> On 4/27/22 07:37, Juergen Gross wrote:
>> On 27.04.22 14:28, Borislav Petkov wrote:
>>> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>>>> On 26.04.22 19:35, Borislav Petkov wrote:
>>>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>>>    /* protected virtualization */
>>>>>>    static void pv_init(void)
>>>>>>    {
>>>>>>        if (!is_prot_virt_guest())
>>>>>>            return;
>>>>>> +    platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>>>
>>>>> Kinda long-ish for my taste. I'll probably call it:
>>>>>
>>>>>     platform_set()
>>>>>
>>>>> as it is implicit that it sets a feature bit.
>>>>
>>>> Okay, fine with me.
>>>>
>>>>>
>>>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c 
>>>>>> b/arch/x86/mm/mem_encrypt_identity.c
>>>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>>>> @@ -40,6 +40,7 @@
>>>>>>    #include <linux/mm.h>
>>>>>>    #include <linux/mem_encrypt.h>
>>>>>>    #include <linux/cc_platform.h>
>>>>>> +#include <linux/platform-feature.h>
>>>>>>    #include <asm/setup.h>
>>>>>>    #include <asm/sections.h>
>>>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>>>        } else {
>>>>>>            /* SEV state cannot be controlled by a command line option */
>>>>>>            sme_me_mask = me_mask;
>>>>>> +
>>>>>> +        /* Set restricted memory access for virtio. */
>>>>>> +        platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> This is way early in the boot, but it appears that marking the platform feature 
> bitmap as __read_mostly puts this in the .data section, so avoids the issue of 
> bss being cleared.

In V2 (not yet posted) I have moved the call to sev_setup_arch().

> 
> TDX support also uses the arch_has_restricted_virtio_memory_access() function 
> and will need to be updated.

Yes.

> Seems like a lot of changes, I just wonder if the the arch_has...() function 
> couldn't be updated to also include a Xen check?

This was not seen to be a nice solution.

And TBH, I think this series is making the code much cleaner. Look at the
diffstat of this patch.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 2/2] virtio: replace arch_has_restricted_virtio_memory_access()

[Juergen Gross via Virtualization]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2398 bytes --]

On 27.04.22 16:09, Tom Lendacky wrote:
> On 4/27/22 07:37, Juergen Gross wrote:
>> On 27.04.22 14:28, Borislav Petkov wrote:
>>> On Wed, Apr 27, 2022 at 08:37:31AM +0200, Juergen Gross wrote:
>>>> On 26.04.22 19:35, Borislav Petkov wrote:
>>>>> On Tue, Apr 26, 2022 at 03:40:21PM +0200, Juergen Gross wrote:
>>>>>>    /* protected virtualization */
>>>>>>    static void pv_init(void)
>>>>>>    {
>>>>>>        if (!is_prot_virt_guest())
>>>>>>            return;
>>>>>> +    platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
>>>>>
>>>>> Kinda long-ish for my taste. I'll probably call it:
>>>>>
>>>>>     platform_set()
>>>>>
>>>>> as it is implicit that it sets a feature bit.
>>>>
>>>> Okay, fine with me.
>>>>
>>>>>
>>>>>> diff --git a/arch/x86/mm/mem_encrypt_identity.c 
>>>>>> b/arch/x86/mm/mem_encrypt_identity.c
>>>>>> index b43bc24d2bb6..6043ba6cd17d 100644
>>>>>> --- a/arch/x86/mm/mem_encrypt_identity.c
>>>>>> +++ b/arch/x86/mm/mem_encrypt_identity.c
>>>>>> @@ -40,6 +40,7 @@
>>>>>>    #include <linux/mm.h>
>>>>>>    #include <linux/mem_encrypt.h>
>>>>>>    #include <linux/cc_platform.h>
>>>>>> +#include <linux/platform-feature.h>
>>>>>>    #include <asm/setup.h>
>>>>>>    #include <asm/sections.h>
>>>>>> @@ -566,6 +567,10 @@ void __init sme_enable(struct boot_params *bp)
>>>>>>        } else {
>>>>>>            /* SEV state cannot be controlled by a command line option */
>>>>>>            sme_me_mask = me_mask;
>>>>>> +
>>>>>> +        /* Set restricted memory access for virtio. */
>>>>>> +        platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
> 
> This is way early in the boot, but it appears that marking the platform feature 
> bitmap as __read_mostly puts this in the .data section, so avoids the issue of 
> bss being cleared.

In V2 (not yet posted) I have moved the call to sev_setup_arch().

> 
> TDX support also uses the arch_has_restricted_virtio_memory_access() function 
> and will need to be updated.

Yes.

> Seems like a lot of changes, I just wonder if the the arch_has...() function 
> couldn't be updated to also include a Xen check?

This was not seen to be a nice solution.

And TBH, I think this series is making the code much cleaner. Look at the
diffstat of this patch.


Juergen

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/2] kernel: add new infrastructure for platform_has() support

[Oleksandr]

On 26.04.22 16:40, Juergen Gross wrote:

Hello Juergen, all

> In another patch series [1] the need has come up to have support for
> a generic feature flag infrastructure.
>
> This patch series is introducing that infrastructure and adds the first
> use case.
>
> I have decided to use a similar interface as the already known x86
> cpu_has() function. As the new infrastructure is meant to be usable for
> general and arch-specific feature flags, the flags are being spread
> between a general bitmap and an arch specific one.
>
> The bitmaps start all being zero, single features can be set or reset
> at any time by using the related platform_[re]set_feature() functions.
>
> The platform_has() function is using a simple test_bit() call for now,
> further optimization might be added when needed.
>
> [1]: https://lore.kernel.org/lkml/1650646263-22047-1-git-send-email-olekstysh@gmail.com/T/#t

I have tested the series on Arm64 in the context of xen-virtio enabling 
work. I didn't see any issues with it. Thank you.

I reworked patch #3 [1] to use new functionality:


diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ec5b082..f3b9e20 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -438,6 +438,8 @@ static int __init xen_guest_init(void)
         if (!xen_domain())
                 return 0;

+       xen_set_restricted_virtio_memory_access();
+
         if (!acpi_disabled)
                 xen_acpi_guest_init();
         else
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 517a9d8..8b71b1d 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -195,6 +195,8 @@ static void __init xen_hvm_guest_init(void)
         if (xen_pv_domain())
                 return;

+       xen_set_restricted_virtio_memory_access();
+
         init_hvm_pv_info();

         reserve_shared_info();
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 5038edb..fcd5d5d 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -109,6 +109,8 @@ static DEFINE_PER_CPU(struct tls_descs, 
shadow_tls_desc);

  static void __init xen_pv_init_platform(void)
  {
+       xen_set_restricted_virtio_memory_access();
+
         populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));

         set_fixmap(FIX_PARAVIRT_BOOTMAP, xen_start_info->shared_info);
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 313a9127..a7bd8ce 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -339,4 +339,15 @@ config XEN_GRANT_DMA_OPS
         bool
         select DMA_OPS

+config XEN_VIRTIO
+       bool "Xen virtio support"
+       depends on VIRTIO
+       select XEN_GRANT_DMA_OPS
+       help
+         Enable virtio support for running as Xen guest. Depending on the
+         guest type this will require special support on the backend side
+         (qemu or kernel, depending on the virtio device types used).
+
+         If in doubt, say n.
+
  endmenu
diff --git a/include/xen/xen.h b/include/xen/xen.h
index a99bab8..e2849c9 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -52,6 +52,14 @@ bool xen_biovec_phys_mergeable(const struct bio_vec 
*vec1,
  extern u64 xen_saved_max_mem_size;
  #endif

+#include <linux/platform-feature.h>
+
+static inline void xen_set_restricted_virtio_memory_access(void)
+{
+       if (IS_ENABLED(CONFIG_XEN_VIRTIO) && xen_domain())
+ platform_set_feature(PLATFORM_VIRTIO_RESTRICTED_MEM_ACCESS);
+}
+
  #ifdef CONFIG_XEN_UNPOPULATED_ALLOC
  int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page 
**pages);
  void xen_free_unpopulated_pages(unsigned int nr_pages, struct page 
**pages);
(END)


[1] 
https://lore.kernel.org/lkml/1650646263-22047-4-git-send-email-olekstysh@gmail.com/


>
> Juergen Gross (2):
>    kernel: add platform_has() infrastructure
>    virtio: replace arch_has_restricted_virtio_memory_access()
>
>   MAINTAINERS                            |  8 +++++++
>   arch/s390/Kconfig                      |  1 -
>   arch/s390/mm/init.c                    | 13 +++--------
>   arch/x86/Kconfig                       |  1 -
>   arch/x86/kernel/cpu/mshyperv.c         |  5 ++++-
>   arch/x86/mm/mem_encrypt.c              |  6 ------
>   arch/x86/mm/mem_encrypt_identity.c     |  5 +++++
>   drivers/virtio/Kconfig                 |  6 ------
>   drivers/virtio/virtio.c                |  5 ++---
>   include/asm-generic/Kbuild             |  1 +
>   include/asm-generic/platform-feature.h |  8 +++++++
>   include/linux/platform-feature.h       | 30 ++++++++++++++++++++++++++
>   include/linux/virtio_config.h          |  9 --------
>   kernel/Makefile                        |  2 +-
>   kernel/platform-feature.c              |  7 ++++++
>   15 files changed, 69 insertions(+), 38 deletions(-)
>   create mode 100644 include/asm-generic/platform-feature.h
>   create mode 100644 include/linux/platform-feature.h
>   create mode 100644 kernel/platform-feature.c
>
-- 
Regards,

Oleksandr Tyshchenko