[virtio-comment] [PATCH 0/1] virtio-pmem: Support PCI BAR-relative addresses

[virtio-comment] [PATCH 0/1] virtio-pmem: Support PCI BAR-relative addresses

[tstark]

From: Taylor Stark <tstark@microsoft.com>

This patch updates the virtio-pmem spec to add support for describing the pmem
region via PCI BARs. This is required to support virtio-pmem in Hyper-V, since
Hyper-V only allows PCI devices to operate on memory ranges defined via BARs.

As virtio-pmem hasn't been added to the virtio spec yet (see this issue [1]),
this patch is based off the RFC spec [2]. The linux driver implementation has
been posted at [3].

[1] https://github.com/oasis-tcs/virtio-spec/issues/78
[2] https://lists.oasis-open.org/archives/virtio-dev/201903/msg00083.html
[3] https://lore.kernel.org/nvdimm/20210715223505.GA29329@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net

<Apologies for the resend - forgot to subscribe to virtio-comment so my original mail bounced>

Taylor Stark (1):
  virtio_pmem: Support PCI BAR-relative addresses

 virtio-pmem.tex | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

-- 
2.31.0.vfs.0.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 1/1] virtio-pmem: Support PCI BAR-relative addresses

[tstark]

From: Taylor Stark <tstark@microsoft.com>

Update the virtio-pmem RFC spec to add support for describing the pmem region
via PCI BARs. Shared memory windows are used to accomplish this, similar to
virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
since Hyper-V only allows PCI devices to operate on memory ranges defined via
BARs.

Signed-off-by: Taylor Stark <tstark@microsoft.com>
---
 virtio-pmem.tex | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/virtio-pmem.tex b/virtio-pmem.tex
index 04e07bb..3f7d48e 100644
--- a/virtio-pmem.tex
+++ b/virtio-pmem.tex
@@ -40,11 +40,21 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
 Device hotplugs physical memory to guest address space. Persistent memory device
 is emulated with file backed memory at host side.
 
+The device MUST indicate the guest physical address to the driver in one of two
+ways:
 \begin{enumerate}
-\item Guest vpmem start is read from \field{start}.
-\item Guest vpmem end is read from \field{size}.
+\item As a guest absolute address.
+\item As a shared memory region.
 \end{enumerate}
 
+If the guest physical address is indicated as an absolute address, the device
+MUST set \field{start} to the absolute address and \field{size} to the size of
+the address range, in bytes.
+
+If the guest physical address is indicated as a shared memory region, the shared
+memory region MUST be shared memory region ID 0. The device SHOULD set
+\field{start} and \field{size} to zero.
+
 \devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
 
 File backed memory SHOULD be memory mapped to guest address space with SHARED
@@ -52,9 +62,11 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
 
 \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
 
-Driver hotplugs the physical memory and registers associated
-region with the pmem API. Also, configures a flush callback
-function with the corresponding region.
+The driver SHOULD query the physical address ranges where the pmem was mapped.
+When performing the query, the driver MUST first query if shared memory region
+ID 0 is supported by the device. If present, the driver MUST NOT use \field{start}
+or \field{size}. If not present, the driver SHOULD fallback to reading the
+physical address ranges from \field{start} and \field{size}.
 
 \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}
 
-- 
2.31.0.vfs.0.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 0/1] virtio-pmem: Support PCI BAR-relative addresses

[Cornelia Huck]

On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:

> From: Taylor Stark <tstark@microsoft.com>
>
> This patch updates the virtio-pmem spec to add support for describing the pmem
> region via PCI BARs. This is required to support virtio-pmem in Hyper-V, since
> Hyper-V only allows PCI devices to operate on memory ranges defined via BARs.
>
> As virtio-pmem hasn't been added to the virtio spec yet (see this issue [1]),
> this patch is based off the RFC spec [2]. The linux driver implementation has
> been posted at [3].

What's the general status of the pmem spec? I had pinged in the issue,
but I have not seen any progress.

>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/78
> [2] https://lists.oasis-open.org/archives/virtio-dev/201903/msg00083.html
> [3] https://lore.kernel.org/nvdimm/20210715223505.GA29329@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net
>
> <Apologies for the resend - forgot to subscribe to virtio-comment so my original mail bounced>
>
> Taylor Stark (1):
>   virtio_pmem: Support PCI BAR-relative addresses
>
>  virtio-pmem.tex | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)


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 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Cornelia Huck]

On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:

> From: Taylor Stark <tstark@microsoft.com>
>
> Update the virtio-pmem RFC spec to add support for describing the pmem region
> via PCI BARs. Shared memory windows are used to accomplish this, similar to
> virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
> since Hyper-V only allows PCI devices to operate on memory ranges defined via
> BARs.

Given that we already have pmem support out there (even though the spec
has not been included yet, should this get a feature?

>
> Signed-off-by: Taylor Stark <tstark@microsoft.com>
> ---
>  virtio-pmem.tex | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/virtio-pmem.tex b/virtio-pmem.tex
> index 04e07bb..3f7d48e 100644
> --- a/virtio-pmem.tex
> +++ b/virtio-pmem.tex
> @@ -40,11 +40,21 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
>  Device hotplugs physical memory to guest address space. Persistent memory device
>  is emulated with file backed memory at host side.
>  
> +The device MUST indicate the guest physical address to the driver in one of two
> +ways:

This is not a normative section; better use "The device indicates...."?

>  \begin{enumerate}
> -\item Guest vpmem start is read from \field{start}.
> -\item Guest vpmem end is read from \field{size}.
> +\item As a guest absolute address.
> +\item As a shared memory region.
>  \end{enumerate}
>  
> +If the guest physical address is indicated as an absolute address, the device
> +MUST set \field{start} to the absolute address and \field{size} to the size of
> +the address range, in bytes.
> +
> +If the guest physical address is indicated as a shared memory region, the shared
> +memory region MUST be shared memory region ID 0. The device SHOULD set
> +\field{start} and \field{size} to zero.

And these two paragraphs should go into a normative section.

> +
>  \devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
>  
>  File backed memory SHOULD be memory mapped to guest address space with SHARED
> @@ -52,9 +62,11 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
>  
>  \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
>  
> -Driver hotplugs the physical memory and registers associated
> -region with the pmem API. Also, configures a flush callback
> -function with the corresponding region.
> +The driver SHOULD query the physical address ranges where the pmem was mapped.
> +When performing the query, the driver MUST first query if shared memory region
> +ID 0 is supported by the device. If present, the driver MUST NOT use \field{start}
> +or \field{size}. If not present, the driver SHOULD fallback to reading the
> +physical address ranges from \field{start} and \field{size}.

Reading this, I think we really need to have a feature to distinguish
between the two modes; otherwise, old drivers will be confused.

Also, requirements belong in a normative section; it's better to just
describe textually what the driver needs to do here and split out the
normative statements.

>  
>  \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}


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 0/1] virtio-pmem: Support PCI BAR-relative addresses

[Pankaj Gupta]

Hi Cornelia,

> > This patch updates the virtio-pmem spec to add support for describing the pmem
> > region via PCI BARs. This is required to support virtio-pmem in Hyper-V, since
> > Hyper-V only allows PCI devices to operate on memory ranges defined via BARs.
> >
> > As virtio-pmem hasn't been added to the virtio spec yet (see this issue [1]),
> > this patch is based off the RFC spec [2]. The linux driver implementation has
> > been posted at [3].
>
> What's the general status of the pmem spec? I had pinged in the issue,
> but I have not seen any progress.

Appologies for missing your ping (As I was down for some time). I will
try to post the
virtio-pmem spec new version in coming week.

Thanks,
Pankaj

Re: [virtio-comment] [PATCH 0/1] virtio-pmem: Support PCI BAR-relative addresses

[Cornelia Huck]

On Thu, Jul 22 2021, Pankaj Gupta <pankaj.gupta@ionos.com> wrote:

> Hi Cornelia,
>
>> > This patch updates the virtio-pmem spec to add support for describing the pmem
>> > region via PCI BARs. This is required to support virtio-pmem in Hyper-V, since
>> > Hyper-V only allows PCI devices to operate on memory ranges defined via BARs.
>> >
>> > As virtio-pmem hasn't been added to the virtio spec yet (see this issue [1]),
>> > this patch is based off the RFC spec [2]. The linux driver implementation has
>> > been posted at [3].
>>
>> What's the general status of the pmem spec? I had pinged in the issue,
>> but I have not seen any progress.
>
> Appologies for missing your ping (As I was down for some time). I will
> try to post the
> virtio-pmem spec new version in coming week.

Thanks, that's good news!


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 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Taylor Stark]

On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
> On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
> 
> > From: Taylor Stark <tstark@microsoft.com>
> >
> > Update the virtio-pmem RFC spec to add support for describing the pmem region
> > via PCI BARs. Shared memory windows are used to accomplish this, similar to
> > virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
> > since Hyper-V only allows PCI devices to operate on memory ranges defined via
> > BARs.
> 
> Given that we already have pmem support out there (even though the spec
> has not been included yet, should this get a feature?

I wasn't sure if we needed to handle backwards compatibility given that pmem
hasn't been merged into the spec yet. If we do, then yes I think it makes sense
to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?
 
> >
> > Signed-off-by: Taylor Stark <tstark@microsoft.com>
> > ---
> >  virtio-pmem.tex | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/virtio-pmem.tex b/virtio-pmem.tex
> > index 04e07bb..3f7d48e 100644
> > --- a/virtio-pmem.tex
> > +++ b/virtio-pmem.tex
> > @@ -40,11 +40,21 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
> >  Device hotplugs physical memory to guest address space. Persistent memory device
> >  is emulated with file backed memory at host side.
> >  
> > +The device MUST indicate the guest physical address to the driver in one of two
> > +ways:
> 
> This is not a normative section; better use "The device indicates...."?
> 
> >  \begin{enumerate}
> > -\item Guest vpmem start is read from \field{start}.
> > -\item Guest vpmem end is read from \field{size}.
> > +\item As a guest absolute address.
> > +\item As a shared memory region.
> >  \end{enumerate}
> >  
> > +If the guest physical address is indicated as an absolute address, the device
> > +MUST set \field{start} to the absolute address and \field{size} to the size of
> > +the address range, in bytes.
> > +
> > +If the guest physical address is indicated as a shared memory region, the shared
> > +memory region MUST be shared memory region ID 0. The device SHOULD set
> > +\field{start} and \field{size} to zero.
> 
> And these two paragraphs should go into a normative section.
> 
> > +
> >  \devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
> >  
> >  File backed memory SHOULD be memory mapped to guest address space with SHARED
> > @@ -52,9 +62,11 @@ \subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device
> >  
> >  \subsection{Driver Initialization}\label{sec:Device Types / PMEM Driver / Driver Initialization}
> >  
> > -Driver hotplugs the physical memory and registers associated
> > -region with the pmem API. Also, configures a flush callback
> > -function with the corresponding region.
> > +The driver SHOULD query the physical address ranges where the pmem was mapped.
> > +When performing the query, the driver MUST first query if shared memory region
> > +ID 0 is supported by the device. If present, the driver MUST NOT use \field{start}
> > +or \field{size}. If not present, the driver SHOULD fallback to reading the
> > +physical address ranges from \field{start} and \field{size}.
> 
> Reading this, I think we really need to have a feature to distinguish
> between the two modes; otherwise, old drivers will be confused.
> 
> Also, requirements belong in a normative section; it's better to just
> describe textually what the driver needs to do here and split out the
> normative statements.

Will do (ditto for your other comments). Thanks for the suggestions. It's
a learning process for me - had to go and lookup what normative meant.. :)

> >  
> >  \drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}

Re: [virtio-comment] [PATCH 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Cornelia Huck]

On Thu, Jul 22 2021, Taylor Stark <tstark@linux.microsoft.com> wrote:

> On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
>> On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
>> 
>> > From: Taylor Stark <tstark@microsoft.com>
>> >
>> > Update the virtio-pmem RFC spec to add support for describing the pmem region
>> > via PCI BARs. Shared memory windows are used to accomplish this, similar to
>> > virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
>> > since Hyper-V only allows PCI devices to operate on memory ranges defined via
>> > BARs.
>> 
>> Given that we already have pmem support out there (even though the spec
>> has not been included yet, should this get a feature?
>
> I wasn't sure if we needed to handle backwards compatibility given that pmem
> hasn't been merged into the spec yet. If we do, then yes I think it makes sense
> to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?

Sounds good to me.

>> Also, requirements belong in a normative section; it's better to just
>> describe textually what the driver needs to do here and split out the
>> normative statements.
>
> Will do (ditto for your other comments). Thanks for the suggestions. It's
> a learning process for me - had to go and lookup what normative meant.. :)

If you have any further questions (or any suggestions how we can make
this easier for newcomers), feel free to follow up :)


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 1/1] virtio-pmem: Support PCI BAR-relative addresses

[David Hildenbrand]

On 23.07.21 01:26, Taylor Stark wrote:
> On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
>> On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
>>
>>> From: Taylor Stark <tstark@microsoft.com>
>>>
>>> Update the virtio-pmem RFC spec to add support for describing the pmem region
>>> via PCI BARs. Shared memory windows are used to accomplish this, similar to
>>> virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
>>> since Hyper-V only allows PCI devices to operate on memory ranges defined via
>>> BARs.
>>
>> Given that we already have pmem support out there (even though the spec
>> has not been included yet, should this get a feature?
> 
> I wasn't sure if we needed to handle backwards compatibility given that pmem
> hasn't been merged into the spec yet. If we do, then yes I think it makes sense
> to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?

The implementations that are upstream win, and usually we get a spec 
after the implementation is mature. The spec merely documents what the 
existing implementations are doing. Of course, one can plan ahead and 
propose additions to the spec that won't break existing setups.

The feature seems to be glued to PCI BARs, right? Maybe that should be 
part of the feature name.

-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Taylor Stark]

On Fri, Jul 23, 2021 at 09:01:48AM +0200, David Hildenbrand wrote:
> On 23.07.21 01:26, Taylor Stark wrote:
> >On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
> >>On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
> >>
> >>>From: Taylor Stark <tstark@microsoft.com>
> >>>
> >>>Update the virtio-pmem RFC spec to add support for describing the pmem region
> >>>via PCI BARs. Shared memory windows are used to accomplish this, similar to
> >>>virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
> >>>since Hyper-V only allows PCI devices to operate on memory ranges defined via
> >>>BARs.
> >>
> >>Given that we already have pmem support out there (even though the spec
> >>has not been included yet, should this get a feature?
> >
> >I wasn't sure if we needed to handle backwards compatibility given that pmem
> >hasn't been merged into the spec yet. If we do, then yes I think it makes sense
> >to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?
> 
> The implementations that are upstream win, and usually we get a spec
> after the implementation is mature. The spec merely documents what
> the existing implementations are doing. Of course, one can plan
> ahead and propose additions to the spec that won't break existing
> setups.
> 
> The feature seems to be glued to PCI BARs, right? Maybe that should
> be part of the feature name.
> 
> -- 
> Thanks,
> 
> David / dhildenb

I had to do a bit of research to develop an opinion on including BAR in
the name. Using BARs is the implementation for the PCI transport, but there
are other transports that have support for shared memory regions but don't
use BARs (e.g. mmio - see vm_get_shm_region). Using shared memory regions
was initially a convenient way to use PCI BARs, but I think it could be
valuable for other transports as well (it adds similarity with other virtio
devices). So I think it makes sense to leave BAR out of the name, since
that's transport specific. Maybe I should retitle the patch? Let me know
your thoughts (especially since I'm learning on the fly).

Thanks,
Taylor

Re: [virtio-comment] [PATCH 1/1] virtio-pmem: Support PCI BAR-relative addresses

[David Hildenbrand]

On 23.07.21 20:38, Taylor Stark wrote:
> On Fri, Jul 23, 2021 at 09:01:48AM +0200, David Hildenbrand wrote:
>> On 23.07.21 01:26, Taylor Stark wrote:
>>> On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
>>>> On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
>>>>
>>>>> From: Taylor Stark <tstark@microsoft.com>
>>>>>
>>>>> Update the virtio-pmem RFC spec to add support for describing the pmem region
>>>>> via PCI BARs. Shared memory windows are used to accomplish this, similar to
>>>>> virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
>>>>> since Hyper-V only allows PCI devices to operate on memory ranges defined via
>>>>> BARs.
>>>>
>>>> Given that we already have pmem support out there (even though the spec
>>>> has not been included yet, should this get a feature?
>>>
>>> I wasn't sure if we needed to handle backwards compatibility given that pmem
>>> hasn't been merged into the spec yet. If we do, then yes I think it makes sense
>>> to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?
>>
>> The implementations that are upstream win, and usually we get a spec
>> after the implementation is mature. The spec merely documents what
>> the existing implementations are doing. Of course, one can plan
>> ahead and propose additions to the spec that won't break existing
>> setups.
>>
>> The feature seems to be glued to PCI BARs, right? Maybe that should
>> be part of the feature name.
>>
>> -- 
>> Thanks,
>>
>> David / dhildenb
> 
> I had to do a bit of research to develop an opinion on including BAR in
> the name. Using BARs is the implementation for the PCI transport, but there
> are other transports that have support for shared memory regions but don't
> use BARs (e.g. mmio - see vm_get_shm_region). Using shared memory regions
> was initially a convenient way to use PCI BARs, but I think it could be
> valuable for other transports as well (it adds similarity with other virtio
> devices). So I think it makes sense to leave BAR out of the name, since
> that's transport specific. Maybe I should retitle the patch? Let me know
> your thoughts (especially since I'm learning on the fly).

Ah, the content doesn't actually talk about PCI BARs, that's my source 
of confusion :)

I know that virtio-fs also uses BARs in DAX mode, but I have no idea how 
they named features (if any?) or how they described it in their spec. 
Might be worth exploring.


-- 
Thanks,

David / dhildenb

Re: [virtio-comment] [PATCH 1/1] virtio-pmem: Support PCI BAR-relative addresses

[Taylor Stark]

On Fri, Jul 23, 2021 at 09:18:21PM +0200, David Hildenbrand wrote:
> On 23.07.21 20:38, Taylor Stark wrote:
> >On Fri, Jul 23, 2021 at 09:01:48AM +0200, David Hildenbrand wrote:
> >>On 23.07.21 01:26, Taylor Stark wrote:
> >>>On Thu, Jul 22, 2021 at 01:24:17PM +0200, Cornelia Huck wrote:
> >>>>On Wed, Jul 21 2021, tstark@linux.microsoft.com wrote:
> >>>>
> >>>>>From: Taylor Stark <tstark@microsoft.com>
> >>>>>
> >>>>>Update the virtio-pmem RFC spec to add support for describing the pmem region
> >>>>>via PCI BARs. Shared memory windows are used to accomplish this, similar to
> >>>>>virtio-fs and virtio-gpu. This is required to support virtio-pmem in Hyper-V,
> >>>>>since Hyper-V only allows PCI devices to operate on memory ranges defined via
> >>>>>BARs.
> >>>>
> >>>>Given that we already have pmem support out there (even though the spec
> >>>>has not been included yet, should this get a feature?
> >>>
> >>>I wasn't sure if we needed to handle backwards compatibility given that pmem
> >>>hasn't been merged into the spec yet. If we do, then yes I think it makes sense
> >>>to add a feature bit. How about VIRTIO_PMEM_F_SHMEM_REGION?
> >>
> >>The implementations that are upstream win, and usually we get a spec
> >>after the implementation is mature. The spec merely documents what
> >>the existing implementations are doing. Of course, one can plan
> >>ahead and propose additions to the spec that won't break existing
> >>setups.
> >>
> >>The feature seems to be glued to PCI BARs, right? Maybe that should
> >>be part of the feature name.
> >>
> >>-- 
> >>Thanks,
> >>
> >>David / dhildenb
> >
> >I had to do a bit of research to develop an opinion on including BAR in
> >the name. Using BARs is the implementation for the PCI transport, but there
> >are other transports that have support for shared memory regions but don't
> >use BARs (e.g. mmio - see vm_get_shm_region). Using shared memory regions
> >was initially a convenient way to use PCI BARs, but I think it could be
> >valuable for other transports as well (it adds similarity with other virtio
> >devices). So I think it makes sense to leave BAR out of the name, since
> >that's transport specific. Maybe I should retitle the patch? Let me know
> >your thoughts (especially since I'm learning on the fly).
> 
> Ah, the content doesn't actually talk about PCI BARs, that's my
> source of confusion :)
> 
> I know that virtio-fs also uses BARs in DAX mode, but I have no idea
> how they named features (if any?) or how they described it in their
> spec. Might be worth exploring.
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

After doing the research from my previous mail, I agree including BARs
in the name is confusing. I think I'll update the patch to just refer to
shared memory regions.

virtio-fs is actually how we landed on adding shared memory region support
to virtio-pmem. We got virtio-fs working in Hyper-V, then tried virtio-pmem,
ran into the guest absolute address problem, then went "well virtio-fs
works, what does that do?", and here we are :)

virtio-fs doesn't have any relevant features. The presence of the shared
memory region indicates to the driver that additional functionality is
available, if desired. I initially did the same thing with this update to
virtio-pmem, instead of using a feature. But a feature makes more sense
for preserving backwards compatibility. And it's actually useful for
Hyper-V as well, since we need to reject any drivers that don't support
the feature since we have no way to configure them.

Thanks,
Taylor