Re: [virtio-comment] [PATCH v4 1/3] content: Document balloon feature page poison

[Alexander Duyck <alexander.duyck@gmail.com>]

On Fri, May 29, 2020 at 1:13 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 27.05.20 06:06, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Page poison provides a way for the guest to notify the host that it is
> > initializing or poisoning freed pages with some specific poison value. As a
> > result of this we can infer a couple traits about the guest:
> >
> > 1. Free pages will contain a specific pattern within the guest.
> > 2. Modifying free pages from this value may cause an error in the guest.
> > 3. Pages will be immediately written to by the driver when deflated.
> >
> > There are currently no existing features that make use of this data. In the
> > upcoming feature free page reporting we will need to make use of this to
> > identify if we can evict pages from the guest without causing data
> > corruption.
> >
> > Add documentation for the page poison feature describing the basic
> > functionality and requirements.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> >  conformance.tex |    2 ++
> >  content.tex     |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/conformance.tex b/conformance.tex
> > index b6fdec090383..4ed9d62e8088 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -149,6 +149,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >  \item \ref{drivernormative:Device Types / Memory Balloon Device / Feature bits}
> >  \item \ref{drivernormative:Device Types / Memory Balloon Device / Device Operation}
> >  \item \ref{drivernormative:Device Types / Memory Balloon Device / Device Operation / Memory Statistics}
> > +\item \ref{drivernormative:Device Types / Memory Balloon Device / Device Operation / Page Poison}
> >  \end{itemize}
> >
> >  \conformance{\subsection}{SCSI Host Driver Conformance}\label{sec:Conformance / Driver Conformance / SCSI Host Driver Conformance}
> > @@ -331,6 +332,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >  \item \ref{devicenormative:Device Types / Memory Balloon Device / Feature bits}
> >  \item \ref{devicenormative:Device Types / Memory Balloon Device / Device Operation}
> >  \item \ref{devicenormative:Device Types / Memory Balloon Device / Device Operation / Memory Statistics}
> > +\item \ref{devicenormative:Device Types / Memory Balloon Device / Device Operation / Page Poison}
> >  \end{itemize}
> >
> >  \conformance{\subsection}{SCSI Host Device Conformance}\label{sec:Conformance / Device Conformance / SCSI Host Device Conformance}
> > diff --git a/content.tex b/content.tex
> > index 91735e3eb018..4a0ab90260ff 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -5019,6 +5019,9 @@ \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Featu
> >      memory statistics is present.
> >  \item[VIRTIO_BALLOON_F_DEFLATE_ON_OOM (2) ] Deflate balloon on
> >      guest out of memory condition.
> > +\item[ VIRTIO_BALLOON_F_PAGE_POISON(4) ] A hint to the device, that the driver
> > +    will immediately write \field{poison_val} to pages after deflating them.
> > +    Configuration field \field{poison_val} is valid.
> >
>
> Here we have "that the driver will immediately" ...
>
> But we never document that in form of a normative statement (e.g., "The
> driver MUST initialize pages with \field{poison_val} after deflating").

I'm pretty sure we did document that. In the normative statement for
the driver below we have:
+The driver MUST initialize the deflated pages with \field{poison_val} when
+they are reused by the driver.

> Just wondering if that is intended (I imagine it will be different with
> free page reporting)?

I think we add some additional items with page reporting, but I think
those are in the page reporting section if I recall correctly.

> Apart from that looks good to me!
>
> --
> Thanks,
>
> David / dhildenb
>

Thanks for taking all the time to review this.

- Alex