* [U-Boot] [PATCH] board: amlogic: enable PHY_REALTEK for selected boards
From: Neil Armstrong @ 2019-04-10 14:46 UTC (permalink / raw)
To: u-boot
When using the generic PHY on boards using an RGMII Realtek PHY,
gigabit speed is not always reliable.
This patch enables the Realtek PHY driver for such boards.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
configs/khadas-vim2_defconfig | 1 +
configs/nanopi-k2_defconfig | 1 +
configs/odroid-c2_defconfig | 1 +
configs/s400_defconfig | 1 +
4 files changed, 4 insertions(+)
diff --git a/configs/khadas-vim2_defconfig b/configs/khadas-vim2_defconfig
index 080dd19590..f7fb9338e2 100644
--- a/configs/khadas-vim2_defconfig
+++ b/configs/khadas-vim2_defconfig
@@ -31,6 +31,7 @@ CONFIG_MMC_MESON_GX=y
CONFIG_MTD=y
CONFIG_DM_ETH=y
CONFIG_ETH_DESIGNWARE=y
+CONFIG_PHY_REALTEK=y
CONFIG_PHY=y
CONFIG_MESON_GXL_USB_PHY=y
CONFIG_PINCTRL=y
diff --git a/configs/nanopi-k2_defconfig b/configs/nanopi-k2_defconfig
index 8bbf48f41f..106c5c0193 100644
--- a/configs/nanopi-k2_defconfig
+++ b/configs/nanopi-k2_defconfig
@@ -28,6 +28,7 @@ CONFIG_DM_MMC=y
CONFIG_MMC_MESON_GX=y
CONFIG_DM_ETH=y
CONFIG_ETH_DESIGNWARE=y
+CONFIG_PHY_REALTEK=y
CONFIG_PINCTRL=y
CONFIG_PINCTRL_MESON_GXBB=y
CONFIG_DM_REGULATOR=y
diff --git a/configs/odroid-c2_defconfig b/configs/odroid-c2_defconfig
index 747da184b3..91baddbb60 100644
--- a/configs/odroid-c2_defconfig
+++ b/configs/odroid-c2_defconfig
@@ -29,6 +29,7 @@ CONFIG_DM_MMC=y
CONFIG_MMC_MESON_GX=y
CONFIG_DM_ETH=y
CONFIG_ETH_DESIGNWARE=y
+CONFIG_PHY_REALTEK=y
CONFIG_PINCTRL=y
CONFIG_PINCTRL_MESON_GXBB=y
CONFIG_DM_REGULATOR=y
diff --git a/configs/s400_defconfig b/configs/s400_defconfig
index 1bd4b7127b..9b8448e003 100644
--- a/configs/s400_defconfig
+++ b/configs/s400_defconfig
@@ -26,6 +26,7 @@ CONFIG_DM_MMC=y
CONFIG_MMC_MESON_GX=y
CONFIG_DM_ETH=y
CONFIG_ETH_DESIGNWARE=y
+CONFIG_PHY_REALTEK=y
CONFIG_PINCTRL=y
CONFIG_PINCTRL_MESON_AXG=y
CONFIG_DM_REGULATOR=y
--
2.21.0
^ permalink raw reply related
* [U-Boot] [PULL] u-boot-usb/master
From: Marek Vasut @ 2019-04-10 14:45 UTC (permalink / raw)
To: u-boot
The following changes since commit 3c99166441bf3ea325af2da83cfe65430b49c066:
Prepare v2019.04 (2019-04-08 21:40:40 -0400)
are available in the Git repository at:
git://git.denx.de/u-boot-usb.git master
for you to fetch changes up to 69535b33bc1fce43dcc10b646cf44db81cffa131:
usb: ehci-mx6: Use common code to extract dr_mode (2019-04-09 13:11:45
+0200)
----------------------------------------------------------------
Adam Ford (1):
usb: ehci-mx6: Use common code to extract dr_mode
Ismael Luceno Cortes (2):
usb: host: Print device name when scanning
usb: Make portspeed return a read-only string
cmd/usb.c | 18 +++++-------------
common/usb_hub.c | 18 +++++-------------
drivers/usb/host/ehci-mx6.c | 29 +++++++++++++++--------------
drivers/usb/host/usb-uclass.c | 12 ++++--------
4 files changed, 29 insertions(+), 48 deletions(-)
^ permalink raw reply
* Re: [RFC][PATCH 13/16] sched: Add core wide task selection and scheduling.
From: Peter Zijlstra @ 2019-04-10 14:44 UTC (permalink / raw)
To: Aaron Lu
Cc: Tim Chen, mingo, tglx, pjt, torvalds, linux-kernel,
subhra.mazumdar, fweisbec, keescook, kerrnel, Aubrey Li,
Julien Desfossez
In-Reply-To: <20190410043633.GA67532@aaronlu>
On Wed, Apr 10, 2019 at 12:36:33PM +0800, Aaron Lu wrote:
> On Tue, Apr 09, 2019 at 11:09:45AM -0700, Tim Chen wrote:
> > Now that we have accumulated quite a number of different fixes to your orginal
> > posted patches. Would you like to post a v2 of the core scheduler with the fixes?
>
> One more question I'm not sure: should a task with cookie=0, i.e. tasks
> that are untagged, be allowed to scheduled on the the same core with
> another tagged task?
That was not meant to be possible.
> The current patch seems to disagree on this, e.g. in pick_task(),
> if max is already chosen but max->core_cookie == 0, then we didn't care
> about cookie and simply use class_pick for the other cpu. This means we
> could schedule two tasks with different cookies(one is zero and the
> other can be tagged).
When core_cookie==0 we shouldn't schedule the other siblings at all.
> But then sched_core_find() only allow idle task to match with any tagged
> tasks(we didn't place untagged tasks to the core tree of course :-).
>
> Thoughts? Do I understand this correctly? If so, I think we probably
> want to make this clear before v2. I personally feel, we shouldn't allow
> untagged tasks(like kernel threads) to match with tagged tasks.
Agreed, cookie should always match or idle.
^ permalink raw reply
* [PATCH RT] bpf: Guard bpf_prog_active with a locallock
From: Sebastian Andrzej Siewior @ 2019-04-10 14:43 UTC (permalink / raw)
To: bpf, linux-rt-users; +Cc: tglx, Steven Rostedt, linux-kernel
The bpf_prog_active counter is used to avoid recursion on the same CPU.
On RT we can't keep it with the preempt-disable part because the syscall
may need to acquire locks or allocate memory.
Use a locallock() to avoid recursion on the same CPU.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
include/linux/bpf.h | 2 ++
kernel/bpf/hashtab.c | 4 ++--
kernel/bpf/syscall.c | 13 +++++++------
kernel/events/core.c | 3 ++-
kernel/trace/bpf_trace.c | 5 ++---
5 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e734f163bd0b9..667f45de65be8 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -16,6 +16,7 @@
#include <linux/rbtree_latch.h>
#include <linux/numa.h>
#include <linux/wait.h>
+#include <linux/locallock.h>
struct bpf_verifier_env;
struct perf_event;
@@ -467,6 +468,7 @@ _out: \
#ifdef CONFIG_BPF_SYSCALL
DECLARE_PER_CPU(int, bpf_prog_active);
+DECLARE_LOCAL_IRQ_LOCK(bpf_prog_active_lock);
extern const struct file_operations bpf_map_fops;
extern const struct file_operations bpf_prog_fops;
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index b4f903a5ef36e..15120d2d8b659 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -668,11 +668,11 @@ static void htab_elem_free_rcu(struct rcu_head *head)
* we're calling kfree, otherwise deadlock is possible if kprobes
* are placed somewhere inside of slub
*/
- preempt_disable();
+ local_lock(bpf_prog_active_lock);
__this_cpu_inc(bpf_prog_active);
htab_elem_free(htab, l);
__this_cpu_dec(bpf_prog_active);
- preempt_enable();
+ local_unlock(bpf_prog_active_lock);
}
static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 84470d1480aa4..73f2edbe3b28c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -42,6 +42,7 @@
#define BPF_OBJ_FLAG_MASK (BPF_F_RDONLY | BPF_F_WRONLY)
DEFINE_PER_CPU(int, bpf_prog_active);
+DEFINE_LOCAL_IRQ_LOCK(bpf_prog_active_lock);
static DEFINE_IDR(prog_idr);
static DEFINE_SPINLOCK(prog_idr_lock);
static DEFINE_IDR(map_idr);
@@ -716,7 +717,7 @@ static int map_lookup_elem(union bpf_attr *attr)
goto done;
}
- preempt_disable();
+ local_lock(bpf_prog_active_lock);
this_cpu_inc(bpf_prog_active);
if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH) {
@@ -750,7 +751,7 @@ static int map_lookup_elem(union bpf_attr *attr)
rcu_read_unlock();
}
this_cpu_dec(bpf_prog_active);
- preempt_enable();
+ local_unlock(bpf_prog_active_lock);
done:
if (err)
@@ -845,7 +846,7 @@ static int map_update_elem(union bpf_attr *attr)
/* must increment bpf_prog_active to avoid kprobe+bpf triggering from
* inside bpf map update or delete otherwise deadlocks are possible
*/
- preempt_disable();
+ local_lock(bpf_prog_active_lock);
__this_cpu_inc(bpf_prog_active);
if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH) {
@@ -878,7 +879,7 @@ static int map_update_elem(union bpf_attr *attr)
rcu_read_unlock();
}
__this_cpu_dec(bpf_prog_active);
- preempt_enable();
+ local_unlock(bpf_prog_active_lock);
maybe_wait_bpf_programs(map);
out:
free_value:
@@ -925,13 +926,13 @@ static int map_delete_elem(union bpf_attr *attr)
goto out;
}
- preempt_disable();
+ local_lock(bpf_prog_active_lock);
__this_cpu_inc(bpf_prog_active);
rcu_read_lock();
err = map->ops->map_delete_elem(map, key);
rcu_read_unlock();
__this_cpu_dec(bpf_prog_active);
- preempt_enable();
+ local_unlock(bpf_prog_active_lock);
maybe_wait_bpf_programs(map);
out:
kfree(key);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index b3155a155a645..6facb80af7c0e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8546,7 +8546,7 @@ static void bpf_overflow_handler(struct perf_event *event,
int ret = 0;
ctx.regs = perf_arch_bpf_user_pt_regs(regs);
- preempt_disable();
+ local_lock(bpf_prog_active_lock);
if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1))
goto out;
rcu_read_lock();
@@ -8555,6 +8555,7 @@ static void bpf_overflow_handler(struct perf_event *event,
out:
__this_cpu_dec(bpf_prog_active);
preempt_enable();
+ local_unlock(bpf_prog_active_lock);
if (!ret)
return;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f1a86a0d881dd..bb92cf31481b4 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -78,8 +78,7 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
if (in_nmi()) /* not supported yet */
return 1;
- preempt_disable();
-
+ local_lock(bpf_prog_active_lock);
if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
/*
* since some bpf program is already running on this cpu,
@@ -110,7 +109,7 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
out:
__this_cpu_dec(bpf_prog_active);
- preempt_enable();
+ local_unlock(bpf_prog_active_lock);
return ret;
}
--
2.20.1
^ permalink raw reply related
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, mst, jasowang, david, qemu-devel,
virtualization, adilger.kernel, zwisler, aarcange, dave.jiang,
linux-nvdimm, vishal.l.verma, david, willy, hch, linux-acpi,
jmoyer, linux-ext4, lenb, kilobyte, riel, stefanha, pbonzini,
dan.j.williams, lcapitulino, kwolf, nilal, tytso,
xiaoguangrong.eric, darrick.wong, rjw, linux-kernel, linux-xfs,
linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
^ permalink raw reply
* [virtio-dev] Re: [RFC] virtio-pmem: PMEM device spec
From: Stefan Hajnoczi @ 2019-04-10 14:42 UTC (permalink / raw)
To: Pankaj Gupta
Cc: virtio-dev, mst, david, riel, dan.j.williams, aarcange, cohuck,
lcapitulino, nilal
In-Reply-To: <20190326143507.12607-1-pagupta@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 7834 bytes --]
On Tue, Mar 26, 2019 at 08:05:07PM +0530, Pankaj Gupta wrote:
> diff --git a/virtio-pmem.tex b/virtio-pmem.tex
> new file mode 100644
> index 0000000..04e07bb
> --- /dev/null
> +++ b/virtio-pmem.tex
> @@ -0,0 +1,134 @@
> +\section{PMEM Device}\label{sec:Device Types / PMEM Device}
> +
> +The virtio pmem is a fake persistent memory (NVDIMM) device
> +used to bypass the guest page cache and provide a virtio
> +based asynchronous flush mechanism. This avoids the need
Is there anything "fake" about virtio-pmem from the perspective of the
device interface?
What you are describing is one use case. But on a platform that doesn't
have existing physical NVDIMM interfaces, ACPI, etc maybe virtio-pmem
would be used to pass through physical NVDIMMs from the host? If you
agree, then it might make sense to describe the device simply as a
persistent memory device and give "fake NVDIMM" as an example use case
in a separate paragraph. This would make the text more future-proof.
> +of a separate page cache in guest and keeps page cache only
> +in the host. Under memory pressure, the host makes use of
> +effecient memory reclaim decisions for page cache pages
efficient
Please use a spell-checker like aspell or ispell.
> +of all the guests. This helps to reduce the memory footprint
> +and fit more guests in the host system.
> +
> +\subsection{Device ID}\label{sec:Device Types / PMEM Device / Device ID}
> + 25
> +
> +\subsection{Virtqueues}\label{sec:Device Types / PMEM Device / Virtqueues}
> +\begin{description}
> +\item[0] req_vq
> +\end{description}
> +
> +\subsection{Feature bits}\label{sec:Device Types / PMEM Device / Feature bits}
> +
> +There are currently no feature bits defined for this device.
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / PMEM Device / Device configuration layout}
> +
> +\begin{lstlisting}
> +struct virtio_pmem_config {
> + le64 start;
> + le64 size;
> +};
> +\end{lstlisting}
> +
> +\field{start} contains the guest physical address of the start of the
> +physical address range to be hotplugged into the guest address space
> +using the pmem API.
What pmem API? This specification shouldn't be written from a Linux
kernel internals point of view. It should be possible to implement BSD,
Windows, etc drivers too.
Just saying that this is the physical address of the start of the
persistent memory range is enough.
> +\field{size} contains the length of this address range.
"in bytes" would be nice because it makes the units explicit.
> +
> +\subsection{Device Initialization}\label{sec:Device Types / PMEM Device / Device Initialization}
> +
> +Device hotplugs physical memory to guest address space. Persistent memory device
> +is emulated with file backed memory at host side.
File-backed memory is a detail of one possible use case. The spec
shouldn't be tied to this single use case.
> +
> +\begin{enumerate}
> +\item Guest vpmem start is read from \field{start}.
> +\item Guest vpmem end is read from \field{size}.
> +\end{enumerate}
"vpmem" is not defined. Please use consistent terminology.
I suggest structuring this subsection as follows:
Initialization proceeds as follows:
\begin{enumerate}
\item The driver reads the physical start address from \field{start}.
\item The driver reads the length of the persistent memory range from \field{size}.
\end{enumerate}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / PMEM Device / Device Initialization}
> +
> +File backed memory SHOULD be memory mapped to guest address space with SHARED
> +memory mapping.
I'm not sure this point is appropriate for the virtio spec since it's a
device implementation detail. This is outside the scope of the device
interface and should be dropped.
If you'd like to describe the file-backed memory use case, please do it
in a non-normative section and use the appropriate terminology
(MAP_SHARED) and explanations (shared mapping so that changes will be
written back to the file).
> +
> +\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.
There are Linux internals that don't need to be in the spec. Some
kernels may not have a similar pmem API with a flush callback,
registration, etc.
Please describe the functionality instead:
Memory stores to the persistent memory range are not guaranteed to be
persistent without further action. An explicit flush command is
required to ensure persistence. The req_vq is used to perform flush
commands.
> +
> +\drivernormative{\subsubsection}{Driver Initialization: Filesystem direct access}{Device Types / PMEM Driver / Driver Initialization / Direct access}
> +
> +Driver SHOULD enable filesystem direct access operations for
> +read/write on the device.
This is specific to one possible use case for virtio-pmem and it's a
driver implementation detail, not a requirement of the device interface.
Imagine a unikernel application that doesn't have a file system. They
just want to access the persistent memory range, they don't care about
filesystem direct access.
Please focus on the requirements/constraints/assumptions of the device
interface, not the implementation details of the device emulation or the
Linux guest driver. That way the specification will be generally useful
and will age better as the use cases change.
> +
> +\drivernormative{\subsubsection}{Driver Initialization: Virtio flush}{Device Types / PMEM Driver / Driver Initialization / Virtio flush}
> +
> +Driver SHOULD add implement a virtio based flush callback.
If flush is optional, how should a device that does not implement it
behave:
1. Don't implement req_vq
Or
2. Fail flush commands with a certain error code
?
> +Driver SHOULD disable other FLUSH/SYNC mechanisms for the device
> +when virtio flush is configured.
What does this mean?
> +
> +\subsection{Driver Operations}\label{sec:Device Types / PMEM Driver / Driver Operation}
> +\drivernormative{\subsubsection}{Driver Operation: Virtqueue command}{Device Types / PMEM Driver / Driver Operation / Virtqueue command}
> +
> +Driver SHOULD send VIRTIO_FLUSH command on request virtqueue,
> +blocks guest userspace process utill host completes fsync/flush.
userspace processes are outside the scope of the virtio specification.
There may be no "userspace" inside the guest. Please stick to
describing the device interface.
I think the intention here is:
The VIRTIO_FLUSH command persists memory writes that were performed
before the command was submitted. Once the command completes the
writes are guaranteed to be persistent.
And the normative part is:
The driver MUST submit a VIRTIO_FLUSH command after performing memory
writes that require persistence.
The driver MUST wait for the VIRTIO_FLUSH command to complete before
assuming previous writes are persistent.
> +Driver SHOULD handle multiple fsync requests on files present
> +on the device.
fsync is outside the scope of the device interface and, hence, this
specification. Perhaps:
The driver MAY have multiple VIRTIO_FLUSH commands pending, though
each command completion only guarantees persistence of memory writes
that were performed before the command was submitted.
> +\subsection{Device Operations}\label{sec:Device Types / PMEM Driver / Device Operation}
> +
> +\devicenormative{\subsubsection}{Device Operations}{Device Types / PMEM Device / Device Operation / Virtqueue flush}
> +
> +Device SHOULD handle multiple flush requests simultaneously using
> +host filesystem fsync/flush call.
fsync/flush is an implementation detail. I'll stop reviewing at this
point to avoid repetition :).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 07/12] virtio/s390: use DMA memory for ccw I/O
From: Halil Pasic @ 2019-04-10 14:42 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, linux-s390, Martin Schwidefsky, Sebastian Ott,
virtualization, Christian Borntraeger, Viktor Mihajlovski,
Vasily Gorbik, Janosch Frank, Claudio Imbrenda, Farhan Ali,
Eric Farman
In-Reply-To: <20190410104251.38fe7405.cohuck@redhat.com>
On Wed, 10 Apr 2019 10:42:51 +0200
Cornelia Huck <cohuck@redhat.com> wrote:
> On Fri, 5 Apr 2019 01:16:17 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
> > Before virtio-ccw could get away with not using DMA API for the pieces of
> > memory it does ccw I/O with. With protected virtualization this has to
> > change, since the hypervisor needs to read and sometimes also write these
> > pieces of memory.
> >
> > Let us make sure all ccw I/O is done through shared memory.
> >
> > Note: The control blocks of I/O instructions do not need to be shared.
> > These are marshalled by the ultravisor.
>
> Ok, so direct parameters of I/O instructions are handled by the
> ultravisor?
>
Yes.
> >
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> > drivers/s390/virtio/virtio_ccw.c | 177 +++++++++++++++++++++++----------------
> > 1 file changed, 107 insertions(+), 70 deletions(-)
> >
> (...)
> > @@ -167,6 +170,28 @@ static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
> > return container_of(vdev, struct virtio_ccw_device, vdev);
> > }
> >
> > +#define vc_dma_decl_struct(type, field) \
> > + dma_addr_t field ## _dma_addr; \
> > + struct type *field
> > +
> > +static inline void *__vc_dma_alloc(struct virtio_device *vdev, size_t size,
> > + dma_addr_t *dma_handle)
> > +{
> > + return dma_alloc_coherent(vdev->dev.parent, size, dma_handle,
> > + GFP_DMA | GFP_KERNEL | __GFP_ZERO);
> > +}
> > +
> > +static inline void __vc_dma_free(struct virtio_device *vdev, size_t size,
> > + void *cpu_addr, dma_addr_t dma_handle)
> > +{
> > + dma_free_coherent(vdev->dev.parent, size, cpu_addr, dma_handle);
> > +}
> > +
> > +#define vc_dma_alloc_struct(vdev, ptr) \
> > + ({ ptr = __vc_dma_alloc(vdev, (sizeof(*(ptr))), &(ptr ## _dma_addr)); })
> > +#define vc_dma_free_struct(vdev, ptr) \
> > + __vc_dma_free(vdev, sizeof(*(ptr)), (ptr), (ptr ## _dma_addr))
>
> Not sure I'm a fan of those wrappers... I think they actually hurt
> readability of the code.
>
By wrappers you mean just the macros or also the inline functions?
If we agree to go with the cio DMA pool instead of using DMA API
facilities for allocation (dma_alloc_coherent or maybe a per ccw-device
dma_pool) I think I could just use cio_dma_zalloc() directly if you like.
I was quite insecure about how this gen_pool idea is going to be received
here. That's why I decided to keep the dma_alloc_coherent() version in
for the RFC.
If you prefer I can squash patches #7 #9 #10 and #11 together and
pull #8 forward. Would you prefer that?
> > +
> > static void drop_airq_indicator(struct virtqueue *vq, struct airq_info *info)
> > {
> > unsigned long i, flags;
> > @@ -322,12 +347,12 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> > {
> > int ret;
> > unsigned long *indicatorp = NULL;
> > - struct virtio_thinint_area *thinint_area = NULL;
> > + vc_dma_decl_struct(virtio_thinint_area, thinint_area) = NULL;
> > + dma_addr_t indicatorp_dma_addr;
> > struct airq_info *airq_info = vcdev->airq_info;
> >
> > if (vcdev->is_thinint) {
> > - thinint_area = kzalloc(sizeof(*thinint_area),
> > - GFP_DMA | GFP_KERNEL);
> > + vc_dma_alloc_struct(&vcdev->vdev, thinint_area);
> > if (!thinint_area)
> > return;
> > thinint_area->summary_indicator =
> > @@ -338,8 +363,9 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> > ccw->cda = (__u32)(unsigned long) thinint_area;
> > } else {
> > /* payload is the address of the indicators */
> > - indicatorp = kmalloc(sizeof(&vcdev->indicators),
> > - GFP_DMA | GFP_KERNEL);
> > + indicatorp = __vc_dma_alloc(&vcdev->vdev,
> > + sizeof(&vcdev->indicators),
> > + &indicatorp_dma_addr);
> > if (!indicatorp)
> > return;
> > *indicatorp = 0;
> > @@ -359,8 +385,10 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> > "Failed to deregister indicators (%d)\n", ret);
> > else if (vcdev->is_thinint)
> > virtio_ccw_drop_indicators(vcdev);
> > - kfree(indicatorp);
> > - kfree(thinint_area);
> > + if (indicatorp)
> > + __vc_dma_free(&vcdev->vdev, sizeof(&vcdev->indicators),
> > + indicatorp, indicatorp_dma_addr);
> > + vc_dma_free_struct(&vcdev->vdev, thinint_area);
>
> Don't you need to check for !NULL here as well?
Good catch!
I could take care of it in __vc_dma_free().
void cio_dma_free(void *cpu_addr, size_t size) {
+ if (!cpu_addr)
+ return;
also seems to me like a good idea right now.
>
> > }
> >
> > static inline long __do_kvm_notify(struct subchannel_id schid,
> (...)
> > @@ -1280,7 +1318,6 @@ static int virtio_ccw_online(struct ccw_device *cdev)
> >
> > vcdev->is_thinint = virtio_ccw_use_airq; /* at least try */
> >
> > - vcdev->vdev.dev.parent = &cdev->dev;
>
> Hm?
>
> (You added a line like that in a previous patch; should it simply have
> been a movement instead? Or am I misremembering?)
Right the move was supposed to take place in patch #2. Not sure how
I ended up with this. Maybe a messed up rebase.
>
> > vcdev->vdev.dev.release = virtio_ccw_release_dev;
> > vcdev->vdev.config = &virtio_ccw_config_ops;
> > vcdev->cdev = cdev;
>
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
linux-acpi, qemu-devel, linux-ext4, linux-xfs, dan.j.williams,
zwisler, vishal.l.verma, dave.jiang, mst, jasowang, willy, rjw,
hch, lenb, jack, tytso, adilger.kernel, darrick.wong, lcapitulino,
kwolf, imammedo, jmoyer, nilal, riel, stefanha, aarcange, david,
david, cohuck, xiaoguangrong.eric, pbonzini, kilobyte
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
linux-acpi, qemu-devel, linux-ext4, linux-xfs, dan.j.williams,
zwisler, vishal.l.verma, dave.jiang, mst, jasowang, willy, rjw,
hch, lenb, jack, tytso, adilger.kernel, darrick.wong, lcapitulino,
kwolf, imammedo, jmoyer, nilal, riel, stefanha, aarcange, david,
david, cohuck, xiaoguangrong.eric, pbonzini, kilobyte
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, mst, jasowang, david, qemu-devel,
virtualization, adilger.kernel, zwisler, aarcange, linux-nvdimm,
david, willy, hch, linux-acpi, linux-ext4, lenb, kilobyte, riel,
stefanha, pbonzini, lcapitulino, kwolf, nilal, tytso,
xiaoguangrong.eric, darrick.wong, rjw, linux-kernel, linux-xfs,
linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, mst, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, dave.jiang, linux-nvdimm,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, stefanha, pbonzini, dan.j.williams, lcapitulino,
nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw, linux-kernel,
linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:41 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck-H+wXaHxf7aLQT0dZR+AlfA, jack-AlSwsSmVLrQ,
kvm-u79uwXL29TY76Z2rM5mHXA, mst-H+wXaHxf7aLQT0dZR+AlfA,
jasowang-H+wXaHxf7aLQT0dZR+AlfA, david-FqsqvQoI3Ljby3iVrkZq2A,
qemu-devel-qX2TKyscuCcdnm+yROfE0A,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
adilger.kernel-m1MBpc4rdrD3fQ9qLvQP4Q,
zwisler-DgEjT+Ai2ygdnm+yROfE0A, aarcange-H+wXaHxf7aLQT0dZR+AlfA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, david-H+wXaHxf7aLQT0dZR+AlfA,
willy-wEGCiKHe2LqWVfeAwA7xHQ, hch-wEGCiKHe2LqWVfeAwA7xHQ,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-ext4-u79uwXL29TY76Z2rM5mHXA, lenb-DgEjT+Ai2ygdnm+yROfE0A,
kilobyte-b9QjgO8OEXPVItvQsEIGlw, riel-ebMLmSuQjDVBDgjK7y7TUQ,
stefanha-H+wXaHxf7aLQT0dZR+AlfA, pbonzini-H+wXaHxf7aLQT0dZR+AlfA,
lcapitulino-H+wXaHxf7aLQT0dZR+AlfA, kwolf-H+wXaHxf7aLQT0dZR+AlfA,
nilal-H+wXaHxf7aLQT0dZR+AlfA, tytso-3s7WtUTddSA,
xiaoguangrong.eric-Re5JQEeQqe8AvxtiuMwx3w,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA, rjw-LthD3rsA81gm4RdzfppkhA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
imammedo-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20190410040826.24371-3-pagupta-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
Pleas delete the extra space.
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
^ permalink raw reply
* Re: [PATCH 3/3] drm/amdgpu: Avoid HW reset if guilty job already signaled.
From: Christian König @ 2019-04-10 14:41 UTC (permalink / raw)
To: Grodzovsky, Andrey, Koenig, Christian,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
etnaviv-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
eric-WhKQ6XTQaPysTnJN9+BGXg@public.gmane.org
In-Reply-To: <c77c574c-4613-693f-3f11-3f7eb8feae66-5C7GfCeVMHo@public.gmane.org>
Am 10.04.19 um 16:28 schrieb Grodzovsky, Andrey:
> On 4/10/19 10:06 AM, Christian König wrote:
>> Am 09.04.19 um 18:42 schrieb Grodzovsky, Andrey:
>>> On 4/9/19 10:50 AM, Christian König wrote:
>>>> Am 08.04.19 um 18:08 schrieb Andrey Grodzovsky:
>>>>> Also reject TDRs if another one already running.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 94
>>>>> +++++++++++++++++++++---------
>>>>> 1 file changed, 67 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> index aabd043..4446892 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>>> @@ -3327,10 +3327,12 @@ bool amdgpu_device_should_recover_gpu(struct
>>>>> amdgpu_device *adev)
>>>>> static int amdgpu_device_pre_asic_reset(struct amdgpu_device
>>>>> *adev,
>>>>> struct amdgpu_job *job,
>>>>> - bool *need_full_reset_arg)
>>>>> + bool *need_full_reset_arg,
>>>>> + bool *job_signaled)
>>>>> {
>>>>> int i, r = 0;
>>>>> bool need_full_reset = *need_full_reset_arg;
>>>>> + struct amdgpu_ring *job_ring = job ?
>>>>> to_amdgpu_ring(job->base.sched) : NULL;
>>>>> /* block all schedulers and reset given job's ring */
>>>>> for (i = 0; i < AMDGPU_MAX_RINGS; ++i) {
>>>>> @@ -3341,6 +3343,17 @@ static int amdgpu_device_pre_asic_reset(struct
>>>>> amdgpu_device *adev,
>>>>> drm_sched_stop(&ring->sched, &job->base);
>>>>> + /*
>>>>> + * Must check guilty signal here since after this point
>>>>> all old
>>>>> + * HW fences are force signaled.
>>>>> + *
>>>>> + * job->base holds a reference to parent fence
>>>>> + */
>>>>> + if (job_signaled && job && ring == job_ring &&
>>>>> + job->base.s_fence->parent &&
>>>>> + dma_fence_is_signaled(job->base.s_fence->parent))
>>>>> + *job_signaled = true;
>>>>> +
>>>> That won't work correctly. See when the guilty job is not on the first
>>>> scheduler, you would already have force completed some before getting
>>>> here.
>>>>
>>>> Better to stop all schedulers first and then do the check.
>>>>
>>>> Christian.
>>> What do you mean by first scheduler ? There is one scheduler object per
>>> ring so I am not clear what 'first' means here.
>> Well for example if the guilty job is from a compute ring the we have
>> already force signaled the gfx ring here.
>>
>> Same is true for other devices in the same hive, so it would probably
>> be a good idea to move the force signaling and the IP reset somewhere
>> else and this check up a layer.
>>
>> Christian.
>
> Let me see if I understand you correctly - you want to AVOID ANY force
> signaling in case we are not going to HW reset and so you want to have
> the check if guilty is signaled BEFORE any ring fences are force
> signaled. Correct ?
Correct.
Basically we should do the following:
1. Stop all schedulers to make sure that nothing is going on.
2. Check the guilty job once more to make sure that it hasn't signaled
in the meantime.
3. Start our reset procedure, with force complete, soft reset eventually
hard reset etc etc..
4. Resubmit all not yet completed jobs.
5. Start the schedulers again.
Christian.
>
> Andrey
>
>>> Andrey
>>>
>>>
>>>>> /* after all hw jobs are reset, hw fence is meaningless, so
>>>>> force_completion */
>>>>> amdgpu_fence_driver_force_completion(ring);
>>>>> }
>>>>> @@ -3358,7 +3371,8 @@ static int amdgpu_device_pre_asic_reset(struct
>>>>> amdgpu_device *adev,
>>>>> - if (!amdgpu_sriov_vf(adev)) {
>>>>> + /* Don't suspend on bare metal if we are not going to HW reset
>>>>> the ASIC */
>>>>> + if (!amdgpu_sriov_vf(adev) && !(*job_signaled)) {
>>>>> if (!need_full_reset)
>>>>> need_full_reset =
>>>>> amdgpu_device_ip_need_full_reset(adev);
>>>>> @@ -3495,7 +3509,7 @@ static int amdgpu_do_asic_reset(struct
>>>>> amdgpu_hive_info *hive,
>>>>> return r;
>>>>> }
>>>>> -static void amdgpu_device_post_asic_reset(struct amdgpu_device
>>>>> *adev)
>>>>> +static void amdgpu_device_post_asic_reset(struct amdgpu_device
>>>>> *adev, bool job_signaled)
>>>>> {
>>>>> int i;
>>>>> @@ -3505,7 +3519,8 @@ static void
>>>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>>>>> if (!ring || !ring->sched.thread)
>>>>> continue;
>>>>> - if (!adev->asic_reset_res)
>>>>> + /* No point to resubmit jobs if we didn't HW reset*/
>>>>> + if (!adev->asic_reset_res && !job_signaled)
>>>>> drm_sched_resubmit_jobs(&ring->sched);
>>>>> drm_sched_start(&ring->sched, !adev->asic_reset_res);
>>>>> @@ -3518,14 +3533,21 @@ static void
>>>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev)
>>>>> adev->asic_reset_res = 0;
>>>>> }
>>>>> -static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
>>>>> +static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool
>>>>> trylock)
>>>>> {
>>>>> - mutex_lock(&adev->lock_reset);
>>>>> + if (trylock) {
>>>>> + if (!mutex_trylock(&adev->lock_reset))
>>>>> + return false;
>>>>> + } else
>>>>> + mutex_lock(&adev->lock_reset);
>>>>> +
>>>>> atomic_inc(&adev->gpu_reset_counter);
>>>>> adev->in_gpu_reset = 1;
>>>>> /* Block kfd: SRIOV would do it separately */
>>>>> if (!amdgpu_sriov_vf(adev))
>>>>> amdgpu_amdkfd_pre_reset(adev);
>>>>> +
>>>>> + return true;
>>>>> }
>>>>> static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
>>>>> @@ -3555,29 +3577,44 @@ int amdgpu_device_gpu_recover(struct
>>>>> amdgpu_device *adev,
>>>>> {
>>>>> int r;
>>>>> struct amdgpu_hive_info *hive = NULL;
>>>>> - bool need_full_reset = false;
>>>>> struct amdgpu_device *tmp_adev = NULL;
>>>>> struct list_head device_list, *device_list_handle = NULL;
>>>>> + bool xgmi_topology_present, need_full_reset, job_signaled;
>>>>> + need_full_reset = job_signaled = false;
>>>>> INIT_LIST_HEAD(&device_list);
>>>>> dev_info(adev->dev, "GPU reset begin!\n");
>>>>> + hive = amdgpu_get_xgmi_hive(adev, 0);
>>>>> + xgmi_topology_present = hive &&
>>>>> adev->gmc.xgmi.num_physical_nodes > 1;
>>>>> +
>>>>> /*
>>>>> - * In case of XGMI hive disallow concurrent resets to be
>>>>> triggered
>>>>> - * by different nodes. No point also since the one node already
>>>>> executing
>>>>> - * reset will also reset all the other nodes in the hive.
>>>>> + * Here we trylock to avoid chain of resets executing from
>>>>> + * either trigger by jobs on different adevs in XGMI hive or
>>>>> jobs on
>>>>> + * different schedulers for same device while this tdr is
>>>>> running.
>>>>> + * We always reset all schedulers for device and all devices for
>>>>> XGMI
>>>>> + * hive so that should take care of them too.
>>>>> */
>>>>> - hive = amdgpu_get_xgmi_hive(adev, 0);
>>>>> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
>>>>> - !mutex_trylock(&hive->reset_lock))
>>>>> +
>>>>> + if (xgmi_topology_present && !mutex_trylock(&hive->reset_lock)) {
>>>>> + DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as
>>>>> another already in progress",
>>>>> + job->base.id, hive->hive_id);
>>>>> return 0;
>>>>> + }
>>>>> /* Start with adev pre asic reset first for soft reset
>>>>> check.*/
>>>>> - amdgpu_device_lock_adev(adev);
>>>>> + if (!amdgpu_device_lock_adev(adev, !xgmi_topology_present)) {
>>>>> + DRM_INFO("Bailing on TDR for s_job:%llx, as another already
>>>>> in progress",
>>>>> + job->base.id);
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + /* Guilty job will be freed after this*/
>>>>> r = amdgpu_device_pre_asic_reset(adev,
>>>>> job,
>>>>> - &need_full_reset);
>>>>> + &need_full_reset,
>>>>> + &job_signaled);
>>>>> if (r) {
>>>>> /*TODO Should we stop ?*/
>>>>> DRM_ERROR("GPU pre asic reset failed with err, %d for drm
>>>>> dev, %s ",
>>>>> @@ -3609,10 +3646,11 @@ int amdgpu_device_gpu_recover(struct
>>>>> amdgpu_device *adev,
>>>>> if (tmp_adev == adev)
>>>>> continue;
>>>>> - amdgpu_device_lock_adev(tmp_adev);
>>>>> + amdgpu_device_lock_adev(tmp_adev, false);
>>>>> r = amdgpu_device_pre_asic_reset(tmp_adev,
>>>>> NULL,
>>>>> - &need_full_reset);
>>>>> + &need_full_reset,
>>>>> + &job_signaled);
>>>>> /*TODO Should we stop ?*/
>>>>> if (r) {
>>>>> DRM_ERROR("GPU pre asic reset failed with err, %d for
>>>>> drm dev, %s ",
>>>>> @@ -3623,19 +3661,21 @@ int amdgpu_device_gpu_recover(struct
>>>>> amdgpu_device *adev,
>>>>> /* Actual ASIC resets if needed.*/
>>>>> /* TODO Implement XGMI hive reset logic for SRIOV */
>>>>> - if (amdgpu_sriov_vf(adev)) {
>>>>> - r = amdgpu_device_reset_sriov(adev, job ? false : true);
>>>>> - if (r)
>>>>> - adev->asic_reset_res = r;
>>>>> - } else {
>>>>> - r = amdgpu_do_asic_reset(hive, device_list_handle,
>>>>> &need_full_reset);
>>>>> - if (r && r == -EAGAIN)
>>>>> - goto retry;
>>>>> + if (!job || !job_signaled) {
>>>>> + if (amdgpu_sriov_vf(adev)) {
>>>>> + r = amdgpu_device_reset_sriov(adev, job ? false : true);
>>>>> + if (r)
>>>>> + adev->asic_reset_res = r;
>>>>> + } else {
>>>>> + r = amdgpu_do_asic_reset(hive, device_list_handle,
>>>>> &need_full_reset);
>>>>> + if (r && r == -EAGAIN)
>>>>> + goto retry;
>>>>> + }
>>>>> }
>>>>> /* Post ASIC reset for all devs .*/
>>>>> list_for_each_entry(tmp_adev, device_list_handle,
>>>>> gmc.xgmi.head) {
>>>>> - amdgpu_device_post_asic_reset(tmp_adev);
>>>>> + amdgpu_device_post_asic_reset(tmp_adev, job_signaled);
>>>>> if (r) {
>>>>> /* bad news, how to tell it to userspace ? */
>>>>> @@ -3648,7 +3688,7 @@ int amdgpu_device_gpu_recover(struct
>>>>> amdgpu_device *adev,
>>>>> amdgpu_device_unlock_adev(tmp_adev);
>>>>> }
>>>>> - if (hive && adev->gmc.xgmi.num_physical_nodes > 1)
>>>>> + if (xgmi_topology_present)
>>>>> mutex_unlock(&hive->reset_lock);
>>>>> if (r)
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck, pagupta
Cc: Pankaj Gupta, jack, kvm, mst, jasowang, david, qemu-devel,
virtualization, adilger.kernel, zwisler, aarcange, dave.jiang,
linux-nvdimm, vishal.l.verma, david, willy, hch, linux-acpi,
jmoyer, linux-ext4, lenb, kilobyte, riel, stefanha, pbonzini,
dan.j.williams, lcapitulino, kwolf, nilal, tytso,
xiaoguangrong.eric, darrick.wong, rjw, linux-kernel, linux-xfs,
linux-fsdevel, imammedo
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> (...)
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
From: Thomas Huth @ 2019-04-10 14:39 UTC (permalink / raw)
To: Stephen Checkoway
Cc: Kevin Wolf, Laurent Vivier, open list:Block layer core,
QEMU Developers, Max Reitz, Paolo Bonzini
In-Reply-To: <98B3F0C7-6554-4455-B441-86605DA5D6C1@oberlin.edu>
On 09/04/2019 17.37, Stephen Checkoway wrote:
>
>
> On Apr 9, 2019, at 02:13, Thomas Huth <thuth@redhat.com> wrote:
>
>> We'd like to get rid of global_qtest in the long run (since it is
>> causing trouble for tests that run multiple instances of QEMU in
>> parallel, e.g. migration tests)... so if it is feasible, please don't
>> use it in new code anymore. Try to use a local variable in the function
>> that call qtest_initf() and pass the test state around via a parameter
>> to the functions that need it.
>
> One of the patches introduces a FlashConfig structure which gets passed around to all the functions. I could stuff the local qtest in there. The earlier patches would still use global_qtest. Would that be acceptable or would you prefer that global_qtest not appear in any of the patches?
Using FlashConfig sounds fine to me. If you need to use global_qtest in
some of the earlier patches, that's ok, too (especially if these go away
again in the later patches ;-)).
Thomas
^ permalink raw reply
* Re: [PATCH 2/2] drm/i915/execlists: Always reset the context's RING registers
From: Mika Kuoppala @ 2019-04-10 14:40 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
In-Reply-To: <20190408223857.7035-2-chris@chris-wilson.co.uk>
Chris Wilson <chris@chris-wilson.co.uk> writes:
> During reset, we try and stop the active ring. This has the consequence
> that we often clobber the RING registers within the context image. When
> we find an active request, we update the context image to rerun that
> request (if it was guilty, we replace the hanging user payload with
> NOPs). However, we were ignoring an active context if the request had
> completed, with the consequence that the next submission on that request
> would start with RING_HEAD==0 and not the tail of the previous request,
> causing all requests still in the ring to be rerun. Rare, but
> occasionally seen within CI where we would spot that the context seqno
> would reverse and complain that we were retiring an incomplete request.
>
> <0> [412.390350] <idle>-0 3d.s2 408373352us : __i915_request_submit: rcs0 fence 1e95b:3640 -> current 3638
> <0> [412.390350] <idle>-0 3d.s2 408373353us : __i915_request_submit: rcs0 fence 1e95b:3642 -> current 3638
> <0> [412.390350] <idle>-0 3d.s2 408373354us : __i915_request_submit: rcs0 fence 1e95b:3644 -> current 3638
> <0> [412.390350] <idle>-0 3d.s2 408373354us : __i915_request_submit: rcs0 fence 1e95b:3646 -> current 3638
> <0> [412.390350] <idle>-0 3d.s2 408373356us : __execlists_submission_tasklet: rcs0 in[0]: ctx=2.1, fence 1e95b:3646 (current 3638), prio=4
> <0> [412.390350] i915_sel-4613 0.... 408373374us : __i915_request_commit: rcs0 fence 1e95b:3648
> <0> [412.390350] i915_sel-4613 0d..1 408373377us : process_csb: rcs0 cs-irq head=2, tail=3
> <0> [412.390350] i915_sel-4613 0d..1 408373377us : process_csb: rcs0 csb[3]: status=0x00000001:0x00000000, active=0x1
> <0> [412.390350] i915_sel-4613 0d..1 408373378us : __i915_request_submit: rcs0 fence 1e95b:3648 -> current 3638
> <0> [412.390350] <idle>-0 3..s1 408373378us : execlists_submission_tasklet: rcs0 awake?=1, active=5
> <0> [412.390350] i915_sel-4613 0d..1 408373379us : __execlists_submission_tasklet: rcs0 in[0]: ctx=2.2, fence 1e95b:3648 (current 3638), prio=4
> <0> [412.390350] i915_sel-4613 0.... 408373381us : i915_reset_engine: rcs0 flags=4
> <0> [412.390350] i915_sel-4613 0.... 408373382us : execlists_reset_prepare: rcs0: depth<-0
> <0> [412.390350] <idle>-0 3d.s2 408373390us : process_csb: rcs0 cs-irq head=3, tail=4
> <0> [412.390350] <idle>-0 3d.s2 408373390us : process_csb: rcs0 csb[4]: status=0x00008002:0x00000002, active=0x1
> <0> [412.390350] <idle>-0 3d.s2 408373390us : process_csb: rcs0 out[0]: ctx=2.2, fence 1e95b:3648 (current 3640), prio=4
> <0> [412.390350] i915_sel-4613 0.... 408373401us : intel_engine_stop_cs: rcs0
> <0> [412.390350] i915_sel-4613 0d..1 408373402us : process_csb: rcs0 cs-irq head=4, tail=4
> <0> [412.390350] i915_sel-4613 0.... 408373403us : intel_gpu_reset: engine_mask=1
> <0> [412.390350] i915_sel-4613 0d..1 408373408us : execlists_cancel_port_requests: rcs0:port0 fence 1e95b:3648, (current 3648)
> <0> [412.390350] i915_sel-4613 0.... 408373442us : intel_engine_cancel_stop_cs: rcs0
> <0> [412.390350] i915_sel-4613 0.... 408373442us : execlists_reset_finish: rcs0: depth->0
> <0> [412.390350] ksoftirq-26 3..s. 408373442us : execlists_submission_tasklet: rcs0 awake?=1, active=0
> <0> [412.390350] ksoftirq-26 3d.s1 408373443us : process_csb: rcs0 cs-irq head=5, tail=5
> <0> [412.390350] i915_sel-4613 0.... 408373475us : i915_request_retire: rcs0 fence 1e95b:3640, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373476us : i915_request_retire: __retire_engine_request(rcs0) fence 1e95b:3640, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373494us : __i915_request_commit: rcs0 fence 1e95b:3650
> <0> [412.390350] i915_sel-4613 0d..1 408373496us : process_csb: rcs0 cs-irq head=5, tail=5
> <0> [412.390350] i915_sel-4613 0d..1 408373496us : __i915_request_submit: rcs0 fence 1e95b:3650 -> current 3648
> <0> [412.390350] i915_sel-4613 0d..1 408373498us : __execlists_submission_tasklet: rcs0 in[0]: ctx=2.1, fence 1e95b:3650 (current 3648), prio=6
> <0> [412.390350] i915_sel-4613 0.... 408373500us : i915_request_retire_upto: rcs0 fence 1e95b:3648, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373500us : i915_request_retire: rcs0 fence 1e95b:3642, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373501us : i915_request_retire: __retire_engine_request(rcs0) fence 1e95b:3642, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373514us : i915_request_retire: rcs0 fence 1e95b:3644, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373515us : i915_request_retire: __retire_engine_request(rcs0) fence 1e95b:3644, current 3648
> <0> [412.390350] i915_sel-4613 0.... 408373527us : i915_request_retire: rcs0 fence 1e95b:3646, current 3640
> <0> [412.390350] <idle>-0 3..s1 408373569us : execlists_submission_tasklet: rcs0 awake?=1, active=1
> <0> [412.390350] <idle>-0 3d.s2 408373569us : process_csb: rcs0 cs-irq head=5, tail=1
> <0> [412.390350] <idle>-0 3d.s2 408373570us : process_csb: rcs0 csb[0]: status=0x00000001:0x00000000, active=0x1
> <0> [412.390350] <idle>-0 3d.s2 408373570us : process_csb: rcs0 csb[1]: status=0x00000018:0x00000002, active=0x5
> <0> [412.390350] <idle>-0 3d.s2 408373570us : process_csb: rcs0 out[0]: ctx=2.1, fence 1e95b:3650 (current 3650), prio=6
> <0> [412.390350] <idle>-0 3d.s2 408373571us : process_csb: rcs0 completed ctx=2
> <0> [412.390350] i915_sel-4613 0.... 408373621us : i915_request_retire: i915_request_retire:253 GEM_BUG_ON(!i915_request_completed(request))
>
> v2: Fixup the cancellation path to drain the CSB and reset the pointers.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 239 +++++++++++++++++--------------
> 1 file changed, 132 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index f096bc7bbe35..b020bc59b233 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -893,96 +893,6 @@ invalidate_csb_entries(const u32 *first, const u32 *last)
> clflush((void *)last);
> }
>
> -static void reset_csb_pointers(struct intel_engine_execlists *execlists)
> -{
> - const unsigned int reset_value = GEN8_CSB_ENTRIES - 1;
> -
> - /*
> - * After a reset, the HW starts writing into CSB entry [0]. We
> - * therefore have to set our HEAD pointer back one entry so that
> - * the *first* entry we check is entry 0. To complicate this further,
> - * as we don't wait for the first interrupt after reset, we have to
> - * fake the HW write to point back to the last entry so that our
> - * inline comparison of our cached head position against the last HW
> - * write works even before the first interrupt.
> - */
> - execlists->csb_head = reset_value;
> - WRITE_ONCE(*execlists->csb_write, reset_value);
> -
> - invalidate_csb_entries(&execlists->csb_status[0],
> - &execlists->csb_status[GEN8_CSB_ENTRIES - 1]);
> -}
> -
> -static void nop_submission_tasklet(unsigned long data)
> -{
> - /* The driver is wedged; don't process any more events. */
> -}
> -
> -static void execlists_cancel_requests(struct intel_engine_cs *engine)
> -{
> - struct intel_engine_execlists * const execlists = &engine->execlists;
> - struct i915_request *rq, *rn;
> - struct rb_node *rb;
> - unsigned long flags;
> -
> - GEM_TRACE("%s\n", engine->name);
> -
> - /*
> - * Before we call engine->cancel_requests(), we should have exclusive
> - * access to the submission state. This is arranged for us by the
> - * caller disabling the interrupt generation, the tasklet and other
> - * threads that may then access the same state, giving us a free hand
> - * to reset state. However, we still need to let lockdep be aware that
> - * we know this state may be accessed in hardirq context, so we
> - * disable the irq around this manipulation and we want to keep
> - * the spinlock focused on its duties and not accidentally conflate
> - * coverage to the submission's irq state. (Similarly, although we
> - * shouldn't need to disable irq around the manipulation of the
> - * submission's irq state, we also wish to remind ourselves that
> - * it is irq state.)
> - */
> - spin_lock_irqsave(&engine->timeline.lock, flags);
> -
> - /* Cancel the requests on the HW and clear the ELSP tracker. */
> - execlists_cancel_port_requests(execlists);
> - execlists_user_end(execlists);
Cancel all active in the new __reset seems to handle this now.
> -
> - /* Mark all executing requests as skipped. */
> - list_for_each_entry(rq, &engine->timeline.requests, link) {
> - if (!i915_request_signaled(rq))
> - dma_fence_set_error(&rq->fence, -EIO);
> -
> - i915_request_mark_complete(rq);
> - }
> -
> - /* Flush the queued requests to the timeline list (for retiring). */
> - while ((rb = rb_first_cached(&execlists->queue))) {
> - struct i915_priolist *p = to_priolist(rb);
> - int i;
> -
> - priolist_for_each_request_consume(rq, rn, p, i) {
> - list_del_init(&rq->sched.link);
> - __i915_request_submit(rq);
> - dma_fence_set_error(&rq->fence, -EIO);
> - i915_request_mark_complete(rq);
> - }
> -
> - rb_erase_cached(&p->node, &execlists->queue);
> - i915_priolist_free(p);
> - }
> -
> - /* Remaining _unready_ requests will be nop'ed when submitted */
> -
> - execlists->queue_priority_hint = INT_MIN;
> - execlists->queue = RB_ROOT_CACHED;
> - GEM_BUG_ON(port_isset(execlists->port));
> -
> - GEM_BUG_ON(__tasklet_is_enabled(&execlists->tasklet));
> - execlists->tasklet.func = nop_submission_tasklet;
> -
> - spin_unlock_irqrestore(&engine->timeline.lock, flags);
> -}
> -
> static inline bool
> reset_in_progress(const struct intel_engine_execlists *execlists)
> {
> @@ -1904,7 +1814,6 @@ static void execlists_reset_prepare(struct intel_engine_cs *engine)
>
> /* And flush any current direct submission. */
> spin_lock_irqsave(&engine->timeline.lock, flags);
> - process_csb(engine); /* drain preemption events */
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
> @@ -1925,14 +1834,47 @@ static bool lrc_regs_ok(const struct i915_request *rq)
> return true;
> }
>
> -static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> +static void reset_csb_pointers(struct intel_engine_execlists *execlists)
> +{
> + const unsigned int reset_value = GEN8_CSB_ENTRIES - 1;
> +
> + /*
> + * After a reset, the HW starts writing into CSB entry [0]. We
> + * therefore have to set our HEAD pointer back one entry so that
> + * the *first* entry we check is entry 0. To complicate this further,
> + * as we don't wait for the first interrupt after reset, we have to
> + * fake the HW write to point back to the last entry so that our
> + * inline comparison of our cached head position against the last HW
> + * write works even before the first interrupt.
> + */
> + execlists->csb_head = reset_value;
> + WRITE_ONCE(*execlists->csb_write, reset_value);
> +
> + invalidate_csb_entries(&execlists->csb_status[0],
> + &execlists->csb_status[GEN8_CSB_ENTRIES - 1]);
> +}
> +
> +static void __execlists_reset(struct intel_engine_cs *engine, bool stalled)
> {
> struct intel_engine_execlists * const execlists = &engine->execlists;
> + struct intel_context *ce;
> struct i915_request *rq;
> - unsigned long flags;
> u32 *regs;
>
> - spin_lock_irqsave(&engine->timeline.lock, flags);
> + process_csb(engine); /* drain preemption events */
> +
> + /* Following the reset, we need to reload the CSB read/write pointers */
> + reset_csb_pointers(&engine->execlists);
> +
> + /*
> + * Save the currently executing context, even if we completed
> + * its request, it was still running at the time of the
> + * reset and will have been clobbered.
> + */
> + if (!port_isset(execlists->port))
> + goto out_clear;
> +
> + ce = port_request(execlists->port)->hw_context;
>
> /*
> * Catch up with any missed context-switch interrupts.
> @@ -1947,12 +1889,13 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
>
> /* Push back any incomplete requests for replay after the reset. */
> rq = __unwind_incomplete_requests(engine);
> -
> - /* Following the reset, we need to reload the CSB read/write pointers */
> - reset_csb_pointers(&engine->execlists);
> -
> if (!rq)
> - goto out_unlock;
> + goto out_replay;
> +
> + if (rq->hw_context != ce) { /* caught just before a CS event */
> + rq = NULL;
So it was complete but not yet processed.
> + goto out_replay;
> + }
>
> /*
> * If this request hasn't started yet, e.g. it is waiting on a
> @@ -1967,7 +1910,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> * perfectly and we do not need to flag the result as being erroneous.
> */
> if (!i915_request_started(rq) && lrc_regs_ok(rq))
> - goto out_unlock;
> + goto out_replay;
>
> /*
> * If the request was innocent, we leave the request in the ELSP
> @@ -1982,7 +1925,7 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> */
> i915_reset_request(rq, stalled);
> if (!stalled && lrc_regs_ok(rq))
> - goto out_unlock;
> + goto out_replay;
>
> /*
> * We want a simple context + ring to execute the breadcrumb update.
> @@ -1992,21 +1935,103 @@ static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> * future request will be after userspace has had the opportunity
> * to recreate its own state.
> */
> - regs = rq->hw_context->lrc_reg_state;
> + regs = ce->lrc_reg_state;
Yes, looks much saner. Only play with guaranteed active one.
> if (engine->pinned_default_state) {
> memcpy(regs, /* skip restoring the vanilla PPHWSP */
> engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> engine->context_size - PAGE_SIZE);
> }
> + execlists_init_reg_state(regs, ce, engine, ce->ring);
>
> /* Rerun the request; its payload has been neutered (if guilty). */
> - rq->ring->head = intel_ring_wrap(rq->ring, rq->head);
> - intel_ring_update_space(rq->ring);
> +out_replay:
> + ce->ring->head =
> + rq ? intel_ring_wrap(ce->ring, rq->head) : ce->ring->tail;
The ce and rq ring should be same with the rq set. I guess
you had a reasons to keep it as ce, perhaps because it is
the culprit.
Rest is mostly code movement and can't poke holes in this.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> + intel_ring_update_space(ce->ring);
> + __execlists_update_reg_state(ce, engine);
> +
> +out_clear:
> + execlists_clear_all_active(execlists);
> +}
> +
> +static void execlists_reset(struct intel_engine_cs *engine, bool stalled)
> +{
> + unsigned long flags;
> +
> + GEM_TRACE("%s\n", engine->name);
> +
> + spin_lock_irqsave(&engine->timeline.lock, flags);
> +
> + __execlists_reset(engine, stalled);
> +
> + spin_unlock_irqrestore(&engine->timeline.lock, flags);
> +}
> +
> +static void nop_submission_tasklet(unsigned long data)
> +{
> + /* The driver is wedged; don't process any more events. */
> +}
> +
> +static void execlists_cancel_requests(struct intel_engine_cs *engine)
> +{
> + struct intel_engine_execlists * const execlists = &engine->execlists;
> + struct i915_request *rq, *rn;
> + struct rb_node *rb;
> + unsigned long flags;
>
> - execlists_init_reg_state(regs, rq->hw_context, engine, rq->ring);
> - __execlists_update_reg_state(rq->hw_context, engine);
> + GEM_TRACE("%s\n", engine->name);
> +
> + /*
> + * Before we call engine->cancel_requests(), we should have exclusive
> + * access to the submission state. This is arranged for us by the
> + * caller disabling the interrupt generation, the tasklet and other
> + * threads that may then access the same state, giving us a free hand
> + * to reset state. However, we still need to let lockdep be aware that
> + * we know this state may be accessed in hardirq context, so we
> + * disable the irq around this manipulation and we want to keep
> + * the spinlock focused on its duties and not accidentally conflate
> + * coverage to the submission's irq state. (Similarly, although we
> + * shouldn't need to disable irq around the manipulation of the
> + * submission's irq state, we also wish to remind ourselves that
> + * it is irq state.)
> + */
> + spin_lock_irqsave(&engine->timeline.lock, flags);
> +
> + __execlists_reset(engine, true);
> +
> + /* Mark all executing requests as skipped. */
> + list_for_each_entry(rq, &engine->timeline.requests, link) {
> + if (!i915_request_signaled(rq))
> + dma_fence_set_error(&rq->fence, -EIO);
> +
> + i915_request_mark_complete(rq);
> + }
> +
> + /* Flush the queued requests to the timeline list (for retiring). */
> + while ((rb = rb_first_cached(&execlists->queue))) {
> + struct i915_priolist *p = to_priolist(rb);
> + int i;
> +
> + priolist_for_each_request_consume(rq, rn, p, i) {
> + list_del_init(&rq->sched.link);
> + __i915_request_submit(rq);
> + dma_fence_set_error(&rq->fence, -EIO);
> + i915_request_mark_complete(rq);
> + }
> +
> + rb_erase_cached(&p->node, &execlists->queue);
> + i915_priolist_free(p);
> + }
> +
> + /* Remaining _unready_ requests will be nop'ed when submitted */
> +
> + execlists->queue_priority_hint = INT_MIN;
> + execlists->queue = RB_ROOT_CACHED;
> + GEM_BUG_ON(port_isset(execlists->port));
> +
> + GEM_BUG_ON(__tasklet_is_enabled(&execlists->tasklet));
> + execlists->tasklet.func = nop_submission_tasklet;
>
> -out_unlock:
> spin_unlock_irqrestore(&engine->timeline.lock, flags);
> }
>
> --
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [dpdk-dev] [PATCH] doc: add guideines for initial PMD submission
From: Rami Rosen @ 2019-04-10 14:40 UTC (permalink / raw)
To: Akhil Goyal
Cc: Trahe, Fiona, dev@dpdk.org, Yigit, Ferruh, De Lara Guarch, Pablo,
Mcnamara, John, Kovacevic, Marko, stable@dpdk.org
In-Reply-To: <VI1PR04MB489397D1A6C4CDD4C4636FA6E62E0@VI1PR04MB4893.eurprd04.prod.outlook.com>
Hi Akhil,
Thanks for your response. It seems to me though that the MAINTAINER file is
not really part of the documentation,
it is more of a reference type of file. Not sure that new DPDK developers
will know to look in it for this type of information.
I will send V2 of the patch and add event and bbdev.
Regards,
Rami Rosen
^ permalink raw reply
* Re: transitioning from 4.17.18 to 5.0.6
From: Patrick Venture @ 2019-04-10 14:39 UTC (permalink / raw)
To: Joel Stanley; +Cc: Jeremy Kerr, OpenBMC Maillist, Andrew Jeffery
In-Reply-To: <CACPK8XfwP_DwqthSwgFGMjqRepGkC1XdziZ+eEds=6ND_Ov71Q@mail.gmail.com>
On Tue, Apr 9, 2019 at 7:17 PM Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 9 Apr 2019 at 16:35, Patrick Venture <venture@google.com> wrote:
> >
> > HI, I ran into some weirdness when transitioning from 4.17.18 to 5.0.6
> > with the quanta-q71l board. More specifically now my host can't talk
> > to the BMC via ipmi-bt. All it returns is 0xd5. on the BMC itself, I
> > can talk to ipmid file via dbus, and btbridge is running.
> >
> > Has anyone else run into weirdness?
>
> I've not seen that.
>
> My only guess is there is an issue with clocking. Can you try booting
> the machine with clk_ignore_unused in your kernel command line?
I'll give it a shot, although IPMI not working is odd since the
LPC_CLK is enabled. But yeah, I'll try that today and reply back,
thanks.
>
> >
> > I am seeing:
> >
> > [ 4.645814] ipmi-bt-host 1e789140.ibt: Found bt bmc device
> > [ 4.650341] ipmi-bt-host 1e789140.ibt: Using IRQ 32
> >
> > The dts file I'm using is basically identical to the upstream one, and
> > my defconfig is also nearly identical to what's in meta-aspeed for the
> > ast2400.
> >
> > Patrick
^ permalink raw reply
* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
From: Hans Verkuil @ 2019-04-10 14:39 UTC (permalink / raw)
To: Stanimir Varbanov, linux-media
Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam
In-Reply-To: <b6a231ea-c039-d276-313d-1b6dd19cab14@linaro.org>
On 4/10/19 4:21 PM, Stanimir Varbanov wrote:
> Hi Hans,
>
> On 3/14/19 3:11 PM, Hans Verkuil wrote:
>> On 1/21/19 11:48 AM, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 1/18/19 11:13 AM, Hans Verkuil wrote:
>>>> On 1/16/19 1:37 PM, Stanimir Varbanov wrote:
>>>>> This changes v4l2_pix_format and v4l2_plane_pix_format sizeimage
>>>>> field description to allow v4l clients to set bigger image size
>>>>> in case of variable length compressed data.
>>>>>
>>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>>> ---
>>>>> Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst | 5 ++++-
>>>>> Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 3 ++-
>>>>> 2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> index 7f82dad9013a..dbe0b74e9ba4 100644
>>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> @@ -30,7 +30,10 @@ describing all planes of that format.
>>>>>
>>>>> * - __u32
>>>>> - ``sizeimage``
>>>>> - - Maximum size in bytes required for image data in this plane.
>>>>> + - Maximum size in bytes required for image data in this plane,
>>>>> + set by the driver. When the image consists of variable length
>>>>> + compressed data this is the maximum number of bytes required
>>>>> + to hold an image, and it is allowed to be set by the client.
>>>>> * - __u32
>>>>> - ``bytesperline``
>>>>> - Distance in bytes between the leftmost pixels in two adjacent
>>>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>>> index 71eebfc6d853..54b6d2b67bd7 100644
>>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>>> @@ -89,7 +89,8 @@ Single-planar format structure
>>>>> - Size in bytes of the buffer to hold a complete image, set by the
>>>>> driver. Usually this is ``bytesperline`` times ``height``. When
>>>>> the image consists of variable length compressed data this is the
>>>>> - maximum number of bytes required to hold an image.
>>>>> + maximum number of bytes required to hold an image, and it is
>>>>> + allowed to be set by the client.
>>>>> * - __u32
>>>>> - ``colorspace``
>>>>> - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>>>>>
>>>>
>>>> Hmm. "maximum number of bytes required to hold an image": that's not actually true
>>>> for bitstream formats like MPEG. It's just the size of the buffer used to store the
>>>> bitstream, i.e. one buffer may actually contain multiple compressed images, or a
>>>> compressed image is split over multiple buffers.
>>>>
>>>
>>> Do you want me to change something in the current documentation, i.e.
>>> the quoted above?
>>
>> Hmm, it looks like this discussion stalled (i.e. I forgot to reply).
>>
>> How about this:
>>
>> "When the image consists of variable length compressed data this is the
>> number of bytes required by the encoder to support the worst-case
>
> I don't think 'encoder' is the right word here:
>
> s/encoder/encoder or decoder
or perhaps just 'codec'? Either is fine by me.
>
>> compression scenario. Clients are allowed to set this field. However,
>> drivers may ignore the value or modify it."
>
> Can we rephrase to:
>
> "Clients are allowed to set sizeimage field, but however drivers my
> ignore the value or modify it."
>
How about this:
"Clients are allowed to set the sizeimage field, but drivers may
modify it."
'ignore the value' isn't right, since that suggests that the sizeimage
value that is returned may not be in actual use. Since sizeimage can now
be set by userspace, this means that a sanity check is needed in drivers
as well (i.e. what if userspace sets sizeimage to ~0?)
This should be tested in v4l2-compliance as well.
Regards,
Hans
^ permalink raw reply
* Re: [dpdk-dev] [EXT] [PATCH 1/3] acl: fix arm argument types
From: Jerin Jacob Kollanukkaran @ 2019-04-10 14:39 UTC (permalink / raw)
To: dev@dpdk.org, aconole@redhat.com
Cc: gavin.hu@arm.com, konstantin.ananyev@intel.com
In-Reply-To: <20190408182420.4398-2-aconole@redhat.com>
On Mon, 2019-04-08 at 14:24 -0400, Aaron Conole wrote:
> -------------------------------------------------------------------
> ---
> Compiler complains of argument type mismatch, like:
Can you share more details on how to reproduce this issue?
We already have
CFLAGS_acl_run_neon.o += -flax-vector-conversions
in the Makefile.
If you are taking out -flax-vector-conversions the correct way to
fix will be use vreinterpret*.
For me the code looks clean, If unnecessary casting is avoided.
>
> ../lib/librte_acl/acl_run_neon.h: In function ‘transition4’:
> ../lib/librte_acl/acl_run_neon.h:115:2: note: use -flax-vector-
> conversions
> to permit conversions between vectors with differing element
> types
> or numbers of subparts
> node_type = vbicq_s32(tr_hi_lo.val[0], index_msk);
> ^
> ../lib/librte_acl/acl_run_neon.h:115:41: error: incompatible type
> for
> argument 2 of ‘vbicq_s32’
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> lib/librte_acl/acl_run_neon.h | 46 ++++++++++++++++++++-------------
> --
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
>
>
> /*
> @@ -179,6 +183,9 @@ search_neon_8(const struct rte_acl_ctx *ctx,
> const uint8_t **data,
> acl_match_check_x4(0, ctx, parms, &flows, &index_array[0]);
> acl_match_check_x4(4, ctx, parms, &flows, &index_array[4]);
>
> + memset(&input0, 0, sizeof(input0));
> + memset(&input1, 0, sizeof(input1));
Why this memset only required for arm64? If it real issue, Shouldn't
it required for x86 and ppc ?
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck, pagupta
Cc: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs,
dan.j.williams, zwisler, vishal.l.verma, dave.jiang, mst,
jasowang, willy, rjw, hch, lenb, jack, tytso, adilger.kernel,
darrick.wong, lcapitulino, kwolf, imammedo, jmoyer, nilal, riel,
stefanha, aarcange, david, david, xiaoguangrong.eric, pbonzini,
kilobyte
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> (...)
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck, pagupta
Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
linux-acpi, qemu-devel, linux-ext4, linux-xfs, dan.j.williams,
zwisler, vishal.l.verma, dave.jiang, mst, jasowang, willy, rjw,
hch, lenb, jack, tytso, adilger.kernel, darrick.wong, lcapitulino,
kwolf, imammedo, jmoyer, nilal, riel, stefanha, aarcange, david,
david, xiaoguangrong.eric, pbonzini, kilobyte
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> (...)
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck, pagupta
Cc: linux-nvdimm, linux-kernel, virtualization, kvm, linux-fsdevel,
linux-acpi, qemu-devel, linux-ext4, linux-xfs, dan.j.williams,
zwisler, vishal.l.verma, dave.jiang, mst, jasowang, willy, rjw,
hch, lenb, jack, tytso, adilger.kernel, darrick.wong, lcapitulino,
kwolf, imammedo, jmoyer, nilal, riel, stefanha, aarcange, david,
david, xiaoguangrong.eric, pbonzini, kilobyte
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> (...)
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH 01/10] block/pflash_cfi02: Add test for supported commands
From: Thomas Huth @ 2019-04-10 14:39 UTC (permalink / raw)
To: Stephen Checkoway
Cc: QEMU Developers, Kevin Wolf, open list:Block layer core,
Laurent Vivier, Max Reitz, Paolo Bonzini
In-Reply-To: <98B3F0C7-6554-4455-B441-86605DA5D6C1@oberlin.edu>
On 09/04/2019 17.37, Stephen Checkoway wrote:
>
>
> On Apr 9, 2019, at 02:13, Thomas Huth <thuth@redhat.com> wrote:
>
>> We'd like to get rid of global_qtest in the long run (since it is
>> causing trouble for tests that run multiple instances of QEMU in
>> parallel, e.g. migration tests)... so if it is feasible, please don't
>> use it in new code anymore. Try to use a local variable in the function
>> that call qtest_initf() and pass the test state around via a parameter
>> to the functions that need it.
>
> One of the patches introduces a FlashConfig structure which gets passed around to all the functions. I could stuff the local qtest in there. The earlier patches would still use global_qtest. Would that be acceptable or would you prefer that global_qtest not appear in any of the patches?
Using FlashConfig sounds fine to me. If you need to use global_qtest in
some of the earlier patches, that's ok, too (especially if these go away
again in the later patches ;-)).
Thomas
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck, pagupta
Cc: jack, kvm, mst, jasowang, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, linux-nvdimm, david, willy,
hch, linux-acpi, linux-ext4, lenb, kilobyte, riel, stefanha,
pbonzini, lcapitulino, kwolf, nilal, tytso, xiaoguangrong.eric,
darrick.wong, rjw, linux-kernel, linux-xfs, linux-fsdevel,
imammedo
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from
> > Qemu over VIRTIO and registers it on nvdimm_bus. It also
> > creates a nd_region object with the persistent memory
> > range information so that existing 'nvdimm/pmem' driver
> > can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem
> > driver to register persistent memory compatible for DAX
> > capable filesystems.
> >
> > This also provides function to perform guest flush over
> > VIRTIO from 'pmem' driver when userspace performs flush
> > on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> (...)
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.