[virtio-dev] Re: [PATCH 4/6] vhost_vdpa: support doorbell mapping via mmap
Given the need for 4K doorbell such that QEMU can easily map, ect, and
assuming that I have a HW device which exposes 2 VQ's, with a notification
area off of BAR3, offset=whatever, notifier_multiplier=4, we don't need to
have 2 x 4K pages mapped into the VM for both doorbells do we? The guest
driver would ring DB0 at BAR4+offset, and DB1 at BAR4+offset+(4*1).
The 4K per DB is useful how? This allows for QEMU trapping of individual
DBs, that can then be used to do what, just forward the DBs via some other
scheme - this makes sense for non-HW related Virtio devices I guess. Is
this why there is a qemu option?
Rob Miller
rob.mil...@broadcom.com
(919)721-3339
On Fri, May 29, 2020 at 4:03 AM Jason Wang wrote:
> Currently the doorbell is relayed via eventfd which may have
> significant overhead because of the cost of vmexits or syscall. This
> patch introduces mmap() based doorbell mapping which can eliminate the
> overhead caused by vmexit or syscall.
>
> To ease the userspace modeling of the doorbell layout (usually
> virtio-pci), this patch starts from a doorbell per page
> model. Vhost-vdpa only support the hardware doorbell that sit at the
> boundary of a page and does not share the page with other registers.
>
> Doorbell of each virtqueue must be mapped separately, pgoff is the
> index of the virtqueue. This allows userspace to map a subset of the
> doorbell which may be useful for the implementation of software
> assisted virtqueue (control vq) in the future.
>
> Signed-off-by: Jason Wang
> ---
> drivers/vhost/vdpa.c | 59
> 1 file changed, 59 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 6ff72289f488..bbe23cea139a 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -741,12 +742,70 @@ static int vhost_vdpa_release(struct inode *inode,
> struct file *filep)
> return 0;
> }
>
> +static vm_fault_t vhost_vdpa_fault(struct vm_fault *vmf)
> +{
> + struct vhost_vdpa *v = vmf->vma->vm_file->private_data;
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> + struct vdpa_notification_area notify;
> + struct vm_area_struct *vma = vmf->vma;
> + u16 index = vma->vm_pgoff;
> +
> + notify = ops->get_vq_notification(vdpa, index);
> +
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> + if (remap_pfn_range(vma, vmf->address & PAGE_MASK,
> + notify.addr >> PAGE_SHIFT, PAGE_SIZE,
> + vma->vm_page_prot))
> + return VM_FAULT_SIGBUS;
> +
> + return VM_FAULT_NOPAGE;
> +}
> +
> +static const struct vm_operations_struct vhost_vdpa_vm_ops = {
> + .fault = vhost_vdpa_fault,
> +};
> +
> +static int vhost_vdpa_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct vhost_vdpa *v = vma->vm_file->private_data;
> + struct vdpa_device *vdpa = v->vdpa;
> + const struct vdpa_config_ops *ops = vdpa->config;
> + struct vdpa_notification_area notify;
> + int index = vma->vm_pgoff;
> +
> + if (vma->vm_end - vma->vm_start != PAGE_SIZE)
> + return -EINVAL;
> + if ((vma->vm_flags & VM_SHARED) == 0)
> + return -EINVAL;
> + if (vma->vm_flags & VM_READ)
> + return -EINVAL;
> + if (index > 65535)
> + return -EINVAL;
> + if (!ops->get_vq_notification)
> + return -ENOTSUPP;
> +
> + /* To be safe and easily modelled by userspace, We only
> +* support the doorbell which sits on the page boundary and
> +* does not share the page with other registers.
> +*/
> + notify = ops->get_vq_notification(vdpa, index);
> + if (notify.addr & (PAGE_SIZE - 1))
> + return -EINVAL;
> + if (vma->vm_end - vma->vm_start != notify.size)
> + return -ENOTSUPP;
> +
> + vma->vm_ops = _vdpa_vm_ops;
> + return 0;
> +}
> +
> static const struct file_operations vhost_vdpa_fops = {
> .owner = THIS_MODULE,
> .open = vhost_vdpa_open,
> .release= vhost_vdpa_release,
> .write_iter = vhost_vdpa_chr_write_iter,
> .unlocked_ioctl = vhost_vdpa_unlocked_ioctl,
> + .mmap = vhost_vdpa_mmap,
> .compat_ioctl = compat_ptr_ioctl,
> };
>
> --
> 2.20.1
>
>
[virtio-dev] Re: [virtio-comment] [PATCH v4 1/3] content: Document balloon feature page poison
On 29.05.20 18:57, Alexander Duyck wrote:
> On Fri, May 29, 2020 at 1:13 AM David Hildenbrand wrote:
>>
>> On 27.05.20 06:06, Alexander Duyck wrote:
>>> From: Alexander Duyck
>>>
>>> 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
>>> ---
>>> 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.
>
Doh! I think I missed that somehow
Reviewed-by: David Hildenbrand
Thanks!
--
Thanks,
David / dhildenb
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] [PATCH v4 1/3] content: Document balloon feature page poison
On Fri, May 29, 2020 at 1:13 AM David Hildenbrand wrote:
>
> On 27.05.20 06:06, Alexander Duyck wrote:
> > From: Alexander Duyck
> >
> > 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
> > ---
> > 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
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v3 1/2] virtio-video: Add virtio video device specification
Hi Dmitry,
On Wed, May 27, 2020 at 9:12 PM Dmitry Sepp wrote:
>
> Hi Keiichi,
>
> On Montag, 18. Mai 2020 07:17:53 CEST Keiichi Watanabe wrote:
> > > +struct virtio_video_stream_create {
> > > +struct virtio_video_cmd_hdr hdr;
> > > +le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > +le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > +le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > +u8 padding[4];
> > > +u8 tag[64];
> > > +};
> > > +\end{lstlisting}
> > > +\begin{description}
> > > +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> > > + management for input /output buffers. The driver MUST set a value in
> > > + \field{enum virtio_video_mem_type} that the device reported a
> > > + corresponding feature bit.
> > > +\begin{description}
> > > +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> > > +\end{description}
> > > +\item[\field{coded_format}] is the encoded format that will be
> > > + processed.
> > > +\item[\field{tag}] is the name associated with this stream. The tag
> > > + MUST be encoded in UTF-8 and NUL-terminated.
> >
> > I wonder why we need this "tag" field. I have kept this field from
> > Dmitry's first proposal, where this was called "char debug_name[64]".
> > However, on second thought, I have no idea what is the necessity to
> > have this field. Our VMM implementation in ChromeOS simply ignores
> > this field.
> > If OpenSynergy's implementation relies on this field, I'm curious
> > about the usage. We might want to have an enum value instead of this
> > field where arbitrary values can be stored.
> >
>
> The use of this field is not so clear because it was renamed. In fact, one can
> have an idea how it is used by simply looking at the driver code: the field is
> useful to know about the guest client app that uses the context. If someone
> wants to store arbitrary values, they have 64 bytes to do so with this so-
> called tag.
Hmm, though I understand this can be useful for you, I don't think we
should support it in the standard.
For the first example, I feel something is not abstracted well if you
want to send some information from a user app to the host device. User
applications shouldn't have a way to send messages to hardware
directly.
For the second example, who is "someone"? Driver or device? In any
case, I don't think it's the right way. They should extend existing
structs or add commands or feature flags, I think. Also, if arbitrary
values are allowed, the field won't be used correctly except in cases
where both driver implementation and device implementation are
available. This is against what the spec should be: virtio protocol
must work independently from the implementations.
Of course, it's obviously okay to have it as a downstream extension in
your product's local repository.
>
> > > +\end{description}
> > > +
> > > +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> > > +to an integer that is not used before. If a used value is passed as
> > > +\field{stream_id}, the device MUST reports an error with
> > > +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
> >
> > I'm wondering if we can't generate stream_id in the host side so that
> > we will have less error control code. In the current design, both the
> > device and the driver have error checks; the device must check that a
> > given ID is available and the driver must check if the device didn't
> > return the INVALID_STREAM_ID error. Instead, by generating IDs in the
> > device, we will be free from this type of failure. Same for
> > resource_id in RESOURCE_CREATE.
> >
> > I guess this design originally came from the virtio-gpu protocol.
> > However, I couldn't find a benefit of adopting the same design here.
> >
>
> Honestly I don't see too much difference: device still needs to check whether
> the id provided by the driver within some particular command is correct. If it
> is not, it will return an error. The driver needs to check (or skip checking)
> for an error either way as long as it is possible for the driver code to send
> a wrong number.
I'm talking about creation commands only. So, other commands won't be affected.
Let me try to explain my idea in a different way. The relationship
between the driver and the device can be seen as a client-server
model.
The client (driver) sends a request and the server (device) sends a
response by processing or generating some data.
Thus, I feel it's more natural that new data, including IDs, are
generated and provided by the device.
Best regards,
Keiichi
>
> Best regards,
> Dmitry.
>
> > Any feedback is welcome.
> >
> > Best regards,
> > Keiichi
> >
>
>
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] [PATCH v4 1/3] content: Document balloon feature page poison
On 27.05.20 06:06, Alexander Duyck wrote:
> From: Alexander Duyck
>
> 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
> ---
> 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").
Just wondering if that is intended (I imagine it will be different with
free page reporting)?
Apart from that looks good to me!
--
Thanks,
David / dhildenb
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
