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

[Cornelia Huck <cohuck@redhat.com>]

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.