[PATCH v1 0/2] virtio-mem: VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE and interaction with memory properties

[PATCH v1 0/2] virtio-mem: VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE and interaction with memory properties

[David Hildenbrand]

Looking into supporting virtio-mem
a) without a shared zeropage for the memory backing of the device --
   not allowing the driver to read unplugged memory
b) on architectures with memory properties for RAM (e.g., s390x with
   storage keys and storage attributes)
requires extension of the spec to handle both cases cleanly and describe
the expected semantics.

I'll open a github issue soon; in the meantime, I'll work on the actual
implementation in QEMU and Linux.

For a), I already shared a Linux implementation in the past [1], which will
be simplified once virito-mem memory can no longer be mapped via /dev/mem
[2].

For b), it's actually unlocking virtio-mem on s390x in QEMU/Linux,
initially supporting storage keys but not supporting storage attributes
for simplicity.

[1] https://lkml.kernel.org/r/20210215122421.27964-1-david@redhat.com
[2] https://lkml.kernel.org/r/20210811203612.138506-1-david@redhat.com

Cc: Hui Zhu <teawater@gmail.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>

David Hildenbrand (2):
  virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
  virtio-mem: describe interaction with memory properties

 virtio-mem.tex | 73 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 16 deletions(-)

-- 
2.31.1

[virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[David Hildenbrand]

Until now, we allowed a driver to read unplugged memory within the
usable device-managed region: this simplified bring-up of virtio-mem in
Linux quite a bit, especially when it came to physical memory dumping.

When the device is using a memory backend that supports a shared
zeropage, such as virtio-mem in QEMU under Linux on anonymous memory, the
old behavior could be realized easily.

However, when using other memory backends (such as hugetlbfs or shmem)
or architectures, such as s390x, where a shared zeropage either does not
exist or cannot be used, letting the driver read unplugged memory can
result in undesired memory consumption in the hypervisor. The device
wants to make sure that the guest is aware and will not read unplugged
memory, not even in corner cases.

In the meantime, the Linux implementation matured such that it will no
longer access unplugged memory, for example, during kdump, when reading
/proc/kcore, or via (now removed) /dev/kmem.

Similar to VIRTIO_F_ACCESS_PLATFORM, this change will be disruptive and
require driver adaptions -- even if it's just accepting the new feature.
Devices are expected to only set the bit when really required, to keep
existing setups working.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 virtio-mem.tex | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/virtio-mem.tex b/virtio-mem.tex
index 62a1d02..c4dd0d0 100644
--- a/virtio-mem.tex
+++ b/virtio-mem.tex
@@ -46,6 +46,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
 \begin{description}
 \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
 configuration is valid and corresponds to an ACPI PXM.
+\item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
+unplugged memory.
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Memory Device / Device configuration layout}
@@ -144,11 +146,17 @@ \subsection{Device Initialization}\label{Device Types / Memory Device / Device I
 
 \drivernormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
 
+The driver SHOULD accept VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE if it is
+offered and the driver supports it.
+
 The driver SHOULD issue UNPLUG ALL requests until successful if the device
 still has memory plugged and the plugged memory is not in use.
 
 \devicenormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
 
+A device MAY fail to operate further if VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
+is not accepted.
+
 The device MUST NOT change the state of memory blocks during device reset.
 
 The device MUST NOT change the content of plugged memory blocks during
@@ -220,8 +228,11 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
 The driver MUST NOT read from unplugged memory blocks outside
 \field{usable_region_size}.
 
-The driver SHOULD NOT read from unplugged memory blocks inside
-\field{usable_region_size}.
+Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver SHOULD NOT read
+memory of unplugged memory blocks inside \field{usable_region_size}.
+
+With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory of
+unplugged memory blocks.
 
 The driver MUST NOT request to unplug memory blocks while the memory is
 still in use.
@@ -246,10 +257,13 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
 
 The device MUST NOT change the content of plugged memory blocks.
 
-The device MUST allow the CPU to read from unplugged memory blocks inside
-the usable device-managed region. \footnote{To allow for simplified dumping of
-memory. The CPU is expected to copy such memory to another location before
-starting DMA.}
+Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MUST allow the CPU to
+read memory of unplugged memory blocks inside \field{usable_region_size}.
+\footnote{To allow for simplified dumping of memory. The CPU is expected to
+copy such memory to another location before starting DMA.}
+
+With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
+read memory of unplugged memory blocks inside \field{usable_region_size}.
 
 The device MAY allow to read from unplugged memory blocks inside the
 usable device-managed region via DMA.
-- 
2.31.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[David Hildenbrand]

Let's describe how we expect the interaction with memory properties that
might be available on a specific platform for ordinary system RAM.

This is primarily a preparation for s390x support, which provides
storage keys and may provide storage attributes, depending on the system
configuration.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 virtio-mem.tex | 71 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 22 deletions(-)

diff --git a/virtio-mem.tex b/virtio-mem.tex
index c4dd0d0..a1057cd 100644
--- a/virtio-mem.tex
+++ b/virtio-mem.tex
@@ -32,6 +32,16 @@ \section{Memory Device}\label{sec:Device Types / Memory Device}
 expose plugged memory blocks to the operating system as system RAM,
 available for the page allocator.
 
+Some platforms provide memory properties for system RAM that are usually
+queried and modified using special CPU instructions. Memory properties might
+be implicitly queried or modified on memory access. Memory properties can
+include advanced memory protection, access and change indication, or memory
+usage indication relevant in virtualized environments. \footnote{For example,
+s390x provides storage keys for each 4 KiB page and may, depending on the
+configuration, provide storage attributes for each 4 KiB page.} The device
+provides the exact same properties with the exact same semantics for
+plugged device memory as available for ordinary RAM in the same configuration.
+
 \subsection{Device ID}\label{sec:Device Types / Memory Device / Device ID}
 24
 
@@ -47,7 +57,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
 \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
 configuration is valid and corresponds to an ACPI PXM.
 \item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
-unplugged memory.
+unplugged memory. \footnote{This feature is expected to be enabled on platforms
+with memory properties that might get modified implicitly on memory access.}
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Memory Device / Device configuration layout}
@@ -159,8 +170,8 @@ \subsection{Device Initialization}\label{Device Types / Memory Device / Device I
 
 The device MUST NOT change the state of memory blocks during device reset.
 
-The device MUST NOT change the content of plugged memory blocks during
-device reset.
+The device MUST NOT modify memory or memory properties of plugged memory
+blocks during device reset.
 
 \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Operation}
 
@@ -223,23 +234,30 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
 
 \drivernormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
 
-The driver MUST NOT write to unplugged memory blocks.
+The driver MUST NOT write memory or modify memory properties of
+unplugged memory blocks.
 
-The driver MUST NOT read from unplugged memory blocks outside
-\field{usable_region_size}.
+The driver MUST NOT read memory or query memory properties of unplugged
+memory blocks outside \field{usable_region_size}.
 
 Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver SHOULD NOT read
-memory of unplugged memory blocks inside \field{usable_region_size}.
+memory or query memory properties of unplugged memory blocks inside
+\field{usable_region_size}.
 
-With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory of
-unplugged memory blocks.
+With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory
+or query memory properties of unplugged memory blocks inside
+\field{usable_region_size}.
 
-The driver MUST NOT request to unplug memory blocks while the memory is
-still in use.
+The driver MUST NOT request unplug of memory blocks while corresponding memory
+or memory properties are still in use.
 
 The driver SHOULD initialize memory blocks after plugging them, the content
 is undefined.
 
+The driver SHOULD initialize memory properties of memory blocks after plugging
+them if it cannot deal with either the default settings or the previous
+setting.
+
 The driver SHOULD react to resize requests from the device
 (\field{requested_size} in the device configuration changed) by
 (un)plugging memory blocks.
@@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
 
 \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
 
-The device MAY change the content of unplugged memory blocks at any time.
+The device MUST provide the exact same memory properties with the exact same
+semantics for device memory the platform provides in the same configuration for
+ordinary RAM.
+
+The device MAY modify memory or reset memory properties to defaults of unplugged
+memory blocks at any time.
 
-The device MUST NOT change the content of plugged memory blocks.
+The device MUST NOT modify memory or memory properties of plugged memory
+blocks.
 
 Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MUST allow the CPU to
-read memory of unplugged memory blocks inside \field{usable_region_size}.
-\footnote{To allow for simplified dumping of memory. The CPU is expected to
-copy such memory to another location before starting DMA.}
+read memory or query memory properties of unplugged memory blocks inside
+\field{usable_region_size}. \footnote{To allow for simplified dumping of
+memory. The CPU is expected to copy such memory to another location before
+starting DMA.}
 
 With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
-read memory of unplugged memory blocks inside \field{usable_region_size}.
+read memory or query memory properties of unplugged memory blocks inside
+\field{usable_region_size}.
 
-The device MAY allow to read from unplugged memory blocks inside the
-usable device-managed region via DMA.
+The device MAY allow to read memory or query memory properties of unplugged
+memory blocks inside \field{usable_region_size} via DMA.
 
-The device MAY allow to read from unplugged memory blocks outside
-the usable device-managed region.
+The device MAY allow to read memory or query memory properties of unplugged
+memory blocks outside \field{usable_region_size} via DMA.
 
-The device MAY allow to write to unplugged memory blocks.
+The device MAY allow to write memory or modify memory properties of unplugged
+memory blocks.
 
 The device MAY change the state of memory blocks during system resets.
 
-- 
2.31.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[Cornelia Huck]

On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:

> Until now, we allowed a driver to read unplugged memory within the
> usable device-managed region: this simplified bring-up of virtio-mem in
> Linux quite a bit, especially when it came to physical memory dumping.
>
> When the device is using a memory backend that supports a shared
> zeropage, such as virtio-mem in QEMU under Linux on anonymous memory, the
> old behavior could be realized easily.
>
> However, when using other memory backends (such as hugetlbfs or shmem)
> or architectures, such as s390x, where a shared zeropage either does not
> exist or cannot be used, letting the driver read unplugged memory can
> result in undesired memory consumption in the hypervisor. The device
> wants to make sure that the guest is aware and will not read unplugged
> memory, not even in corner cases.
>
> In the meantime, the Linux implementation matured such that it will no
> longer access unplugged memory, for example, during kdump, when reading
> /proc/kcore, or via (now removed) /dev/kmem.
>
> Similar to VIRTIO_F_ACCESS_PLATFORM, this change will be disruptive and
> require driver adaptions -- even if it's just accepting the new feature.
> Devices are expected to only set the bit when really required, to keep
> existing setups working.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  virtio-mem.tex | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/virtio-mem.tex b/virtio-mem.tex
> index 62a1d02..c4dd0d0 100644
> --- a/virtio-mem.tex
> +++ b/virtio-mem.tex
> @@ -46,6 +46,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
>  \begin{description}
>  \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
>  configuration is valid and corresponds to an ACPI PXM.
> +\item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
> +unplugged memory.

Nit: "MUST NOT" should not be used outside of normative
statements... maybe "The driver is not allowed to access unplugged memory."?

>  \end{description}
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Memory Device / Device configuration layout}
> @@ -144,11 +146,17 @@ \subsection{Device Initialization}\label{Device Types / Memory Device / Device I
>  
>  \drivernormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
>  
> +The driver SHOULD accept VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE if it is
> +offered and the driver supports it.
> +
>  The driver SHOULD issue UNPLUG ALL requests until successful if the device
>  still has memory plugged and the plugged memory is not in use.
>  
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
>  
> +A device MAY fail to operate further if VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
> +is not accepted.
> +
>  The device MUST NOT change the state of memory blocks during device reset.
>  
>  The device MUST NOT change the content of plugged memory blocks during
> @@ -220,8 +228,11 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>  The driver MUST NOT read from unplugged memory blocks outside
>  \field{usable_region_size}.
>  
> -The driver SHOULD NOT read from unplugged memory blocks inside
> -\field{usable_region_size}.
> +Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver SHOULD NOT read

"If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has not been negotiated, ..."

> +memory of unplugged memory blocks inside \field{usable_region_size}.
> +
> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory of

"If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."

> +unplugged memory blocks.
>  
>  The driver MUST NOT request to unplug memory blocks while the memory is
>  still in use.
> @@ -246,10 +257,13 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>  
>  The device MUST NOT change the content of plugged memory blocks.
>  
> -The device MUST allow the CPU to read from unplugged memory blocks inside
> -the usable device-managed region. \footnote{To allow for simplified dumping of
> -memory. The CPU is expected to copy such memory to another location before
> -starting DMA.}
> +Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MUST allow the CPU to

"If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has not been negotiated, ..."


> +read memory of unplugged memory blocks inside \field{usable_region_size}.
> +\footnote{To allow for simplified dumping of memory. The CPU is expected to
> +copy such memory to another location before starting DMA.}
> +
> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to

"If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."

> +read memory of unplugged memory blocks inside \field{usable_region_size}.

A compliant driver would not read that memory, would it?

>  
>  The device MAY allow to read from unplugged memory blocks inside the
>  usable device-managed region via DMA.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[Cornelia Huck]

On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:

> Let's describe how we expect the interaction with memory properties that
> might be available on a specific platform for ordinary system RAM.
>
> This is primarily a preparation for s390x support, which provides
> storage keys and may provide storage attributes, depending on the system
> configuration.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  virtio-mem.tex | 71 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/virtio-mem.tex b/virtio-mem.tex
> index c4dd0d0..a1057cd 100644
> --- a/virtio-mem.tex
> +++ b/virtio-mem.tex
> @@ -32,6 +32,16 @@ \section{Memory Device}\label{sec:Device Types / Memory Device}
>  expose plugged memory blocks to the operating system as system RAM,
>  available for the page allocator.
>  
> +Some platforms provide memory properties for system RAM that are usually
> +queried and modified using special CPU instructions. Memory properties might
> +be implicitly queried or modified on memory access. Memory properties can
> +include advanced memory protection, access and change indication, or memory
> +usage indication relevant in virtualized environments. \footnote{For example,
> +s390x provides storage keys for each 4 KiB page and may, depending on the
> +configuration, provide storage attributes for each 4 KiB page.} The device
> +provides the exact same properties with the exact same semantics for
> +plugged device memory as available for ordinary RAM in the same configuration.
> +
>  \subsection{Device ID}\label{sec:Device Types / Memory Device / Device ID}
>  24
>  
> @@ -47,7 +57,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
>  \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
>  configuration is valid and corresponds to an ACPI PXM.
>  \item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
> -unplugged memory.
> +unplugged memory. \footnote{This feature is expected to be enabled on platforms
> +with memory properties that might get modified implicitly on memory access.}

"On platforms with memory properties (...), this feature is expected to
be offered by the device." ?

>  \end{description}
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Memory Device / Device configuration layout}
> @@ -159,8 +170,8 @@ \subsection{Device Initialization}\label{Device Types / Memory Device / Device I
>  
>  The device MUST NOT change the state of memory blocks during device reset.
>  
> -The device MUST NOT change the content of plugged memory blocks during
> -device reset.
> +The device MUST NOT modify memory or memory properties of plugged memory
> +blocks during device reset.
>  
>  \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Operation}
>  
> @@ -223,23 +234,30 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>  
>  \drivernormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>  
> -The driver MUST NOT write to unplugged memory blocks.
> +The driver MUST NOT write memory or modify memory properties of
> +unplugged memory blocks.
>  
> -The driver MUST NOT read from unplugged memory blocks outside
> -\field{usable_region_size}.
> +The driver MUST NOT read memory or query memory properties of unplugged
> +memory blocks outside \field{usable_region_size}.
>  
>  Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver SHOULD NOT read
> -memory of unplugged memory blocks inside \field{usable_region_size}.
> +memory or query memory properties of unplugged memory blocks inside
> +\field{usable_region_size}.
>  
> -With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory of
> -unplugged memory blocks.
> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory
> +or query memory properties of unplugged memory blocks inside
> +\field{usable_region_size}.
>  
> -The driver MUST NOT request to unplug memory blocks while the memory is
> -still in use.
> +The driver MUST NOT request unplug of memory blocks while corresponding memory
> +or memory properties are still in use.
>  
>  The driver SHOULD initialize memory blocks after plugging them, the content
>  is undefined.
>  
> +The driver SHOULD initialize memory properties of memory blocks after plugging
> +them if it cannot deal with either the default settings or the previous
> +setting.

This would imply that any memory properties need to allow modification
by the driver, right? It's clear what you want to introduce this for,
but do we need to tighten the definition of what kind of memory
properties we are talking about?

> +
>  The driver SHOULD react to resize requests from the device
>  (\field{requested_size} in the device configuration changed) by
>  (un)plugging memory blocks.
> @@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>  
>  \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>  
> -The device MAY change the content of unplugged memory blocks at any time.
> +The device MUST provide the exact same memory properties with the exact same
> +semantics for device memory the platform provides in the same configuration for
> +ordinary RAM.

This supposes that all RAM has the same properties across the whole
platform, right? Do we want to be able to support some kind of
heterogeneous platforms, or is that too much of an odd case?

> +
> +The device MAY modify memory or reset memory properties to defaults of unplugged
> +memory blocks at any time.
>  
> -The device MUST NOT change the content of plugged memory blocks.
> +The device MUST NOT modify memory or memory properties of plugged memory
> +blocks.
>  
>  Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MUST allow the CPU to
> -read memory of unplugged memory blocks inside \field{usable_region_size}.
> -\footnote{To allow for simplified dumping of memory. The CPU is expected to
> -copy such memory to another location before starting DMA.}
> +read memory or query memory properties of unplugged memory blocks inside
> +\field{usable_region_size}. \footnote{To allow for simplified dumping of
> +memory. The CPU is expected to copy such memory to another location before
> +starting DMA.}
>  
>  With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
> -read memory of unplugged memory blocks inside \field{usable_region_size}.
> +read memory or query memory properties of unplugged memory blocks inside
> +\field{usable_region_size}.
>  
> -The device MAY allow to read from unplugged memory blocks inside the
> -usable device-managed region via DMA.
> +The device MAY allow to read memory or query memory properties of unplugged
> +memory blocks inside \field{usable_region_size} via DMA.
>  
> -The device MAY allow to read from unplugged memory blocks outside
> -the usable device-managed region.
> +The device MAY allow to read memory or query memory properties of unplugged
> +memory blocks outside \field{usable_region_size} via DMA.
>  
> -The device MAY allow to write to unplugged memory blocks.
> +The device MAY allow to write memory or modify memory properties of unplugged
> +memory blocks.
>  
>  The device MAY change the state of memory blocks during system resets.
>  


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[David Hildenbrand]

On 17.08.21 10:50, Cornelia Huck wrote:
> On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>> Until now, we allowed a driver to read unplugged memory within the
>> usable device-managed region: this simplified bring-up of virtio-mem in
>> Linux quite a bit, especially when it came to physical memory dumping.
>>
>> When the device is using a memory backend that supports a shared
>> zeropage, such as virtio-mem in QEMU under Linux on anonymous memory, the
>> old behavior could be realized easily.
>>
>> However, when using other memory backends (such as hugetlbfs or shmem)
>> or architectures, such as s390x, where a shared zeropage either does not
>> exist or cannot be used, letting the driver read unplugged memory can
>> result in undesired memory consumption in the hypervisor. The device
>> wants to make sure that the guest is aware and will not read unplugged
>> memory, not even in corner cases.
>>
>> In the meantime, the Linux implementation matured such that it will no
>> longer access unplugged memory, for example, during kdump, when reading
>> /proc/kcore, or via (now removed) /dev/kmem.
>>
>> Similar to VIRTIO_F_ACCESS_PLATFORM, this change will be disruptive and
>> require driver adaptions -- even if it's just accepting the new feature.
>> Devices are expected to only set the bit when really required, to keep
>> existing setups working.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   virtio-mem.tex | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/virtio-mem.tex b/virtio-mem.tex
>> index 62a1d02..c4dd0d0 100644
>> --- a/virtio-mem.tex
>> +++ b/virtio-mem.tex
>> @@ -46,6 +46,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
>>   \begin{description}
>>   \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
>>   configuration is valid and corresponds to an ACPI PXM.
>> +\item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
>> +unplugged memory.
> 
> Nit: "MUST NOT" should not be used outside of normative
> statements... maybe "The driver is not allowed to access unplugged memory."?
> 

"should not" haha, so it can be done? ;)

Sure, can change that.


>>   \end{description}
>>   
>>   \subsection{Device configuration layout}\label{sec:Device Types / Memory Device / Device configuration layout}
>> @@ -144,11 +146,17 @@ \subsection{Device Initialization}\label{Device Types / Memory Device / Device I
>>   
>>   \drivernormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
>>   
>> +The driver SHOULD accept VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE if it is
>> +offered and the driver supports it.
>> +
>>   The driver SHOULD issue UNPLUG ALL requests until successful if the device
>>   still has memory plugged and the plugged memory is not in use.
>>   
>>   \devicenormative{\subsubsection}{Device Initialization}{Device Types / Memory Device / Device Initialization}
>>   
>> +A device MAY fail to operate further if VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE
>> +is not accepted.
>> +
>>   The device MUST NOT change the state of memory blocks during device reset.
>>   
>>   The device MUST NOT change the content of plugged memory blocks during
>> @@ -220,8 +228,11 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>   The driver MUST NOT read from unplugged memory blocks outside
>>   \field{usable_region_size}.
>>   
>> -The driver SHOULD NOT read from unplugged memory blocks inside
>> -\field{usable_region_size}.
>> +Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver SHOULD NOT read
> 
> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has not been negotiated, ..."

ack

> 
>> +memory of unplugged memory blocks inside \field{usable_region_size}.
>> +
>> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the driver MUST NOT read memory of
> 
> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."
> 

ack

>> +unplugged memory blocks.
>>   
>>   The driver MUST NOT request to unplug memory blocks while the memory is
>>   still in use.
>> @@ -246,10 +257,13 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>   
>>   The device MUST NOT change the content of plugged memory blocks.
>>   
>> -The device MUST allow the CPU to read from unplugged memory blocks inside
>> -the usable device-managed region. \footnote{To allow for simplified dumping of
>> -memory. The CPU is expected to copy such memory to another location before
>> -starting DMA.}
>> +Without VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MUST allow the CPU to
> 
> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has not been negotiated, ..."
> 
> 

ack

>> +read memory of unplugged memory blocks inside \field{usable_region_size}.
>> +\footnote{To allow for simplified dumping of memory. The CPU is expected to
>> +copy such memory to another location before starting DMA.}
>> +
>> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
> 
> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."
> 

ack

>> +read memory of unplugged memory blocks inside \field{usable_region_size}.
> 
> A compliant driver would not read that memory, would it?

Indeed. The device could decide to allow for reading, but it's pretty 
much undefined behavior.

-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[Cornelia Huck]

On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:

> On 17.08.21 10:50, Cornelia Huck wrote:
>> On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:
>>> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
>> 
>> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."
>> 
>
> ack
>
>>> +read memory of unplugged memory blocks inside \field{usable_region_size}.
>> 
>> A compliant driver would not read that memory, would it?
>
> Indeed. The device could decide to allow for reading, but it's pretty 
> much undefined behavior.

Maybe make it "SHOULD NOT"? Less strong than "MUST NOT", but still makes
clear that a driver reading it is pretty much doing the wrong thing.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[David Hildenbrand]

On 17.08.21 11:23, Cornelia Huck wrote:
> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>> On 17.08.21 10:50, Cornelia Huck wrote:
>>> On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:
>>>> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
>>>
>>> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."
>>>
>>
>> ack
>>
>>>> +read memory of unplugged memory blocks inside \field{usable_region_size}.
>>>
>>> A compliant driver would not read that memory, would it?
>>
>> Indeed. The device could decide to allow for reading, but it's pretty
>> much undefined behavior.
> 
> Maybe make it "SHOULD NOT"? Less strong than "MUST NOT", but still makes
> clear that a driver reading it is pretty much doing the wrong thing.
> 

I was also wondering to just drop it completely, because it's actually 
undefined behavior. What do you think?

-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH v1 1/2] virtio-mem: introduce VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE

[Cornelia Huck]

On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:

> On 17.08.21 11:23, Cornelia Huck wrote:
>> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
>> 
>>> On 17.08.21 10:50, Cornelia Huck wrote:
>>>> On Thu, Aug 12 2021, David Hildenbrand <david@redhat.com> wrote:
>>>>> +With VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE, the device MAY allow the CPU to
>>>>
>>>> "If VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE has been negotiated, ..."
>>>>
>>>
>>> ack
>>>
>>>>> +read memory of unplugged memory blocks inside \field{usable_region_size}.
>>>>
>>>> A compliant driver would not read that memory, would it?
>>>
>>> Indeed. The device could decide to allow for reading, but it's pretty
>>> much undefined behavior.
>> 
>> Maybe make it "SHOULD NOT"? Less strong than "MUST NOT", but still makes
>> clear that a driver reading it is pretty much doing the wrong thing.
>> 
>
> I was also wondering to just drop it completely, because it's actually 
> undefined behavior. What do you think?

Right, I'm not sure we need to specify this at all. Do others have an
opinion? The one thing it does is adding a nicely symmetrical statement.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[David Hildenbrand]

[...]

>>   \subsection{Device ID}\label{sec:Device Types / Memory Device / Device ID}
>>   24
>>   
>> @@ -47,7 +57,8 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Device / Feature bits}
>>   \item[VIRTIO_MEM_F_ACPI_PXM (0)] The field \field{node_id} in the device
>>   configuration is valid and corresponds to an ACPI PXM.
>>   \item[VIRTIO_MEM_F_UNPLUGGED_INACCESSIBLE (1)] The driver MUST NOT access
>> -unplugged memory.
>> +unplugged memory. \footnote{This feature is expected to be enabled on platforms
>> +with memory properties that might get modified implicitly on memory access.}
> 
> "On platforms with memory properties (...), this feature is expected to
> be offered by the device." ?
> 

Ack.

[...]

>>   
>> -The driver MUST NOT request to unplug memory blocks while the memory is
>> -still in use.
>> +The driver MUST NOT request unplug of memory blocks while corresponding memory
>> +or memory properties are still in use.
>>   
>>   The driver SHOULD initialize memory blocks after plugging them, the content
>>   is undefined.
>>   
>> +The driver SHOULD initialize memory properties of memory blocks after plugging
>> +them if it cannot deal with either the default settings or the previous
>> +setting.
> 
> This would imply that any memory properties need to allow modification
> by the driver, right? It's clear what you want to introduce this for,
> but do we need to tighten the definition of what kind of memory
> properties we are talking about?

Right, we actually want to have:

"The device MUST allow to read and write memory and querying and 
modifying memory properties of plugged memory blocks."

(I would have thought we'd have that but I cannot find it right now; too 
basic that I seem to have forgotten to add it initially)


What I want to document here, for example, is that on s390x you might 
just get the "0" storage key or the "stable" storage attribute (--> 
platform default) -- platform defaults. But you won't suddenly get a 
setting that would result in unexpected behavior (e.g., strange 
protection via the storage key, data loss due to the storage attribute).

So consequently, when e.g., plugging memory in Linux where we don't have 
to care about storage keys at all; we won't have to initialize storage 
keys when plugging memory, because it will contain a safe/default value 
(or just the old values the driver set earlier) that won't give us surprises

What would be your suggestion?

> 
>> +
>>   The driver SHOULD react to resize requests from the device
>>   (\field{requested_size} in the device configuration changed) by
>>   (un)plugging memory blocks.
>> @@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>   
>>   \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>>   
>> -The device MAY change the content of unplugged memory blocks at any time.
>> +The device MUST provide the exact same memory properties with the exact same
>> +semantics for device memory the platform provides in the same configuration for
>> +ordinary RAM.
> 
> This supposes that all RAM has the same properties across the whole
> platform, right? Do we want to be able to support some kind of
> heterogeneous platforms, or is that too much of an odd case?

I think, if the platform already doesn't have the same memory properties 
for all ordinary RAM (note that PMEM might be special and is not 
ordinary RAM) in the configuration, then we'd be dealing with an odd 
corner case already.

If we would have such a platform, then I'd assume that the device would 
also be flexible to either provide memory properties or not. Again, I'd 
say we'd mimic the exact same semantics as the paltform.

I guess once we actually run into such an odd case and want to handle it 
properly for virtio-mem, we'd have to document how to detect on such a 
platform if memory properties actually apply; there would have to be a 
way already to detect the same for other ordinary RAM.

Maybe we can rephrase this statement to cover this case already.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[Cornelia Huck]

On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:

>>> -The driver MUST NOT request to unplug memory blocks while the memory is
>>> -still in use.
>>> +The driver MUST NOT request unplug of memory blocks while corresponding memory
>>> +or memory properties are still in use.
>>>   
>>>   The driver SHOULD initialize memory blocks after plugging them, the content
>>>   is undefined.
>>>   
>>> +The driver SHOULD initialize memory properties of memory blocks after plugging
>>> +them if it cannot deal with either the default settings or the previous
>>> +setting.
>> 
>> This would imply that any memory properties need to allow modification
>> by the driver, right? It's clear what you want to introduce this for,
>> but do we need to tighten the definition of what kind of memory
>> properties we are talking about?
>
> Right, we actually want to have:
>
> "The device MUST allow to read and write memory and querying and 
> modifying memory properties of plugged memory blocks."
>

"...must allow the driver to..."

Yes, I agree.

> (I would have thought we'd have that but I cannot find it right now; too 
> basic that I seem to have forgotten to add it initially)
>
>
> What I want to document here, for example, is that on s390x you might 
> just get the "0" storage key or the "stable" storage attribute (--> 
> platform default) -- platform defaults. But you won't suddenly get a 
> setting that would result in unexpected behavior (e.g., strange 
> protection via the storage key, data loss due to the storage attribute).
>
> So consequently, when e.g., plugging memory in Linux where we don't have 
> to care about storage keys at all; we won't have to initialize storage 
> keys when plugging memory, because it will contain a safe/default value 
> (or just the old values the driver set earlier) that won't give us
> surprises

But isn't that something that the device needs to do? E.g. plug the
memory with the default storage key, or an old one if it replugs (and
whatever is done for normal memory.)

If the driver needs to modify those values, it should just go ahead and
do it, I guess; I don't think that needs to go into a normative
statement, but maybe into a paragraph that explains the general
functionality.

>
> What would be your suggestion?
>
>> 
>>> +
>>>   The driver SHOULD react to resize requests from the device
>>>   (\field{requested_size} in the device configuration changed) by
>>>   (un)plugging memory blocks.
>>> @@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>>   
>>>   \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>>>   
>>> -The device MAY change the content of unplugged memory blocks at any time.
>>> +The device MUST provide the exact same memory properties with the exact same
>>> +semantics for device memory the platform provides in the same configuration for
>>> +ordinary RAM.
>> 
>> This supposes that all RAM has the same properties across the whole
>> platform, right? Do we want to be able to support some kind of
>> heterogeneous platforms, or is that too much of an odd case?
>
> I think, if the platform already doesn't have the same memory properties 
> for all ordinary RAM (note that PMEM might be special and is not 
> ordinary RAM) in the configuration, then we'd be dealing with an odd 
> corner case already.
>
> If we would have such a platform, then I'd assume that the device would 
> also be flexible to either provide memory properties or not. Again, I'd 
> say we'd mimic the exact same semantics as the paltform.
>
> I guess once we actually run into such an odd case and want to handle it 
> properly for virtio-mem, we'd have to document how to detect on such a 
> platform if memory properties actually apply; there would have to be a 
> way already to detect the same for other ordinary RAM.
>
> Maybe we can rephrase this statement to cover this case already.

Perhaps we can talk about "comparable" RAM? IOW, if RAM in the same
conditions would have attributes, the device memory must have it as
well.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[David Hildenbrand]

On 17.08.21 12:24, Cornelia Huck wrote:
> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>>>> -The driver MUST NOT request to unplug memory blocks while the memory is
>>>> -still in use.
>>>> +The driver MUST NOT request unplug of memory blocks while corresponding memory
>>>> +or memory properties are still in use.
>>>>    
>>>>    The driver SHOULD initialize memory blocks after plugging them, the content
>>>>    is undefined.
>>>>    
>>>> +The driver SHOULD initialize memory properties of memory blocks after plugging
>>>> +them if it cannot deal with either the default settings or the previous
>>>> +setting.
>>>
>>> This would imply that any memory properties need to allow modification
>>> by the driver, right? It's clear what you want to introduce this for,
>>> but do we need to tighten the definition of what kind of memory
>>> properties we are talking about?
>>
>> Right, we actually want to have:
>>
>> "The device MUST allow to read and write memory and querying and
>> modifying memory properties of plugged memory blocks."
>>
> 
> "...must allow the driver to..."

Right, although we used something like "the device MUST allow the CPU 
to" ... and "The device MAY allow to read from unplugged memory blocks 
inside the region via DMA."

I guess if we want to cleanup, that would be e.g., "the device MUST 
allow the driver .. via the CPU".

> 
> Yes, I agree.
> 
>> (I would have thought we'd have that but I cannot find it right now; too
>> basic that I seem to have forgotten to add it initially)
>>
>>
>> What I want to document here, for example, is that on s390x you might
>> just get the "0" storage key or the "stable" storage attribute (-->
>> platform default) -- platform defaults. But you won't suddenly get a
>> setting that would result in unexpected behavior (e.g., strange
>> protection via the storage key, data loss due to the storage attribute).
>>
>> So consequently, when e.g., plugging memory in Linux where we don't have
>> to care about storage keys at all; we won't have to initialize storage
>> keys when plugging memory, because it will contain a safe/default value
>> (or just the old values the driver set earlier) that won't give us
>> surprises
> 
> But isn't that something that the device needs to do? E.g. plug the
> memory with the default storage key, or an old one if it replugs (and
> whatever is done for normal memory.)
> 
> If the driver needs to modify those values, it should just go ahead and
> do it, I guess; I don't think that needs to go into a normative
> statement, but maybe into a paragraph that explains the general
> functionality.

It's about which guarantees we give to the guest when plugging blocks. 
Optimally, we allow for sufficient freedom such that the device can 
avoid initializing things and the driver can avoid initializing things 
if the guest just doesn't care.

We also have in this patch: "The device MAY modify memory or reset 
memory properties to defaults of unplugged memory blocks at any time."

So the statement here is just the other way of looking at things: from 
the driver. If we can agree, we can just drop it, because the device 
description already tells us what can happen.

> 
>>
>> What would be your suggestion?
>>
>>>
>>>> +
>>>>    The driver SHOULD react to resize requests from the device
>>>>    (\field{requested_size} in the device configuration changed) by
>>>>    (un)plugging memory blocks.
>>>> @@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>>>    
>>>>    \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>>>>    
>>>> -The device MAY change the content of unplugged memory blocks at any time.
>>>> +The device MUST provide the exact same memory properties with the exact same
>>>> +semantics for device memory the platform provides in the same configuration for
>>>> +ordinary RAM.
>>>
>>> This supposes that all RAM has the same properties across the whole
>>> platform, right? Do we want to be able to support some kind of
>>> heterogeneous platforms, or is that too much of an odd case?
>>
>> I think, if the platform already doesn't have the same memory properties
>> for all ordinary RAM (note that PMEM might be special and is not
>> ordinary RAM) in the configuration, then we'd be dealing with an odd
>> corner case already.
>>
>> If we would have such a platform, then I'd assume that the device would
>> also be flexible to either provide memory properties or not. Again, I'd
>> say we'd mimic the exact same semantics as the paltform.
>>
>> I guess once we actually run into such an odd case and want to handle it
>> properly for virtio-mem, we'd have to document how to detect on such a
>> platform if memory properties actually apply; there would have to be a
>> way already to detect the same for other ordinary RAM.
>>
>> Maybe we can rephrase this statement to cover this case already.
> 
> Perhaps we can talk about "comparable" RAM? IOW, if RAM in the same
> conditions would have attributes, the device memory must have it as
> well.

Exactly, although it's still a bit vague it would give us a better idea 
what's actually expected.

Thanks!

-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[Cornelia Huck]

On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:

> On 17.08.21 12:24, Cornelia Huck wrote:
>> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
>> 
>>>>> -The driver MUST NOT request to unplug memory blocks while the memory is
>>>>> -still in use.
>>>>> +The driver MUST NOT request unplug of memory blocks while corresponding memory
>>>>> +or memory properties are still in use.
>>>>>    
>>>>>    The driver SHOULD initialize memory blocks after plugging them, the content
>>>>>    is undefined.
>>>>>    
>>>>> +The driver SHOULD initialize memory properties of memory blocks after plugging
>>>>> +them if it cannot deal with either the default settings or the previous
>>>>> +setting.
>>>>
>>>> This would imply that any memory properties need to allow modification
>>>> by the driver, right? It's clear what you want to introduce this for,
>>>> but do we need to tighten the definition of what kind of memory
>>>> properties we are talking about?
>>>
>>> Right, we actually want to have:
>>>
>>> "The device MUST allow to read and write memory and querying and
>>> modifying memory properties of plugged memory blocks."
>>>
>> 
>> "...must allow the driver to..."
>
> Right, although we used something like "the device MUST allow the CPU 
> to" ... and "The device MAY allow to read from unplugged memory blocks 
> inside the region via DMA."
>
> I guess if we want to cleanup, that would be e.g., "the device MUST 
> allow the driver .. via the CPU".

Hm, maybe. Need to think about that.

>
>> 
>> Yes, I agree.
>> 
>>> (I would have thought we'd have that but I cannot find it right now; too
>>> basic that I seem to have forgotten to add it initially)
>>>
>>>
>>> What I want to document here, for example, is that on s390x you might
>>> just get the "0" storage key or the "stable" storage attribute (-->
>>> platform default) -- platform defaults. But you won't suddenly get a
>>> setting that would result in unexpected behavior (e.g., strange
>>> protection via the storage key, data loss due to the storage attribute).
>>>
>>> So consequently, when e.g., plugging memory in Linux where we don't have
>>> to care about storage keys at all; we won't have to initialize storage
>>> keys when plugging memory, because it will contain a safe/default value
>>> (or just the old values the driver set earlier) that won't give us
>>> surprises
>> 
>> But isn't that something that the device needs to do? E.g. plug the
>> memory with the default storage key, or an old one if it replugs (and
>> whatever is done for normal memory.)
>> 
>> If the driver needs to modify those values, it should just go ahead and
>> do it, I guess; I don't think that needs to go into a normative
>> statement, but maybe into a paragraph that explains the general
>> functionality.
>
> It's about which guarantees we give to the guest when plugging blocks. 
> Optimally, we allow for sufficient freedom such that the device can 
> avoid initializing things and the driver can avoid initializing things 
> if the guest just doesn't care.
>
> We also have in this patch: "The device MAY modify memory or reset 
> memory properties to defaults of unplugged memory blocks at any time."
>
> So the statement here is just the other way of looking at things: from 
> the driver. If we can agree, we can just drop it, because the device 
> description already tells us what can happen.

So we already have the default values covered, which should be good
enough. Let's drop it, unless someone disagrees.

>
>> 
>>>
>>> What would be your suggestion?
>>>
>>>>
>>>>> +
>>>>>    The driver SHOULD react to resize requests from the device
>>>>>    (\field{requested_size} in the device configuration changed) by
>>>>>    (un)plugging memory blocks.
>>>>> @@ -253,25 +271,34 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
>>>>>    
>>>>>    \devicenormative{\subsubsection}{Device Operation}{Device Types / Memory Device / Device Operation}
>>>>>    
>>>>> -The device MAY change the content of unplugged memory blocks at any time.
>>>>> +The device MUST provide the exact same memory properties with the exact same
>>>>> +semantics for device memory the platform provides in the same configuration for
>>>>> +ordinary RAM.
>>>>
>>>> This supposes that all RAM has the same properties across the whole
>>>> platform, right? Do we want to be able to support some kind of
>>>> heterogeneous platforms, or is that too much of an odd case?
>>>
>>> I think, if the platform already doesn't have the same memory properties
>>> for all ordinary RAM (note that PMEM might be special and is not
>>> ordinary RAM) in the configuration, then we'd be dealing with an odd
>>> corner case already.
>>>
>>> If we would have such a platform, then I'd assume that the device would
>>> also be flexible to either provide memory properties or not. Again, I'd
>>> say we'd mimic the exact same semantics as the paltform.
>>>
>>> I guess once we actually run into such an odd case and want to handle it
>>> properly for virtio-mem, we'd have to document how to detect on such a
>>> platform if memory properties actually apply; there would have to be a
>>> way already to detect the same for other ordinary RAM.
>>>
>>> Maybe we can rephrase this statement to cover this case already.
>> 
>> Perhaps we can talk about "comparable" RAM? IOW, if RAM in the same
>> conditions would have attributes, the device memory must have it as
>> well.
>
> Exactly, although it's still a bit vague it would give us a better idea 
> what's actually expected.

If we need to be able to specify something more concrete/complex, we can
always use a feature bit, although I don't think it will be needed.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] [PATCH v1 2/2] virtio-mem: describe interaction with memory properties

[David Hildenbrand]

On 17.08.21 15:25, Cornelia Huck wrote:
> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
> 
>> On 17.08.21 12:24, Cornelia Huck wrote:
>>> On Tue, Aug 17 2021, David Hildenbrand <david@redhat.com> wrote:
>>>
>>>>>> -The driver MUST NOT request to unplug memory blocks while the memory is
>>>>>> -still in use.
>>>>>> +The driver MUST NOT request unplug of memory blocks while corresponding memory
>>>>>> +or memory properties are still in use.
>>>>>>     
>>>>>>     The driver SHOULD initialize memory blocks after plugging them, the content
>>>>>>     is undefined.
>>>>>>     
>>>>>> +The driver SHOULD initialize memory properties of memory blocks after plugging
>>>>>> +them if it cannot deal with either the default settings or the previous
>>>>>> +setting.
>>>>>
>>>>> This would imply that any memory properties need to allow modification
>>>>> by the driver, right? It's clear what you want to introduce this for,
>>>>> but do we need to tighten the definition of what kind of memory
>>>>> properties we are talking about?
>>>>
>>>> Right, we actually want to have:
>>>>
>>>> "The device MUST allow to read and write memory and querying and
>>>> modifying memory properties of plugged memory blocks."
>>>>
>>>
>>> "...must allow the driver to..."
>>
>> Right, although we used something like "the device MUST allow the CPU
>> to" ... and "The device MAY allow to read from unplugged memory blocks
>> inside the region via DMA."
>>
>> I guess if we want to cleanup, that would be e.g., "the device MUST
>> allow the driver .. via the CPU".
> 
> Hm, maybe. Need to think about that.

Something like this:


commit 2afa0c5e00889e2030189d843bc6df7bf2ee10c7 (HEAD)
Author: David Hildenbrand <david@redhat.com>
Date:   Tue Aug 17 14:24:18 2021 +0200

     virtio-mem: simplify statements that express unexpected behavior on memory access
     
     Some statements express that the device MAY allow access to memory insided
     unplugged memory blocks, although it's really just unexpected behavior and
     conforming drivers MUST NOT perform such access.
     
     Clarify that, and move the special CPU vs. DMA handling for some
     unplugged memory blocks to the driver section instead.
     
     While at it, start rephrasing our statements to clarify and prepare for
     further changes.
     
     Signed-off-by: David Hildenbrand <david@redhat.com>

diff --git a/virtio-mem.tex b/virtio-mem.tex
index 62a1d02..f0c5970 100644
--- a/virtio-mem.tex
+++ b/virtio-mem.tex
@@ -220,8 +220,11 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
  The driver MUST NOT read from unplugged memory blocks outside
  \field{usable_region_size}.
  
-The driver SHOULD NOT read from unplugged memory blocks inside
-\field{usable_region_size}.
+The driver MUST NOT read memory of unplugged memory blocks inside
+\field{usable_region_size} via DMA.
+
+The driver SHOULD NOT read memory of unplugged memory blocks inside
+\field{usable_region_size} via the CPU.
  
  The driver MUST NOT request to unplug memory blocks while the memory is
  still in use.
@@ -246,18 +249,10 @@ \subsection{Device Operation}\label{sec:Device Types / Memory Device / Device Op
  
  The device MUST NOT change the content of plugged memory blocks.
  
-The device MUST allow the CPU to read from unplugged memory blocks inside
-the usable device-managed region. \footnote{To allow for simplified dumping of
-memory. The CPU is expected to copy such memory to another location before
-starting DMA.}
-
-The device MAY allow to read from unplugged memory blocks inside the
-usable device-managed region via DMA.
-
-The device MAY allow to read from unplugged memory blocks outside
-the usable device-managed region.
-
-The device MAY allow to write to unplugged memory blocks.
+The device MUST allow the driver to read memory of unplugged memory blocks
+inside \field{usable_region_size} via the CPU. \footnote{To allow for simplified
+dumping of memory. The CPU is expected to copy such memory to another location
+before starting DMA.}
  
  The device MAY change the state of memory blocks during system resets.
  



-- 
Thanks,

David / dhildenb