* [PATCH v8 13/18] ARM: STi: DT: STiH407: Add sti-sasg-codec dt node
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
This patch adds the dt node for the internal audio
codec IP.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih407-family.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 45cab30..d1258d5 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -873,5 +873,12 @@
<&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>,
<&clk_s_c0_flexgen CLK_EXT2F_A9>;
};
+
+ sti_sasg_codec: sti-sasg-codec {
+ compatible = "st,stih407-sas-codec";
+ #sound-dai-cells = <1>;
+ status = "disabled";
+ st,syscfg = <&syscfg_core>;
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH v8 14/18] ARM: STi: DT: STiH407: Add uniperif player dt nodes
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
This patch adds the DT nodes for the uniperif player
IP blocks found on STiH407 family silicon.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih407-family.dtsi | 76 +++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index d1258d5..d263c96 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -880,5 +880,81 @@
status = "disabled";
st,syscfg = <&syscfg_core>;
};
+
+ sti_uni_player0: sti-uni-player@0 {
+ compatible = "st,sti-uni-player";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ clocks = <&clk_s_d0_flexgen CLK_PCM_0>;
+ assigned-clocks = <&clk_s_d0_quadfs 0>, <&clk_s_d0_flexgen CLK_PCM_0>;
+ assigned-clock-parents = <0>, <&clk_s_d0_quadfs 0>;
+ assigned-clock-rates = <50000000>;
+ reg = <0x8D80000 0x158>;
+ interrupts = <GIC_SPI 84 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 2 0 1>;
+ dai-name = "Uni Player #0 (HDMI)";
+ dma-names = "tx";
+ st,uniperiph-id = <0>;
+ st,version = <5>;
+ st,mode = "HDMI";
+ };
+
+ sti_uni_player1: sti-uni-player@1 {
+ compatible = "st,sti-uni-player";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ clocks = <&clk_s_d0_flexgen CLK_PCM_1>;
+ assigned-clocks = <&clk_s_d0_quadfs 1>, <&clk_s_d0_flexgen CLK_PCM_1>;
+ assigned-clock-parents = <0>, <&clk_s_d0_quadfs 1>;
+ assigned-clock-rates = <50000000>;
+ reg = <0x8D81000 0x158>;
+ interrupts = <GIC_SPI 85 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 3 0 1>;
+ dai-name = "Uni Player #1 (PIO)";
+ dma-names = "tx";
+ st,uniperiph-id = <1>;
+ st,version = <5>;
+ st,mode = "PCM";
+ };
+
+ sti_uni_player2: sti-uni-player@2 {
+ compatible = "st,sti-uni-player";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ clocks = <&clk_s_d0_flexgen CLK_PCM_2>;
+ assigned-clocks = <&clk_s_d0_quadfs 2>, <&clk_s_d0_flexgen CLK_PCM_2>;
+ assigned-clock-parents = <0>, <&clk_s_d0_quadfs 2>;
+ assigned-clock-rates = <50000000>;
+ reg = <0x8D82000 0x158>;
+ interrupts = <GIC_SPI 86 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 4 0 1>;
+ dai-name = "Uni Player #1 (DAC)";
+ dma-names = "tx";
+ st,uniperiph-id = <2>;
+ st,version = <5>;
+ st,mode = "PCM";
+ };
+
+ sti_uni_player3: sti-uni-player@3 {
+ compatible = "st,sti-uni-player";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ clocks = <&clk_s_d0_flexgen CLK_SPDIFF>;
+ assigned-clocks = <&clk_s_d0_quadfs 3>, <&clk_s_d0_flexgen CLK_SPDIFF>;
+ assigned-clock-parents = <0>, <&clk_s_d0_quadfs 3>;
+ assigned-clock-rates = <50000000>;
+ reg = <0x8D85000 0x158>;
+ interrupts = <GIC_SPI 89 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 7 0 1>;
+ dma-names = "tx";
+ dai-name = "Uni Player #1 (PIO)";
+ st,uniperiph-id = <3>;
+ st,version = <5>;
+ st,mode = "SPDIF";
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH v8 15/18] ARM: STi: DT: STiH407: Add uniperif reader dt nodes
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
This patch adds the DT node for the uniperif reader
IP block found on STiH407 family silicon.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih407-family.dtsi | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index d263c96..bdddf2c 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -956,5 +956,31 @@
st,version = <5>;
st,mode = "SPDIF";
};
+
+ sti_uni_reader0: sti-uni-reader@0 {
+ compatible = "st,sti-uni-reader";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ reg = <0x8D83000 0x158>;
+ interrupts = <GIC_SPI 87 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 5 0 1>;
+ dma-names = "rx";
+ dai-name = "Uni Reader #0 (PCM IN)";
+ st,version = <3>;
+ };
+
+ sti_uni_reader1: sti-uni-reader@1 {
+ compatible = "st,sti-uni-reader";
+ status = "disabled";
+ #sound-dai-cells = <0>;
+ st,syscfg = <&syscfg_core>;
+ reg = <0x8D84000 0x158>;
+ interrupts = <GIC_SPI 88 IRQ_TYPE_NONE>;
+ dmas = <&fdma0 6 0 1>;
+ dma-names = "rx";
+ dai-name = "Uni Reader #1 (HDMI RX)";
+ st,version = <3>;
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH v8 16/18] ARM: DT: STi: stihxxx-b2120: Add DT nodes for STi audio card
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
This patch enables the uniperif players 2 & 3 for b2120 boards
and also adds the "simple-audio-card" device node to interconnect
the SoC sound device and the codec.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stihxxx-b2120.dtsi | 45 ++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
index 722c63f..1f64bb6 100644
--- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
+++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi
@@ -131,5 +131,50 @@
dvb-card = <STV0367_TDA18212_NIMA_1>;
};
};
+
+ sti_uni_player2: sti-uni-player@2 {
+ status = "okay";
+ };
+
+ sti_uni_player3: sti-uni-player@3 {
+ status = "okay";
+ };
+
+ sti_sasg_codec: sti-sasg-codec {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_spdif_out>;
+ };
+
+ sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,name = "sti audio card";
+ status = "okay";
+
+ simple-audio-card,dai-link@0 {
+ /* DAC */
+ format = "i2s";
+ mclk-fs = <256>;
+ cpu {
+ sound-dai = <&sti_uni_player2>;
+ };
+
+ codec {
+ sound-dai = <&sti_sasg_codec 1>;
+ };
+ };
+ simple-audio-card,dai-link@1 {
+ /* SPDIF */
+ format = "left_j";
+ mclk-fs = <128>;
+ cpu {
+ sound-dai = <&sti_uni_player3>;
+ };
+
+ codec {
+ sound-dai = <&sti_sasg_codec 0>;
+ };
+ };
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH v8 17/18] drm/virtio: kconfig: Fix recursive dependency.
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, linux-remoteproc, dri-devel, virtualization,
peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
[..]
drivers/video/fbdev/Kconfig:5:error: recursive dependency detected!
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:42: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/Kconfig:36: symbol DRM_KMS_HELPER is selected by DRM_VIRTIO_GPU
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/gpu/drm/virtio/Kconfig:1: symbol DRM_VIRTIO_GPU depends on VIRTIO
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/virtio/Kconfig:1: symbol VIRTIO is selected by REMOTEPROC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/remoteproc/Kconfig:4: symbol REMOTEPROC is selected by ST_SLIM_REMOTEPROC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/remoteproc/Kconfig:103: symbol ST_SLIM_REMOTEPROC is selected by ST_FDMA
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/dma/Kconfig:440: symbol ST_FDMA depends on DMADEVICES
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SND_SOC_SH4_SIU
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
sound/soc/sh/Kconfig:29: symbol SND_SOC_SH4_SIU is selected by SND_SIU_MIGOR
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
sound/soc/sh/Kconfig:64: symbol SND_SIU_MIGOR depends on I2C
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:378: symbol FB_CYBER2000_DDC depends on FB_CYBER2000
For a resolution refer to Documentation/kbuild/kconfig-language.txt
subsection "Kconfig recursive dependency limitations"
drivers/video/fbdev/Kconfig:366: symbol FB_CYBER2000 depends on FB
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
drivers/gpu/drm/virtio/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index e1afc3d..90357d9 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -1,6 +1,7 @@
config DRM_VIRTIO_GPU
tristate "Virtio GPU driver"
- depends on DRM && VIRTIO
+ depends on DRM
+ select VIRTIO
select DRM_KMS_HELPER
select DRM_TTM
help
--
1.9.1
^ permalink raw reply related
* [PATCH v8 18/18] drm/virtio: kconfig: Fixup white space.
From: Peter Griffin @ 2016-08-26 14:56 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, kernel, vinod.koul,
patrice.chotard, dan.j.williams, airlied, kraxel, ohad,
bjorn.andersson
Cc: devicetree, linux-remoteproc, dri-devel, virtualization,
peter.griffin, dmaengine, lee.jones
In-Reply-To: <1472223413-7254-1-git-send-email-peter.griffin@linaro.org>
Use tabs instead of spaces.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
drivers/gpu/drm/virtio/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 90357d9..2d83932 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -2,10 +2,10 @@ config DRM_VIRTIO_GPU
tristate "Virtio GPU driver"
depends on DRM
select VIRTIO
- select DRM_KMS_HELPER
- select DRM_TTM
+ select DRM_KMS_HELPER
+ select DRM_TTM
help
This is the virtual GPU driver for virtio. It can be used with
- QEMU based VMMs (like KVM or Xen).
+ QEMU based VMMs (like KVM or Xen).
If unsure say M.
--
1.9.1
^ permalink raw reply related
* [PATCH v6 0/6] Support calling functions on dedicated physical cpu
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
Some hardware (e.g. Dell Studio laptops) require special functions to
be called on physical cpu 0 in order to avoid occasional hangs. When
running as dom0 under Xen this could be achieved only via special boot
parameters (vcpu pinning) limiting the hypervisor in it's scheduling
decisions.
This patch series is adding a generic function to be able to temporarily
pin a (virtual) cpu to a dedicated physical cpu for executing above
mentioned functions on that specific cpu. The drivers (dcdbas and i8k)
requiring this functionality are modified accordingly.
Changes in V6:
- rebased to kernel 4.8-rc4
Changes in V5:
- patch 3: rename and reshuffle parameters of smp_call_on_cpu() as requested
by Peter Zijlstra
- patch 3: test target cpu to be online as requested by Peter Zijlstra
- patch 4: less wordy messages as requested by David Vrabel
Changes in V4:
- move patches 5 and 6 further up in the series
- patch 2 (was 5): WARN_ONCE in case platform doesn't support pinning
as requested by Peter Zijlstra
- patch 3 (was 2): change return value in case of illegal cpu as
requested by Peter Zijlstra
- patch 3 (was 2): make pinning of vcpu an option as suggested by
Peter Zijlstra
- patches 5 and 6 (were 3 and 4): add call to get_online_cpus()
Changes in V3:
- use get_cpu()/put_cpu() as suggested by David Vrabel
Changes in V2:
- instead of manipulating the allowed set of cpus use cpu specific
workqueue as requested by Peter Zijlstra
- add include/linux/hypervisor.h to hide architecture specific stuff
from generic kernel code
Juergen Gross (6):
xen: sync xen header
virt, sched: add generic vcpu pinning support
smp: add function to execute a function synchronously on a cpu
xen: add xen_pin_vcpu() to support calling functions on a dedicated
pcpu
dcdbas: make use of smp_call_on_cpu()
hwmon: use smp_call_on_cpu() for dell-smm i8k
MAINTAINERS | 1 +
arch/x86/include/asm/hypervisor.h | 4 ++
arch/x86/kernel/cpu/hypervisor.c | 11 +++++
arch/x86/xen/enlighten.c | 40 +++++++++++++++
drivers/firmware/dcdbas.c | 51 +++++++++----------
drivers/hwmon/dell-smm-hwmon.c | 36 ++++++++------
include/linux/hypervisor.h | 17 +++++++
include/linux/smp.h | 3 ++
include/xen/interface/sched.h | 100 +++++++++++++++++++++++++++++++-------
kernel/smp.c | 51 +++++++++++++++++++
kernel/up.c | 18 +++++++
11 files changed, 273 insertions(+), 59 deletions(-)
create mode 100644 include/linux/hypervisor.h
--
2.6.6
^ permalink raw reply
* [PATCH v6 1/6] xen: sync xen header
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
Import the actual version of include/xen/interface/sched.h from Xen.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
---
include/xen/interface/sched.h | 100 ++++++++++++++++++++++++++++++++++--------
1 file changed, 82 insertions(+), 18 deletions(-)
diff --git a/include/xen/interface/sched.h b/include/xen/interface/sched.h
index f184909..a4c4d73 100644
--- a/include/xen/interface/sched.h
+++ b/include/xen/interface/sched.h
@@ -3,6 +3,24 @@
*
* Scheduler state interactions
*
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
* Copyright (c) 2005, Keir Fraser <keir@xensource.com>
*/
@@ -12,18 +30,30 @@
#include <xen/interface/event_channel.h>
/*
+ * Guest Scheduler Operations
+ *
+ * The SCHEDOP interface provides mechanisms for a guest to interact
+ * with the scheduler, including yield, blocking and shutting itself
+ * down.
+ */
+
+/*
* The prototype for this hypercall is:
- * long sched_op_new(int cmd, void *arg)
+ * long HYPERVISOR_sched_op(enum sched_op cmd, void *arg, ...)
+ *
* @cmd == SCHEDOP_??? (scheduler operation).
* @arg == Operation-specific extra argument(s), as described below.
+ * ... == Additional Operation-specific extra arguments, described below.
*
- * **NOTE**:
- * Versions of Xen prior to 3.0.2 provide only the following legacy version
+ * Versions of Xen prior to 3.0.2 provided only the following legacy version
* of this hypercall, supporting only the commands yield, block and shutdown:
* long sched_op(int cmd, unsigned long arg)
* @cmd == SCHEDOP_??? (scheduler operation).
* @arg == 0 (SCHEDOP_yield and SCHEDOP_block)
* == SHUTDOWN_* code (SCHEDOP_shutdown)
+ *
+ * This legacy version is available to new guests as:
+ * long HYPERVISOR_sched_op_compat(enum sched_op cmd, unsigned long arg)
*/
/*
@@ -44,12 +74,17 @@
/*
* Halt execution of this domain (all VCPUs) and notify the system controller.
* @arg == pointer to sched_shutdown structure.
+ *
+ * If the sched_shutdown_t reason is SHUTDOWN_suspend then
+ * x86 PV guests must also set RDX (EDX for 32-bit guests) to the MFN
+ * of the guest's start info page. RDX/EDX is the third hypercall
+ * argument.
+ *
+ * In addition, which reason is SHUTDOWN_suspend this hypercall
+ * returns 1 if suspend was cancelled or the domain was merely
+ * checkpointed, and 0 if it is resuming in a new domain.
*/
#define SCHEDOP_shutdown 2
-struct sched_shutdown {
- unsigned int reason; /* SHUTDOWN_* */
-};
-DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
/*
* Poll a set of event-channel ports. Return when one or more are pending. An
@@ -57,12 +92,6 @@ DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
* @arg == pointer to sched_poll structure.
*/
#define SCHEDOP_poll 3
-struct sched_poll {
- GUEST_HANDLE(evtchn_port_t) ports;
- unsigned int nr_ports;
- uint64_t timeout;
-};
-DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
/*
* Declare a shutdown for another domain. The main use of this function is
@@ -71,15 +100,11 @@ DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
* @arg == pointer to sched_remote_shutdown structure.
*/
#define SCHEDOP_remote_shutdown 4
-struct sched_remote_shutdown {
- domid_t domain_id; /* Remote domain ID */
- unsigned int reason; /* SHUTDOWN_xxx reason */
-};
/*
* Latch a shutdown code, so that when the domain later shuts down it
* reports this code to the control tools.
- * @arg == as for SCHEDOP_shutdown.
+ * @arg == sched_shutdown, as for SCHEDOP_shutdown.
*/
#define SCHEDOP_shutdown_code 5
@@ -92,10 +117,47 @@ struct sched_remote_shutdown {
* With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
*/
#define SCHEDOP_watchdog 6
+
+/*
+ * Override the current vcpu affinity by pinning it to one physical cpu or
+ * undo this override restoring the previous affinity.
+ * @arg == pointer to sched_pin_override structure.
+ *
+ * A negative pcpu value will undo a previous pin override and restore the
+ * previous cpu affinity.
+ * This call is allowed for the hardware domain only and requires the cpu
+ * to be part of the domain's cpupool.
+ */
+#define SCHEDOP_pin_override 7
+
+struct sched_shutdown {
+ unsigned int reason; /* SHUTDOWN_* => shutdown reason */
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_shutdown);
+
+struct sched_poll {
+ GUEST_HANDLE(evtchn_port_t) ports;
+ unsigned int nr_ports;
+ uint64_t timeout;
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_poll);
+
+struct sched_remote_shutdown {
+ domid_t domain_id; /* Remote domain ID */
+ unsigned int reason; /* SHUTDOWN_* => shutdown reason */
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_remote_shutdown);
+
struct sched_watchdog {
uint32_t id; /* watchdog ID */
uint32_t timeout; /* timeout */
};
+DEFINE_GUEST_HANDLE_STRUCT(sched_watchdog);
+
+struct sched_pin_override {
+ int32_t pcpu;
+};
+DEFINE_GUEST_HANDLE_STRUCT(sched_pin_override);
/*
* Reason codes for SCHEDOP_shutdown. These may be interpreted by control
@@ -107,6 +169,7 @@ struct sched_watchdog {
#define SHUTDOWN_suspend 2 /* Clean up, save suspend info, kill. */
#define SHUTDOWN_crash 3 /* Tell controller we've crashed. */
#define SHUTDOWN_watchdog 4 /* Restart because watchdog time expired. */
+
/*
* Domain asked to perform 'soft reset' for it. The expected behavior is to
* reset internal Xen state for the domain returning it to the point where it
@@ -115,5 +178,6 @@ struct sched_watchdog {
* interfaces again.
*/
#define SHUTDOWN_soft_reset 5
+#define SHUTDOWN_MAX 5 /* Maximum valid shutdown reason. */
#endif /* __XEN_PUBLIC_SCHED_H__ */
--
2.6.6
^ permalink raw reply related
* [PATCH v6 2/6] virt, sched: add generic vcpu pinning support
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
Add generic virtualization support for pinning the current vcpu to a
specified physical cpu. As this operation isn't performance critical
(a very limited set of operations like BIOS calls and SMIs is expected
to need this) just add a hypervisor specific indirection.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: move this patch some places up in the series
WARN_ONCE in case platform doesn't support pinning as requested by
Peter Zijlstra
V3: use getc_cpu()/put_cpu() as suggested by David Vrabel
V2: adapt to using workqueues
add include/linux/hypervisor.h to hide architecture specific stuff
from generic kernel code
In case paravirt maintainers don't want to be responsible for
include/linux/hypervisor.h I could take it.
---
MAINTAINERS | 1 +
arch/x86/include/asm/hypervisor.h | 4 ++++
arch/x86/kernel/cpu/hypervisor.c | 11 +++++++++++
include/linux/hypervisor.h | 17 +++++++++++++++++
kernel/smp.c | 1 +
kernel/up.c | 1 +
6 files changed, 35 insertions(+)
create mode 100644 include/linux/hypervisor.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 71aa5da..93bcac5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8844,6 +8844,7 @@ S: Supported
F: Documentation/virtual/paravirt_ops.txt
F: arch/*/kernel/paravirt*
F: arch/*/include/asm/paravirt.h
+F: include/linux/hypervisor.h
PARIDE DRIVERS FOR PARALLEL PORT IDE DEVICES
M: Tim Waugh <tim@cyberelk.net>
diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 055ea99..67942b6 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -43,6 +43,9 @@ struct hypervisor_x86 {
/* X2APIC detection (run once per boot) */
bool (*x2apic_available)(void);
+
+ /* pin current vcpu to specified physical cpu (run rarely) */
+ void (*pin_vcpu)(int);
};
extern const struct hypervisor_x86 *x86_hyper;
@@ -56,6 +59,7 @@ extern const struct hypervisor_x86 x86_hyper_kvm;
extern void init_hypervisor(struct cpuinfo_x86 *c);
extern void init_hypervisor_platform(void);
extern bool hypervisor_x2apic_available(void);
+extern void hypervisor_pin_vcpu(int cpu);
#else
static inline void init_hypervisor(struct cpuinfo_x86 *c) { }
static inline void init_hypervisor_platform(void) { }
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 27e4665..35691a6 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -86,3 +86,14 @@ bool __init hypervisor_x2apic_available(void)
x86_hyper->x2apic_available &&
x86_hyper->x2apic_available();
}
+
+void hypervisor_pin_vcpu(int cpu)
+{
+ if (!x86_hyper)
+ return;
+
+ if (x86_hyper->pin_vcpu)
+ x86_hyper->pin_vcpu(cpu);
+ else
+ WARN_ONCE(1, "vcpu pinning requested but not supported!\n");
+}
diff --git a/include/linux/hypervisor.h b/include/linux/hypervisor.h
new file mode 100644
index 0000000..3fa5ef2
--- /dev/null
+++ b/include/linux/hypervisor.h
@@ -0,0 +1,17 @@
+#ifndef __LINUX_HYPEVISOR_H
+#define __LINUX_HYPEVISOR_H
+
+/*
+ * Generic Hypervisor support
+ * Juergen Gross <jgross@suse.com>
+ */
+
+#ifdef CONFIG_HYPERVISOR_GUEST
+#include <asm/hypervisor.h>
+#else
+static inline void hypervisor_pin_vcpu(int cpu)
+{
+}
+#endif
+
+#endif /* __LINUX_HYPEVISOR_H */
diff --git a/kernel/smp.c b/kernel/smp.c
index 3aa642d..4274ca5 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -14,6 +14,7 @@
#include <linux/smp.h>
#include <linux/cpu.h>
#include <linux/sched.h>
+#include <linux/hypervisor.h>
#include "smpboot.h"
diff --git a/kernel/up.c b/kernel/up.c
index 1760bf3..3ccee2b 100644
--- a/kernel/up.c
+++ b/kernel/up.c
@@ -6,6 +6,7 @@
#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/smp.h>
+#include <linux/hypervisor.h>
int smp_call_function_single(int cpu, void (*func) (void *info), void *info,
int wait)
--
2.6.6
^ permalink raw reply related
* [PATCH v6 3/6] smp: add function to execute a function synchronously on a cpu
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
On some hardware models (e.g. Dell Studio 1555 laptop) some hardware
related functions (e.g. SMIs) are to be executed on physical cpu 0
only. Instead of open coding such a functionality multiple times in
the kernel add a service function for this purpose. This will enable
the possibility to take special measures in virtualized environments
like Xen, too.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5: rename and reshuffle parameters of smp_call_on_cpu() as requested by
Peter Zijlstra
test target cpu to be online as requested by Peter Zijlstra
V4: change return value in case of illegal cpu as requested by Peter Zijlstra
make pinning of vcpu an option as suggested by Peter Zijlstra
V2: instead of manipulating the allowed set of cpus use cpu specific
workqueue as requested by Peter Zijlstra
---
include/linux/smp.h | 3 +++
kernel/smp.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
kernel/up.c | 17 +++++++++++++++++
3 files changed, 70 insertions(+)
diff --git a/include/linux/smp.h b/include/linux/smp.h
index eccae469..8e0cb7a 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -196,6 +196,9 @@ extern void arch_enable_nonboot_cpus_end(void);
void smp_setup_processor_id(void);
+int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par,
+ bool phys);
+
/* SMP core functions */
int smpcfd_prepare_cpu(unsigned int cpu);
int smpcfd_dead_cpu(unsigned int cpu);
diff --git a/kernel/smp.c b/kernel/smp.c
index 4274ca5..f4f6137 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -725,3 +725,53 @@ void wake_up_all_idle_cpus(void)
preempt_enable();
}
EXPORT_SYMBOL_GPL(wake_up_all_idle_cpus);
+
+/**
+ * smp_call_on_cpu - Call a function on a specific cpu
+ *
+ * Used to call a function on a specific cpu and wait for it to return.
+ * Optionally make sure the call is done on a specified physical cpu via vcpu
+ * pinning in order to support virtualized environments.
+ */
+struct smp_call_on_cpu_struct {
+ struct work_struct work;
+ struct completion done;
+ int (*func)(void *);
+ void *data;
+ int ret;
+ int cpu;
+};
+
+static void smp_call_on_cpu_callback(struct work_struct *work)
+{
+ struct smp_call_on_cpu_struct *sscs;
+
+ sscs = container_of(work, struct smp_call_on_cpu_struct, work);
+ if (sscs->cpu >= 0)
+ hypervisor_pin_vcpu(sscs->cpu);
+ sscs->ret = sscs->func(sscs->data);
+ if (sscs->cpu >= 0)
+ hypervisor_pin_vcpu(-1);
+
+ complete(&sscs->done);
+}
+
+int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par, bool phys)
+{
+ struct smp_call_on_cpu_struct sscs = {
+ .work = __WORK_INITIALIZER(sscs.work, smp_call_on_cpu_callback),
+ .done = COMPLETION_INITIALIZER_ONSTACK(sscs.done),
+ .func = func,
+ .data = par,
+ .cpu = phys ? cpu : -1,
+ };
+
+ if (cpu >= nr_cpu_ids || !cpu_online(cpu))
+ return -ENXIO;
+
+ queue_work_on(cpu, system_wq, &sscs.work);
+ wait_for_completion(&sscs.done);
+
+ return sscs.ret;
+}
+EXPORT_SYMBOL_GPL(smp_call_on_cpu);
diff --git a/kernel/up.c b/kernel/up.c
index 3ccee2b..ee81ac9 100644
--- a/kernel/up.c
+++ b/kernel/up.c
@@ -83,3 +83,20 @@ void on_each_cpu_cond(bool (*cond_func)(int cpu, void *info),
preempt_enable();
}
EXPORT_SYMBOL(on_each_cpu_cond);
+
+int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par, bool phys)
+{
+ int ret;
+
+ if (cpu != 0)
+ return -ENXIO;
+
+ if (phys)
+ hypervisor_pin_vcpu(0);
+ ret = func(par);
+ if (phys)
+ hypervisor_pin_vcpu(-1);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(smp_call_on_cpu);
--
2.6.6
^ permalink raw reply related
* [PATCH v6 4/6] xen: add xen_pin_vcpu() to support calling functions on a dedicated pcpu
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
Some hardware models (e.g. Dell Studio 1555 laptops) require calls to
the firmware to be issued on cpu 0 only. As Dom0 might have to use
these calls, add xen_pin_vcpu() to achieve this functionality.
In case either the domain doesn't have the privilege to make the
related hypercall or the hypervisor isn't supporting it, issue a
warning once and disable further pinning attempts.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
---
V5: less wordy messages as requested by David Vrabel
---
arch/x86/xen/enlighten.c | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index b86ebb1..bc9aaba 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1925,6 +1925,45 @@ static void xen_set_cpu_features(struct cpuinfo_x86 *c)
}
}
+static void xen_pin_vcpu(int cpu)
+{
+ static bool disable_pinning;
+ struct sched_pin_override pin_override;
+ int ret;
+
+ if (disable_pinning)
+ return;
+
+ pin_override.pcpu = cpu;
+ ret = HYPERVISOR_sched_op(SCHEDOP_pin_override, &pin_override);
+
+ /* Ignore errors when removing override. */
+ if (cpu < 0)
+ return;
+
+ switch (ret) {
+ case -ENOSYS:
+ pr_warn("Unable to pin on physical cpu %d. In case of problems consider vcpu pinning.\n",
+ cpu);
+ disable_pinning = true;
+ break;
+ case -EPERM:
+ WARN(1, "Trying to pin vcpu without having privilege to do so\n");
+ disable_pinning = true;
+ break;
+ case -EINVAL:
+ case -EBUSY:
+ pr_warn("Physical cpu %d not available for pinning. Check Xen cpu configuration.\n",
+ cpu);
+ break;
+ case 0:
+ break;
+ default:
+ WARN(1, "rc %d while trying to pin vcpu\n", ret);
+ disable_pinning = true;
+ }
+}
+
const struct hypervisor_x86 x86_hyper_xen = {
.name = "Xen",
.detect = xen_platform,
@@ -1933,6 +1972,7 @@ const struct hypervisor_x86 x86_hyper_xen = {
#endif
.x2apic_available = xen_x2apic_para_available,
.set_cpu_features = xen_set_cpu_features,
+ .pin_vcpu = xen_pin_vcpu,
};
EXPORT_SYMBOL(x86_hyper_xen);
--
2.6.6
^ permalink raw reply related
* [PATCH v6 5/6] dcdbas: make use of smp_call_on_cpu()
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
Use smp_call_on_cpu() to raise SMI on cpu 0.
Make call secure by adding get_online_cpus() to avoid e.g. suspend
resume cycles in between.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: add call to get_online_cpus()
---
drivers/firmware/dcdbas.c | 51 ++++++++++++++++++++++++-----------------------
1 file changed, 26 insertions(+), 25 deletions(-)
diff --git a/drivers/firmware/dcdbas.c b/drivers/firmware/dcdbas.c
index 829eec8..2fe1a13 100644
--- a/drivers/firmware/dcdbas.c
+++ b/drivers/firmware/dcdbas.c
@@ -23,6 +23,7 @@
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/errno.h>
+#include <linux/cpu.h>
#include <linux/gfp.h>
#include <linux/init.h>
#include <linux/kernel.h>
@@ -238,33 +239,14 @@ static ssize_t host_control_on_shutdown_store(struct device *dev,
return count;
}
-/**
- * dcdbas_smi_request: generate SMI request
- *
- * Called with smi_data_lock.
- */
-int dcdbas_smi_request(struct smi_cmd *smi_cmd)
+static int raise_smi(void *par)
{
- cpumask_var_t old_mask;
- int ret = 0;
+ struct smi_cmd *smi_cmd = par;
- if (smi_cmd->magic != SMI_CMD_MAGIC) {
- dev_info(&dcdbas_pdev->dev, "%s: invalid magic value\n",
- __func__);
- return -EBADR;
- }
-
- /* SMI requires CPU 0 */
- if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
- return -ENOMEM;
-
- cpumask_copy(old_mask, ¤t->cpus_allowed);
- set_cpus_allowed_ptr(current, cpumask_of(0));
if (smp_processor_id() != 0) {
dev_dbg(&dcdbas_pdev->dev, "%s: failed to get CPU 0\n",
__func__);
- ret = -EBUSY;
- goto out;
+ return -EBUSY;
}
/* generate SMI */
@@ -280,9 +262,28 @@ int dcdbas_smi_request(struct smi_cmd *smi_cmd)
: "memory"
);
-out:
- set_cpus_allowed_ptr(current, old_mask);
- free_cpumask_var(old_mask);
+ return 0;
+}
+/**
+ * dcdbas_smi_request: generate SMI request
+ *
+ * Called with smi_data_lock.
+ */
+int dcdbas_smi_request(struct smi_cmd *smi_cmd)
+{
+ int ret;
+
+ if (smi_cmd->magic != SMI_CMD_MAGIC) {
+ dev_info(&dcdbas_pdev->dev, "%s: invalid magic value\n",
+ __func__);
+ return -EBADR;
+ }
+
+ /* SMI requires CPU 0 */
+ get_online_cpus();
+ ret = smp_call_on_cpu(0, raise_smi, smi_cmd, true);
+ put_online_cpus();
+
return ret;
}
--
2.6.6
^ permalink raw reply related
* [PATCH v6 6/6] hwmon: use smp_call_on_cpu() for dell-smm i8k
From: Juergen Gross @ 2016-08-29 6:48 UTC (permalink / raw)
To: linux-kernel, xen-devel
Cc: Juergen Gross, jeremy, jdelvare, peterz, hpa, akataria, x86,
virtualization, chrisw, mingo, david.vrabel, Douglas_Warzecha,
pali.rohar, boris.ostrovsky, tglx, linux
In-Reply-To: <1472453327-19050-1-git-send-email-jgross@suse.com>
Use the smp_call_on_cpu() function to call system management
mode on cpu 0.
Make call secure by adding get_online_cpus() to avoid e.g. suspend
resume cycles in between.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4: add call to get_online_cpus()
---
drivers/hwmon/dell-smm-hwmon.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index acf9c03..34704b0 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -21,6 +21,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/cpu.h>
#include <linux/delay.h>
#include <linux/module.h>
#include <linux/types.h>
@@ -36,6 +37,7 @@
#include <linux/io.h>
#include <linux/sched.h>
#include <linux/ctype.h>
+#include <linux/smp.h>
#include <linux/i8k.h>
@@ -134,11 +136,11 @@ static inline const char *i8k_get_dmi_data(int field)
/*
* Call the System Management Mode BIOS. Code provided by Jonathan Buzzard.
*/
-static int i8k_smm(struct smm_regs *regs)
+static int i8k_smm_func(void *par)
{
int rc;
+ struct smm_regs *regs = par;
int eax = regs->eax;
- cpumask_var_t old_mask;
#ifdef DEBUG
int ebx = regs->ebx;
@@ -149,16 +151,8 @@ static int i8k_smm(struct smm_regs *regs)
#endif
/* SMM requires CPU 0 */
- if (!alloc_cpumask_var(&old_mask, GFP_KERNEL))
- return -ENOMEM;
- cpumask_copy(old_mask, ¤t->cpus_allowed);
- rc = set_cpus_allowed_ptr(current, cpumask_of(0));
- if (rc)
- goto out;
- if (smp_processor_id() != 0) {
- rc = -EBUSY;
- goto out;
- }
+ if (smp_processor_id() != 0)
+ return -EBUSY;
#if defined(CONFIG_X86_64)
asm volatile("pushq %%rax\n\t"
@@ -216,10 +210,6 @@ static int i8k_smm(struct smm_regs *regs)
if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax)
rc = -EINVAL;
-out:
- set_cpus_allowed_ptr(current, old_mask);
- free_cpumask_var(old_mask);
-
#ifdef DEBUG
rettime = ktime_get();
delta = ktime_sub(rettime, calltime);
@@ -232,6 +222,20 @@ out:
}
/*
+ * Call the System Management Mode BIOS.
+ */
+static int i8k_smm(struct smm_regs *regs)
+{
+ int ret;
+
+ get_online_cpus();
+ ret = smp_call_on_cpu(0, i8k_smm_func, regs, true);
+ put_online_cpus();
+
+ return ret;
+}
+
+/*
* Read the fan status.
*/
static int i8k_get_fan_status(int fan)
--
2.6.6
^ permalink raw reply related
* [PATCH v2 0/2] vfio: blacklist legacy virtio devices
From: Michael S. Tsirkin @ 2016-08-30 2:27 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, qemu-devel, virtualization
Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
never safe. This adds a quirk detecting these and disabling VFIO unless the
noiommu mode is used. At the moment, this only applies to virtio-pci devices.
The patch might make sense on stable as well.
Michael S. Tsirkin (2):
vfio: report group noiommu status
vfio: add virtio pci quirk
drivers/vfio/pci/vfio_pci_private.h | 1 +
include/linux/vfio.h | 2 +
drivers/vfio/pci/vfio_pci.c | 14 ++++
drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
drivers/vfio/vfio.c | 12 ++++
drivers/vfio/pci/Makefile | 1 +
6 files changed, 170 insertions(+)
create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
--
MST
^ permalink raw reply
* [PATCH v2 1/2] vfio: report group noiommu status
From: Michael S. Tsirkin @ 2016-08-30 2:27 UTC (permalink / raw)
To: linux-kernel; +Cc: kvm, qemu-devel, virtualization
In-Reply-To: <1472523968-9540-1-git-send-email-mst@redhat.com>
When using vfio, callers might want to know whether device is added
to a regular group or an non-iommu group.
Report this status from vfio_is_noiommu_group_dev.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/vfio.h | 2 ++
drivers/vfio/vfio.c | 12 ++++++++++++
2 files changed, 14 insertions(+)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 0ecae0b..584757b 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -51,6 +51,8 @@ extern int vfio_add_group_dev(struct device *dev,
const struct vfio_device_ops *ops,
void *device_data);
+extern bool vfio_is_noiommu_group_dev(struct device *dev);
+
extern void *vfio_del_group_dev(struct device *dev);
extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
extern void vfio_device_put(struct vfio_device *device);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index d1d70e0..22f279f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -872,6 +872,18 @@ static bool vfio_dev_present(struct vfio_group *group, struct device *dev)
}
/*
+ * Is device part of a noiommu group?
+ * Note: must call vfio_add_group_dev first.
+ */
+bool vfio_is_noiommu_group_dev(struct device *dev)
+{
+ struct vfio_device *device = dev_get_drvdata(dev);
+ struct vfio_group *group = device->group;
+
+ return group->noiommu;
+}
+
+/*
* Decrement the device reference count and wait for the device to be
* removed. Open file descriptors for the device... */
void *vfio_del_group_dev(struct device *dev)
--
MST
^ permalink raw reply related
* [PATCH v2 2/2] vfio: add virtio pci quirk
From: Michael S. Tsirkin @ 2016-08-30 2:27 UTC (permalink / raw)
To: linux-kernel
Cc: Feng Wu, kvm, qemu-devel, virtualization, Julia Lawall,
Yongji Xie, Paolo Bonzini, Dan Carpenter
In-Reply-To: <1472523968-9540-1-git-send-email-mst@redhat.com>
Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
to signal they are safe to use with an IOMMU.
Without this bit, exposing the device to userspace is unsafe, so probe
and fail VFIO initialization unless noiommu is enabled.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vfio/pci/vfio_pci_private.h | 1 +
drivers/vfio/pci/vfio_pci.c | 14 ++++
drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
drivers/vfio/pci/Makefile | 1 +
4 files changed, 156 insertions(+)
create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 2128de8..2bd5616 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
return -ENODEV;
}
#endif
+extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
#endif /* VFIO_PCI_PRIVATE_H */
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index d624a52..e93bf0c 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return ret;
}
+ if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
+ bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
+
+ ret = vfio_pci_virtio_quirk(vdev, noiommu);
+ if (ret) {
+ dev_warn(&vdev->pdev->dev,
+ "Failed to setup Virtio for VFIO\n");
+ vfio_del_group_dev(&pdev->dev);
+ vfio_iommu_group_put(group, &pdev->dev);
+ kfree(vdev);
+ return ret;
+ }
+ }
+
if (vfio_pci_is_vga(pdev)) {
vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
vga_set_legacy_decoding(pdev,
diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
new file mode 100644
index 0000000..e1ecffd
--- /dev/null
+++ b/drivers/vfio/pci/vfio_pci_virtio.c
@@ -0,0 +1,140 @@
+/*
+ * VFIO PCI Intel Graphics support
+ *
+ * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
+ * Author: Alex Williamson <alex.williamson@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Register a device specific region through which to provide read-only
+ * access to the Intel IGD opregion. The register defining the opregion
+ * address is also virtualized to prevent user modification.
+ */
+
+#include <linux/io.h>
+#include <linux/pci.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/virtio_pci.h>
+#include <linux/virtio_config.h>
+
+#include "vfio_pci_private.h"
+
+/**
+ * virtio_pci_find_capability - walk capabilities to find device info.
+ * @dev: the pci device
+ * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
+ *
+ * Returns offset of the capability, or 0.
+ */
+static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
+{
+ int pos;
+
+ for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
+ pos > 0;
+ pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
+ u8 type;
+ pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
+ cfg_type),
+ &type);
+
+ if (type != cfg_type)
+ continue;
+
+ /* Ignore structures with reserved BAR values */
+ if (type != VIRTIO_PCI_CAP_PCI_CFG) {
+ u8 bar;
+
+ pci_read_config_byte(dev, pos +
+ offsetof(struct virtio_pci_cap,
+ bar),
+ &bar);
+ if (bar > 0x5)
+ continue;
+ }
+
+ return pos;
+ }
+ return 0;
+}
+
+
+int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
+{
+ struct pci_dev *dev = vdev->pdev;
+ int common, cfg;
+ u32 features;
+ u32 offset;
+ u8 bar;
+
+ /* Without an IOMMU, we don't care */
+ if (noiommu)
+ return 0;
+
+ /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
+ if (dev->device < 0x1000 || dev->device > 0x107f)
+ return 0;
+
+ /* Check whether device enforces the IOMMU correctly */
+
+ /*
+ * All modern devices must have common and cfg capabilities. We use cfg
+ * capability for access so that we don't need to worry about resource
+ * availability. Slow but sure.
+ * Note that all vendor-specific fields we access are little-endian
+ * which matches what pci config accessors expect, so they do byteswap
+ * for us if appropriate.
+ */
+ common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
+ cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
+ if (!cfg || !common) {
+ dev_warn(&dev->dev,
+ "Virtio device lacks common or pci cfg.\n");
+ return -ENODEV;
+ }
+
+ pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
+ bar),
+ &bar);
+ pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
+ offset),
+ &offset);
+
+ /* Program cfg capability for dword access into common cfg. */
+ pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ cap.bar),
+ bar);
+ pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ cap.length),
+ 0x4);
+
+ /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
+ pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ cap.offset),
+ offset + offsetof(struct virtio_pci_common_cfg,
+ device_feature_select));
+ pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ pci_cfg_data),
+ VIRTIO_F_IOMMU_PLATFORM / 32);
+
+ /* Get the features dword. */
+ pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ cap.offset),
+ offset + offsetof(struct virtio_pci_common_cfg,
+ device_feature));
+ pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
+ pci_cfg_data),
+ &features);
+
+ /* Does this device obey the platform's IOMMU? If not it's an error. */
+ if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
+ dev_warn(&dev->dev,
+ "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
+ return -ENODEV;
+ }
+
+ return 0;
+}
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 76d8ec0..e9b20e7 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -1,5 +1,6 @@
vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
+vfio-pci-y += vfio_pci_virtio.o
vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
--
MST
^ permalink raw reply related
* Re: [Qemu-devel] [PATCH v2 0/2] vfio: blacklist legacy virtio devices
From: Jason Wang @ 2016-08-30 3:16 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: qemu-devel, kvm, virtualization
In-Reply-To: <1472523968-9540-1-git-send-email-mst@redhat.com>
On 2016年08月30日 10:27, Michael S. Tsirkin wrote:
> Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
> never safe.
And it actually won't work since GPA is assumed in the device. So I'm
not sure this is must since we should get a IOMMU fault in this case.
> This adds a quirk detecting these and disabling VFIO unless the
> noiommu mode is used. At the moment, this only applies to virtio-pci devices.
>
> The patch might make sense on stable as well.
>
> Michael S. Tsirkin (2):
> vfio: report group noiommu status
> vfio: add virtio pci quirk
>
> drivers/vfio/pci/vfio_pci_private.h | 1 +
> include/linux/vfio.h | 2 +
> drivers/vfio/pci/vfio_pci.c | 14 ++++
> drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> drivers/vfio/vfio.c | 12 ++++
> drivers/vfio/pci/Makefile | 1 +
> 6 files changed, 170 insertions(+)
> create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Alex Williamson @ 2016-08-30 3:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <1472523968-9540-3-git-send-email-mst@redhat.com>
On Tue, 30 Aug 2016 05:27:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> to signal they are safe to use with an IOMMU.
>
> Without this bit, exposing the device to userspace is unsafe, so probe
> and fail VFIO initialization unless noiommu is enabled.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vfio/pci/vfio_pci_private.h | 1 +
> drivers/vfio/pci/vfio_pci.c | 14 ++++
> drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> drivers/vfio/pci/Makefile | 1 +
> 4 files changed, 156 insertions(+)
> create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
>
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index 2128de8..2bd5616 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> return -ENODEV;
> }
> #endif
> +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> #endif /* VFIO_PCI_PRIVATE_H */
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index d624a52..e93bf0c 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return ret;
> }
>
> + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
ID range initially as well, this test raised a big red flag for me
whether all devices within this vendor ID were virtio.
> + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
I think you can use iommu_present() for this and avoid patch 1of2.
noiommu is mutually exclusive to an iommu being present. Seems like
all of this logic should be in the quirk itself, I'm not sure what it
buys to get the value here but wait until later to use it. Using
iommu_present() could also move this test much earlier in
vfio_pci_probe() making the exit path easier.
> +
> + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> + if (ret) {
> + dev_warn(&vdev->pdev->dev,
> + "Failed to setup Virtio for VFIO\n");
> + vfio_del_group_dev(&pdev->dev);
> + vfio_iommu_group_put(group, &pdev->dev);
> + kfree(vdev);
> + return ret;
> + }
> + }
> +
> if (vfio_pci_is_vga(pdev)) {
> vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> vga_set_legacy_decoding(pdev,
> diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> new file mode 100644
> index 0000000..e1ecffd
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> @@ -0,0 +1,140 @@
> +/*
> + * VFIO PCI Intel Graphics support
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> + * Author: Alex Williamson <alex.williamson@redhat.com>
Update.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Register a device specific region through which to provide read-only
> + * access to the Intel IGD opregion. The register defining the opregion
> + * address is also virtualized to prevent user modification.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/pci.h>
> +#include <linux/uaccess.h>
Are io.h and uaccess.h needed?
> +#include <linux/vfio.h>
> +#include <linux/virtio_pci.h>
> +#include <linux/virtio_config.h>
> +
> +#include "vfio_pci_private.h"
> +
> +/**
> + * virtio_pci_find_capability - walk capabilities to find device info.
> + * @dev: the pci device
> + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> + *
> + * Returns offset of the capability, or 0.
> + */
> +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
Does inlining this really make sense?
> +{
> + int pos;
> +
> + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> + pos > 0;
> + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> + u8 type;
> + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> + cfg_type),
> + &type);
> +
> + if (type != cfg_type)
> + continue;
> +
> + /* Ignore structures with reserved BAR values */
> + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> + u8 bar;
> +
> + pci_read_config_byte(dev, pos +
> + offsetof(struct virtio_pci_cap,
> + bar),
> + &bar);
> + if (bar > 0x5)
s/0x5/PCI_STD_RESOURCE_END/
> + continue;
> + }
> +
> + return pos;
> + }
> + return 0;
> +}
> +
> +
> +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> +{
> + struct pci_dev *dev = vdev->pdev;
Please use *pdev for consistency with the rest of drivers/vfio/pci/*
Also, is there any reason to pass the vfio_pci_device? There's nothing
vfio here otherwise and we could remove more #includes.
> + int common, cfg;
> + u32 features;
> + u32 offset;
> + u8 bar;
> +
> + /* Without an IOMMU, we don't care */
> + if (noiommu)
> + return 0;
> +
> + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> + if (dev->device < 0x1000 || dev->device > 0x107f)
> + return 0;
Whitespace
> +
> + /* Check whether device enforces the IOMMU correctly */
> +
> + /*
> + * All modern devices must have common and cfg capabilities. We use cfg
> + * capability for access so that we don't need to worry about resource
> + * availability. Slow but sure.
> + * Note that all vendor-specific fields we access are little-endian
> + * which matches what pci config accessors expect, so they do byteswap
> + * for us if appropriate.
> + */
> + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> + if (!cfg || !common) {
> + dev_warn(&dev->dev,
> + "Virtio device lacks common or pci cfg.\n");
Whitespace
> + return -ENODEV;
> + }
> +
> + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> + bar),
> + &bar);
> + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> + offset),
> + &offset);
> +
> + /* Program cfg capability for dword access into common cfg. */
> + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + cap.bar),
> + bar);
> + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + cap.length),
> + 0x4);
> +
> + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + cap.offset),
> + offset + offsetof(struct virtio_pci_common_cfg,
> + device_feature_select));
> + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + pci_cfg_data),
> + VIRTIO_F_IOMMU_PLATFORM / 32);
> +
> + /* Get the features dword. */
> + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + cap.offset),
> + offset + offsetof(struct virtio_pci_common_cfg,
> + device_feature));
> + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> + pci_cfg_data),
> + &features);
> +
> + /* Does this device obey the platform's IOMMU? If not it's an error. */
> + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> + dev_warn(&dev->dev,
> + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
Whitespace
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 76d8ec0..e9b20e7 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,5 +1,6 @@
>
> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> +vfio-pci-y += vfio_pci_virtio.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>
> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Michael S. Tsirkin @ 2016-08-30 3:48 UTC (permalink / raw)
To: Alex Williamson
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <20160829212325.32467efc@t450s.home>
On Mon, Aug 29, 2016 at 09:23:25PM -0600, Alex Williamson wrote:
> On Tue, 30 Aug 2016 05:27:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > to signal they are safe to use with an IOMMU.
> >
> > Without this bit, exposing the device to userspace is unsafe, so probe
> > and fail VFIO initialization unless noiommu is enabled.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > drivers/vfio/pci/Makefile | 1 +
> > 4 files changed, 156 insertions(+)
> > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > index 2128de8..2bd5616 100644
> > --- a/drivers/vfio/pci/vfio_pci_private.h
> > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > return -ENODEV;
> > }
> > #endif
> > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > #endif /* VFIO_PCI_PRIVATE_H */
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index d624a52..e93bf0c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > return ret;
> > }
> >
> > + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
>
> Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
> ID range initially as well, this test raised a big red flag for me
> whether all devices within this vendor ID were virtio.
>
> > + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
>
> I think you can use iommu_present() for this and avoid patch 1of2.
So in presence of an IOMMU in the system, is it impossible to bind the
noiommu device?
I did not test this yet.
If yes this is something we'll need to fix as well -
we do want to allow binding noiommu to a legacy virtio device.
> noiommu is mutually exclusive to an iommu being present. Seems like
> all of this logic should be in the quirk itself, I'm not sure what it
> buys to get the value here but wait until later to use it. Using
> iommu_present() could also move this test much earlier in
> vfio_pci_probe() making the exit path easier.
>
> > +
> > + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > + if (ret) {
> > + dev_warn(&vdev->pdev->dev,
> > + "Failed to setup Virtio for VFIO\n");
> > + vfio_del_group_dev(&pdev->dev);
> > + vfio_iommu_group_put(group, &pdev->dev);
> > + kfree(vdev);
> > + return ret;
> > + }
> > + }
> > +
> > if (vfio_pci_is_vga(pdev)) {
> > vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > vga_set_legacy_decoding(pdev,
> > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > new file mode 100644
> > index 0000000..e1ecffd
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * VFIO PCI Intel Graphics support
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > + * Author: Alex Williamson <alex.williamson@redhat.com>
>
> Update.
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Register a device specific region through which to provide read-only
> > + * access to the Intel IGD opregion. The register defining the opregion
> > + * address is also virtualized to prevent user modification.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/pci.h>
> > +#include <linux/uaccess.h>
>
> Are io.h and uaccess.h needed?
>
> > +#include <linux/vfio.h>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_config.h>
> > +
> > +#include "vfio_pci_private.h"
> > +
> > +/**
> > + * virtio_pci_find_capability - walk capabilities to find device info.
> > + * @dev: the pci device
> > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + *
> > + * Returns offset of the capability, or 0.
> > + */
> > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
>
> Does inlining this really make sense?
>
> > +{
> > + int pos;
> > +
> > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > + pos > 0;
> > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > + u8 type;
> > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > + cfg_type),
> > + &type);
> > +
> > + if (type != cfg_type)
> > + continue;
> > +
> > + /* Ignore structures with reserved BAR values */
> > + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > + u8 bar;
> > +
> > + pci_read_config_byte(dev, pos +
> > + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > + if (bar > 0x5)
>
> s/0x5/PCI_STD_RESOURCE_END/
>
> > + continue;
> > + }
> > +
> > + return pos;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > +{
> > + struct pci_dev *dev = vdev->pdev;
>
> Please use *pdev for consistency with the rest of drivers/vfio/pci/*
>
> Also, is there any reason to pass the vfio_pci_device? There's nothing
> vfio here otherwise and we could remove more #includes.
>
> > + int common, cfg;
> > + u32 features;
> > + u32 offset;
> > + u8 bar;
> > +
> > + /* Without an IOMMU, we don't care */
> > + if (noiommu)
> > + return 0;
> > +
> > + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > + if (dev->device < 0x1000 || dev->device > 0x107f)
> > + return 0;
>
> Whitespace
>
> > +
> > + /* Check whether device enforces the IOMMU correctly */
> > +
> > + /*
> > + * All modern devices must have common and cfg capabilities. We use cfg
> > + * capability for access so that we don't need to worry about resource
> > + * availability. Slow but sure.
> > + * Note that all vendor-specific fields we access are little-endian
> > + * which matches what pci config accessors expect, so they do byteswap
> > + * for us if appropriate.
> > + */
> > + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > + if (!cfg || !common) {
> > + dev_warn(&dev->dev,
> > + "Virtio device lacks common or pci cfg.\n");
>
> Whitespace
>
> > + return -ENODEV;
> > + }
> > +
> > + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > + offset),
> > + &offset);
> > +
> > + /* Program cfg capability for dword access into common cfg. */
> > + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.bar),
> > + bar);
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.length),
> > + 0x4);
> > +
> > + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.offset),
> > + offset + offsetof(struct virtio_pci_common_cfg,
> > + device_feature_select));
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + pci_cfg_data),
> > + VIRTIO_F_IOMMU_PLATFORM / 32);
> > +
> > + /* Get the features dword. */
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.offset),
> > + offset + offsetof(struct virtio_pci_common_cfg,
> > + device_feature));
> > + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + pci_cfg_data),
> > + &features);
> > +
> > + /* Does this device obey the platform's IOMMU? If not it's an error. */
> > + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > + dev_warn(&dev->dev,
> > + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
>
> Whitespace
>
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 76d8ec0..e9b20e7 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,5 +1,6 @@
> >
> > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > +vfio-pci-y += vfio_pci_virtio.o
> > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >
> > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>
> Thanks,
> Alex
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v2 0/2] vfio: blacklist legacy virtio devices
From: Michael S. Tsirkin @ 2016-08-30 3:49 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, qemu-devel, linux-kernel, virtualization
In-Reply-To: <b3183b57-2335-9195-4cf1-bad7b9f30956@redhat.com>
On Tue, Aug 30, 2016 at 11:16:25AM +0800, Jason Wang wrote:
>
>
> On 2016年08月30日 10:27, Michael S. Tsirkin wrote:
> > Legacy virtio devices always bypassed an IOMMU, so using them with vfio was
> > never safe.
>
> And it actually won't work since GPA is assumed in the device. So I'm not
> sure this is must since we should get a IOMMU fault in this case.
We won't get an IOMMU fault for legacy systems since they
bypass the IOMMU. Instead guest userspace will get full
access to all of guest memory through the device.
> > This adds a quirk detecting these and disabling VFIO unless the
> > noiommu mode is used. At the moment, this only applies to virtio-pci devices.
> >
> > The patch might make sense on stable as well.
> >
> > Michael S. Tsirkin (2):
> > vfio: report group noiommu status
> > vfio: add virtio pci quirk
> >
> > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > include/linux/vfio.h | 2 +
> > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > drivers/vfio/vfio.c | 12 ++++
> > drivers/vfio/pci/Makefile | 1 +
> > 6 files changed, 170 insertions(+)
> > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Alex Williamson @ 2016-08-30 3:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <20160829212325.32467efc@t450s.home>
On Mon, 29 Aug 2016 21:23:25 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue, 30 Aug 2016 05:27:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > to signal they are safe to use with an IOMMU.
> >
> > Without this bit, exposing the device to userspace is unsafe, so probe
> > and fail VFIO initialization unless noiommu is enabled.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > drivers/vfio/pci/Makefile | 1 +
> > 4 files changed, 156 insertions(+)
> > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > index 2128de8..2bd5616 100644
> > --- a/drivers/vfio/pci/vfio_pci_private.h
> > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > return -ENODEV;
> > }
> > #endif
> > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > #endif /* VFIO_PCI_PRIVATE_H */
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index d624a52..e93bf0c 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > return ret;
> > }
> >
> > + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
>
> Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
> ID range initially as well, this test raised a big red flag for me
> whether all devices within this vendor ID were virtio.
>
> > + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
>
> I think you can use iommu_present() for this and avoid patch 1of2.
> noiommu is mutually exclusive to an iommu being present. Seems like
> all of this logic should be in the quirk itself, I'm not sure what it
> buys to get the value here but wait until later to use it. Using
> iommu_present() could also move this test much earlier in
> vfio_pci_probe() making the exit path easier.
Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
iommu_present() assumes an IOMMU API based device. I'll try to think if
there's another way to avoid adding the is_noiommu function. Thanks,
Alex
>
> > +
> > + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > + if (ret) {
> > + dev_warn(&vdev->pdev->dev,
> > + "Failed to setup Virtio for VFIO\n");
> > + vfio_del_group_dev(&pdev->dev);
> > + vfio_iommu_group_put(group, &pdev->dev);
> > + kfree(vdev);
> > + return ret;
> > + }
> > + }
> > +
> > if (vfio_pci_is_vga(pdev)) {
> > vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > vga_set_legacy_decoding(pdev,
> > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > new file mode 100644
> > index 0000000..e1ecffd
> > --- /dev/null
> > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * VFIO PCI Intel Graphics support
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > + *
> > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > + * Author: Alex Williamson <alex.williamson@redhat.com>
>
> Update.
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * Register a device specific region through which to provide read-only
> > + * access to the Intel IGD opregion. The register defining the opregion
> > + * address is also virtualized to prevent user modification.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/pci.h>
> > +#include <linux/uaccess.h>
>
> Are io.h and uaccess.h needed?
>
> > +#include <linux/vfio.h>
> > +#include <linux/virtio_pci.h>
> > +#include <linux/virtio_config.h>
> > +
> > +#include "vfio_pci_private.h"
> > +
> > +/**
> > + * virtio_pci_find_capability - walk capabilities to find device info.
> > + * @dev: the pci device
> > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > + *
> > + * Returns offset of the capability, or 0.
> > + */
> > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
>
> Does inlining this really make sense?
>
> > +{
> > + int pos;
> > +
> > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > + pos > 0;
> > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > + u8 type;
> > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > + cfg_type),
> > + &type);
> > +
> > + if (type != cfg_type)
> > + continue;
> > +
> > + /* Ignore structures with reserved BAR values */
> > + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > + u8 bar;
> > +
> > + pci_read_config_byte(dev, pos +
> > + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > + if (bar > 0x5)
>
> s/0x5/PCI_STD_RESOURCE_END/
>
> > + continue;
> > + }
> > +
> > + return pos;
> > + }
> > + return 0;
> > +}
> > +
> > +
> > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > +{
> > + struct pci_dev *dev = vdev->pdev;
>
> Please use *pdev for consistency with the rest of drivers/vfio/pci/*
>
> Also, is there any reason to pass the vfio_pci_device? There's nothing
> vfio here otherwise and we could remove more #includes.
>
> > + int common, cfg;
> > + u32 features;
> > + u32 offset;
> > + u8 bar;
> > +
> > + /* Without an IOMMU, we don't care */
> > + if (noiommu)
> > + return 0;
> > +
> > + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > + if (dev->device < 0x1000 || dev->device > 0x107f)
> > + return 0;
>
> Whitespace
>
> > +
> > + /* Check whether device enforces the IOMMU correctly */
> > +
> > + /*
> > + * All modern devices must have common and cfg capabilities. We use cfg
> > + * capability for access so that we don't need to worry about resource
> > + * availability. Slow but sure.
> > + * Note that all vendor-specific fields we access are little-endian
> > + * which matches what pci config accessors expect, so they do byteswap
> > + * for us if appropriate.
> > + */
> > + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > + if (!cfg || !common) {
> > + dev_warn(&dev->dev,
> > + "Virtio device lacks common or pci cfg.\n");
>
> Whitespace
>
> > + return -ENODEV;
> > + }
> > +
> > + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > + bar),
> > + &bar);
> > + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > + offset),
> > + &offset);
> > +
> > + /* Program cfg capability for dword access into common cfg. */
> > + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.bar),
> > + bar);
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.length),
> > + 0x4);
> > +
> > + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.offset),
> > + offset + offsetof(struct virtio_pci_common_cfg,
> > + device_feature_select));
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + pci_cfg_data),
> > + VIRTIO_F_IOMMU_PLATFORM / 32);
> > +
> > + /* Get the features dword. */
> > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + cap.offset),
> > + offset + offsetof(struct virtio_pci_common_cfg,
> > + device_feature));
> > + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > + pci_cfg_data),
> > + &features);
> > +
> > + /* Does this device obey the platform's IOMMU? If not it's an error. */
> > + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > + dev_warn(&dev->dev,
> > + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
>
> Whitespace
>
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 76d8ec0..e9b20e7 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,5 +1,6 @@
> >
> > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > +vfio-pci-y += vfio_pci_virtio.o
> > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >
> > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>
> Thanks,
> Alex
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Michael S. Tsirkin @ 2016-08-30 3:57 UTC (permalink / raw)
To: Alex Williamson
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <20160829215220.0efb91b7@t450s.home>
On Mon, Aug 29, 2016 at 09:52:20PM -0600, Alex Williamson wrote:
> On Mon, 29 Aug 2016 21:23:25 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > On Tue, 30 Aug 2016 05:27:17 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > to signal they are safe to use with an IOMMU.
> > >
> > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > and fail VFIO initialization unless noiommu is enabled.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > > drivers/vfio/pci/Makefile | 1 +
> > > 4 files changed, 156 insertions(+)
> > > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > index 2128de8..2bd5616 100644
> > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > > return -ENODEV;
> > > }
> > > #endif
> > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > > #endif /* VFIO_PCI_PRIVATE_H */
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index d624a52..e93bf0c 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > return ret;
> > > }
> > >
> > > + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
> >
> > Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
> > ID range initially as well, this test raised a big red flag for me
> > whether all devices within this vendor ID were virtio.
> >
> > > + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
> >
> > I think you can use iommu_present() for this and avoid patch 1of2.
> > noiommu is mutually exclusive to an iommu being present. Seems like
> > all of this logic should be in the quirk itself, I'm not sure what it
> > buys to get the value here but wait until later to use it. Using
> > iommu_present() could also move this test much earlier in
> > vfio_pci_probe() making the exit path easier.
>
> Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> iommu_present() assumes an IOMMU API based device. I'll try to think if
> there's another way to avoid adding the is_noiommu function. Thanks,
>
> Alex
FWIW I'm only too happy if you take over this patch.
You need Jason's recent patchset to QEMU to test,
but otherwise no special hardware is required.
> >
> > > +
> > > + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > + if (ret) {
> > > + dev_warn(&vdev->pdev->dev,
> > > + "Failed to setup Virtio for VFIO\n");
> > > + vfio_del_group_dev(&pdev->dev);
> > > + vfio_iommu_group_put(group, &pdev->dev);
> > > + kfree(vdev);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > if (vfio_pci_is_vga(pdev)) {
> > > vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > > vga_set_legacy_decoding(pdev,
> > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > new file mode 100644
> > > index 0000000..e1ecffd
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > @@ -0,0 +1,140 @@
> > > +/*
> > > + * VFIO PCI Intel Graphics support
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > + *
> > > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > > + * Author: Alex Williamson <alex.williamson@redhat.com>
> >
> > Update.
> >
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * Register a device specific region through which to provide read-only
> > > + * access to the Intel IGD opregion. The register defining the opregion
> > > + * address is also virtualized to prevent user modification.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/uaccess.h>
> >
> > Are io.h and uaccess.h needed?
> >
> > > +#include <linux/vfio.h>
> > > +#include <linux/virtio_pci.h>
> > > +#include <linux/virtio_config.h>
> > > +
> > > +#include "vfio_pci_private.h"
> > > +
> > > +/**
> > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > + * @dev: the pci device
> > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > + *
> > > + * Returns offset of the capability, or 0.
> > > + */
> > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
> >
> > Does inlining this really make sense?
> >
> > > +{
> > > + int pos;
> > > +
> > > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > + pos > 0;
> > > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > + u8 type;
> > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > + cfg_type),
> > > + &type);
> > > +
> > > + if (type != cfg_type)
> > > + continue;
> > > +
> > > + /* Ignore structures with reserved BAR values */
> > > + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > + u8 bar;
> > > +
> > > + pci_read_config_byte(dev, pos +
> > > + offsetof(struct virtio_pci_cap,
> > > + bar),
> > > + &bar);
> > > + if (bar > 0x5)
> >
> > s/0x5/PCI_STD_RESOURCE_END/
> >
> > > + continue;
> > > + }
> > > +
> > > + return pos;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > +{
> > > + struct pci_dev *dev = vdev->pdev;
> >
> > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> >
> > Also, is there any reason to pass the vfio_pci_device? There's nothing
> > vfio here otherwise and we could remove more #includes.
> >
> > > + int common, cfg;
> > > + u32 features;
> > > + u32 offset;
> > > + u8 bar;
> > > +
> > > + /* Without an IOMMU, we don't care */
> > > + if (noiommu)
> > > + return 0;
> > > +
> > > + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > + if (dev->device < 0x1000 || dev->device > 0x107f)
> > > + return 0;
> >
> > Whitespace
> >
> > > +
> > > + /* Check whether device enforces the IOMMU correctly */
> > > +
> > > + /*
> > > + * All modern devices must have common and cfg capabilities. We use cfg
> > > + * capability for access so that we don't need to worry about resource
> > > + * availability. Slow but sure.
> > > + * Note that all vendor-specific fields we access are little-endian
> > > + * which matches what pci config accessors expect, so they do byteswap
> > > + * for us if appropriate.
> > > + */
> > > + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > + if (!cfg || !common) {
> > > + dev_warn(&dev->dev,
> > > + "Virtio device lacks common or pci cfg.\n");
> >
> > Whitespace
> >
> > > + return -ENODEV;
> > > + }
> > > +
> > > + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > + bar),
> > > + &bar);
> > > + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > + offset),
> > > + &offset);
> > > +
> > > + /* Program cfg capability for dword access into common cfg. */
> > > + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.bar),
> > > + bar);
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.length),
> > > + 0x4);
> > > +
> > > + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.offset),
> > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > + device_feature_select));
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + pci_cfg_data),
> > > + VIRTIO_F_IOMMU_PLATFORM / 32);
> > > +
> > > + /* Get the features dword. */
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.offset),
> > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > + device_feature));
> > > + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + pci_cfg_data),
> > > + &features);
> > > +
> > > + /* Does this device obey the platform's IOMMU? If not it's an error. */
> > > + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > + dev_warn(&dev->dev,
> > > + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
> >
> > Whitespace
> >
> > > + return -ENODEV;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 76d8ec0..e9b20e7 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,5 +1,6 @@
> > >
> > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > +vfio-pci-y += vfio_pci_virtio.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > >
> > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> >
> > Thanks,
> > Alex
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Alex Williamson @ 2016-08-30 4:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <20160829215220.0efb91b7@t450s.home>
On Mon, 29 Aug 2016 21:52:20 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Mon, 29 Aug 2016 21:23:25 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > On Tue, 30 Aug 2016 05:27:17 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > to signal they are safe to use with an IOMMU.
> > >
> > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > and fail VFIO initialization unless noiommu is enabled.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > > drivers/vfio/pci/Makefile | 1 +
> > > 4 files changed, 156 insertions(+)
> > > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > index 2128de8..2bd5616 100644
> > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > > return -ENODEV;
> > > }
> > > #endif
> > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > > #endif /* VFIO_PCI_PRIVATE_H */
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index d624a52..e93bf0c 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > return ret;
> > > }
> > >
> > > + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
> >
> > Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
> > ID range initially as well, this test raised a big red flag for me
> > whether all devices within this vendor ID were virtio.
> >
> > > + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
> >
> > I think you can use iommu_present() for this and avoid patch 1of2.
> > noiommu is mutually exclusive to an iommu being present. Seems like
> > all of this logic should be in the quirk itself, I'm not sure what it
> > buys to get the value here but wait until later to use it. Using
> > iommu_present() could also move this test much earlier in
> > vfio_pci_probe() making the exit path easier.
>
> Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> iommu_present() assumes an IOMMU API based device. I'll try to think if
> there's another way to avoid adding the is_noiommu function. Thanks,
I think something like this would do it.
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1214,6 +1214,22 @@ static int vfio_pci_probe(struct pci_dev *pdev, const str
if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
return -EINVAL;
+ /*
+ * Filter out virtio devices that do not honor the iommu,
+ * but only for real iommu groups.
+ */
+ if (vfio_pci_is_virtio(pdev)) {
+ struct iommu_group *tmp = iommu_group_get(&pdev->dev);
+
+ if (tmp) {
+ iommu_group_put(tmp);
+
+ ret = vfio_pci_virtio_quirk(pdev);
+ if (ret)
+ return ret;
+ }
+ }
+
group = vfio_iommu_group_get(&pdev->dev);
if (!group)
return -EINVAL;
Thanks,
Alex
> > > +
> > > + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > + if (ret) {
> > > + dev_warn(&vdev->pdev->dev,
> > > + "Failed to setup Virtio for VFIO\n");
> > > + vfio_del_group_dev(&pdev->dev);
> > > + vfio_iommu_group_put(group, &pdev->dev);
> > > + kfree(vdev);
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > if (vfio_pci_is_vga(pdev)) {
> > > vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > > vga_set_legacy_decoding(pdev,
> > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > new file mode 100644
> > > index 0000000..e1ecffd
> > > --- /dev/null
> > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > @@ -0,0 +1,140 @@
> > > +/*
> > > + * VFIO PCI Intel Graphics support
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > + *
> > > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > > + * Author: Alex Williamson <alex.williamson@redhat.com>
> >
> > Update.
> >
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * Register a device specific region through which to provide read-only
> > > + * access to the Intel IGD opregion. The register defining the opregion
> > > + * address is also virtualized to prevent user modification.
> > > + */
> > > +
> > > +#include <linux/io.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/uaccess.h>
> >
> > Are io.h and uaccess.h needed?
> >
> > > +#include <linux/vfio.h>
> > > +#include <linux/virtio_pci.h>
> > > +#include <linux/virtio_config.h>
> > > +
> > > +#include "vfio_pci_private.h"
> > > +
> > > +/**
> > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > + * @dev: the pci device
> > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > + *
> > > + * Returns offset of the capability, or 0.
> > > + */
> > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
> >
> > Does inlining this really make sense?
> >
> > > +{
> > > + int pos;
> > > +
> > > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > + pos > 0;
> > > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > + u8 type;
> > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > + cfg_type),
> > > + &type);
> > > +
> > > + if (type != cfg_type)
> > > + continue;
> > > +
> > > + /* Ignore structures with reserved BAR values */
> > > + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > + u8 bar;
> > > +
> > > + pci_read_config_byte(dev, pos +
> > > + offsetof(struct virtio_pci_cap,
> > > + bar),
> > > + &bar);
> > > + if (bar > 0x5)
> >
> > s/0x5/PCI_STD_RESOURCE_END/
> >
> > > + continue;
> > > + }
> > > +
> > > + return pos;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > +{
> > > + struct pci_dev *dev = vdev->pdev;
> >
> > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> >
> > Also, is there any reason to pass the vfio_pci_device? There's nothing
> > vfio here otherwise and we could remove more #includes.
> >
> > > + int common, cfg;
> > > + u32 features;
> > > + u32 offset;
> > > + u8 bar;
> > > +
> > > + /* Without an IOMMU, we don't care */
> > > + if (noiommu)
> > > + return 0;
> > > +
> > > + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > + if (dev->device < 0x1000 || dev->device > 0x107f)
> > > + return 0;
> >
> > Whitespace
> >
> > > +
> > > + /* Check whether device enforces the IOMMU correctly */
> > > +
> > > + /*
> > > + * All modern devices must have common and cfg capabilities. We use cfg
> > > + * capability for access so that we don't need to worry about resource
> > > + * availability. Slow but sure.
> > > + * Note that all vendor-specific fields we access are little-endian
> > > + * which matches what pci config accessors expect, so they do byteswap
> > > + * for us if appropriate.
> > > + */
> > > + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > + if (!cfg || !common) {
> > > + dev_warn(&dev->dev,
> > > + "Virtio device lacks common or pci cfg.\n");
> >
> > Whitespace
> >
> > > + return -ENODEV;
> > > + }
> > > +
> > > + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > + bar),
> > > + &bar);
> > > + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > + offset),
> > > + &offset);
> > > +
> > > + /* Program cfg capability for dword access into common cfg. */
> > > + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.bar),
> > > + bar);
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.length),
> > > + 0x4);
> > > +
> > > + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.offset),
> > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > + device_feature_select));
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + pci_cfg_data),
> > > + VIRTIO_F_IOMMU_PLATFORM / 32);
> > > +
> > > + /* Get the features dword. */
> > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + cap.offset),
> > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > + device_feature));
> > > + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > + pci_cfg_data),
> > > + &features);
> > > +
> > > + /* Does this device obey the platform's IOMMU? If not it's an error. */
> > > + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > + dev_warn(&dev->dev,
> > > + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
> >
> > Whitespace
> >
> > > + return -ENODEV;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 76d8ec0..e9b20e7 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,5 +1,6 @@
> > >
> > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > +vfio-pci-y += vfio_pci_virtio.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > >
> > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> >
> > Thanks,
> > Alex
^ permalink raw reply
* Re: [PATCH v2 2/2] vfio: add virtio pci quirk
From: Michael S. Tsirkin @ 2016-08-30 5:20 UTC (permalink / raw)
To: Alex Williamson
Cc: Feng Wu, kvm, qemu-devel, linux-kernel, Julia Lawall, Yongji Xie,
Paolo Bonzini, virtualization, Dan Carpenter
In-Reply-To: <20160829225304.17238cfd@t450s.home>
On Mon, Aug 29, 2016 at 10:53:04PM -0600, Alex Williamson wrote:
> On Mon, 29 Aug 2016 21:52:20 -0600
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > On Mon, 29 Aug 2016 21:23:25 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >
> > > On Tue, 30 Aug 2016 05:27:17 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > Modern virtio pci devices can set VIRTIO_F_IOMMU_PLATFORM
> > > > to signal they are safe to use with an IOMMU.
> > > >
> > > > Without this bit, exposing the device to userspace is unsafe, so probe
> > > > and fail VFIO initialization unless noiommu is enabled.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > > > drivers/vfio/pci/vfio_pci.c | 14 ++++
> > > > drivers/vfio/pci/vfio_pci_virtio.c | 140 ++++++++++++++++++++++++++++++++++++
> > > > drivers/vfio/pci/Makefile | 1 +
> > > > 4 files changed, 156 insertions(+)
> > > > create mode 100644 drivers/vfio/pci/vfio_pci_virtio.c
> > > >
> > > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> > > > index 2128de8..2bd5616 100644
> > > > --- a/drivers/vfio/pci/vfio_pci_private.h
> > > > +++ b/drivers/vfio/pci/vfio_pci_private.h
> > > > @@ -139,4 +139,5 @@ static inline int vfio_pci_igd_init(struct vfio_pci_device *vdev)
> > > > return -ENODEV;
> > > > }
> > > > #endif
> > > > +extern int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu);
> > > > #endif /* VFIO_PCI_PRIVATE_H */
> > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > > index d624a52..e93bf0c 100644
> > > > --- a/drivers/vfio/pci/vfio_pci.c
> > > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > > @@ -1236,6 +1236,20 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > return ret;
> > > > }
> > > >
> > > > + if (pdev->vendor == PCI_VENDOR_ID_REDHAT_QUMRANET) {
> > >
> > > Perhaps a vfio_pci_is_virtio() like vga below? Let's test the device
> > > ID range initially as well, this test raised a big red flag for me
> > > whether all devices within this vendor ID were virtio.
> > >
> > > > + bool noiommu = vfio_is_noiommu_group_dev(&pdev->dev);
> > >
> > > I think you can use iommu_present() for this and avoid patch 1of2.
> > > noiommu is mutually exclusive to an iommu being present. Seems like
> > > all of this logic should be in the quirk itself, I'm not sure what it
> > > buys to get the value here but wait until later to use it. Using
> > > iommu_present() could also move this test much earlier in
> > > vfio_pci_probe() making the exit path easier.
> >
> > Except then I'm reintroducing the bug fixed by 16ab8a5cbea4 since
> > iommu_present() assumes an IOMMU API based device. I'll try to think if
> > there's another way to avoid adding the is_noiommu function. Thanks,
>
> I think something like this would do it.
>
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1214,6 +1214,22 @@ static int vfio_pci_probe(struct pci_dev *pdev, const str
> if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
> return -EINVAL;
>
> + /*
> + * Filter out virtio devices that do not honor the iommu,
> + * but only for real iommu groups.
> + */
> + if (vfio_pci_is_virtio(pdev)) {
> + struct iommu_group *tmp = iommu_group_get(&pdev->dev);
> +
> + if (tmp) {
> + iommu_group_put(tmp);
> +
> + ret = vfio_pci_virtio_quirk(pdev);
> + if (ret)
> + return ret;
> + }
> + }
> +
> group = vfio_iommu_group_get(&pdev->dev);
> if (!group)
> return -EINVAL;
>
> Thanks,
> Alex
Yes but I think this will also prevent binding
a vfio-noiommu to this device.
Arguably this is a separate bug as it's already impossible ...
but now that we are disabling regular vfio the noiommu
fallback becomes more important.
Any hints on how to fix?
> > > > +
> > > > + ret = vfio_pci_virtio_quirk(vdev, noiommu);
> > > > + if (ret) {
> > > > + dev_warn(&vdev->pdev->dev,
> > > > + "Failed to setup Virtio for VFIO\n");
> > > > + vfio_del_group_dev(&pdev->dev);
> > > > + vfio_iommu_group_put(group, &pdev->dev);
> > > > + kfree(vdev);
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > if (vfio_pci_is_vga(pdev)) {
> > > > vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> > > > vga_set_legacy_decoding(pdev,
> > > > diff --git a/drivers/vfio/pci/vfio_pci_virtio.c b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > new file mode 100644
> > > > index 0000000..e1ecffd
> > > > --- /dev/null
> > > > +++ b/drivers/vfio/pci/vfio_pci_virtio.c
> > > > @@ -0,0 +1,140 @@
> > > > +/*
> > > > + * VFIO PCI Intel Graphics support
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > + *
> > > > + * Copyright (C) 2016 Red Hat, Inc. All rights reserved.
> > > > + * Author: Alex Williamson <alex.williamson@redhat.com>
> > >
> > > Update.
> > >
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License version 2 as
> > > > + * published by the Free Software Foundation.
> > > > + *
> > > > + * Register a device specific region through which to provide read-only
> > > > + * access to the Intel IGD opregion. The register defining the opregion
> > > > + * address is also virtualized to prevent user modification.
> > > > + */
> > > > +
> > > > +#include <linux/io.h>
> > > > +#include <linux/pci.h>
> > > > +#include <linux/uaccess.h>
> > >
> > > Are io.h and uaccess.h needed?
> > >
> > > > +#include <linux/vfio.h>
> > > > +#include <linux/virtio_pci.h>
> > > > +#include <linux/virtio_config.h>
> > > > +
> > > > +#include "vfio_pci_private.h"
> > > > +
> > > > +/**
> > > > + * virtio_pci_find_capability - walk capabilities to find device info.
> > > > + * @dev: the pci device
> > > > + * @cfg_type: the VIRTIO_PCI_CAP_* value we seek
> > > > + *
> > > > + * Returns offset of the capability, or 0.
> > > > + */
> > > > +static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type)
> > >
> > > Does inlining this really make sense?
> > >
> > > > +{
> > > > + int pos;
> > > > +
> > > > + for (pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
> > > > + pos > 0;
> > > > + pos = pci_find_next_capability(dev, pos, PCI_CAP_ID_VNDR)) {
> > > > + u8 type;
> > > > + pci_read_config_byte(dev, pos + offsetof(struct virtio_pci_cap,
> > > > + cfg_type),
> > > > + &type);
> > > > +
> > > > + if (type != cfg_type)
> > > > + continue;
> > > > +
> > > > + /* Ignore structures with reserved BAR values */
> > > > + if (type != VIRTIO_PCI_CAP_PCI_CFG) {
> > > > + u8 bar;
> > > > +
> > > > + pci_read_config_byte(dev, pos +
> > > > + offsetof(struct virtio_pci_cap,
> > > > + bar),
> > > > + &bar);
> > > > + if (bar > 0x5)
> > >
> > > s/0x5/PCI_STD_RESOURCE_END/
> > >
> > > > + continue;
> > > > + }
> > > > +
> > > > + return pos;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +
> > > > +int vfio_pci_virtio_quirk(struct vfio_pci_device *vdev, bool noiommu)
> > > > +{
> > > > + struct pci_dev *dev = vdev->pdev;
> > >
> > > Please use *pdev for consistency with the rest of drivers/vfio/pci/*
> > >
> > > Also, is there any reason to pass the vfio_pci_device? There's nothing
> > > vfio here otherwise and we could remove more #includes.
> > >
> > > > + int common, cfg;
> > > > + u32 features;
> > > > + u32 offset;
> > > > + u8 bar;
> > > > +
> > > > + /* Without an IOMMU, we don't care */
> > > > + if (noiommu)
> > > > + return 0;
> > > > +
> > > > + /* Virtio only owns devices >= 0x1000 and <= 0x107f: leave the rest. */
> > > > + if (dev->device < 0x1000 || dev->device > 0x107f)
> > > > + return 0;
> > >
> > > Whitespace
> > >
> > > > +
> > > > + /* Check whether device enforces the IOMMU correctly */
> > > > +
> > > > + /*
> > > > + * All modern devices must have common and cfg capabilities. We use cfg
> > > > + * capability for access so that we don't need to worry about resource
> > > > + * availability. Slow but sure.
> > > > + * Note that all vendor-specific fields we access are little-endian
> > > > + * which matches what pci config accessors expect, so they do byteswap
> > > > + * for us if appropriate.
> > > > + */
> > > > + common = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > > + cfg = virtio_pci_find_capability(dev, VIRTIO_PCI_CAP_PCI_CFG);
> > > > + if (!cfg || !common) {
> > > > + dev_warn(&dev->dev,
> > > > + "Virtio device lacks common or pci cfg.\n");
> > >
> > > Whitespace
> > >
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + pci_read_config_byte(dev, common + offsetof(struct virtio_pci_cap,
> > > > + bar),
> > > > + &bar);
> > > > + pci_read_config_dword(dev, common + offsetof(struct virtio_pci_cap,
> > > > + offset),
> > > > + &offset);
> > > > +
> > > > + /* Program cfg capability for dword access into common cfg. */
> > > > + pci_write_config_byte(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + cap.bar),
> > > > + bar);
> > > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + cap.length),
> > > > + 0x4);
> > > > +
> > > > + /* Select features dword that has VIRTIO_F_IOMMU_PLATFORM. */
> > > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + cap.offset),
> > > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > > + device_feature_select));
> > > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + pci_cfg_data),
> > > > + VIRTIO_F_IOMMU_PLATFORM / 32);
> > > > +
> > > > + /* Get the features dword. */
> > > > + pci_write_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + cap.offset),
> > > > + offset + offsetof(struct virtio_pci_common_cfg,
> > > > + device_feature));
> > > > + pci_read_config_dword(dev, cfg + offsetof(struct virtio_pci_cfg_cap,
> > > > + pci_cfg_data),
> > > > + &features);
> > > > +
> > > > + /* Does this device obey the platform's IOMMU? If not it's an error. */
> > > > + if (!(features & (0x1 << (VIRTIO_F_IOMMU_PLATFORM % 32)))) {
> > > > + dev_warn(&dev->dev,
> > > > + "Virtio device lacks VIRTIO_F_IOMMU_PLATFORM.\n");
> > >
> > > Whitespace
> > >
> > > > + return -ENODEV;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > index 76d8ec0..e9b20e7 100644
> > > > --- a/drivers/vfio/pci/Makefile
> > > > +++ b/drivers/vfio/pci/Makefile
> > > > @@ -1,5 +1,6 @@
> > > >
> > > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > +vfio-pci-y += vfio_pci_virtio.o
> > > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > >
> > > > obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> > >
> > > Thanks,
> > > Alex
^ permalink raw reply
* Re: [PATCH v8 18/18] drm/virtio: kconfig: Fixup white space.
From: Lee Jones @ 2016-08-30 9:54 UTC (permalink / raw)
To: Peter Griffin
Cc: devicetree, kernel, vinod.koul, linux-remoteproc, patrice.chotard,
dri-devel, linux-kernel, airlied, dmaengine, dan.j.williams,
bjorn.andersson, virtualization, linux-arm-kernel
In-Reply-To: <1472223413-7254-19-git-send-email-peter.griffin@linaro.org>
On Fri, 26 Aug 2016, Peter Griffin wrote:
> Use tabs instead of spaces.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> drivers/gpu/drm/virtio/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
For my own reference:
Acked-by: Lee Jones <lee.jones@linaro.org>
> diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> index 90357d9..2d83932 100644
> --- a/drivers/gpu/drm/virtio/Kconfig
> +++ b/drivers/gpu/drm/virtio/Kconfig
> @@ -2,10 +2,10 @@ config DRM_VIRTIO_GPU
> tristate "Virtio GPU driver"
> depends on DRM
> select VIRTIO
> - select DRM_KMS_HELPER
> - select DRM_TTM
> + select DRM_KMS_HELPER
> + select DRM_TTM
> help
> This is the virtual GPU driver for virtio. It can be used with
> - QEMU based VMMs (like KVM or Xen).
> + QEMU based VMMs (like KVM or Xen).
>
> If unsure say M.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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