* Re: [PATCH 1/2] ibmvscsi: Protect ibmvscsi_head from concurrent modificaiton
From: Martin K. Petersen @ 2019-03-21 0:13 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: martin.petersen, linux-scsi, stable, james.bottomley, brking,
linuxppc-dev
In-Reply-To: <20190320184151.31321-1-tyreld@linux.vnet.ibm.com>
Tyrel,
> For each ibmvscsi host created during a probe or destroyed during a
> remove we either add or remove that host to/from the global
> ibmvscsi_head list. This runs the risk of concurrent modification.
>
> This patch adds a simple spinlock around the list modification calls
> to prevent concurrent updates as is done similarly in the ibmvfc
> driver and ipr driver.
Applied to 5.1/scsi-fixes.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* [PATCH] powerpc/security: Fix spectre_v2 reporting
From: Michael Ellerman @ 2019-03-21 4:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, diana.craciun, andrew.donnellan, msuchanek, dja
When I updated the spectre_v2 reporting to handle software count cache
flush I got the logic wrong when there's no software count cache
enabled at all.
The result is that on systems with the software count cache flush
disabled we print:
Mitigation: Indirect branch cache disabled, Software count cache flush
Which correctly indicates that the count cache is disabled, but
incorrectly says the software count cache flush is enabled.
The root of the problem is that we are trying to handle all
combinations of options. But we know now that we only expect to see
the software count cache flush enabled if the other options are false.
So split the two cases, which simplifies the logic and fixes the bug.
We were also missing a space before "(hardware accelerated)".
The result is we see one of:
Mitigation: Indirect branch serialisation (kernel only)
Mitigation: Indirect branch cache disabled
Mitigation: Software count cache flush
Mitigation: Software count cache flush (hardware accelerated)
Fixes: ee13cb249fab ("powerpc/64s: Add support for software count cache flush")
Cc: stable@vger.kernel.org # v4.19+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/security.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 9b8631533e02..b33bafb8fcea 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -190,29 +190,22 @@ ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
bcs = security_ftr_enabled(SEC_FTR_BCCTRL_SERIALISED);
ccd = security_ftr_enabled(SEC_FTR_COUNT_CACHE_DISABLED);
- if (bcs || ccd || count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
- bool comma = false;
+ if (bcs || ccd) {
seq_buf_printf(&s, "Mitigation: ");
- if (bcs) {
+ if (bcs)
seq_buf_printf(&s, "Indirect branch serialisation (kernel only)");
- comma = true;
- }
- if (ccd) {
- if (comma)
- seq_buf_printf(&s, ", ");
- seq_buf_printf(&s, "Indirect branch cache disabled");
- comma = true;
- }
-
- if (comma)
+ if (bcs && ccd)
seq_buf_printf(&s, ", ");
- seq_buf_printf(&s, "Software count cache flush");
+ if (ccd)
+ seq_buf_printf(&s, "Indirect branch cache disabled");
+ } else if (count_cache_flush_type != COUNT_CACHE_FLUSH_NONE) {
+ seq_buf_printf(&s, "Mitigation: Software count cache flush");
if (count_cache_flush_type == COUNT_CACHE_FLUSH_HW)
- seq_buf_printf(&s, "(hardware accelerated)");
+ seq_buf_printf(&s, " (hardware accelerated)");
} else if (btb_flush_enabled) {
seq_buf_printf(&s, "Mitigation: Branch predictor state flush");
} else {
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/4] add generic builtin command line
From: Andrew Morton @ 2019-03-21 3:14 UTC (permalink / raw)
To: Daniel Walker
Cc: Maksym Kokhan, linux-kernel, Rob Herring, Paul Mackerras,
xe-linux-external, Daniel Walker, linuxppc-dev
In-Reply-To: <20190320232328.3bijcxek2yg43a25@zorba>
On Wed, 20 Mar 2019 16:23:28 -0700 Daniel Walker <danielwa@cisco.com> wrote:
> On Wed, Mar 20, 2019 at 03:53:19PM -0700, Andrew Morton wrote:
> > On Tue, 19 Mar 2019 16:24:45 -0700 Daniel Walker <danielwa@cisco.com> wrote:
> >
> > > This code allows architectures to use a generic builtin command line.
> >
> > I wasn't cc'ed on [2/4]. No mailing lists were cc'ed on [0/4] but it
> > didn't say anything useful anyway ;)
> >
> > I'll queue them up for testing and shall await feedback from the
> > powerpc developers.
> >
>
> You weren't CC'd , but it was To: you,
>
> 35 From: Daniel Walker <danielwa@cisco.com>
> 36 To: Andrew Morton <akpm@linux-foundation.org>,
> 37 Christophe Leroy <christophe.leroy@c-s.fr>,
> 38 Michael Ellerman <mpe@ellerman.id.au>,
> 39 Rob Herring <robh+dt@kernel.org>, xe-linux-external@cisco.com,
> 40 linuxppc-dev@lists.ozlabs.org, Frank Rowand <frowand.list@gmail.com>
> 41 Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
> 42 Subject: [PATCH 2/4] drivers: of: generic command line support
hm.
> Thanks for picking it up.
The patches (or some version of them) are already in linux-next,
which messes me up. I'll disable them for now.
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-21 3:12 UTC (permalink / raw)
To: Oliver
Cc: Jan Kara, linux-nvdimm, Aneesh Kumar K.V, Ross Zwisler,
Linux Kernel Mailing List, Linux MM, Andrew Morton, linuxppc-dev,
Kirill A . Shutemov
In-Reply-To: <CAOSf1CEZoLw5QqEMTKwiZ+d_qPLp_D9pJZUtnQWMXWpAXOQ2YA@mail.gmail.com>
On Wed, Mar 20, 2019 at 8:09 PM Oliver <oohall@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 7:57 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 8:34 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > On Wed, Mar 20, 2019 at 1:09 AM Aneesh Kumar K.V
> > > <aneesh.kumar@linux.ibm.com> wrote:
> > > >
> > > > Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
> > > >
> > > > > Dan Williams <dan.j.williams@intel.com> writes:
> > > > >
> > > > >>
> > > > >>> Now what will be page size used for mapping vmemmap?
> > > > >>
> > > > >> That's up to the architecture's vmemmap_populate() implementation.
> > > > >>
> > > > >>> Architectures
> > > > >>> possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
> > > > >>> device-dax with struct page in the device will have pfn reserve area aligned
> > > > >>> to PAGE_SIZE with the above example? We can't map that using
> > > > >>> PMD_SIZE page size?
> > > > >>
> > > > >> IIUC, that's a different alignment. Currently that's handled by
> > > > >> padding the reservation area up to a section (128MB on x86) boundary,
> > > > >> but I'm working on patches to allow sub-section sized ranges to be
> > > > >> mapped.
> > > > >
> > > > > I am missing something w.r.t code. The below code align that using nd_pfn->align
> > > > >
> > > > > if (nd_pfn->mode == PFN_MODE_PMEM) {
> > > > > unsigned long memmap_size;
> > > > >
> > > > > /*
> > > > > * vmemmap_populate_hugepages() allocates the memmap array in
> > > > > * HPAGE_SIZE chunks.
> > > > > */
> > > > > memmap_size = ALIGN(64 * npfns, HPAGE_SIZE);
> > > > > offset = ALIGN(start + SZ_8K + memmap_size + dax_label_reserve,
> > > > > nd_pfn->align) - start;
> > > > > }
> > > > >
> > > > > IIUC that is finding the offset where to put vmemmap start. And that has
> > > > > to be aligned to the page size with which we may end up mapping vmemmap
> > > > > area right?
> > >
> > > Right, that's the physical offset of where the vmemmap ends, and the
> > > memory to be mapped begins.
> > >
> > > > > Yes we find the npfns by aligning up using PAGES_PER_SECTION. But that
> > > > > is to compute howmany pfns we should map for this pfn dev right?
> > > > >
> > > >
> > > > Also i guess those 4K assumptions there is wrong?
> > >
> > > Yes, I think to support non-4K-PAGE_SIZE systems the 'pfn' metadata
> > > needs to be revved and the PAGE_SIZE needs to be recorded in the
> > > info-block.
> >
> > How often does a system change page-size. Is it fixed or do
> > environment change it from one boot to the next? I'm thinking through
> > the behavior of what do when the recorded PAGE_SIZE in the info-block
> > does not match the current system page size. The simplest option is to
> > just fail the device and require it to be reconfigured. Is that
> > acceptable?
>
> The kernel page size is set at build time and as far as I know every
> distro configures their ppc64(le) kernel for 64K. I've used 4K kernels
> a few times in the past to debug PAGE_SIZE dependent problems, but I'd
> be surprised if anyone is using 4K in production.
Ah, ok.
> Anyway, my view is that using 4K here isn't really a problem since
> it's just the accounting unit of the pfn superblock format. The kernel
> reading form it should understand that and scale it to whatever
> accounting unit it wants to use internally. Currently we don't so that
> should probably be fixed, but that doesn't seem to cause any real
> issues. As far as I can tell the only user of npfns in
> __nvdimm_setup_pfn() whih prints the "number of pfns truncated"
> message.
>
> Am I missing something?
No, I don't think so. The only time it would break is if a system with
64K page size laid down an info-block with not enough reserved
capacity when the page-size is 4K (npfns too small). However, that
sounds like an exceptional case which is why no problems have been
reported to date.
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Oliver @ 2019-03-21 3:08 UTC (permalink / raw)
To: Dan Williams
Cc: Jan Kara, linux-nvdimm, Aneesh Kumar K.V, Ross Zwisler,
Linux Kernel Mailing List, Linux MM, Andrew Morton, linuxppc-dev,
Kirill A . Shutemov
In-Reply-To: <CAPcyv4hMzVuOYzy2tTq-my8Z1y+X6Ug-fyObpKTxVU44p5rBZw@mail.gmail.com>
On Thu, Mar 21, 2019 at 7:57 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Mar 20, 2019 at 8:34 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Wed, Mar 20, 2019 at 1:09 AM Aneesh Kumar K.V
> > <aneesh.kumar@linux.ibm.com> wrote:
> > >
> > > Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
> > >
> > > > Dan Williams <dan.j.williams@intel.com> writes:
> > > >
> > > >>
> > > >>> Now what will be page size used for mapping vmemmap?
> > > >>
> > > >> That's up to the architecture's vmemmap_populate() implementation.
> > > >>
> > > >>> Architectures
> > > >>> possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
> > > >>> device-dax with struct page in the device will have pfn reserve area aligned
> > > >>> to PAGE_SIZE with the above example? We can't map that using
> > > >>> PMD_SIZE page size?
> > > >>
> > > >> IIUC, that's a different alignment. Currently that's handled by
> > > >> padding the reservation area up to a section (128MB on x86) boundary,
> > > >> but I'm working on patches to allow sub-section sized ranges to be
> > > >> mapped.
> > > >
> > > > I am missing something w.r.t code. The below code align that using nd_pfn->align
> > > >
> > > > if (nd_pfn->mode == PFN_MODE_PMEM) {
> > > > unsigned long memmap_size;
> > > >
> > > > /*
> > > > * vmemmap_populate_hugepages() allocates the memmap array in
> > > > * HPAGE_SIZE chunks.
> > > > */
> > > > memmap_size = ALIGN(64 * npfns, HPAGE_SIZE);
> > > > offset = ALIGN(start + SZ_8K + memmap_size + dax_label_reserve,
> > > > nd_pfn->align) - start;
> > > > }
> > > >
> > > > IIUC that is finding the offset where to put vmemmap start. And that has
> > > > to be aligned to the page size with which we may end up mapping vmemmap
> > > > area right?
> >
> > Right, that's the physical offset of where the vmemmap ends, and the
> > memory to be mapped begins.
> >
> > > > Yes we find the npfns by aligning up using PAGES_PER_SECTION. But that
> > > > is to compute howmany pfns we should map for this pfn dev right?
> > > >
> > >
> > > Also i guess those 4K assumptions there is wrong?
> >
> > Yes, I think to support non-4K-PAGE_SIZE systems the 'pfn' metadata
> > needs to be revved and the PAGE_SIZE needs to be recorded in the
> > info-block.
>
> How often does a system change page-size. Is it fixed or do
> environment change it from one boot to the next? I'm thinking through
> the behavior of what do when the recorded PAGE_SIZE in the info-block
> does not match the current system page size. The simplest option is to
> just fail the device and require it to be reconfigured. Is that
> acceptable?
The kernel page size is set at build time and as far as I know every
distro configures their ppc64(le) kernel for 64K. I've used 4K kernels
a few times in the past to debug PAGE_SIZE dependent problems, but I'd
be surprised if anyone is using 4K in production.
Anyway, my view is that using 4K here isn't really a problem since
it's just the accounting unit of the pfn superblock format. The kernel
reading form it should understand that and scale it to whatever
accounting unit it wants to use internally. Currently we don't so that
should probably be fixed, but that doesn't seem to cause any real
issues. As far as I can tell the only user of npfns in
__nvdimm_setup_pfn() whih prints the "number of pfns truncated"
message.
Am I missing something?
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply
* Re: [PATCH v2 13/13] syscall_get_arch: add "struct task_struct *" argument
From: Paul Moore @ 2019-03-21 1:22 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: linux-ia64, linux-sh, Palmer Dabbelt, linux-mips, sparclinux,
linux-hexagon, linux-riscv, linux-arch, linux-s390, linux-c6x-dev,
x86, Eugene Syromyatnikov, Elvira Khabirova, Mark Salter,
linux-snps-arc, uclinux-h8-devel, linux-xtensa, Kees Cook,
linux-um, linux-m68k, openrisc, Andy Lutomirski, linux-arm-kernel,
linux-parisc, Oleg Nesterov, linux-kernel, Paul Burton,
linux-audit, linux-alpha, nios2-dev, linuxppc-dev
In-Reply-To: <20190317233018.GN16301@altlinux.org>
On Sun, Mar 17, 2019 at 7:30 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> This argument is required to extend the generic ptrace API with
> PTRACE_GET_SYSCALL_INFO request: syscall_get_arch() is going
> to be called from ptrace_request() along with syscall_get_nr(),
> syscall_get_arguments(), syscall_get_error(), and
> syscall_get_return_value() functions with a tracee as their argument.
>
> The primary intent is that the triple (audit_arch, syscall_nr, arg1..arg6)
> should describe what system call is being called and what its arguments
> are.
>
> Reverts: 5e937a9ae913 ("syscall_get_arch: remove useless function arguments")
> Reverts: 1002d94d3076 ("syscall.h: fix doc text for syscall_get_arch()")
> Reviewed-by: Andy Lutomirski <luto@kernel.org> # for x86
> Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Acked-by: Paul Burton <paul.burton@mips.com> # MIPS parts
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> Acked-by: Kees Cook <keescook@chromium.org> # seccomp parts
> Acked-by: Mark Salter <msalter@redhat.com> # for the c6x bit
> Cc: Elvira Khabirova <lineprinter@altlinux.org>
> Cc: Eugene Syromyatnikov <esyr@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: x86@kernel.org
> Cc: linux-alpha@vger.kernel.org
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-c6x-dev@linux-c6x.org
> Cc: uclinux-h8-devel@lists.sourceforge.jp
> Cc: linux-hexagon@vger.kernel.org
> Cc: linux-ia64@vger.kernel.org
> Cc: linux-m68k@lists.linux-m68k.org
> Cc: linux-mips@vger.kernel.org
> Cc: nios2-dev@lists.rocketboards.org
> Cc: openrisc@lists.librecores.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> Cc: sparclinux@vger.kernel.org
> Cc: linux-um@lists.infradead.org
> Cc: linux-xtensa@linux-xtensa.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-audit@redhat.com
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>
> Notes:
> v2: unchanged
>
> arch/alpha/include/asm/syscall.h | 2 +-
> arch/arc/include/asm/syscall.h | 2 +-
> arch/arm/include/asm/syscall.h | 2 +-
> arch/arm64/include/asm/syscall.h | 4 ++--
> arch/c6x/include/asm/syscall.h | 2 +-
> arch/csky/include/asm/syscall.h | 2 +-
> arch/h8300/include/asm/syscall.h | 2 +-
> arch/hexagon/include/asm/syscall.h | 2 +-
> arch/ia64/include/asm/syscall.h | 2 +-
> arch/m68k/include/asm/syscall.h | 2 +-
> arch/microblaze/include/asm/syscall.h | 2 +-
> arch/mips/include/asm/syscall.h | 6 +++---
> arch/mips/kernel/ptrace.c | 2 +-
> arch/nds32/include/asm/syscall.h | 2 +-
> arch/nios2/include/asm/syscall.h | 2 +-
> arch/openrisc/include/asm/syscall.h | 2 +-
> arch/parisc/include/asm/syscall.h | 4 ++--
> arch/powerpc/include/asm/syscall.h | 10 ++++++++--
> arch/riscv/include/asm/syscall.h | 2 +-
> arch/s390/include/asm/syscall.h | 4 ++--
> arch/sh/include/asm/syscall_32.h | 2 +-
> arch/sh/include/asm/syscall_64.h | 2 +-
> arch/sparc/include/asm/syscall.h | 5 +++--
> arch/unicore32/include/asm/syscall.h | 2 +-
> arch/x86/include/asm/syscall.h | 8 +++++---
> arch/x86/um/asm/syscall.h | 2 +-
> arch/xtensa/include/asm/syscall.h | 2 +-
> include/asm-generic/syscall.h | 5 +++--
> kernel/auditsc.c | 4 ++--
> kernel/seccomp.c | 4 ++--
> 30 files changed, 52 insertions(+), 42 deletions(-)
Merged into audit/next, thanks everyone.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 2/2] ibmvscsi: Fix empty event pool access during host removal
From: Martin K. Petersen @ 2019-03-21 0:14 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: martin.petersen, linux-scsi, stable, james.bottomley, brking,
linuxppc-dev
In-Reply-To: <20190320184151.31321-2-tyreld@linux.vnet.ibm.com>
Tyrel,
> The event pool used for queueing commands is destroyed fairly early in
> the ibmvscsi_remove() code path. Since, this happens prior to the call
> so scsi_remove_host() it is possible for further calls to queuecommand
> to be processed which manifest as a panic due to a NULL pointer
> dereference as seen here:
Applied to 5.1/scsi-fixes. Thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* [PATCH 4/4] ibmvfc: Clean up transport events
From: Tyrel Datwyler @ 2019-03-20 19:56 UTC (permalink / raw)
To: james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
In-Reply-To: <20190320195654.4364-1-tyreld@linux.vnet.ibm.com>
No change to functionality. Simply make transport event messages a litle
clearer, and rework CRQ format enums such that we have separate enums
for INIT messages and XPORT events.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 8 +++++---
drivers/scsi/ibmvscsi/ibmvfc.h | 7 ++++++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 33dda4d32f65..3ad997ac3510 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -2756,16 +2756,18 @@ static void ibmvfc_handle_crq(struct ibmvfc_crq *crq, struct ibmvfc_host *vhost)
ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_NONE);
if (crq->format == IBMVFC_PARTITION_MIGRATED) {
/* We need to re-setup the interpartition connection */
- dev_info(vhost->dev, "Re-enabling adapter\n");
+ dev_info(vhost->dev, "Partition migrated, Re-enabling adapter\n");
vhost->client_migrated = 1;
ibmvfc_purge_requests(vhost, DID_REQUEUE);
ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_REENABLE);
- } else {
- dev_err(vhost->dev, "Virtual adapter failed (rc=%d)\n", crq->format);
+ } else if (crq->format == IBMVFC_PARTNER_FAILED || crq->format == IBMVFC_PARTNER_DEREGISTER) {
+ dev_err(vhost->dev, "Host partner adapter deregistered or failed (rc=%d)\n", crq->format);
ibmvfc_purge_requests(vhost, DID_ERROR);
ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_RESET);
+ } else {
+ dev_err(vhost->dev, "Received unknown transport event from partner (rc=%d)\n", crq->format);
}
return;
case IBMVFC_CRQ_CMD_RSP:
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index b81a53c4a9a8..459cc288ba1d 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -78,9 +78,14 @@ enum ibmvfc_crq_valid {
IBMVFC_CRQ_XPORT_EVENT = 0xFF,
};
-enum ibmvfc_crq_format {
+enum ibmvfc_crq_init_msg {
IBMVFC_CRQ_INIT = 0x01,
IBMVFC_CRQ_INIT_COMPLETE = 0x02,
+};
+
+enum ibmvfc_crq_xport_evts {
+ IBMVFC_PARTNER_FAILED = 0x01,
+ IBMVFC_PARTNER_DEREGISTER = 0x02,
IBMVFC_PARTITION_MIGRATED = 0x06,
};
--
2.12.3
^ permalink raw reply related
* [PATCH 3/4] ibmvfc: Byte swap status and error codes when logging
From: Tyrel Datwyler @ 2019-03-20 19:56 UTC (permalink / raw)
To: james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
In-Reply-To: <20190320195654.4364-1-tyreld@linux.vnet.ibm.com>
Status and error codes are returned in big endian from the VIOS. The
values are translated into a human readable format when logged, but
the values are also logged. This patch byte swaps those values so
that they are consistent between BE and LE platforms.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 18ee2a8ec3d5..33dda4d32f65 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1497,7 +1497,7 @@ static void ibmvfc_log_error(struct ibmvfc_event *evt)
scmd_printk(KERN_ERR, cmnd, "Command (%02X) : %s (%x:%x) "
"flags: %x fcp_rsp: %x, resid=%d, scsi_status: %x\n",
- cmnd->cmnd[0], err, vfc_cmd->status, vfc_cmd->error,
+ cmnd->cmnd[0], err, be16_to_cpu(vfc_cmd->status), be16_to_cpu(vfc_cmd->error),
rsp->flags, rsp_code, scsi_get_resid(cmnd), rsp->scsi_status);
}
@@ -2023,7 +2023,7 @@ static int ibmvfc_reset_device(struct scsi_device *sdev, int type, char *desc)
sdev_printk(KERN_ERR, sdev, "%s reset failed: %s (%x:%x) "
"flags: %x fcp_rsp: %x, scsi_status: %x\n", desc,
ibmvfc_get_cmd_error(be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error)),
- rsp_iu.cmd.status, rsp_iu.cmd.error, fc_rsp->flags, rsp_code,
+ be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error), fc_rsp->flags, rsp_code,
fc_rsp->scsi_status);
rsp_rc = -EIO;
} else
@@ -2382,7 +2382,7 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev)
sdev_printk(KERN_ERR, sdev, "Abort failed: %s (%x:%x) "
"flags: %x fcp_rsp: %x, scsi_status: %x\n",
ibmvfc_get_cmd_error(be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error)),
- rsp_iu.cmd.status, rsp_iu.cmd.error, fc_rsp->flags, rsp_code,
+ be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error), fc_rsp->flags, rsp_code,
fc_rsp->scsi_status);
rsp_rc = -EIO;
} else
@@ -3349,7 +3349,7 @@ static void ibmvfc_tgt_prli_done(struct ibmvfc_event *evt)
tgt_log(tgt, level, "Process Login failed: %s (%x:%x) rc=0x%02X\n",
ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)),
- rsp->status, rsp->error, status);
+ be16_to_cpu(rsp->status), be16_to_cpu(rsp->error), status);
break;
}
@@ -3447,9 +3447,10 @@ static void ibmvfc_tgt_plogi_done(struct ibmvfc_event *evt)
ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_DEL_RPORT);
tgt_log(tgt, level, "Port Login failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n",
- ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)), rsp->status, rsp->error,
- ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)), rsp->fc_type,
- ibmvfc_get_ls_explain(be16_to_cpu(rsp->fc_explain)), rsp->fc_explain, status);
+ ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)),
+ be16_to_cpu(rsp->status), be16_to_cpu(rsp->error),
+ ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)), be16_to_cpu(rsp->fc_type),
+ ibmvfc_get_ls_explain(be16_to_cpu(rsp->fc_explain)), be16_to_cpu(rsp->fc_explain), status);
break;
}
@@ -3620,7 +3621,7 @@ static void ibmvfc_tgt_adisc_done(struct ibmvfc_event *evt)
fc_explain = (be32_to_cpu(mad->fc_iu.response[1]) & 0x0000ff00) >> 8;
tgt_info(tgt, "ADISC failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n",
ibmvfc_get_cmd_error(be16_to_cpu(mad->iu.status), be16_to_cpu(mad->iu.error)),
- mad->iu.status, mad->iu.error,
+ be16_to_cpu(mad->iu.status), be16_to_cpu(mad->iu.error),
ibmvfc_get_fc_type(fc_reason), fc_reason,
ibmvfc_get_ls_explain(fc_explain), fc_explain, status);
break;
@@ -3832,9 +3833,10 @@ static void ibmvfc_tgt_query_target_done(struct ibmvfc_event *evt)
tgt_log(tgt, level, "Query Target failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n",
ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)),
- rsp->status, rsp->error, ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)),
- rsp->fc_type, ibmvfc_get_gs_explain(be16_to_cpu(rsp->fc_explain)),
- rsp->fc_explain, status);
+ be16_to_cpu(rsp->status), be16_to_cpu(rsp->error),
+ ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)), be16_to_cpu(rsp->fc_type),
+ ibmvfc_get_gs_explain(be16_to_cpu(rsp->fc_explain)), be16_to_cpu(rsp->fc_explain),
+ status);
break;
}
@@ -3960,7 +3962,7 @@ static void ibmvfc_discover_targets_done(struct ibmvfc_event *evt)
level += ibmvfc_retry_host_init(vhost);
ibmvfc_log(vhost, level, "Discover Targets failed: %s (%x:%x)\n",
ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)),
- rsp->status, rsp->error);
+ be16_to_cpu(rsp->status), be16_to_cpu(rsp->error));
break;
case IBMVFC_MAD_DRIVER_FAILED:
break;
@@ -4025,7 +4027,7 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event *evt)
ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
ibmvfc_log(vhost, level, "NPIV Login failed: %s (%x:%x)\n",
ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)),
- rsp->status, rsp->error);
+ be16_to_cpu(rsp->status), be16_to_cpu(rsp->error));
ibmvfc_free_event(evt);
return;
case IBMVFC_MAD_CRQ_ERROR:
--
2.12.3
^ permalink raw reply related
* [PATCH 2/4] ibmvfc: Add failed PRLI to cmd_status lookup array
From: Tyrel Datwyler @ 2019-03-20 19:56 UTC (permalink / raw)
To: james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
In-Reply-To: <20190320195654.4364-1-tyreld@linux.vnet.ibm.com>
The VIOS uses the SCSI_ERROR class to report PRLI failures. These
errors are indicated with the combination of a IBMVFC_FC_SCSI_ERROR
return status and 0x8000 error code. Add these codes to cmd_status[]
with appropriate human readable error message.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index c3ce27039552..18ee2a8ec3d5 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -139,6 +139,7 @@ static const struct {
{ IBMVFC_FC_FAILURE, IBMVFC_VENDOR_SPECIFIC, DID_ERROR, 1, 1, "vendor specific" },
{ IBMVFC_FC_SCSI_ERROR, 0, DID_OK, 1, 0, "SCSI error" },
+ { IBMVFC_FC_SCSI_ERROR, IBMVFC_COMMAND_FAILED, DID_ERROR, 0, 1, "PRLI to device failed." },
};
static void ibmvfc_npiv_login(struct ibmvfc_host *);
--
2.12.3
^ permalink raw reply related
* [PATCH 1/4] ibmvfc: Remove "failed" from logged errors
From: Tyrel Datwyler @ 2019-03-20 19:56 UTC (permalink / raw)
To: james.bottomley
Cc: brking, linuxppc-dev, linux-scsi, martin.petersen, Tyrel Datwyler
The text of messages logged with ibmvfc_log_error() always contain
the term "failed". In the case of cancelled commands during EH they
are reported back by the VIOS using error codes. This can be
confusing to somebody looking at these log messages as to whether
a command was successfully cancelled. The following real log
message for example it is unclear if the transaction was actaully
cancelled.
<6>sd 0:0:1:1: Cancelling outstanding commands.
<3>sd 0:0:1:1: [sde] Command (28) failed: transaction cancelled (2:6) flags: 0 fcp_rsp: 0, resid=0, scsi_status: 0
Remove prefixing of "failed" to all error logged messages. The
ibmvfc_log_error() function translates the returned error/status
codes to a human readable message already.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvfc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index dbaa4f131433..c3ce27039552 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -1494,7 +1494,7 @@ static void ibmvfc_log_error(struct ibmvfc_event *evt)
if (rsp->flags & FCP_RSP_LEN_VALID)
rsp_code = rsp->data.info.rsp_code;
- scmd_printk(KERN_ERR, cmnd, "Command (%02X) failed: %s (%x:%x) "
+ scmd_printk(KERN_ERR, cmnd, "Command (%02X) : %s (%x:%x) "
"flags: %x fcp_rsp: %x, resid=%d, scsi_status: %x\n",
cmnd->cmnd[0], err, vfc_cmd->status, vfc_cmd->error,
rsp->flags, rsp_code, scsi_get_resid(cmnd), rsp->scsi_status);
--
2.12.3
^ permalink raw reply related
* [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Joel Stanley @ 2019-03-21 0:32 UTC (permalink / raw)
To: linuxppc-dev
Segher added some workarounds for GCC 4.2 and bintuils 2.18. We now set
4.6 and 2.20 as the minimum, so they can be dropped.
This is mostly a revert of c69cccc95fe4 ("powerpc: Fix build bug with
binutils < 2.18 and GCC < 4.2").
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
arch/powerpc/kernel/vmlinux.lds.S | 35 ++++---------------------------
1 file changed, 4 insertions(+), 31 deletions(-)
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 060a1acd7c6d..0551e9846676 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -17,25 +17,6 @@
ENTRY(_stext)
-PHDRS {
- kernel PT_LOAD FLAGS(7); /* RWX */
- notes PT_NOTE FLAGS(0);
- dummy PT_NOTE FLAGS(0);
-
- /* binutils < 2.18 has a bug that makes it misbehave when taking an
- ELF file with all segments at load address 0 as input. This
- happens when running "strip" on vmlinux, because of the AT() magic
- in this linker script. People using GCC >= 4.2 won't run into
- this problem, because the "build-id" support will put some data
- into the "notes" segment (at a non-zero load address).
-
- To work around this, we force some data into both the "dummy"
- segment and the kernel segment, so the dummy segment will get a
- non-zero load address. It's not enough to always create the
- "notes" segment, since if nothing gets assigned to it, its load
- address will be zero. */
-}
-
#ifdef CONFIG_PPC64
OUTPUT_ARCH(powerpc:common64)
jiffies = jiffies_64;
@@ -77,7 +58,7 @@ SECTIONS
#else /* !CONFIG_PPC64 */
HEAD_TEXT
#endif
- } :kernel
+ }
__head_end = .;
@@ -126,7 +107,7 @@ SECTIONS
__got2_end = .;
#endif /* CONFIG_PPC32 */
- } :kernel
+ }
. = ALIGN(ETEXT_ALIGN_SIZE);
_etext = .;
@@ -177,15 +158,7 @@ SECTIONS
#endif
EXCEPTION_TABLE(0)
- NOTES :kernel :notes
-
- /* The dummy segment contents for the bug workaround mentioned above
- near PHDRS. */
- .dummy : AT(ADDR(.dummy) - LOAD_OFFSET) {
- LONG(0)
- LONG(0)
- LONG(0)
- } :kernel :dummy
+ NOTES
/*
* Init sections discarded at runtime
@@ -200,7 +173,7 @@ SECTIONS
#ifdef CONFIG_PPC64
*(.tramp.ftrace.init);
#endif
- } :kernel
+ }
/* .exit.text is discarded at runtime, not link time,
* to deal with references from __bug_table
--
2.20.1
^ permalink raw reply related
* Re: [PATCH kernel RFC 2/2] vfio-pci-nvlink2: Implement interconnect isolation
From: David Gibson @ 2019-03-20 23:56 UTC (permalink / raw)
To: Alex Williamson
Cc: Jose Ricardo Ziviani, Alexey Kardashevskiy,
Daniel Henrique Barboza, kvm-ppc, Piotr Jaroszynski,
Leonardo Augusto Guimarães Garcia, linuxppc-dev
In-Reply-To: <20190320130908.4f837995@x1.home>
[-- Attachment #1: Type: text/plain, Size: 9078 bytes --]
On Wed, Mar 20, 2019 at 01:09:08PM -0600, Alex Williamson wrote:
> On Wed, 20 Mar 2019 15:38:24 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Tue, Mar 19, 2019 at 10:36:19AM -0600, Alex Williamson wrote:
> > > On Fri, 15 Mar 2019 19:18:35 +1100
> > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > >
> > > > The NVIDIA V100 SXM2 GPUs are connected to the CPU via PCIe links and
> > > > (on POWER9) NVLinks. In addition to that, GPUs themselves have direct
> > > > peer to peer NVLinks in groups of 2 to 4 GPUs. At the moment the POWERNV
> > > > platform puts all interconnected GPUs to the same IOMMU group.
> > > >
> > > > However the user may want to pass individual GPUs to the userspace so
> > > > in order to do so we need to put them into separate IOMMU groups and
> > > > cut off the interconnects.
> > > >
> > > > Thankfully V100 GPUs implement an interface to do by programming link
> > > > disabling mask to BAR0 of a GPU. Once a link is disabled in a GPU using
> > > > this interface, it cannot be re-enabled until the secondary bus reset is
> > > > issued to the GPU.
> > > >
> > > > This defines a reset_done() handler for V100 NVlink2 device which
> > > > determines what links need to be disabled. This relies on presence
> > > > of the new "ibm,nvlink-peers" device tree property of a GPU telling which
> > > > PCI peers it is connected to (which includes NVLink bridges or peer GPUs).
> > > >
> > > > This does not change the existing behaviour and instead adds
> > > > a new "isolate_nvlink" kernel parameter to allow such isolation.
> > > >
> > > > The alternative approaches would be:
> > > >
> > > > 1. do this in the system firmware (skiboot) but for that we would need
> > > > to tell skiboot via an additional OPAL call whether or not we want this
> > > > isolation - skiboot is unaware of IOMMU groups.
> > > >
> > > > 2. do this in the secondary bus reset handler in the POWERNV platform -
> > > > the problem with that is at that point the device is not enabled, i.e.
> > > > config space is not restored so we need to enable the device (i.e. MMIO
> > > > bit in CMD register + program valid address to BAR0) in order to disable
> > > > links and then perhaps undo all this initialization to bring the device
> > > > back to the state where pci_try_reset_function() expects it to be.
> > >
> > > The trouble seems to be that this approach only maintains the isolation
> > > exposed by the IOMMU group when vfio-pci is the active driver for the
> > > device. IOMMU groups can be used by any driver and the IOMMU core is
> > > incorporating groups in various ways.
> >
> > I don't think that reasoning is quite right. An IOMMU group doesn't
> > necessarily represent devices which *are* isolated, just devices which
> > *can be* isolated. There are plenty of instances when we don't need
> > to isolate devices in different IOMMU groups: passing both groups to
> > the same guest or userspace VFIO driver for example, or indeed when
> > both groups are owned by regular host kernel drivers.
> >
> > In at least some of those cases we also don't want to isolate the
> > devices when we don't have to, usually for performance reasons.
>
> I see IOMMU groups as representing the current isolation of the device,
> not just the possible isolation. If there are ways to break down that
> isolation then ideally the group would be updated to reflect it. The
> ACS disable patches seem to support this, at boot time we can choose to
> disable ACS at certain points in the topology to favor peer-to-peer
> performance over isolation. This is then reflected in the group
> composition, because even though ACS *can be* enabled at the given
> isolation points, it's intentionally not with this option. Whether or
> not a given user who owns multiple devices needs that isolation is
> really beside the point, the user can choose to connect groups via IOMMU
> mappings or reconfigure the system to disable ACS and potentially more
> direct routing. The IOMMU groups are still accurately reflecting the
> topology and IOMMU based isolation.
Huh, ok, I think we need to straighten this out. Thinking of iommu
groups as possible rather than potential isolation was a conscious
decision on my part when we were first coming up with them. The
rationale was that that way iommu groups could be static for the
lifetime of boot, with more dynamic isolation state layered on top.
Now, that was based on analogy with PAPR's concept of "Partitionable
Endpoints" which are decided by firmware before boot. However, I
think it makes sense in other contexts too: if iommu groups represent
current isolation, then we need some other way to advertise possible
isolation - otherwise how will the admin (and/or tools) know how it
can configure the iommu groups.
VFIO already has the container, which represents explicitly a "group
of groups" that we don't care to isolate from each other. I don't
actually know what other uses of the iommu group infrastructure we
have at present and how they treat them.
So, if we now have dynamically reconfigurable groups which are a
departure from that design, how can we go from here trying to bring
things back to consistency.
> > > So, if there's a device specific
> > > way to configure the isolation reported in the group, which requires
> > > some sort of active management against things like secondary bus
> > > resets, then I think we need to manage it above the attached endpoint
> > > driver.
> >
> > The problem is that above the endpoint driver, we don't actually have
> > enough information about what should be isolated. For VFIO we want to
> > isolate things if they're in different containers, for most regular
> > host kernel drivers we don't need to isolate at all (although we might
> > as well when it doesn't have a cost).
>
> This idea that we only want to isolate things if they're in different
> containers is bogus, imo. There are performance reasons why we might
> not want things isolated, but there are also address space reasons why
> we do. If there are direct routes between devices, the user needs to
> be aware of the IOVA pollution, if we maintain singleton groups, they
> don't. Granted we don't really account for this well in most
> userspaces and fumble through it by luck of the address space layout
> and lack of devices really attempting peer to peer access.
I don't really follow what you're saying here.
> For in-kernel users, we're still theoretically trying to isolate
> devices such that they have restricted access to only the resources
> they need. Disabling things like ACS in the topology reduces that
> isolation. AFAICT, most users don't really care about that degree of
> isolation, so they run with iommu=pt for native driver performance
> while still having the IOMMU available for isolation use cases running
> in parallel. We don't currently have support for on-demand enabling
> isolation.
Ok.
> > The host side nVidia GPGPU
> > drivers also won't want to isolate the (host owned) NVLink devices
> > from each other, since they'll want to use the fast interconnects
>
> This falls into the same mixed use case scenario above where we don't
> really have a good solution today. Things like ACS are dynamically
> configurable, but we don't expose any interfaces to let drivers or
> users change it (aside from setpci, which we don't account for
> dynamically). We assume a simplistic model where if you want IOMMU,
> then you must also want the maximum configurable isolation.
> Dynamically changing routing is not necessarily the most foolproof
> thing either with potentially in-flight transactions and existing DMA
> mappings, which is why I've suggested a couple times that perhaps we
> could do a software hot-unplug of a sub-hierarchy, muck with isolation
> at the remaining node, then re-discover the removed devices.
Ok, so I feel like we need to go fully one way or the other. Either:
1) Groups represent current isolation status, in which case we
deprecate vfio containers in favour of fusing groups beforehand,
and we need some new concept ("isolation atoms"?) to represent what
isolation is possible
or
2) Groups represent potential isolation, with a higher level construct
representing current isolation. This could involve making vfio
containers essentially a wrapper around some more generic concept
("isolation clusters"?) of a group of groups.
> Of course when we bring NVIDIA into the mix, I have little sympathy
> that the NVLink interfaces are all proprietary and we have no idea how
> to make those dynamic changes or discover the interconnected-ness of a
> device. Thanks,
Yes, well, no argument there.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 2/2] ibmvscsi: Fix empty event pool access during host removal
From: Tyrel Datwyler @ 2019-03-20 18:41 UTC (permalink / raw)
To: james.bottomley
Cc: martin.petersen, linux-scsi, stable, Tyrel Datwyler, brking,
linuxppc-dev
In-Reply-To: <20190320184151.31321-1-tyreld@linux.vnet.ibm.com>
The event pool used for queueing commands is destroyed fairly early in
the ibmvscsi_remove() code path. Since, this happens prior to the call
so scsi_remove_host() it is possible for further calls to queuecommand
to be processed which manifest as a panic due to a NULL pointer
dereference as seen here:
PANIC: "Unable to handle kernel paging request for data at address
0x00000000"
Context process backtrace:
DSISR: 0000000042000000 ????Syscall Result: 0000000000000000
4 [c000000002cb3820] memcpy_power7 at c000000000064204
[Link Register] [c000000002cb3820] ibmvscsi_send_srp_event at d000000003ed14a4
5 [c000000002cb3920] ibmvscsi_send_srp_event at d000000003ed14a4 [ibmvscsi] ?(unreliable)
6 [c000000002cb39c0] ibmvscsi_queuecommand at d000000003ed2388 [ibmvscsi]
7 [c000000002cb3a70] scsi_dispatch_cmd at d00000000395c2d8 [scsi_mod]
8 [c000000002cb3af0] scsi_request_fn at d00000000395ef88 [scsi_mod]
9 [c000000002cb3be0] __blk_run_queue at c000000000429860
10 [c000000002cb3c10] blk_delay_work at c00000000042a0ec
11 [c000000002cb3c40] process_one_work at c0000000000dac30
12 [c000000002cb3cd0] worker_thread at c0000000000db110
13 [c000000002cb3d80] kthread at c0000000000e3378
14 [c000000002cb3e30] ret_from_kernel_thread at c00000000000982c
The kernel buffer log is overfilled with this log:
[11261.952732] ibmvscsi: found no event struct in pool!
This patch reorders the operations during host teardown. Start by
calling the SRP transport and Scsi_Host remove functions to flush any
outstanding work and set the host offline. LLDD teardown follows
including destruction of the event pool, freeing the Command Response
Queue (CRQ), and unmapping any persistent buffers. The event pool
destruction is protected by the scsi_host lock, and the pool is purged
prior of any requests for which we never received a response. Finally,
move the removal of the scsi host from our global list to the end so
that the host is easily locatable for debugging purposes during
teardown.
Cc: <stable@vger.kernel.org> # v2.6.12+
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 2b22969f3f63..8cec5230fe31 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2295,17 +2295,27 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
static int ibmvscsi_remove(struct vio_dev *vdev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
- spin_lock(&ibmvscsi_driver_lock);
- list_del(&hostdata->host_list);
- spin_unlock(&ibmvscsi_driver_lock);
- unmap_persist_bufs(hostdata);
+ unsigned long flags;
+
+ srp_remove_host(hostdata->host);
+ scsi_remove_host(hostdata->host);
+
+ purge_requests(hostdata, DID_ERROR);
+
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
release_event_pool(&hostdata->pool, hostdata);
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+
ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
max_events);
kthread_stop(hostdata->work_thread);
- srp_remove_host(hostdata->host);
- scsi_remove_host(hostdata->host);
+ unmap_persist_bufs(hostdata);
+
+ spin_lock(&ibmvscsi_driver_lock);
+ list_del(&hostdata->host_list);
+ spin_unlock(&ibmvscsi_driver_lock);
+
scsi_host_put(hostdata->host);
return 0;
--
2.12.3
^ permalink raw reply related
* [PATCH 1/2] ibmvscsi: Protect ibmvscsi_head from concurrent modificaiton
From: Tyrel Datwyler @ 2019-03-20 18:41 UTC (permalink / raw)
To: james.bottomley
Cc: martin.petersen, linux-scsi, stable, Tyrel Datwyler, brking,
linuxppc-dev
For each ibmvscsi host created during a probe or destroyed during a
remove we either add or remove that host to/from the global ibmvscsi_head
list. This runs the risk of concurrent modification.
This patch adds a simple spinlock around the list modification calls to
prevent concurrent updates as is done similarly in the ibmvfc driver and
ipr driver.
Fixes: 32d6e4b6e4ea ("scsi: ibmvscsi: add vscsi hosts to global list_head")
Cc: <stable@vger.kernel.org> # v4.10+
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 1135e74646e2..2b22969f3f63 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -96,6 +96,7 @@ static int client_reserve = 1;
static char partition_name[96] = "UNKNOWN";
static unsigned int partition_number = -1;
static LIST_HEAD(ibmvscsi_head);
+static DEFINE_SPINLOCK(ibmvscsi_driver_lock);
static struct scsi_transport_template *ibmvscsi_transport_template;
@@ -2270,7 +2271,9 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
}
dev_set_drvdata(&vdev->dev, hostdata);
+ spin_lock(&ibmvscsi_driver_lock);
list_add_tail(&hostdata->host_list, &ibmvscsi_head);
+ spin_unlock(&ibmvscsi_driver_lock);
return 0;
add_srp_port_failed:
@@ -2292,7 +2295,9 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const struct vio_device_id *id)
static int ibmvscsi_remove(struct vio_dev *vdev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
+ spin_lock(&ibmvscsi_driver_lock);
list_del(&hostdata->host_list);
+ spin_unlock(&ibmvscsi_driver_lock);
unmap_persist_bufs(hostdata);
release_event_pool(&hostdata->pool, hostdata);
ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
--
2.12.3
^ permalink raw reply related
* Re: [PATCH 1/4] add generic builtin command line
From: Daniel Walker @ 2019-03-20 23:23 UTC (permalink / raw)
To: Andrew Morton
Cc: Maksym Kokhan, linux-kernel, Rob Herring, Paul Mackerras,
xe-linux-external, Daniel Walker, linuxppc-dev
In-Reply-To: <20190320155319.2cd3c0f73ef3cdefb65d5d1e@linux-foundation.org>
On Wed, Mar 20, 2019 at 03:53:19PM -0700, Andrew Morton wrote:
> On Tue, 19 Mar 2019 16:24:45 -0700 Daniel Walker <danielwa@cisco.com> wrote:
>
> > This code allows architectures to use a generic builtin command line.
>
> I wasn't cc'ed on [2/4]. No mailing lists were cc'ed on [0/4] but it
> didn't say anything useful anyway ;)
>
> I'll queue them up for testing and shall await feedback from the
> powerpc developers.
>
You weren't CC'd , but it was To: you,
35 From: Daniel Walker <danielwa@cisco.com>
36 To: Andrew Morton <akpm@linux-foundation.org>,
37 Christophe Leroy <christophe.leroy@c-s.fr>,
38 Michael Ellerman <mpe@ellerman.id.au>,
39 Rob Herring <robh+dt@kernel.org>, xe-linux-external@cisco.com,
40 linuxppc-dev@lists.ozlabs.org, Frank Rowand <frowand.list@gmail.com>
41 Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
42 Subject: [PATCH 2/4] drivers: of: generic command line support
and the first one [0/4] should have went to the linuxppc-dev , and xe-linux-external. Maybe
our git-send-email isn't working with our mail servers.
Thanks for picking it up.
Daniel
^ permalink raw reply
* Re: [PATCH v4 06/17] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: David Gibson @ 2019-03-20 23:09 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev, Paul Mackerras, kvm, kvm-ppc
In-Reply-To: <20190320083751.27001-7-clg@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 17590 bytes --]
On Wed, Mar 20, 2019 at 09:37:40AM +0100, Cédric Le Goater wrote:
> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
> H_INT_GET_QUEUE_CONFIG hcalls from QEMU to configure the underlying
> Event Queue in the XIVE IC. They will also be used to restore the
> configuration of the XIVE EQs and to capture the internal run-time
> state of the EQs. Both 'get' and 'set' rely on an OPAL call to access
> the EQ toggle bit and EQ index which are updated by the XIVE IC when
> event notifications are enqueued in the EQ.
>
> The value of the guest physical address of the event queue is saved in
> the XIVE internal xive_q structure for later use. That is when
> migration needs to mark the EQ pages dirty to capture a consistent
> memory state of the VM.
>
> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
> but restoring the EQ state will.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
> Changes since v3 :
>
> - fix the test ont the initial setting of the EQ toggle bit : 0 -> 1
> - renamed qsize to qshift
> - renamed qpage to qaddr
> - checked host page size
> - limited flags to KVM_XIVE_EQ_ALWAYS_NOTIFY to fit sPAPR specs
>
> Changes since v2 :
>
> - fixed comments on the KVM device attribute definitions
> - fixed check on supported EQ size to restrict to 64K pages
> - checked kvm_eq.flags that need to be zero
> - removed the OPAL call when EQ qtoggle bit and index are zero.
>
> arch/powerpc/include/asm/xive.h | 2 +
> arch/powerpc/include/uapi/asm/kvm.h | 19 ++
> arch/powerpc/kvm/book3s_xive.h | 2 +
> arch/powerpc/kvm/book3s_xive.c | 15 +-
> arch/powerpc/kvm/book3s_xive_native.c | 242 +++++++++++++++++++++
> Documentation/virtual/kvm/devices/xive.txt | 34 +++
> 6 files changed, 308 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index b579a943407b..c4e88abd3b67 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -73,6 +73,8 @@ struct xive_q {
> u32 esc_irq;
> atomic_t count;
> atomic_t pending_count;
> + u64 guest_qaddr;
> + u32 guest_qshift;
> };
>
> /* Global enable flags for the XIVE support */
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> index e8161e21629b..85005400fd86 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -681,6 +681,7 @@ struct kvm_ppc_cpu_char {
> #define KVM_DEV_XIVE_GRP_CTRL 1
> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source identifier */
> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source identifier */
> +#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit EQ identifier */
>
> /* Layout of 64-bit XIVE source attribute values */
> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
> @@ -696,4 +697,22 @@ struct kvm_ppc_cpu_char {
> #define KVM_XIVE_SOURCE_EISN_SHIFT 33
> #define KVM_XIVE_SOURCE_EISN_MASK 0xfffffffe00000000ULL
>
> +/* Layout of 64-bit EQ identifier */
> +#define KVM_XIVE_EQ_PRIORITY_SHIFT 0
> +#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
> +#define KVM_XIVE_EQ_SERVER_MASK 0xfffffff8ULL
> +
> +/* Layout of EQ configuration values (64 bytes) */
> +struct kvm_ppc_xive_eq {
> + __u32 flags;
> + __u32 qshift;
> + __u64 qaddr;
> + __u32 qtoggle;
> + __u32 qindex;
> + __u8 pad[40];
> +};
> +
> +#define KVM_XIVE_EQ_ALWAYS_NOTIFY 0x00000001
> +
> #endif /* __LINUX_KVM_POWERPC_H */
> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> index ae26fe653d98..622f594d93e1 100644
> --- a/arch/powerpc/kvm/book3s_xive.h
> +++ b/arch/powerpc/kvm/book3s_xive.h
> @@ -272,6 +272,8 @@ struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
> struct kvmppc_xive *xive, int irq);
> void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
> int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> + bool single_escalation);
>
> #endif /* CONFIG_KVM_XICS */
> #endif /* _KVM_PPC_BOOK3S_XICS_H */
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index e09f3addffe5..c1b7aa7dbc28 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> + bool single_escalation)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> struct xive_q *q = &xc->queues[prio];
> @@ -185,7 +186,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> return -EIO;
> }
>
> - if (xc->xive->single_escalation)
> + if (single_escalation)
> name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
> vcpu->kvm->arch.lpid, xc->server_num);
> else
> @@ -217,7 +218,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> * interrupt, thus leaving it effectively masked after
> * it fires once.
> */
> - if (xc->xive->single_escalation) {
> + if (single_escalation) {
> struct irq_data *d = irq_get_irq_data(xc->esc_virq[prio]);
> struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
>
> @@ -291,7 +292,8 @@ static int xive_check_provisioning(struct kvm *kvm, u8 prio)
> continue;
> rc = xive_provision_queue(vcpu, prio);
> if (rc == 0 && !xive->single_escalation)
> - xive_attach_escalation(vcpu, prio);
> + kvmppc_xive_attach_escalation(vcpu, prio,
> + xive->single_escalation);
> if (rc)
> return rc;
> }
> @@ -1214,7 +1216,8 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> if (xive->qmap & (1 << i)) {
> r = xive_provision_queue(vcpu, i);
> if (r == 0 && !xive->single_escalation)
> - xive_attach_escalation(vcpu, i);
> + kvmppc_xive_attach_escalation(
> + vcpu, i, xive->single_escalation);
> if (r)
> goto bail;
> } else {
> @@ -1229,7 +1232,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> }
>
> /* If not done above, attach priority 0 escalation */
> - r = xive_attach_escalation(vcpu, 0);
> + r = kvmppc_xive_attach_escalation(vcpu, 0, xive->single_escalation);
> if (r)
> goto bail;
>
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index 492825a35958..2c335454da72 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -335,6 +335,236 @@ static int kvmppc_xive_native_set_source_config(struct kvmppc_xive *xive,
> priority, masked, eisn);
> }
>
> +static int xive_native_validate_queue_size(u32 qshift)
> +{
> + /*
> + * We only support 64K pages for the moment. This is also
> + * advertised in the DT property "ibm,xive-eq-sizes"
> + */
> + switch (qshift) {
> + case 0: /* EQ reset */
> + case 16:
> + return 0;
> + case 12:
> + case 21:
> + case 24:
> + default:
> + return -EINVAL;
Now that you're doing a proper test against the guest page size, it
would be very easy to allow this to support things other than a 64kiB
queue. That can be a followup change, though.
> + }
> +}
> +
> +static int kvmppc_xive_native_set_queue_config(struct kvmppc_xive *xive,
> + long eq_idx, u64 addr)
> +{
> + struct kvm *kvm = xive->kvm;
> + struct kvm_vcpu *vcpu;
> + struct kvmppc_xive_vcpu *xc;
> + void __user *ubufp = (void __user *) addr;
> + u32 server;
> + u8 priority;
> + struct kvm_ppc_xive_eq kvm_eq;
> + int rc;
> + __be32 *qaddr = 0;
> + struct page *page;
> + struct xive_q *q;
> + gfn_t gfn;
> + unsigned long page_size;
> +
> + /*
> + * Demangle priority/server tuple from the EQ identifier
> + */
> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
> + KVM_XIVE_EQ_PRIORITY_SHIFT;
> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
> + KVM_XIVE_EQ_SERVER_SHIFT;
> +
> + if (copy_from_user(&kvm_eq, ubufp, sizeof(kvm_eq)))
> + return -EFAULT;
> +
> + vcpu = kvmppc_xive_find_server(kvm, server);
> + if (!vcpu) {
> + pr_err("Can't find server %d\n", server);
> + return -ENOENT;
> + }
> + xc = vcpu->arch.xive_vcpu;
> +
> + if (priority != xive_prio_from_guest(priority)) {
> + pr_err("Trying to restore invalid queue %d for VCPU %d\n",
> + priority, server);
> + return -EINVAL;
> + }
> + q = &xc->queues[priority];
> +
> + pr_devel("%s VCPU %d priority %d fl:%x shift:%d addr:%llx g:%d idx:%d\n",
> + __func__, server, priority, kvm_eq.flags,
> + kvm_eq.qshift, kvm_eq.qaddr, kvm_eq.qtoggle, kvm_eq.qindex);
> +
> + /*
> + * sPAPR specifies a "Unconditional Notify (n) flag" for the
> + * H_INT_SET_QUEUE_CONFIG hcall which forces notification
> + * without using the coalescing mechanisms provided by the
> + * XIVE END ESBs.
> + */
> + if (kvm_eq.flags & ~KVM_XIVE_EQ_ALWAYS_NOTIFY) {
> + pr_err("invalid flags %d\n", kvm_eq.flags);
> + return -EINVAL;
> + }
So this test prevents setting any (as yet undefined) flags other than
EQ_ALWAYS_NOTIFY, which is good. However, AFAICT you never actually
branch based on whether the EQ_ALWAYS_NOTIFY flag is set - you just
assume that it is. You should either actually pass this flag through
to the OPAL call, or have another test here which rejects the ioctl()
if the flag is *not* set.
> +
> + rc = xive_native_validate_queue_size(kvm_eq.qshift);
> + if (rc) {
> + pr_err("invalid queue size %d\n", kvm_eq.qshift);
> + return rc;
> + }
> +
> + /* reset queue and disable queueing */
> + if (!kvm_eq.qshift) {
> + q->guest_qaddr = 0;
> + q->guest_qshift = 0;
> +
> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
> + NULL, 0, true);
> + if (rc) {
> + pr_err("Failed to reset queue %d for VCPU %d: %d\n",
> + priority, xc->server_num, rc);
> + return rc;
> + }
> +
> + if (q->qpage) {
> + put_page(virt_to_page(q->qpage));
> + q->qpage = NULL;
> + }
> +
> + return 0;
> + }
> +
> + gfn = gpa_to_gfn(kvm_eq.qaddr);
> + page = gfn_to_page(kvm, gfn);
> + if (is_error_page(page)) {
> + pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qaddr);
> + return -EINVAL;
> + }
> +
> + page_size = kvm_host_page_size(kvm, gfn);
> + if (1ull << kvm_eq.qshift > page_size) {
> + pr_warn("Incompatible host page size %lx!\n", page_size);
> + return -EINVAL;
> + }
Sorry.. I should have thought of this earlier. If qaddr isn't aligned
to a (page_size) boundary, this test isn't sufficient - you need to
make sure the whole thing fits within the host page, even if it's
offset.
Alternatively, you could check that qaddr is naturally aligned for the
requested qshift. Possibly that's necessary for the hardware anyway.
> +
> + qaddr = page_to_virt(page) + (kvm_eq.qaddr & ~PAGE_MASK);
> +
> + /*
> + * Backup the queue page guest address to the mark EQ page
> + * dirty for migration.
> + */
> + q->guest_qaddr = kvm_eq.qaddr;
> + q->guest_qshift = kvm_eq.qshift;
> +
> + /*
> + * Unconditional Notification is forced by default at the
> + * OPAL level because the use of END ESBs is not supported by
> + * Linux.
> + */
> + rc = xive_native_configure_queue(xc->vp_id, q, priority,
> + (__be32 *) qaddr, kvm_eq.qshift, true);
> + if (rc) {
> + pr_err("Failed to configure queue %d for VCPU %d: %d\n",
> + priority, xc->server_num, rc);
> + put_page(page);
> + return rc;
> + }
> +
> + /*
> + * Only restore the queue state when needed. When doing the
> + * H_INT_SET_SOURCE_CONFIG hcall, it should not.
> + */
> + if (kvm_eq.qtoggle != 1 || kvm_eq.qindex != 0) {
> + rc = xive_native_set_queue_state(xc->vp_id, priority,
> + kvm_eq.qtoggle,
> + kvm_eq.qindex);
> + if (rc)
> + goto error;
> + }
> +
> + rc = kvmppc_xive_attach_escalation(vcpu, priority,
> + xive->single_escalation);
> +error:
> + if (rc)
> + kvmppc_xive_native_cleanup_queue(vcpu, priority);
> + return rc;
> +}
> +
> +static int kvmppc_xive_native_get_queue_config(struct kvmppc_xive *xive,
> + long eq_idx, u64 addr)
> +{
> + struct kvm *kvm = xive->kvm;
> + struct kvm_vcpu *vcpu;
> + struct kvmppc_xive_vcpu *xc;
> + struct xive_q *q;
> + void __user *ubufp = (u64 __user *) addr;
> + u32 server;
> + u8 priority;
> + struct kvm_ppc_xive_eq kvm_eq;
> + u64 qaddr;
> + u64 qshift;
> + u64 qeoi_page;
> + u32 escalate_irq;
> + u64 qflags;
> + int rc;
> +
> + /*
> + * Demangle priority/server tuple from the EQ identifier
> + */
> + priority = (eq_idx & KVM_XIVE_EQ_PRIORITY_MASK) >>
> + KVM_XIVE_EQ_PRIORITY_SHIFT;
> + server = (eq_idx & KVM_XIVE_EQ_SERVER_MASK) >>
> + KVM_XIVE_EQ_SERVER_SHIFT;
> +
> + vcpu = kvmppc_xive_find_server(kvm, server);
> + if (!vcpu) {
> + pr_err("Can't find server %d\n", server);
> + return -ENOENT;
> + }
> + xc = vcpu->arch.xive_vcpu;
> +
> + if (priority != xive_prio_from_guest(priority)) {
> + pr_err("invalid priority for queue %d for VCPU %d\n",
> + priority, server);
> + return -EINVAL;
> + }
> + q = &xc->queues[priority];
> +
> + memset(&kvm_eq, 0, sizeof(kvm_eq));
> +
> + if (!q->qpage)
> + return 0;
> +
> + rc = xive_native_get_queue_info(xc->vp_id, priority, &qaddr, &qshift,
> + &qeoi_page, &escalate_irq, &qflags);
> + if (rc)
> + return rc;
> +
> + kvm_eq.flags = 0;
> + if (qflags & OPAL_XIVE_EQ_ALWAYS_NOTIFY)
> + kvm_eq.flags |= KVM_XIVE_EQ_ALWAYS_NOTIFY;
> +
> + kvm_eq.qshift = q->guest_qshift;
> + kvm_eq.qaddr = q->guest_qaddr;
> +
> + rc = xive_native_get_queue_state(xc->vp_id, priority, &kvm_eq.qtoggle,
> + &kvm_eq.qindex);
> + if (rc)
> + return rc;
> +
> + pr_devel("%s VCPU %d priority %d fl:%x shift:%d addr:%llx g:%d idx:%d\n",
> + __func__, server, priority, kvm_eq.flags,
> + kvm_eq.qshift, kvm_eq.qaddr, kvm_eq.qtoggle, kvm_eq.qindex);
> +
> + if (copy_to_user(ubufp, &kvm_eq, sizeof(kvm_eq)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> @@ -349,6 +579,9 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> case KVM_DEV_XIVE_GRP_SOURCE_CONFIG:
> return kvmppc_xive_native_set_source_config(xive, attr->attr,
> attr->addr);
> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
> + return kvmppc_xive_native_set_queue_config(xive, attr->attr,
> + attr->addr);
> }
> return -ENXIO;
> }
> @@ -356,6 +589,13 @@ static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> static int kvmppc_xive_native_get_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> + struct kvmppc_xive *xive = dev->private;
> +
> + switch (attr->group) {
> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
> + return kvmppc_xive_native_get_queue_config(xive, attr->attr,
> + attr->addr);
> + }
> return -ENXIO;
> }
>
> @@ -371,6 +611,8 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
> attr->attr < KVMPPC_XIVE_NR_IRQS)
> return 0;
> break;
> + case KVM_DEV_XIVE_GRP_EQ_CONFIG:
> + return 0;
> }
> return -ENXIO;
> }
> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
> index 33c64b2cdbe8..8bb5e6e6923a 100644
> --- a/Documentation/virtual/kvm/devices/xive.txt
> +++ b/Documentation/virtual/kvm/devices/xive.txt
> @@ -53,3 +53,37 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
> -ENXIO: CPU event queues not configured or configuration of the
> underlying HW interrupt failed
> -EBUSY: No CPU available to serve interrupt
> +
> + 4. KVM_DEV_XIVE_GRP_EQ_CONFIG (read-write)
> + Configures an event queue of a CPU
> + Attributes:
> + EQ descriptor identifier (64-bit)
> + The EQ descriptor identifier is a tuple (server, priority) :
> + bits: | 63 .... 32 | 31 .. 3 | 2 .. 0
> + values: | unused | server | priority
> + The kvm_device_attr.addr points to :
> + struct kvm_ppc_xive_eq {
> + __u32 flags;
> + __u32 qshift;
> + __u64 qaddr;
> + __u32 qtoggle;
> + __u32 qindex;
> + __u8 pad[40];
> + };
> + - flags: queue flags
> + KVM_XIVE_EQ_ALWAYS_NOTIFY
> + forces notification without using the coalescing mechanism
> + provided by the XIVE END ESBs.
> + - qshift: queue size (power of 2)
> + - qaddr: real address of queue
> + - qtoggle: current queue toggle bit
> + - qindex: current queue index
> + - pad: reserved for future use
> + Errors:
> + -ENOENT: Invalid CPU number
> + -EINVAL: Invalid priority
> + -EINVAL: Invalid flags
> + -EINVAL: Invalid queue size
> + -EINVAL: Invalid queue address
> + -EFAULT: Invalid user pointer for attr->addr.
> + -EIO: Configuration of the underlying HW failed
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] add generic builtin command line
From: Andrew Morton @ 2019-03-20 22:53 UTC (permalink / raw)
To: Daniel Walker
Cc: Maksym Kokhan, linux-kernel, Rob Herring, Paul Mackerras,
xe-linux-external, Daniel Walker, linuxppc-dev
In-Reply-To: <20190319232448.45964-2-danielwa@cisco.com>
On Tue, 19 Mar 2019 16:24:45 -0700 Daniel Walker <danielwa@cisco.com> wrote:
> This code allows architectures to use a generic builtin command line.
I wasn't cc'ed on [2/4]. No mailing lists were cc'ed on [0/4] but it
didn't say anything useful anyway ;)
I'll queue them up for testing and shall await feedback from the
powerpc developers.
^ permalink raw reply
* Re: [PATCH] hotplug/drc-info: ininitialize fndit to zero
From: Bjorn Helgaas @ 2019-03-20 22:12 UTC (permalink / raw)
To: Colin King
Cc: linux-pci, kernel-janitors, linux-kernel, Michael Bringmann,
Paul Mackerras, Tyrel Datwyler, linuxppc-dev
In-Reply-To: <20190316214016.6612-1-colin.king@canonical.com>
[+cc Michael B (original author)]
On Sat, Mar 16, 2019 at 09:40:16PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently variable fndit is not initialized and contains a
> garbage value, later it is set to 1 if a drc entry is found.
> Ensure fndit is not containing garbage by initializing it to
> zero. Also remove an extraneous space at the end of an
> sprintf call.
>
> Detected by static analysis with cppcheck.
>
> Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info property")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Michael E, I assume you'll take this since you took the original?
Let me know if you want me to.
> ---
> drivers/pci/hotplug/rpaphp_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> index bcd5d357ca23..28213f44f64a 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> struct of_drc_info drc;
> const __be32 *value;
> char cell_drc_name[MAX_DRC_NAME_LEN];
> - int j, fndit;
> + int j, fndit = 0;
>
> info = of_find_property(dn->parent, "ibm,drc-info", NULL);
> if (info == NULL)
> @@ -254,7 +254,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
> /* Found it */
>
> if (fndit)
> - sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
> + sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
> my_index);
>
> if (((drc_name == NULL) ||
> --
> 2.20.1
>
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-03-20 21:17 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <87ef71seve.fsf@morokweng.localdomain>
On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
> >> Another way of looking at this issue which also explains our reluctance
> >> is that the only difference between a secure guest and a regular guest
> >> (at least regarding virtio) is that the former uses swiotlb while the
> >> latter doens't.
> >
> > But swiotlb is just one implementation. It's a guest internal thing. The
> > issue is that memory isn't host accessible.
>
> >From what I understand of the ACCESS_PLATFORM definition, the host will
> only ever try to access memory addresses that are supplied to it by the
> guest, so all of the secure guest memory that the host cares about is
> accessible:
>
> If this feature bit is set to 0, then the device has same access to
> memory addresses supplied to it as the driver has. In particular,
> the device will always use physical addresses matching addresses
> used by the driver (typically meaning physical addresses used by the
> CPU) and not translated further, and can access any address supplied
> to it by the driver. When clear, this overrides any
> platform-specific description of whether device access is limited or
> translated in any way, e.g. whether an IOMMU may be present.
>
> All of the above is true for POWER guests, whether they are secure
> guests or not.
>
> Or are you saying that a virtio device may want to access memory
> addresses that weren't supplied to it by the driver?
Your logic would apply to IOMMUs as well. For your mode, there are
specific encrypted memory regions that driver has access to but device
does not. that seems to violate the constraint.
> >> And from the device's point of view they're
> >> indistinguishable. It can't tell one guest that is using swiotlb from
> >> one that isn't. And that implies that secure guest vs regular guest
> >> isn't a virtio interface issue, it's "guest internal affairs". So
> >> there's no reason to reflect that in the feature flags.
> >
> > So don't. The way not to reflect that in the feature flags is
> > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
> >
> >
> > Without ACCESS_PLATFORM
> > virtio has a very specific opinion about the security of the
> > device, and that opinion is that device is part of the guest
> > supervisor security domain.
>
> Sorry for being a bit dense, but not sure what "the device is part of
> the guest supervisor security domain" means. In powerpc-speak,
> "supervisor" is the operating system so perhaps that explains my
> confusion. Are you saying that without ACCESS_PLATFORM, the guest
> considers the host to be part of the guest operating system's security
> domain?
I think so. The spec says "device has same access as driver".
> If so, does that have any other implication besides "the host
> can access any address supplied to it by the driver"? If that is the
> case, perhaps the definition of ACCESS_PLATFORM needs to be amended to
> include that information because it's not part of the current
> definition.
>
> >> That said, we still would like to arrive at a proper design for this
> >> rather than add yet another hack if we can avoid it. So here's another
> >> proposal: considering that the dma-direct code (in kernel/dma/direct.c)
> >> automatically uses swiotlb when necessary (thanks to Christoph's recent
> >> DMA work), would it be ok to replace virtio's own direct-memory code
> >> that is used in the !ACCESS_PLATFORM case with the dma-direct code? That
> >> way we'll get swiotlb even with !ACCESS_PLATFORM, and virtio will get a
> >> code cleanup (replace open-coded stuff with calls to existing
> >> infrastructure).
> >
> > Let's say I have some doubts that there's an API that
> > matches what virtio with its bag of legacy compatibility exactly.
>
> Ok.
>
> >> > But the name "sev_active" makes me scared because at least AMD guys who
> >> > were doing the sensible thing and setting ACCESS_PLATFORM
> >>
> >> My understanding is, AMD guest-platform knows in advance that their
> >> guest will run in secure mode and hence sets the flag at the time of VM
> >> instantiation. Unfortunately we dont have that luxury on our platforms.
> >
> > Well you do have that luxury. It looks like that there are existing
> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
> > with how that path is slow. So you are trying to optimize for
> > them by clearing ACCESS_PLATFORM and then you have lost ability
> > to invoke DMA API.
> >
> > For example if there was another flag just like ACCESS_PLATFORM
> > just not yet used by anyone, you would be all fine using that right?
>
> Yes, a new flag sounds like a great idea. What about the definition
> below?
>
> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
> exception that the IOMMU is explicitly defined to be off or bypassed
> when accessing memory addresses supplied to the device by the
> driver. This flag should be set by the guest if offered, but to
> allow for backward-compatibility device implementations allow for it
> to be left unset by the guest. It is an error to set both this flag
> and VIRTIO_F_ACCESS_PLATFORM.
It looks kind of narrow but it's an option.
I wonder how we'll define what's an iommu though.
Another idea is maybe something like virtio-iommu?
> > Is there any justification to doing that beyond someone putting
> > out slow code in the past?
>
> The definition of the ACCESS_PLATFORM flag is generic and captures the
> notion of memory access restrictions for the device. Unfortunately, on
> powerpc pSeries guests it also implies that the IOMMU is turned on
IIUC that's really because on pSeries IOMMU is *always* turned on.
Platform has no way to say what you want it to say
which is bypass the iommu for the specific device.
> even
> though pSeries guests have never used IOMMU for virtio devices. Combined
> with the lack of a way to turn off or bypass the IOMMU for virtio
> devices, this means that existing guests in the field are compelled to
> use the IOMMU even though that never was the case before, and said
> guests having no mechanism to turn it off.
>
> Therefore, we need a new flag to signal the memory access restriction
> present in secure guests which doesn't also imply turning on the IOMMU.
>
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-20 20:57 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Ross Zwisler, Andrew Morton, linuxppc-dev, Kirill A . Shutemov
In-Reply-To: <CAPcyv4iuAPg3HWh5e8-Ud3oCrvp5AoFmjOzf4bbA+VLgR7NLFg@mail.gmail.com>
On Wed, Mar 20, 2019 at 8:34 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Mar 20, 2019 at 1:09 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
> >
> > Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> writes:
> >
> > > Dan Williams <dan.j.williams@intel.com> writes:
> > >
> > >>
> > >>> Now what will be page size used for mapping vmemmap?
> > >>
> > >> That's up to the architecture's vmemmap_populate() implementation.
> > >>
> > >>> Architectures
> > >>> possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
> > >>> device-dax with struct page in the device will have pfn reserve area aligned
> > >>> to PAGE_SIZE with the above example? We can't map that using
> > >>> PMD_SIZE page size?
> > >>
> > >> IIUC, that's a different alignment. Currently that's handled by
> > >> padding the reservation area up to a section (128MB on x86) boundary,
> > >> but I'm working on patches to allow sub-section sized ranges to be
> > >> mapped.
> > >
> > > I am missing something w.r.t code. The below code align that using nd_pfn->align
> > >
> > > if (nd_pfn->mode == PFN_MODE_PMEM) {
> > > unsigned long memmap_size;
> > >
> > > /*
> > > * vmemmap_populate_hugepages() allocates the memmap array in
> > > * HPAGE_SIZE chunks.
> > > */
> > > memmap_size = ALIGN(64 * npfns, HPAGE_SIZE);
> > > offset = ALIGN(start + SZ_8K + memmap_size + dax_label_reserve,
> > > nd_pfn->align) - start;
> > > }
> > >
> > > IIUC that is finding the offset where to put vmemmap start. And that has
> > > to be aligned to the page size with which we may end up mapping vmemmap
> > > area right?
>
> Right, that's the physical offset of where the vmemmap ends, and the
> memory to be mapped begins.
>
> > > Yes we find the npfns by aligning up using PAGES_PER_SECTION. But that
> > > is to compute howmany pfns we should map for this pfn dev right?
> > >
> >
> > Also i guess those 4K assumptions there is wrong?
>
> Yes, I think to support non-4K-PAGE_SIZE systems the 'pfn' metadata
> needs to be revved and the PAGE_SIZE needs to be recorded in the
> info-block.
How often does a system change page-size. Is it fixed or do
environment change it from one boot to the next? I'm thinking through
the behavior of what do when the recorded PAGE_SIZE in the info-block
does not match the current system page size. The simplest option is to
just fail the device and require it to be reconfigured. Is that
acceptable?
^ permalink raw reply
* [RFC PATCH 1/1] KVM: PPC: Report single stepping capability
From: Fabiano Rosas @ 2019-03-20 18:39 UTC (permalink / raw)
To: kvm-ppc; +Cc: kvm, rkrcmar, aik, pbonzini, linuxppc-dev, david
In-Reply-To: <20190320183951.29537-1-farosas@linux.ibm.com>
When calling the KVM_SET_GUEST_DEBUG ioctl, userspace might request
the next instruction to be single stepped via the
KVM_GUESTDBG_SINGLESTEP control bit of the kvm_guest_debug structure.
We currently don't have support for guest single stepping implemented
in Book3S HV.
This patch adds the KVM_CAP_PPC_GUEST_DEBUG_SSTEP capability in order
to inform userspace about the state of single stepping support.
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
arch/powerpc/kvm/powerpc.c | 5 +++++
include/uapi/linux/kvm.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 8885377ec3e0..5ba990b0ec74 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -538,6 +538,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_IMMEDIATE_EXIT:
r = 1;
break;
+ case KVM_CAP_PPC_GUEST_DEBUG_SSTEP:
+#ifdef CONFIG_BOOKE
+ r = 1;
+ break;
+#endif
case KVM_CAP_PPC_PAIRED_SINGLES:
case KVM_CAP_PPC_OSI:
case KVM_CAP_PPC_GET_PVINFO:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 6d4ea4b6c922..33e8a4db867e 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -988,6 +988,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_VM_IPA_SIZE 165
#define KVM_CAP_MANUAL_DIRTY_LOG_PROTECT 166
#define KVM_CAP_HYPERV_CPUID 167
+#define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 168
#ifdef KVM_CAP_IRQ_ROUTING
--
2.20.1
^ permalink raw reply related
* [RFC PATCH 0/1] KVM: PPC: Inform userspace about singlestep support
From: Fabiano Rosas @ 2019-03-20 18:39 UTC (permalink / raw)
To: kvm-ppc; +Cc: kvm, rkrcmar, aik, pbonzini, linuxppc-dev, david
I am looking for a way to inform userspace about the lack of an
implementation in KVM HV for single stepping of instructions
(KVM_GUESTDGB_SINGLESTEP bit from SET_GUEST_DEBUG ioctl).
This will be used by QEMU to decide whether to attempt a call to the
set_guest_debug ioctl (for BookE, KVM PR) or fallback to a QEMU only
implementation (for KVM HV).
QEMU thread:
http://patchwork.ozlabs.org/cover/1049811/
My current proposal is to introduce a ppc-specific capability for
this. However I'm not sure if this would be better as a cap common for
all architectures or even if it should report on all of the possible
set_guest_debug flags to cover for the future.
Please comment. Thanks.
Fabiano Rosas (1):
KVM: PPC: Report single stepping capability
arch/powerpc/kvm/powerpc.c | 5 +++++
include/uapi/linux/kvm.h | 1 +
2 files changed, 6 insertions(+)
--
2.20.1
^ permalink raw reply
* Re: [PATCH v2] kmemleak: skip scanning holes in the .bss section
From: Qian Cai @ 2019-03-20 20:04 UTC (permalink / raw)
To: Catalin Marinas, Michael Ellerman
Cc: linux-kernel, kvm-ppc, linux-mm, akpm, linuxppc-dev
In-Reply-To: <20190320181656.GB38229@arrakis.emea.arm.com>
On Wed, 2019-03-20 at 18:16 +0000, Catalin Marinas wrote:
> I think I have a simpler idea. Kmemleak allows punching holes in
> allocated objects, so just turn the data/bss sections into dedicated
> kmemleak objects. This happens when kmemleak is initialised, before the
> initcalls are invoked. The kvm_free_tmp() would just free the
> corresponding part of the bss.
>
> Patch below, only tested briefly on arm64. Qian, could you give it a try
> on powerpc? Thanks.
It works great so far!
^ permalink raw reply
* Re: [PATCH kernel RFC 2/2] vfio-pci-nvlink2: Implement interconnect isolation
From: Alex Williamson @ 2019-03-20 19:09 UTC (permalink / raw)
To: David Gibson
Cc: Jose Ricardo Ziviani, Alexey Kardashevskiy,
Daniel Henrique Barboza, kvm-ppc, Piotr Jaroszynski,
Leonardo Augusto Guimarães Garcia, linuxppc-dev
In-Reply-To: <20190320043824.GG31018@umbus.fritz.box>
On Wed, 20 Mar 2019 15:38:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Tue, Mar 19, 2019 at 10:36:19AM -0600, Alex Williamson wrote:
> > On Fri, 15 Mar 2019 19:18:35 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> > > The NVIDIA V100 SXM2 GPUs are connected to the CPU via PCIe links and
> > > (on POWER9) NVLinks. In addition to that, GPUs themselves have direct
> > > peer to peer NVLinks in groups of 2 to 4 GPUs. At the moment the POWERNV
> > > platform puts all interconnected GPUs to the same IOMMU group.
> > >
> > > However the user may want to pass individual GPUs to the userspace so
> > > in order to do so we need to put them into separate IOMMU groups and
> > > cut off the interconnects.
> > >
> > > Thankfully V100 GPUs implement an interface to do by programming link
> > > disabling mask to BAR0 of a GPU. Once a link is disabled in a GPU using
> > > this interface, it cannot be re-enabled until the secondary bus reset is
> > > issued to the GPU.
> > >
> > > This defines a reset_done() handler for V100 NVlink2 device which
> > > determines what links need to be disabled. This relies on presence
> > > of the new "ibm,nvlink-peers" device tree property of a GPU telling which
> > > PCI peers it is connected to (which includes NVLink bridges or peer GPUs).
> > >
> > > This does not change the existing behaviour and instead adds
> > > a new "isolate_nvlink" kernel parameter to allow such isolation.
> > >
> > > The alternative approaches would be:
> > >
> > > 1. do this in the system firmware (skiboot) but for that we would need
> > > to tell skiboot via an additional OPAL call whether or not we want this
> > > isolation - skiboot is unaware of IOMMU groups.
> > >
> > > 2. do this in the secondary bus reset handler in the POWERNV platform -
> > > the problem with that is at that point the device is not enabled, i.e.
> > > config space is not restored so we need to enable the device (i.e. MMIO
> > > bit in CMD register + program valid address to BAR0) in order to disable
> > > links and then perhaps undo all this initialization to bring the device
> > > back to the state where pci_try_reset_function() expects it to be.
> >
> > The trouble seems to be that this approach only maintains the isolation
> > exposed by the IOMMU group when vfio-pci is the active driver for the
> > device. IOMMU groups can be used by any driver and the IOMMU core is
> > incorporating groups in various ways.
>
> I don't think that reasoning is quite right. An IOMMU group doesn't
> necessarily represent devices which *are* isolated, just devices which
> *can be* isolated. There are plenty of instances when we don't need
> to isolate devices in different IOMMU groups: passing both groups to
> the same guest or userspace VFIO driver for example, or indeed when
> both groups are owned by regular host kernel drivers.
>
> In at least some of those cases we also don't want to isolate the
> devices when we don't have to, usually for performance reasons.
I see IOMMU groups as representing the current isolation of the device,
not just the possible isolation. If there are ways to break down that
isolation then ideally the group would be updated to reflect it. The
ACS disable patches seem to support this, at boot time we can choose to
disable ACS at certain points in the topology to favor peer-to-peer
performance over isolation. This is then reflected in the group
composition, because even though ACS *can be* enabled at the given
isolation points, it's intentionally not with this option. Whether or
not a given user who owns multiple devices needs that isolation is
really beside the point, the user can choose to connect groups via IOMMU
mappings or reconfigure the system to disable ACS and potentially more
direct routing. The IOMMU groups are still accurately reflecting the
topology and IOMMU based isolation.
> > So, if there's a device specific
> > way to configure the isolation reported in the group, which requires
> > some sort of active management against things like secondary bus
> > resets, then I think we need to manage it above the attached endpoint
> > driver.
>
> The problem is that above the endpoint driver, we don't actually have
> enough information about what should be isolated. For VFIO we want to
> isolate things if they're in different containers, for most regular
> host kernel drivers we don't need to isolate at all (although we might
> as well when it doesn't have a cost).
This idea that we only want to isolate things if they're in different
containers is bogus, imo. There are performance reasons why we might
not want things isolated, but there are also address space reasons why
we do. If there are direct routes between devices, the user needs to
be aware of the IOVA pollution, if we maintain singleton groups, they
don't. Granted we don't really account for this well in most
userspaces and fumble through it by luck of the address space layout
and lack of devices really attempting peer to peer access.
For in-kernel users, we're still theoretically trying to isolate
devices such that they have restricted access to only the resources
they need. Disabling things like ACS in the topology reduces that
isolation. AFAICT, most users don't really care about that degree of
isolation, so they run with iommu=pt for native driver performance
while still having the IOMMU available for isolation use cases running
in parallel. We don't currently have support for on-demand enabling
isolation.
> The host side nVidia GPGPU
> drivers also won't want to isolate the (host owned) NVLink devices
> from each other, since they'll want to use the fast interconnects
This falls into the same mixed use case scenario above where we don't
really have a good solution today. Things like ACS are dynamically
configurable, but we don't expose any interfaces to let drivers or
users change it (aside from setpci, which we don't account for
dynamically). We assume a simplistic model where if you want IOMMU,
then you must also want the maximum configurable isolation.
Dynamically changing routing is not necessarily the most foolproof
thing either with potentially in-flight transactions and existing DMA
mappings, which is why I've suggested a couple times that perhaps we
could do a software hot-unplug of a sub-hierarchy, muck with isolation
at the remaining node, then re-discover the removed devices.
Of course when we bring NVIDIA into the mix, I have little sympathy
that the NVLink interfaces are all proprietary and we have no idea how
to make those dynamic changes or discover the interconnected-ness of a
device. Thanks,
Alex
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox