Re: [virtio-comment] [PATCH v5 1/1] virtio-pmem: Support describing pmem as shared memory region

[Cornelia Huck <cohuck@redhat.com>]

On Sun, Nov 21 2021, Taylor Stark <tstark@linux.microsoft.com> wrote:

> On Thu, Nov 18, 2021 at 06:37:52PM +0100, Halil Pasic wrote:
>> On Wed, 10 Nov 2021 10:55:55 -0800
>> tstark@linux.microsoft.com wrote:
>> [..]
>> > @@ -36,22 +39,45 @@ \subsection{Device configuration layout}\label{sec:Device Types / PMEM Device /
>> >  \end{lstlisting}
>> >  
>> >  \begin{description}
>> > -\item[\field{start}] contains the physical address of the first byte of the persistent memory region.
>> > +\item[\field{start}] contains the physical address of the first byte of the
>> > +persistent memory region, if VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated.
>> >  
>> > -\item[\field{size}] contains the length of this address range.
>> > +\item[\field{size}] contains the length of this address range, if
>> > +VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated.
>> >  \end{description}
>> >  
>> > +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization}
>> > +
>> > +The device indicates the guest physical address to the driver in one of two ways:
>> >  \begin{enumerate}
>> > -\item Driver vpmem start is read from \field{start}.
>> > -\item Driver vpmem end is read from \field{size}.
>> > +\item As a guest absolute address, using virtio_pmem_config.
>> > +\item As a shared memory region.
>> >  \end{enumerate}
>> >  
>> > -\subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
>> > -
>> >  The driver determines the start address and size of the persistent memory region in preparation for reading or writing data.
>> >  
>> >  The driver initializes req_vq in preparation for making flush requests.
>> >  
>> > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
>> > +
>> > +If VIRTIO_PMEM_F_SHMEM_REGION has been negotiated, the device MUST indicate the
>> > +guest physical address as a shared memory region. The device MUST use shared
>> > +memory region ID 0. The device SHOULD set \field{start} and \field{size} to zero.
>> > +
>> > +
>> > +If VIRTIO_PMEM_F_SHMEM_REGION has not been negotiated, the device MUST indicate

[for the sake of the argument made below, note the 'has not been
negotiated' here]

>> > +the guest physical address as a guest absolute address. The device MUST set
>> > +\field{start} to the absolute address and \field{size} to the size of the
>> > +address range, in bytes.
>> 
>> Sorry for joining in this late. 
>> 
>> I'm wondering if the quoted parts implies that the device SHOULD change
>> its config space (fields start and size) when the driver sets
>> FEATURES_OK, and the feature is VIRTIO_PMEM_F_SHMEM_REGION set at the
>> device (it was offered, and got set).
>> 
>> My train of thought is: initially no feature is considered negotiated,
>> and config space access is allowed before feature negotiation is
>> completed. That means the at this stage the device must indicate via
>> the config space, and thus \field{size} != 0. But as a part of the
>> transition 'features not negotiated' -> 'features negotiated' the device
>> needs to go '\field{size} != 0' -> '\field{size} == 0'

It has been advised that it SHOULD put 0 there, but I don't consider
that a must, just a good idea.

>> 
>> Is that what we want?
>> 
>> Also I understand that Hyper-V would make the device cease operation if
>> the feature VIRTIO_PMEM_F_SHMEM_REGION was not accepted by the driver.
>> I believe I've read that in some previous email.
>> 
>> Do we need a  'MAY fail to operate further if' statement? Anything that
>> ain't guarded by feature bits ain't optional, and anything that is
>> guarded by feature bits is optional unless explicitly stated otherwise.

The device can always fail the setting of FEATURES_OK, see 2.2.2:

"The device SHOULD accept any valid subset of features the driver
accepts, otherwise it MUST fail to set the FEATURES_OK device status bit
when the driver writes it."

I'd argue that "SHMEM_REGION not set" can be considered a non-valid
subset by the device. (Also, there's a SHOULD in there.)

>> 
>> Regards,
>> Halil
>
> No problem, thanks for joining in! :) I think your train of thought makes sense.
> You're right, for Hyper-V we can't support any driver that doesn't support
> VIRTIO_PMEM_F_SHMEM_REGION. So we have no choice but to set the config fields
> to zero from the start. There's no meaningful value we can place in those fields.

I think this is fine: we only require the fields to have meaningful
contents if SHMEM_REGION has not been negotiated -- as long as the
device has not accepted FEATURES_OK from the driver, nothing has been
negotiated respectively not been negotiated.

>
> I'm trying to figure out how best to word this. Saying that devices should set
> config fields prior to FEATURE_OK is redundant. Should I add a section saying that
> config fields MAY be set to 0 if VIRTIO_PMEM_F_SHMEM_REGION is offerred (such as
> in the Hyper-V case)? And that if it's set to 0 and the feature isn't negotiated,
> the device must reject the driver? I'd also have to add in a section saying that
> the pmem region shouldn't be touched by the driver prior to DRIVER_OK being set,
> to save drivers from accessing the zeroed values placed in the config. Additionally,
> encouraging devices to set the config fields to 0 once the feature has been
> negotiated seems like the right thing to do, to catch bugs?

I don't think we can prohibit the driver from reading the config space,
that is explicitly allowed in the general case.

We probably can add a non-normative extra statement, though, that a
device may present zeroes in the config space before feature negotiation
has finished if it doesn't support !SHMEM_REGION. That would also give
the driver a way to discover that it doesn't make sense to continue
without negotiating SHMEM_REGION. No extra normative statement, but
should make it a bit more clear to those trying to implement this.

>
> I'm not sure about the 'MAY fail to operate further statement'. Couldn't there
> be lots of reasons why a device rejects a driver? And wouldn't those be
> implementation details of each device? Is it common to document those?

We probably don't need such a statement (see above).

As voting for this is open until tomorrow, we should quickly decide what
to do. My preference would be to merge it as-is and do a clarification
on top.