* Re: [PATCH v9 15/19] ARM: STi: DT: STiH407: Add uniperif reader dt nodes
From: Patrice Chotard @ 2016-09-14 12:01 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-16-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> 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 | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index 1edc36c..883019a 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -960,5 +960,33 @@
>
> status = "disabled";
> };
> +
> + sti_uni_reader0: sti-uni-reader@8d83000 {
> + compatible = "st,sti-uni-reader";
> + #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>;
> +
> + status = "disabled";
> + };
> +
> + sti_uni_reader1: sti-uni-reader@8d84000 {
> + compatible = "st,sti-uni-reader";
> + #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>;
> +
> + status = "disabled";
> + };
> };
> };
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 14/19] ARM: STi: DT: STiH407: Add uniperif player dt nodes
From: Patrice Chotard @ 2016-09-14 12:01 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-15-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> 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 | 80 +++++++++++++++++++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index d1258d5..1edc36c 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -880,5 +880,85 @@
> status = "disabled";
> st,syscfg = <&syscfg_core>;
> };
> +
> + sti_uni_player0: sti-uni-player@8d80000 {
> + compatible = "st,sti-uni-player";
> + #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";
> +
> + status = "disabled";
> + };
> +
> + sti_uni_player1: sti-uni-player@8d81000 {
> + compatible = "st,sti-uni-player";
> + #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";
> +
> + status = "disabled";
> + };
> +
> + sti_uni_player2: sti-uni-player@8d82000 {
> + compatible = "st,sti-uni-player";
> + #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";
> +
> + status = "disabled";
> + };
> +
> + sti_uni_player3: sti-uni-player@8d85000 {
> + compatible = "st,sti-uni-player";
> + #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";
> +
> + status = "disabled";
> + };
> };
> };
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 13/19] ARM: STi: DT: STiH407: Add sti-sasg-codec dt node
From: Patrice Chotard @ 2016-09-14 12:01 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-14-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> 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>;
> + };
> };
> };
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 12/19] ARM: DT: STiH407: Add spdif_out pinctrl config
From: Patrice Chotard @ 2016-09-14 12:00 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-13-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> This patch adds the pinctrl config for the spidf out
> pins used by the sasg codec IP.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> arch/arm/boot/dts/stih407-pinctrl.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> index 537db7e..598dbab 100644
> --- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> @@ -1114,6 +1114,14 @@
> };
> };
>
> + spdif_out {
> + pinctrl_spdif_out: spdif_out{
> + st,pins {
> + spdif_out = <&pio34 7 ALT1 OUT>;
> + };
> + };
> + };
> +
> serial3 {
> pinctrl_serial3: serial3-0 {
> st,pins {
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 11/19] ARM: DT: STiH407: Add i2s_in pinctrl configuration
From: Patrice Chotard @ 2016-09-14 12:00 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-12-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> This patch adds the pinctrl config for the i2s_in pins
> used by the uniperif reader IP.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> arch/arm/boot/dts/stih407-pinctrl.dtsi | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> index 0fb5c8a..537db7e 100644
> --- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> @@ -1090,6 +1090,30 @@
> };
> };
>
> + i2s_in {
> + pinctrl_i2s_8ch_in: i2s_8ch_in{
> + st,pins {
> + mclk = <&pio32 5 ALT1 IN>;
> + lrclk = <&pio32 7 ALT1 IN>;
> + sclk = <&pio32 6 ALT1 IN>;
> + data0 = <&pio32 4 ALT1 IN>;
> + data1 = <&pio33 0 ALT1 IN>;
> + data2 = <&pio33 1 ALT1 IN>;
> + data3 = <&pio33 2 ALT1 IN>;
> + data4 = <&pio33 3 ALT1 IN>;
> + };
> + };
> +
> + pinctrl_i2s_2ch_in: i2s_2ch_in{
> + st,pins {
> + mclk = <&pio32 5 ALT1 IN>;
> + lrclk = <&pio32 7 ALT1 IN>;
> + sclk = <&pio32 6 ALT1 IN>;
> + data0 = <&pio32 4 ALT1 IN>;
> + };
> + };
> + };
> +
> serial3 {
> pinctrl_serial3: serial3-0 {
> st,pins {
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 10/19] ARM: DT: STiH407: Add i2s_out pinctrl configuration
From: Patrice Chotard @ 2016-09-14 12:00 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, Arnaud Pouliquen, linux-remoteproc, dri-devel,
virtualization, dmaengine, lee.jones
In-Reply-To: <1473081421-16555-11-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> This patch adds the pinctrl config for the i2s_out pins
> used by the uniperif player IP.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> arch/arm/boot/dts/stih407-pinctrl.dtsi | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> index a538ae5..0fb5c8a 100644
> --- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
> @@ -1067,6 +1067,29 @@
> };
> };
>
> + i2s_out {
> + pinctrl_i2s_8ch_out: i2s_8ch_out{
> + st,pins {
> + mclk = <&pio33 5 ALT1 OUT>;
> + lrclk = <&pio33 7 ALT1 OUT>;
> + sclk = <&pio33 6 ALT1 OUT>;
> + data0 = <&pio33 4 ALT1 OUT>;
> + data1 = <&pio34 0 ALT1 OUT>;
> + data2 = <&pio34 1 ALT1 OUT>;
> + data3 = <&pio34 2 ALT1 OUT>;
> + };
> + };
> +
> + pinctrl_i2s_2ch_out: i2s_2ch_out{
> + st,pins {
> + mclk = <&pio33 5 ALT1 OUT>;
> + lrclk = <&pio33 7 ALT1 OUT>;
> + sclk = <&pio33 6 ALT1 OUT>;
> + data0 = <&pio33 4 ALT1 OUT>;
> + };
> + };
> + };
> +
> serial3 {
> pinctrl_serial3: serial3-0 {
> st,pins {
>
Applied for STi next
Thanks
^ permalink raw reply
* Re: [PATCH v9 06/19] ARM: STi: DT: STiH407: Add FDMA driver dt nodes.
From: Patrice Chotard @ 2016-09-14 11:59 UTC (permalink / raw)
To: Peter Griffin, linux-arm-kernel, linux-kernel, kernel, vinod.koul,
dan.j.williams, airlied, kraxel, ohad, bjorn.andersson
Cc: devicetree, linux-remoteproc, dri-devel, virtualization,
dmaengine, lee.jones
In-Reply-To: <1473081421-16555-7-git-send-email-peter.griffin@linaro.org>
Hi Peter
On 09/05/2016 03:16 PM, Peter Griffin wrote:
> These nodes are required to get the fdma driver working
> on STiH407 based silicon.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
> arch/arm/boot/dts/stih407-family.dtsi | 52 +++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index d294e82..45cab30 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -821,5 +821,57 @@
> clock-frequency = <600000000>;
> st,syscfg = <&syscfg_core 0x224>;
> };
> +
> + /* fdma audio */
> + fdma0: dma-controller@8e20000 {
> + compatible = "st,stih407-fdma-mpe31-11", "st,slim-rproc";
> + reg = <0x8e20000 0x8000>,
> + <0x8e30000 0x3000>,
> + <0x8e37000 0x1000>,
> + <0x8e38000 0x8000>;
> + reg-names = "slimcore", "dmem", "peripherals", "imem";
> + clocks = <&clk_s_c0_flexgen CLK_FDMA>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> + interrupts = <GIC_SPI 5 IRQ_TYPE_NONE>;
> + dma-channels = <16>;
> + #dma-cells = <3>;
> + };
> +
> + /* fdma app */
> + fdma1: dma-controller@8e40000 {
> + compatible = "st,stih407-fdma-mpe31-12", "st,slim-rproc";
> + reg = <0x8e40000 0x8000>,
> + <0x8e50000 0x3000>,
> + <0x8e57000 0x1000>,
> + <0x8e58000 0x8000>;
> + reg-names = "slimcore", "dmem", "peripherals", "imem";
> + clocks = <&clk_s_c0_flexgen CLK_FDMA>,
> + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>,
> + <&clk_s_c0_flexgen CLK_TX_ICN_DMU>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> +
> + interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>;
> + dma-channels = <16>;
> + #dma-cells = <3>;
> + };
> +
> + /* fdma free running */
> + fdma2: dma-controller@8e60000 {
> + compatible = "st,stih407-fdma-mpe31-13", "st,slim-rproc";
> + reg = <0x8e60000 0x8000>,
> + <0x8e70000 0x3000>,
> + <0x8e77000 0x1000>,
> + <0x8e78000 0x8000>;
> + reg-names = "slimcore", "dmem", "peripherals", "imem";
> + interrupts = <GIC_SPI 9 IRQ_TYPE_NONE>;
> + dma-channels = <16>;
> + #dma-cells = <3>;
> + clocks = <&clk_s_c0_flexgen CLK_FDMA>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>,
> + <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>,
> + <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> + };
> };
> };
>
Applied for STi next
Thanks
^ permalink raw reply
* [PATCH v2] virtio_pci: Limit DMA mask to 44 bits for legacy virtio devices
From: Will Deacon @ 2016-09-14 11:16 UTC (permalink / raw)
To: virtualization, kvm; +Cc: Will Deacon, Andy Lutomirski, Michael S. Tsirkin
Legacy virtio defines the virtqueue base using a 32-bit PFN field, with
a read-only register indicating a fixed page size of 4k.
This can cause problems for DMA allocators that allocate top down from
the DMA mask, which is set to 64 bits. In this case, the addresses are
silently truncated to 44-bit, leading to IOMMU faults, failure to read
from the queue or data corruption.
This patch restricts the DMA mask for legacy PCI virtio devices to
44 bits, which matches the specification.
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Benjamin Serebrin <serebrin@google.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
drivers/virtio/virtio_pci_legacy.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 8c4e61783441..efb3f5dff4b7 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -212,12 +212,17 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
return -ENODEV;
}
- rc = dma_set_mask_and_coherent(&pci_dev->dev, DMA_BIT_MASK(64));
+ /*
+ * The virtio ring base address is expressed as a 32-bit PFN, with a
+ * page size of 1 << VIRTIO_PCI_QUEUE_ADDR_SHIFT.
+ */
+ rc = dma_set_mask_and_coherent(&pci_dev->dev,
+ DMA_BIT_MASK(32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT));
if (rc)
rc = dma_set_mask_and_coherent(&pci_dev->dev,
DMA_BIT_MASK(32));
if (rc)
- dev_warn(&pci_dev->dev, "Failed to enable 64-bit or 32-bit DMA. Trying to continue, but this might not work.\n");
+ dev_warn(&pci_dev->dev, "Failed to enable %d-bit or 32-bit DMA. Trying to continue, but this might not work.\n", 32 + VIRTIO_PCI_QUEUE_ADDR_SHIFT);
rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
if (rc)
--
2.1.4
^ permalink raw reply related
* Re: virtio_blk: Clarification for communication difficulties?
From: SF Markus Elfring @ 2016-09-14 9:09 UTC (permalink / raw)
To: Cornelia Huck
Cc: Chao Fan, Julia Lawall, Michael S. Tsirkin, kernel-janitors, LKML,
virtualization, Minfei Huang, Stefan Hajnoczi
In-Reply-To: <20160914101009.6abef9f0.cornelia.huck@de.ibm.com>
> FWIW, he already gained a place on my ignore list for pestering me
> offline about his patches and not stopping even when told to do so.
How did I "pester" you "offline"?
> So while I won't object if you choose to apply selected patches,
Another bit of interesting information, isn't it?
> I won't comment on any and won't take any (hey, I even won't see
> them unless someone else replies...)
I find this reaction strange. - I hope that our collaboration potential
can be still constructive, can't it?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v9 01/19] remoteproc: st_slim_rproc: add a slimcore rproc driver
From: Lee Jones @ 2016-09-14 8:30 UTC (permalink / raw)
To: Bjorn Andersson
Cc: devicetree, kernel, vinod.koul, linux-remoteproc, patrice.chotard,
dri-devel, linux-kernel, Peter Griffin, airlied, dmaengine,
dan.j.williams, virtualization, linux-arm-kernel
In-Reply-To: <20160913175656.GC21438@tuxbot>
On Tue, 13 Sep 2016, Bjorn Andersson wrote:
> On Mon 05 Sep 06:16 PDT 2016, Peter Griffin wrote:
>
> > slim core is used as a basis for many IPs in the STi
> > chipsets such as fdma and demux. To avoid duplicating
> > the elf loading code in each device driver a slim
> > rproc driver has been created.
> >
> > This driver is designed to be used by other device drivers
> > such as fdma, or demux whose IP is based around a slim core.
> > The device driver can call slim_rproc_alloc() to allocate
> > a slim rproc and slim_rproc_put() when finished.
> >
> > This driver takes care of ioremapping the slim
> > registers (dmem, imem, slimcore, peripherals), whose offsets
> > and sizes can change between IP's. It also obtains and enables
> > any clocks used by the device. This approach avoids having
> > a double mapping of the registers as slim_rproc does not register
> > its own platform device. It also maps well to device tree
> > abstraction as it allows us to have one dt node for the whole
> > device.
> >
> > All of the generic rproc elf loading code can be reused, and
> > we provide start() stop() hooks to start and stop the slim
> > core once the firmware has been loaded. This has been tested
> > successfully with fdma driver.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
What's preventing this from being applied right away?
> > ---
> > drivers/remoteproc/Kconfig | 4 +
> > drivers/remoteproc/Makefile | 1 +
> > drivers/remoteproc/st_slim_rproc.c | 364 +++++++++++++++++++++++++++++++
> > include/linux/remoteproc/st_slim_rproc.h | 58 +++++
> > 4 files changed, 427 insertions(+)
> > create mode 100644 drivers/remoteproc/st_slim_rproc.c
> > create mode 100644 include/linux/remoteproc/st_slim_rproc.h
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index 1a8bf76a..a7bedc6 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -100,4 +100,8 @@ config ST_REMOTEPROC
> > processor framework.
> > This can be either built-in or a loadable module.
> >
> > +config ST_SLIM_REMOTEPROC
> > + tristate
> > + select REMOTEPROC
> > +
> > endmenu
> > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> > index 92d3758..db1dae7 100644
> > --- a/drivers/remoteproc/Makefile
> > +++ b/drivers/remoteproc/Makefile
> > @@ -14,3 +14,4 @@ obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
> > obj-$(CONFIG_QCOM_MDT_LOADER) += qcom_mdt_loader.o
> > obj-$(CONFIG_QCOM_Q6V5_PIL) += qcom_q6v5_pil.o
> > obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> > +obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> > diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c
> > new file mode 100644
> > index 0000000..1484e97
> > --- /dev/null
> > +++ b/drivers/remoteproc/st_slim_rproc.c
> > @@ -0,0 +1,364 @@
> > +/*
> > + * SLIM core rproc driver
> > + *
> > + * Copyright (C) 2016 STMicroelectronics
> > + *
> > + * Author: Peter Griffin <peter.griffin@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/remoteproc/st_slim_rproc.h>
> > +#include "remoteproc_internal.h"
> > +
> > +/* SLIM core registers */
> > +#define SLIM_ID_OFST 0x0
> > +#define SLIM_VER_OFST 0x4
> > +
> > +#define SLIM_EN_OFST 0x8
> > +#define SLIM_EN_RUN BIT(0)
> > +
> > +#define SLIM_CLK_GATE_OFST 0xC
> > +#define SLIM_CLK_GATE_DIS BIT(0)
> > +#define SLIM_CLK_GATE_RESET BIT(2)
> > +
> > +#define SLIM_SLIM_PC_OFST 0x20
> > +
> > +/* DMEM registers */
> > +#define SLIM_REV_ID_OFST 0x0
> > +#define SLIM_REV_ID_MIN_MASK GENMASK(15, 8)
> > +#define SLIM_REV_ID_MIN(id) ((id & SLIM_REV_ID_MIN_MASK) >> 8)
> > +#define SLIM_REV_ID_MAJ_MASK GENMASK(23, 16)
> > +#define SLIM_REV_ID_MAJ(id) ((id & SLIM_REV_ID_MAJ_MASK) >> 16)
> > +
> > +
> > +/* peripherals registers */
> > +#define SLIM_STBUS_SYNC_OFST 0xF88
> > +#define SLIM_STBUS_SYNC_DIS BIT(0)
> > +
> > +#define SLIM_INT_SET_OFST 0xFD4
> > +#define SLIM_INT_CLR_OFST 0xFD8
> > +#define SLIM_INT_MASK_OFST 0xFDC
> > +
> > +#define SLIM_CMD_CLR_OFST 0xFC8
> > +#define SLIM_CMD_MASK_OFST 0xFCC
> > +
> > +static const char *mem_names[ST_SLIM_MEM_MAX] = {
> > + [ST_SLIM_DMEM] = "dmem",
> > + [ST_SLIM_IMEM] = "imem",
> > +};
> > +
> > +static int slim_clk_get(struct st_slim_rproc *slim_rproc, struct device *dev)
> > +{
> > + int clk, err;
> > +
> > + for (clk = 0; clk < ST_SLIM_MAX_CLK; clk++) {
> > + slim_rproc->clks[clk] = of_clk_get(dev->of_node, clk);
> > + if (IS_ERR(slim_rproc->clks[clk])) {
> > + err = PTR_ERR(slim_rproc->clks[clk]);
> > + if (err == -EPROBE_DEFER)
> > + goto err_put_clks;
> > + slim_rproc->clks[clk] = NULL;
> > + break;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_put_clks:
> > + while (--clk >= 0)
> > + clk_put(slim_rproc->clks[clk]);
> > +
> > + return err;
> > +}
> > +
> > +static void slim_clk_disable(struct st_slim_rproc *slim_rproc)
> > +{
> > + int clk;
> > +
> > + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++)
> > + clk_disable_unprepare(slim_rproc->clks[clk]);
> > +}
> > +
> > +static int slim_clk_enable(struct st_slim_rproc *slim_rproc)
> > +{
> > + int clk, ret;
> > +
> > + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++) {
> > + ret = clk_prepare_enable(slim_rproc->clks[clk]);
> > + if (ret)
> > + goto err_disable_clks;
> > + }
> > +
> > + return 0;
> > +
> > +err_disable_clks:
> > + while (--clk >= 0)
> > + clk_disable_unprepare(slim_rproc->clks[clk]);
> > +
> > + return ret;
> > +}
> > +
> > +/*
> > + * Remoteproc slim specific device handlers
> > + */
> > +static int slim_rproc_start(struct rproc *rproc)
> > +{
> > + struct device *dev = &rproc->dev;
> > + struct st_slim_rproc *slim_rproc = rproc->priv;
> > + unsigned long hw_id, hw_ver, fw_rev;
> > + u32 val;
> > +
> > + /* disable CPU pipeline clock & reset CPU pipeline */
> > + val = SLIM_CLK_GATE_DIS | SLIM_CLK_GATE_RESET;
> > + writel(val, slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> > +
> > + /* disable SLIM core STBus sync */
> > + writel(SLIM_STBUS_SYNC_DIS, slim_rproc->peri + SLIM_STBUS_SYNC_OFST);
> > +
> > + /* enable cpu pipeline clock */
> > + writel(!SLIM_CLK_GATE_DIS,
> > + slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> > +
> > + /* clear int & cmd mailbox */
> > + writel(~0U, slim_rproc->peri + SLIM_INT_CLR_OFST);
> > + writel(~0U, slim_rproc->peri + SLIM_CMD_CLR_OFST);
> > +
> > + /* enable all channels cmd & int */
> > + writel(~0U, slim_rproc->peri + SLIM_INT_MASK_OFST);
> > + writel(~0U, slim_rproc->peri + SLIM_CMD_MASK_OFST);
> > +
> > + /* enable cpu */
> > + writel(SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST);
> > +
> > + hw_id = readl_relaxed(slim_rproc->slimcore + SLIM_ID_OFST);
> > + hw_ver = readl_relaxed(slim_rproc->slimcore + SLIM_VER_OFST);
> > +
> > + fw_rev = readl(slim_rproc->mem[ST_SLIM_DMEM].cpu_addr +
> > + SLIM_REV_ID_OFST);
> > +
> > + dev_info(dev, "fw rev:%ld.%ld on SLIM %ld.%ld\n",
> > + SLIM_REV_ID_MAJ(fw_rev), SLIM_REV_ID_MIN(fw_rev),
> > + hw_id, hw_ver);
> > +
> > + return 0;
> > +}
> > +
> > +static int slim_rproc_stop(struct rproc *rproc)
> > +{
> > + struct st_slim_rproc *slim_rproc = rproc->priv;
> > + u32 val;
> > +
> > + /* mask all (cmd & int) channels */
> > + writel(0UL, slim_rproc->peri + SLIM_INT_MASK_OFST);
> > + writel(0UL, slim_rproc->peri + SLIM_CMD_MASK_OFST);
> > +
> > + /* disable cpu pipeline clock */
> > + writel(SLIM_CLK_GATE_DIS, slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> > +
> > + writel(!SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST);
> > +
> > + val = readl(slim_rproc->slimcore + SLIM_EN_OFST);
> > + if (val & SLIM_EN_RUN)
> > + dev_warn(&rproc->dev, "Failed to disable SLIM");
> > +
> > + dev_dbg(&rproc->dev, "slim stopped\n");
> > +
> > + return 0;
> > +}
> > +
> > +static void *slim_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> > +{
> > + struct st_slim_rproc *slim_rproc = rproc->priv;
> > + void *va = NULL;
> > + int i;
> > +
> > + for (i = 0; i < ST_SLIM_MEM_MAX; i++) {
> > + if (da != slim_rproc->mem[i].bus_addr)
> > + continue;
> > +
> > + if (len <= slim_rproc->mem[i].size) {
> > + /* __force to make sparse happy with type conversion */
> > + va = (__force void *)slim_rproc->mem[i].cpu_addr;
> > + break;
> > + }
> > + }
> > +
> > + dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
> > +
> > + return va;
> > +}
> > +
> > +static struct rproc_ops slim_rproc_ops = {
> > + .start = slim_rproc_start,
> > + .stop = slim_rproc_stop,
> > + .da_to_va = slim_rproc_da_to_va,
> > +};
> > +
> > +/*
> > + * Firmware handler operations: sanity, boot address, load ...
> > + */
> > +
> > +static struct resource_table empty_rsc_tbl = {
> > + .ver = 1,
> > + .num = 0,
> > +};
> > +
> > +static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> > + const struct firmware *fw,
> > + int *tablesz)
> > +{
> > + *tablesz = sizeof(empty_rsc_tbl);
> > + return &empty_rsc_tbl;
> > +}
> > +
> > +static struct rproc_fw_ops slim_rproc_fw_ops = {
> > + .find_rsc_table = slim_rproc_find_rsc_table,
> > +};
> > +
> > +/**
> > + * st_slim_rproc_alloc() - allocate and initialise slim rproc
> > + * @pdev: Pointer to the platform_device struct
> > + * @fw_name: Name of firmware for rproc to use
> > + *
> > + * Function for allocating and initialising a slim rproc for use by
> > + * device drivers whose IP is based around the SLIM core. It
> > + * obtains and enables any clocks required by the SLIM core and also
> > + * ioremaps the various IO.
> > + *
> > + * Returns st_slim_rproc pointer or PTR_ERR() on error.
> > + */
> > +
> > +struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
> > + char *fw_name)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct st_slim_rproc *slim_rproc;
> > + struct device_node *np = dev->of_node;
> > + struct rproc *rproc;
> > + struct resource *res;
> > + int err, i;
> > + const struct rproc_fw_ops *elf_ops;
> > +
> > + if (!fw_name)
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (!of_device_is_compatible(np, "st,slim-rproc"))
> > + return ERR_PTR(-EINVAL);
> > +
> > + rproc = rproc_alloc(dev, np->name, &slim_rproc_ops,
> > + fw_name, sizeof(*slim_rproc));
> > + if (!rproc)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + rproc->has_iommu = false;
> > +
> > + slim_rproc = rproc->priv;
> > + slim_rproc->rproc = rproc;
> > +
> > + elf_ops = rproc->fw_ops;
> > + /* Use some generic elf ops */
> > + slim_rproc_fw_ops.load = elf_ops->load;
> > + slim_rproc_fw_ops.sanity_check = elf_ops->sanity_check;
> > +
> > + rproc->fw_ops = &slim_rproc_fw_ops;
> > +
> > + /* get imem and dmem */
> > + for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > + mem_names[i]);
> > +
> > + slim_rproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(slim_rproc->mem[i].cpu_addr)) {
> > + dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
> > + err = PTR_ERR(slim_rproc->mem[i].cpu_addr);
> > + goto err;
> > + }
> > + slim_rproc->mem[i].bus_addr = res->start;
> > + slim_rproc->mem[i].size = resource_size(res);
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "slimcore");
> > + slim_rproc->slimcore = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(slim_rproc->slimcore)) {
> > + dev_err(&pdev->dev, "failed to ioremap slimcore IO\n");
> > + err = PTR_ERR(slim_rproc->slimcore);
> > + goto err;
> > + }
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "peripherals");
> > + slim_rproc->peri = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(slim_rproc->peri)) {
> > + dev_err(&pdev->dev, "failed to ioremap peripherals IO\n");
> > + err = PTR_ERR(slim_rproc->peri);
> > + goto err;
> > + }
> > +
> > + err = slim_clk_get(slim_rproc, dev);
> > + if (err)
> > + goto err;
> > +
> > + err = slim_clk_enable(slim_rproc);
> > + if (err) {
> > + dev_err(dev, "Failed to enable clocks\n");
> > + goto err_clk_put;
> > + }
> > +
> > + /* Register as a remoteproc device */
> > + err = rproc_add(rproc);
> > + if (err) {
> > + dev_err(dev, "registration of slim remoteproc failed\n");
> > + goto err_clk_dis;
> > + }
> > +
> > + return slim_rproc;
> > +
> > +err_clk_dis:
> > + slim_clk_disable(slim_rproc);
> > +err_clk_put:
> > + for (i = 0; i < ST_SLIM_MAX_CLK && slim_rproc->clks[i]; i++)
> > + clk_put(slim_rproc->clks[i]);
> > +err:
> > + rproc_put(rproc);
> > + return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL(st_slim_rproc_alloc);
> > +
> > +/**
> > + * st_slim_rproc_put() - put slim rproc resources
> > + * @slim_rproc: Pointer to the st_slim_rproc struct
> > + *
> > + * Function for calling respective _put() functions on slim_rproc resources.
> > + *
> > + */
> > +void st_slim_rproc_put(struct st_slim_rproc *slim_rproc)
> > +{
> > + int clk;
> > +
> > + if (!slim_rproc)
> > + return;
> > +
> > + slim_clk_disable(slim_rproc);
> > +
> > + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++)
> > + clk_put(slim_rproc->clks[clk]);
> > +
> > + rproc_del(slim_rproc->rproc);
> > + rproc_put(slim_rproc->rproc);
> > +}
> > +EXPORT_SYMBOL(st_slim_rproc_put);
> > +
> > +MODULE_AUTHOR("Peter Griffin <peter.griffin@linaro.org>");
> > +MODULE_DESCRIPTION("STMicroelectronics SLIM core rproc driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/remoteproc/st_slim_rproc.h b/include/linux/remoteproc/st_slim_rproc.h
> > new file mode 100644
> > index 0000000..4155556
> > --- /dev/null
> > +++ b/include/linux/remoteproc/st_slim_rproc.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * SLIM core rproc driver header
> > + *
> > + * Copyright (C) 2016 STMicroelectronics
> > + *
> > + * Author: Peter Griffin <peter.griffin@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +#ifndef _ST_REMOTEPROC_SLIM_H
> > +#define _ST_REMOTEPROC_SLIM_H
> > +
> > +#define ST_SLIM_MEM_MAX 2
> > +#define ST_SLIM_MAX_CLK 4
> > +
> > +enum {
> > + ST_SLIM_DMEM,
> > + ST_SLIM_IMEM,
> > +};
> > +
> > +/**
> > + * struct st_slim_mem - slim internal memory structure
> > + * @cpu_addr: MPU virtual address of the memory region
> > + * @bus_addr: Bus address used to access the memory region
> > + * @size: Size of the memory region
> > + */
> > +struct st_slim_mem {
> > + void __iomem *cpu_addr;
> > + phys_addr_t bus_addr;
> > + size_t size;
> > +};
> > +
> > +/**
> > + * struct st_slim_rproc - SLIM slim core
> > + * @rproc: rproc handle
> > + * @mem: slim memory information
> > + * @slimcore: slim slimcore regs
> > + * @peri: slim peripheral regs
> > + * @clks: slim clocks
> > + */
> > +struct st_slim_rproc {
> > + struct rproc *rproc;
> > + struct st_slim_mem mem[ST_SLIM_MEM_MAX];
> > + void __iomem *slimcore;
> > + void __iomem *peri;
> > +
> > + /* st_slim_rproc private */
> > + struct clk *clks[ST_SLIM_MAX_CLK];
> > +};
> > +
> > +struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
> > + char *fw_name);
> > +void st_slim_rproc_put(struct st_slim_rproc *slim_rproc);
> > +
> > +#endif
--
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
* Re: virtio_blk: Less function calls in init_vq() after error detection
From: Cornelia Huck @ 2016-09-14 8:10 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Chao Fan, Minfei Huang, Michael S. Tsirkin, kernel-janitors, LKML,
virtualization, Julia Lawall, Stefan Hajnoczi, SF Markus Elfring
In-Reply-To: <44fd46f6-441a-d497-9157-7e2a0f3f45da@de.ibm.com>
On Tue, 13 Sep 2016 20:24:58 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> See, some of your patches are accepted, e.g. the memdup_user changes have usually
> been applied by most maintainers including myself. If maintainers won't take other change,
> please accept that. If you continue to waste peoples time by discussing "maybe" patches
> you actually risk that people will stop taking any patches from you including the "yes"
> ones.
FWIW, he already gained a place on my ignore list for pestering me
offline about his patches and not stopping even when told to do so. So
while I won't object if you choose to apply selected patches, I won't
comment on any and won't take any (hey, I even won't see them unless
someone else replies...)
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v10] virtio-net: add Max MTU configuration field
From: Cornelia Huck @ 2016-09-14 7:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Victor Kaplansky, virtio-dev, Maxime Coquelin, virtualization
In-Reply-To: <20160914000341-mutt-send-email-mst@kernel.org>
On Wed, 14 Sep 2016 00:17:28 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Sep 13, 2016 at 11:18:12AM -0400, Aaron Conole wrote:
(...)
> > If negotiated, the driver uses \field{mtu} as
> > + the maximum MTU value supplied to the driver.
>
> Drop "supplied to the driver" here?
Reword as "If negotiated, the device uses \field{mtu} to supply the
maximum MTU value supported to the driver." ?
I agree with your other comments. Other than that, the update looks
good.
^ permalink raw reply
* Re: [PATCH v9 00/19] Add support for FDMA DMA controller and slim core rproc found on STi chipsets
From: Patrice Chotard @ 2016-09-14 6:59 UTC (permalink / raw)
To: Bjorn Andersson, Peter Griffin, vinod.koul
Cc: devicetree, kernel, airlied, linux-remoteproc, linux-kernel,
dri-devel, virtualization, dmaengine, dan.j.williams, lee.jones,
linux-arm-kernel
In-Reply-To: <20160913180616.GD21438@tuxbot>
Hi Bjorn
On 09/13/2016 08:06 PM, Bjorn Andersson wrote:
> On Tue 13 Sep 02:31 PDT 2016, Peter Griffin wrote:
>
>> Hi Vinod & Bjorn,
>>
>> [..]
>>
>> On Mon, 05 Sep 2016, Peter Griffin wrote:
>>
>>> v8 actions some review feedback from Bjorn to the slim rproc driver, and also includes
>>> a patch which fixes a recursive Kconfig error which is triggered when st_fdma selects
>>> slim_rproc driver. The series has also been rebased on v4.8-rc3.
>>>
>>> v9 actions some review feedback from Bjorn, Lee and Vinod. See below. Importantly a bug
>>> was found during testing now that the platform boots without clk_ignore_unused parameter
>>> whereby the clocks would not be enabled properly before firmware loading was attempted.
>>>
>>> regards,
>>>
>>> Peter.
>>>
>>> Changes since v8:
>>> - Add MODULE_ALIAS (Vinod)
>>> - devm_kzalloc to devm_kcalloc (Vinod)
>>> - quisce tasklet initialised by vchan_init() (Vinod)
>>> - Don't make SLIM rproc user selectable (Bjorn)
>>> - slim_rproc: Ensure clocks enabled before firmware load (Peter)
>>> - Various code style nits / commit message change (Lee)
>>> - Separate patch for '\n' kconfig removal (Vinod)
>>
>> I hate to send a ping,
>
> Sorry about that.
>
>> but do you think we can merge this fdma series? It has gone
>> through quite a few review rounds now.
>>
>
> I think the remoteproc part looks good.
>
> Vinod, I don't have any changes queued in remoteproc that should cause
> merge issues. If you want to you could take the remoteproc patch
> through your tree.
>
>
> I do however think that the dts patches should go through arm-soc.
I will take care about dts patches by adding them in the next STi DT pull request
Thanks
Patrice
>
> Regards,
> Bjorn
>
^ permalink raw reply
* Re: virtio_blk: Less function calls in init_vq() after error detection
From: SF Markus Elfring @ 2016-09-14 6:56 UTC (permalink / raw)
To: Christian Bornträger, virtualization, Michael S. Tsirkin
Cc: Minfei Huang, Chao Fan, kernel-janitors, LKML, Julia Lawall,
Stefan Hajnoczi
In-Reply-To: <44fd46f6-441a-d497-9157-7e2a0f3f45da@de.ibm.com>
>> How much will it matter in general that two function calls are performed
>> in this use case without checking their return values immediately?
>> https://cwe.mitre.org/data/definitions/252.html
>>
>> if (!names || !callbacks || !vqs) { …
>>
>> https://cwe.mitre.org/data/definitions/754.html
>
> The return values are checked,
I agree to this information.
> just a bit later.
I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.
> Markus, kernel patches are not about be formally correct vs. CodingStyle and/or checkpatch
I guess that some of the involved technical advices will also matter here, don't they?
> or against code guidelines from sombody else.
Some ideas or advices are integrated from other information sources also into various
Linux software.
> You will find many people consider gotos an no-go,
I became trained also in this design direction for a while.
> still it is accepted in the kernel for good reasons.
I can follow such reasons to some degree.
> You have to think about each change and its tradeoffs (e.g simplicity vs. performance)
> for each code part again.
This can happen as usual for an ordinary source code review process.
I would be interested to clarify if items could be optimised for repeated checks
in this work flow.
> Here we have slow path error handling, so given the low coverage of these code parts,
Would you like to add any other background information for this aspect?
> simplicity is important.
I can follow this opinion to some degree.
> Yes, your code makes an unlikely error case bail out faster,
Is this kind of functionality desired finally?
> but is the cost of your patch (review time, danger of adding bugs, danger of merge conflicts,
> making git blame less useful) in sync with the expected win?
I hope so.
Do the mentioned aspects express a fear or other general concerns which hinder changes
and make the proposed software improvement unlikely?
> (Well, having a label between the if and the function like
>> if (err)
>> + free_vblk_vqs:
>> kfree(vblk->vqs);
>
> is certainly ugly in itself)
Would you find such a source code approach better if curly brackets will be
added there?
> Do you realize that your discussion style is not very helpful?
I find that this view is debatable.
> I just grepped the last LKML mails and you already pissed of several maintainers
> in totally different areas.
Yes. - It can happen that some contributors get occasionally grumpy.
Would you like to discuss their reasons a bit more?
> When that happens, why don't you stop for a moment
> and think about "what is going wrong right now".
This happened also a few times already.
> Your attitude seems to be "I spend my spare time doing this, please thank me for that".
I guess that I am not so different in this aspect as I am trying to be another
respectable free software developer for years.
> The thing is, with each patch you actually request time from the maintainer.
This is a consequence of software development as usual.
> Now here begins the interesting part: Is the patch just a cosmetic change that does
> not give you any benefit or is the patch improving the code.
How do you categorise my concrete update suggestion which builds on previous contributions
by others?
> And remember: there are always tradeoffs: performance, code size, code maintainability etc.
I find that I pick some of such design goals up for my software contributions already
for a while.
> See, some of your patches are accepted, e.g. the memdup_user changes have usually
> been applied by most maintainers including myself.
I thank you once more that you could also accept one of the proposed special software updates.
> If maintainers won't take other change, please accept that.
It can happen that change acceptance needs to evolve over time.
Can a healthy pressure help to achieve special software improvements?
> If you continue to waste peoples time by discussing "maybe" patches
Can it be that this category of software updates has got a high potential for further
clarifications because some approaches are occasionally interpreted as controversial?
> you actually risk that people will stop taking any patches from you
> including the "yes" ones.
I assume that this risk is hard to avoid. - It is a matter for each contributor
on how the desired communication should evolve further.
My knowledge evolved in the way that I am using some tools for
static source code analysis. Such advanced tools can point various
change opportunities out. I picked a few special search patterns up.
It happened then that hundreds of source files were found which contain
update candidates.
Further challenges are relevant then as usual.
* Handling of the search process and their results
* Communication between contributors
Search patterns can occasionally be categorised as "too special".
The software technology contains also the risk for showing "false positives".
The reactions of code reviewers are varying between agreement and rejection.
The discussed concrete (and small) patch series is just another example
for usual difficulties or more interesting software development challenges.
I hope that they can be resolved in a systematic way.
So there are further constraints to consider, aren't there?
Regards,
Markus
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v10] virtio-net: add Max MTU configuration field
From: Hannes Reinecke @ 2016-09-14 6:21 UTC (permalink / raw)
To: Aaron Conole, virtio-dev, virtualization
Cc: Victor Kaplansky, Maxime Coquelin, Michael S. Tsirkin
In-Reply-To: <1473779892-6234-1-git-send-email-aconole@redhat.com>
On 09/13/2016 05:18 PM, Aaron Conole wrote:
> It is helpful for a host to indicate it's MTU to be set on guest NICs
> other than the assumed 1500 byte value. This helps in situations where
> the host network is using Jumbo Frames, or aiding in PMTU discovery by
> configuring a homogenous network. It is also helpful for sizing receive
> buffers correctly.
>
> The change adds a new field to configuration area of network
> devices. It will be used to pass a maximum MTU from the device to
> the driver. This will be used by the driver as a maximum value for
> packet sizes during transmission, without segmentation offloading.
>
> In addition, in order to support backward and forward compatibility,
> we introduce a new feature bit called VIRTIO_NET_F_MTU.
>
> VIRTIO-152
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Cc: Victor Kaplansky <victork@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
Very good. I'm needing this for my FCoE over virtio work.
Reviewed-by: Hannes Reiencke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH v10] virtio-net: add Max MTU configuration field
From: Michael S. Tsirkin @ 2016-09-13 21:17 UTC (permalink / raw)
To: Aaron Conole
Cc: Victor Kaplansky, virtio-dev, Maxime Coquelin, virtualization
In-Reply-To: <1473779892-6234-1-git-send-email-aconole@redhat.com>
On Tue, Sep 13, 2016 at 11:18:12AM -0400, Aaron Conole wrote:
> It is helpful for a host to indicate it's MTU to be set on guest NICs
> other than the assumed 1500 byte value. This helps in situations where
> the host network is using Jumbo Frames, or aiding in PMTU discovery by
> configuring a homogenous network. It is also helpful for sizing receive
> buffers correctly.
>
> The change adds a new field to configuration area of network
> devices. It will be used to pass a maximum MTU from the device to
> the driver. This will be used by the driver as a maximum value for
> packet sizes during transmission, without segmentation offloading.
>
> In addition, in order to support backward and forward compatibility,
> we introduce a new feature bit called VIRTIO_NET_F_MTU.
>
> VIRTIO-152
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Cc: Victor Kaplansky <victork@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> v1:
> This is an attempt at continuing the work done by Victor Kaplansky on
> mtu negiotiation for virtio-net devices. It attempts to pick up from
> https://lists.oasis-open.org/archives/virtio-dev/201508/msg00007.html
> and is just a minor blurb from the first patch along with the 2nd patch
> from the series, and some of the feedback integrated.
>
> v2:
> Rephrase and provide a mechanism for guest->host and host->guest
> communication through a driver read-only and driver write-only field.
>
> v3:
> Converted to just support initial MTU. Guest->host and Host->guest MTU
> changes are outside the scope of this change.
>
> v4:
> Removed references to 'initial', since that condition cannot be tested.
> Simply state that if the driver will use the mtu field, it must
> negotiate the feature bit, and if not, it must not.
>
> v5:
> After feedback from Michael S. Tsirkin
>
> v6:
> Bit has to change to bit 25 due to an undocumented bit 24 being taken.
>
> v7:
> Partial rewrite with feedback from MST. Additional conformance statements
> added.
>
> v8:
> Clarified the new conformance statements.
>
> v9:
> Updated the commit log for correctness. Added ACKs from
> Michael S. Tsirkin, and Maxime Coquelin. Included the virtio
> issue id.
>
> v10:
> Update the conformance statement wordings from previous 'offered' form
> to 'succesfully negotiated' form.
>
> content.tex | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 4b45678..42c0568 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3049,6 +3049,11 @@ features.
> \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
> reconfiguration support.
>
> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported.
I would say "device MTU reporting" is supported.
> If
> + offered by the device, device advises driver about the value of
> + MTU to be used.
about its maximum MTU value.
> If negotiated, the driver uses \field{mtu} as
> + the maximum MTU value supplied to the driver.
Drop "supplied to the driver" here?
> +
> \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>
> \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> @@ -3140,11 +3145,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
> is negotiated.
>
> +The following driver-read-only field, \field{mtu} only exists if
> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> +use.
> +
> \begin{lstlisting}
> struct virtio_net_config {
> u8 mac[6];
> le16 status;
> le16 max_virtqueue_pairs;
> + le16 mtu;
> };
> \end{lstlisting}
>
> @@ -3153,6 +3163,19 @@ struct virtio_net_config {
> The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
> if it offers VIRTIO_NET_F_MQ.
>
> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> +if it offers VIRTIO_NET_F_MTU.
Apropos, IPv6 requires that every link in the internet have an MTU of
1280 octets or greater. There are also requirements to be able to
accept 576 byte packets. Maybe add a SHOULD?
> +
> +The device MUST NOT modify \field{mtu} once it has been set.
> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with \field{gso_type} NONE or ECN after VIRTIO_NET_F_MTU has been successfully
> +negotiated.
> +
> +The device MUST forward transmitted packets of up to MTU size with
> +\field{gso_type} NONE or ECN, and do so without fragmentation, after
> +VIRTIO_NET_F_MTU has been successfully negotiated.
> +
Maybe clarify that this is MTU + ethernet headers.
E.g. up to MTU size (plus low level headers).
> \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>
> A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3165,6 +3188,15 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
> assume the link is active, otherwise it SHOULD read the link status from
> the bottom bit of \field{status}.
>
> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> +
> +If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive
> +buffers of size \field{mtu}
hmm in fact buffers can and should be bigger, and need to include ethernet headers.
I would say:
> to be able to receive at least one receive
> +packet with \field{gso_type} NONE or ECN.
... of size \field{mtu} (plus low level headers).
> +
> +If the driver negotiates VIRTIO_NET_F_MTU, it MUST NOT transmit packets of
> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> +
> \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
> When using the legacy interface, transitional devices and drivers
> --
> 2.7.4
^ permalink raw reply
* Re: virtio_blk: Less function calls in init_vq() after error detection
From: Christian Borntraeger @ 2016-09-13 18:24 UTC (permalink / raw)
To: SF Markus Elfring, virtualization, Michael S. Tsirkin,
Minfei Huang, Cornelia Huck, Stefan Hajnoczi
Cc: Julia Lawall, Chao Fan, kernel-janitors, LKML
In-Reply-To: <7da823eb-939c-9ee6-32bf-db296e6a96f6@users.sourceforge.net>
On 09/13/2016 07:30 PM, SF Markus Elfring wrote:
[...]
> Unfortunately, I get an other impression here after a closer look.
>
> Can it be that the discussed commit from 2016-08-09 accepted (or tolerated)
> two weaknesses at least?
>
> 1. Commit title:
> Is the word "slient" a typo?
> Would you like to read "silent" there instead?
>
> 2. Source code:
> Why would another memory allocation be attempted if it could be determined quicker
> that a previous one failed and this function implementation can not succeed then?
>
> How much will it matter in general that two function calls are performed
> in this use case without checking their return values immediately?
> https://cwe.mitre.org/data/definitions/252.html
>
> if (!names || !callbacks || !vqs) { …
>
> https://cwe.mitre.org/data/definitions/754.html
>
The return values are checked, just a bit later.
Markus, kernel patches are not about be formally correct vs. CodingStyle and/or checkpatch
or against code guidelines from sombody else. You will find many people consider gotos
an no-go, still it is accepted in the kernel for good reasons.
You have to think about each change and its tradeoffs (e.g simplicity vs. performance)
for each code part again. Here we have slow path error handling, so given the low coverage
of these code parts, simplicity is important.
Yes, your code makes an unlikely error case bail out faster, but is the cost of your
patch (review time, danger of adding bugs, danger of merge conflicts, making git blame less
useful) in sync with the expected win? This is certainly Michaels area of maintainership
and if he wants your patch, it will be fine too. (Well, having a label between the if and
the function like
> if (err)
> + free_vblk_vqs:
> kfree(vblk->vqs);
is certainly ugly in itself)
> Was the software development attention a bit too low as it happens occasionally?
>
>
> I hope that my suggestions can improve the affected situation a bit more
> also for this software module.
Do you realize that your discussion style is not very helpful?
I just grepped the last LKML mails and you already pissed of several maintainers
in totally different areas. When that happens, why don't you stop for a moment and
think about "what is going wrong right now".
Your attitude seems to be "I spend my spare time doing this, please thank me for that".
The thing is, with each patch you actually request time from the maintainer.
Now here begins the interesting part: Is the patch just a cosmetic change that does
not give you any benefit or is the patch improving the code. And remember: there
are always tradeoffs: performance, code size, code maintainability etc.
See, some of your patches are accepted, e.g. the memdup_user changes have usually
been applied by most maintainers including myself. If maintainers won't take other change,
please accept that. If you continue to waste peoples time by discussing "maybe" patches
you actually risk that people will stop taking any patches from you including the "yes"
ones.
Christian
^ permalink raw reply
* Re: [PATCH v9 00/19] Add support for FDMA DMA controller and slim core rproc found on STi chipsets
From: Bjorn Andersson @ 2016-09-13 18:06 UTC (permalink / raw)
To: Peter Griffin, vinod.koul
Cc: devicetree, kernel, airlied, linux-remoteproc, patrice.chotard,
dri-devel, linux-kernel, dmaengine, dan.j.williams,
virtualization, lee.jones, linux-arm-kernel
In-Reply-To: <20160913093107.GA10953@griffinp-ThinkPad-X1-Carbon-2nd>
On Tue 13 Sep 02:31 PDT 2016, Peter Griffin wrote:
> Hi Vinod & Bjorn,
>
> [..]
>
> On Mon, 05 Sep 2016, Peter Griffin wrote:
>
> > v8 actions some review feedback from Bjorn to the slim rproc driver, and also includes
> > a patch which fixes a recursive Kconfig error which is triggered when st_fdma selects
> > slim_rproc driver. The series has also been rebased on v4.8-rc3.
> >
> > v9 actions some review feedback from Bjorn, Lee and Vinod. See below. Importantly a bug
> > was found during testing now that the platform boots without clk_ignore_unused parameter
> > whereby the clocks would not be enabled properly before firmware loading was attempted.
> >
> > regards,
> >
> > Peter.
> >
> > Changes since v8:
> > - Add MODULE_ALIAS (Vinod)
> > - devm_kzalloc to devm_kcalloc (Vinod)
> > - quisce tasklet initialised by vchan_init() (Vinod)
> > - Don't make SLIM rproc user selectable (Bjorn)
> > - slim_rproc: Ensure clocks enabled before firmware load (Peter)
> > - Various code style nits / commit message change (Lee)
> > - Separate patch for '\n' kconfig removal (Vinod)
>
> I hate to send a ping,
Sorry about that.
> but do you think we can merge this fdma series? It has gone
> through quite a few review rounds now.
>
I think the remoteproc part looks good.
Vinod, I don't have any changes queued in remoteproc that should cause
merge issues. If you want to you could take the remoteproc patch
through your tree.
I do however think that the dts patches should go through arm-soc.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v9 01/19] remoteproc: st_slim_rproc: add a slimcore rproc driver
From: Bjorn Andersson @ 2016-09-13 17:56 UTC (permalink / raw)
To: Peter Griffin
Cc: devicetree, kernel, vinod.koul, linux-remoteproc, patrice.chotard,
dri-devel, linux-kernel, airlied, dmaengine, dan.j.williams,
virtualization, lee.jones, linux-arm-kernel
In-Reply-To: <1473081421-16555-2-git-send-email-peter.griffin@linaro.org>
On Mon 05 Sep 06:16 PDT 2016, Peter Griffin wrote:
> slim core is used as a basis for many IPs in the STi
> chipsets such as fdma and demux. To avoid duplicating
> the elf loading code in each device driver a slim
> rproc driver has been created.
>
> This driver is designed to be used by other device drivers
> such as fdma, or demux whose IP is based around a slim core.
> The device driver can call slim_rproc_alloc() to allocate
> a slim rproc and slim_rproc_put() when finished.
>
> This driver takes care of ioremapping the slim
> registers (dmem, imem, slimcore, peripherals), whose offsets
> and sizes can change between IP's. It also obtains and enables
> any clocks used by the device. This approach avoids having
> a double mapping of the registers as slim_rproc does not register
> its own platform device. It also maps well to device tree
> abstraction as it allows us to have one dt node for the whole
> device.
>
> All of the generic rproc elf loading code can be reused, and
> we provide start() stop() hooks to start and stop the slim
> core once the firmware has been loaded. This has been tested
> successfully with fdma driver.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> drivers/remoteproc/Kconfig | 4 +
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/st_slim_rproc.c | 364 +++++++++++++++++++++++++++++++
> include/linux/remoteproc/st_slim_rproc.h | 58 +++++
> 4 files changed, 427 insertions(+)
> create mode 100644 drivers/remoteproc/st_slim_rproc.c
> create mode 100644 include/linux/remoteproc/st_slim_rproc.h
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 1a8bf76a..a7bedc6 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -100,4 +100,8 @@ config ST_REMOTEPROC
> processor framework.
> This can be either built-in or a loadable module.
>
> +config ST_SLIM_REMOTEPROC
> + tristate
> + select REMOTEPROC
> +
> endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 92d3758..db1dae7 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
> obj-$(CONFIG_QCOM_MDT_LOADER) += qcom_mdt_loader.o
> obj-$(CONFIG_QCOM_Q6V5_PIL) += qcom_q6v5_pil.o
> obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o
> +obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o
> diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c
> new file mode 100644
> index 0000000..1484e97
> --- /dev/null
> +++ b/drivers/remoteproc/st_slim_rproc.c
> @@ -0,0 +1,364 @@
> +/*
> + * SLIM core rproc driver
> + *
> + * Copyright (C) 2016 STMicroelectronics
> + *
> + * Author: Peter Griffin <peter.griffin@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/remoteproc/st_slim_rproc.h>
> +#include "remoteproc_internal.h"
> +
> +/* SLIM core registers */
> +#define SLIM_ID_OFST 0x0
> +#define SLIM_VER_OFST 0x4
> +
> +#define SLIM_EN_OFST 0x8
> +#define SLIM_EN_RUN BIT(0)
> +
> +#define SLIM_CLK_GATE_OFST 0xC
> +#define SLIM_CLK_GATE_DIS BIT(0)
> +#define SLIM_CLK_GATE_RESET BIT(2)
> +
> +#define SLIM_SLIM_PC_OFST 0x20
> +
> +/* DMEM registers */
> +#define SLIM_REV_ID_OFST 0x0
> +#define SLIM_REV_ID_MIN_MASK GENMASK(15, 8)
> +#define SLIM_REV_ID_MIN(id) ((id & SLIM_REV_ID_MIN_MASK) >> 8)
> +#define SLIM_REV_ID_MAJ_MASK GENMASK(23, 16)
> +#define SLIM_REV_ID_MAJ(id) ((id & SLIM_REV_ID_MAJ_MASK) >> 16)
> +
> +
> +/* peripherals registers */
> +#define SLIM_STBUS_SYNC_OFST 0xF88
> +#define SLIM_STBUS_SYNC_DIS BIT(0)
> +
> +#define SLIM_INT_SET_OFST 0xFD4
> +#define SLIM_INT_CLR_OFST 0xFD8
> +#define SLIM_INT_MASK_OFST 0xFDC
> +
> +#define SLIM_CMD_CLR_OFST 0xFC8
> +#define SLIM_CMD_MASK_OFST 0xFCC
> +
> +static const char *mem_names[ST_SLIM_MEM_MAX] = {
> + [ST_SLIM_DMEM] = "dmem",
> + [ST_SLIM_IMEM] = "imem",
> +};
> +
> +static int slim_clk_get(struct st_slim_rproc *slim_rproc, struct device *dev)
> +{
> + int clk, err;
> +
> + for (clk = 0; clk < ST_SLIM_MAX_CLK; clk++) {
> + slim_rproc->clks[clk] = of_clk_get(dev->of_node, clk);
> + if (IS_ERR(slim_rproc->clks[clk])) {
> + err = PTR_ERR(slim_rproc->clks[clk]);
> + if (err == -EPROBE_DEFER)
> + goto err_put_clks;
> + slim_rproc->clks[clk] = NULL;
> + break;
> + }
> + }
> +
> + return 0;
> +
> +err_put_clks:
> + while (--clk >= 0)
> + clk_put(slim_rproc->clks[clk]);
> +
> + return err;
> +}
> +
> +static void slim_clk_disable(struct st_slim_rproc *slim_rproc)
> +{
> + int clk;
> +
> + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++)
> + clk_disable_unprepare(slim_rproc->clks[clk]);
> +}
> +
> +static int slim_clk_enable(struct st_slim_rproc *slim_rproc)
> +{
> + int clk, ret;
> +
> + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++) {
> + ret = clk_prepare_enable(slim_rproc->clks[clk]);
> + if (ret)
> + goto err_disable_clks;
> + }
> +
> + return 0;
> +
> +err_disable_clks:
> + while (--clk >= 0)
> + clk_disable_unprepare(slim_rproc->clks[clk]);
> +
> + return ret;
> +}
> +
> +/*
> + * Remoteproc slim specific device handlers
> + */
> +static int slim_rproc_start(struct rproc *rproc)
> +{
> + struct device *dev = &rproc->dev;
> + struct st_slim_rproc *slim_rproc = rproc->priv;
> + unsigned long hw_id, hw_ver, fw_rev;
> + u32 val;
> +
> + /* disable CPU pipeline clock & reset CPU pipeline */
> + val = SLIM_CLK_GATE_DIS | SLIM_CLK_GATE_RESET;
> + writel(val, slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> +
> + /* disable SLIM core STBus sync */
> + writel(SLIM_STBUS_SYNC_DIS, slim_rproc->peri + SLIM_STBUS_SYNC_OFST);
> +
> + /* enable cpu pipeline clock */
> + writel(!SLIM_CLK_GATE_DIS,
> + slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> +
> + /* clear int & cmd mailbox */
> + writel(~0U, slim_rproc->peri + SLIM_INT_CLR_OFST);
> + writel(~0U, slim_rproc->peri + SLIM_CMD_CLR_OFST);
> +
> + /* enable all channels cmd & int */
> + writel(~0U, slim_rproc->peri + SLIM_INT_MASK_OFST);
> + writel(~0U, slim_rproc->peri + SLIM_CMD_MASK_OFST);
> +
> + /* enable cpu */
> + writel(SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST);
> +
> + hw_id = readl_relaxed(slim_rproc->slimcore + SLIM_ID_OFST);
> + hw_ver = readl_relaxed(slim_rproc->slimcore + SLIM_VER_OFST);
> +
> + fw_rev = readl(slim_rproc->mem[ST_SLIM_DMEM].cpu_addr +
> + SLIM_REV_ID_OFST);
> +
> + dev_info(dev, "fw rev:%ld.%ld on SLIM %ld.%ld\n",
> + SLIM_REV_ID_MAJ(fw_rev), SLIM_REV_ID_MIN(fw_rev),
> + hw_id, hw_ver);
> +
> + return 0;
> +}
> +
> +static int slim_rproc_stop(struct rproc *rproc)
> +{
> + struct st_slim_rproc *slim_rproc = rproc->priv;
> + u32 val;
> +
> + /* mask all (cmd & int) channels */
> + writel(0UL, slim_rproc->peri + SLIM_INT_MASK_OFST);
> + writel(0UL, slim_rproc->peri + SLIM_CMD_MASK_OFST);
> +
> + /* disable cpu pipeline clock */
> + writel(SLIM_CLK_GATE_DIS, slim_rproc->slimcore + SLIM_CLK_GATE_OFST);
> +
> + writel(!SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST);
> +
> + val = readl(slim_rproc->slimcore + SLIM_EN_OFST);
> + if (val & SLIM_EN_RUN)
> + dev_warn(&rproc->dev, "Failed to disable SLIM");
> +
> + dev_dbg(&rproc->dev, "slim stopped\n");
> +
> + return 0;
> +}
> +
> +static void *slim_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> +{
> + struct st_slim_rproc *slim_rproc = rproc->priv;
> + void *va = NULL;
> + int i;
> +
> + for (i = 0; i < ST_SLIM_MEM_MAX; i++) {
> + if (da != slim_rproc->mem[i].bus_addr)
> + continue;
> +
> + if (len <= slim_rproc->mem[i].size) {
> + /* __force to make sparse happy with type conversion */
> + va = (__force void *)slim_rproc->mem[i].cpu_addr;
> + break;
> + }
> + }
> +
> + dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
> +
> + return va;
> +}
> +
> +static struct rproc_ops slim_rproc_ops = {
> + .start = slim_rproc_start,
> + .stop = slim_rproc_stop,
> + .da_to_va = slim_rproc_da_to_va,
> +};
> +
> +/*
> + * Firmware handler operations: sanity, boot address, load ...
> + */
> +
> +static struct resource_table empty_rsc_tbl = {
> + .ver = 1,
> + .num = 0,
> +};
> +
> +static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> + const struct firmware *fw,
> + int *tablesz)
> +{
> + *tablesz = sizeof(empty_rsc_tbl);
> + return &empty_rsc_tbl;
> +}
> +
> +static struct rproc_fw_ops slim_rproc_fw_ops = {
> + .find_rsc_table = slim_rproc_find_rsc_table,
> +};
> +
> +/**
> + * st_slim_rproc_alloc() - allocate and initialise slim rproc
> + * @pdev: Pointer to the platform_device struct
> + * @fw_name: Name of firmware for rproc to use
> + *
> + * Function for allocating and initialising a slim rproc for use by
> + * device drivers whose IP is based around the SLIM core. It
> + * obtains and enables any clocks required by the SLIM core and also
> + * ioremaps the various IO.
> + *
> + * Returns st_slim_rproc pointer or PTR_ERR() on error.
> + */
> +
> +struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
> + char *fw_name)
> +{
> + struct device *dev = &pdev->dev;
> + struct st_slim_rproc *slim_rproc;
> + struct device_node *np = dev->of_node;
> + struct rproc *rproc;
> + struct resource *res;
> + int err, i;
> + const struct rproc_fw_ops *elf_ops;
> +
> + if (!fw_name)
> + return ERR_PTR(-EINVAL);
> +
> + if (!of_device_is_compatible(np, "st,slim-rproc"))
> + return ERR_PTR(-EINVAL);
> +
> + rproc = rproc_alloc(dev, np->name, &slim_rproc_ops,
> + fw_name, sizeof(*slim_rproc));
> + if (!rproc)
> + return ERR_PTR(-ENOMEM);
> +
> + rproc->has_iommu = false;
> +
> + slim_rproc = rproc->priv;
> + slim_rproc->rproc = rproc;
> +
> + elf_ops = rproc->fw_ops;
> + /* Use some generic elf ops */
> + slim_rproc_fw_ops.load = elf_ops->load;
> + slim_rproc_fw_ops.sanity_check = elf_ops->sanity_check;
> +
> + rproc->fw_ops = &slim_rproc_fw_ops;
> +
> + /* get imem and dmem */
> + for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + mem_names[i]);
> +
> + slim_rproc->mem[i].cpu_addr = devm_ioremap_resource(dev, res);
> + if (IS_ERR(slim_rproc->mem[i].cpu_addr)) {
> + dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
> + err = PTR_ERR(slim_rproc->mem[i].cpu_addr);
> + goto err;
> + }
> + slim_rproc->mem[i].bus_addr = res->start;
> + slim_rproc->mem[i].size = resource_size(res);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "slimcore");
> + slim_rproc->slimcore = devm_ioremap_resource(dev, res);
> + if (IS_ERR(slim_rproc->slimcore)) {
> + dev_err(&pdev->dev, "failed to ioremap slimcore IO\n");
> + err = PTR_ERR(slim_rproc->slimcore);
> + goto err;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "peripherals");
> + slim_rproc->peri = devm_ioremap_resource(dev, res);
> + if (IS_ERR(slim_rproc->peri)) {
> + dev_err(&pdev->dev, "failed to ioremap peripherals IO\n");
> + err = PTR_ERR(slim_rproc->peri);
> + goto err;
> + }
> +
> + err = slim_clk_get(slim_rproc, dev);
> + if (err)
> + goto err;
> +
> + err = slim_clk_enable(slim_rproc);
> + if (err) {
> + dev_err(dev, "Failed to enable clocks\n");
> + goto err_clk_put;
> + }
> +
> + /* Register as a remoteproc device */
> + err = rproc_add(rproc);
> + if (err) {
> + dev_err(dev, "registration of slim remoteproc failed\n");
> + goto err_clk_dis;
> + }
> +
> + return slim_rproc;
> +
> +err_clk_dis:
> + slim_clk_disable(slim_rproc);
> +err_clk_put:
> + for (i = 0; i < ST_SLIM_MAX_CLK && slim_rproc->clks[i]; i++)
> + clk_put(slim_rproc->clks[i]);
> +err:
> + rproc_put(rproc);
> + return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL(st_slim_rproc_alloc);
> +
> +/**
> + * st_slim_rproc_put() - put slim rproc resources
> + * @slim_rproc: Pointer to the st_slim_rproc struct
> + *
> + * Function for calling respective _put() functions on slim_rproc resources.
> + *
> + */
> +void st_slim_rproc_put(struct st_slim_rproc *slim_rproc)
> +{
> + int clk;
> +
> + if (!slim_rproc)
> + return;
> +
> + slim_clk_disable(slim_rproc);
> +
> + for (clk = 0; clk < ST_SLIM_MAX_CLK && slim_rproc->clks[clk]; clk++)
> + clk_put(slim_rproc->clks[clk]);
> +
> + rproc_del(slim_rproc->rproc);
> + rproc_put(slim_rproc->rproc);
> +}
> +EXPORT_SYMBOL(st_slim_rproc_put);
> +
> +MODULE_AUTHOR("Peter Griffin <peter.griffin@linaro.org>");
> +MODULE_DESCRIPTION("STMicroelectronics SLIM core rproc driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/remoteproc/st_slim_rproc.h b/include/linux/remoteproc/st_slim_rproc.h
> new file mode 100644
> index 0000000..4155556
> --- /dev/null
> +++ b/include/linux/remoteproc/st_slim_rproc.h
> @@ -0,0 +1,58 @@
> +/*
> + * SLIM core rproc driver header
> + *
> + * Copyright (C) 2016 STMicroelectronics
> + *
> + * Author: Peter Griffin <peter.griffin@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef _ST_REMOTEPROC_SLIM_H
> +#define _ST_REMOTEPROC_SLIM_H
> +
> +#define ST_SLIM_MEM_MAX 2
> +#define ST_SLIM_MAX_CLK 4
> +
> +enum {
> + ST_SLIM_DMEM,
> + ST_SLIM_IMEM,
> +};
> +
> +/**
> + * struct st_slim_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +struct st_slim_mem {
> + void __iomem *cpu_addr;
> + phys_addr_t bus_addr;
> + size_t size;
> +};
> +
> +/**
> + * struct st_slim_rproc - SLIM slim core
> + * @rproc: rproc handle
> + * @mem: slim memory information
> + * @slimcore: slim slimcore regs
> + * @peri: slim peripheral regs
> + * @clks: slim clocks
> + */
> +struct st_slim_rproc {
> + struct rproc *rproc;
> + struct st_slim_mem mem[ST_SLIM_MEM_MAX];
> + void __iomem *slimcore;
> + void __iomem *peri;
> +
> + /* st_slim_rproc private */
> + struct clk *clks[ST_SLIM_MAX_CLK];
> +};
> +
> +struct st_slim_rproc *st_slim_rproc_alloc(struct platform_device *pdev,
> + char *fw_name);
> +void st_slim_rproc_put(struct st_slim_rproc *slim_rproc);
> +
> +#endif
> --
> 1.9.1
>
^ permalink raw reply
* Re: virtio_blk: Less function calls in init_vq() after error detection
From: SF Markus Elfring @ 2016-09-13 17:30 UTC (permalink / raw)
To: Christian Bornträger, virtualization, Michael S. Tsirkin,
Minfei Huang, Cornelia Huck, Stefan Hajnoczi
Cc: Julia Lawall, Chao Fan, kernel-janitors, LKML
In-Reply-To: <e918e655-cd86-c3c8-d911-9dfc03b03e19@de.ibm.com>
> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
> virtio_blk: Fix a slient kernel panic
I would like to add another view on the implementation details in this software update.
> which did the opposite of your patch.
This update contained a different approach for error detection and corresponding
exception handling.
> And in fact it fixed a bug.
This is great in principle according to an information in the commit description.
"…
To fix this bug, we should take care of allocation failure,
and return correct value to let caller know what happen.
…"
> Quite obviously multiple labels are harder to read and harder to get right.
> For error handling with just kfree one label is just the right thing to.
Unfortunately, I get an other impression here after a closer look.
Can it be that the discussed commit from 2016-08-09 accepted (or tolerated)
two weaknesses at least?
1. Commit title:
Is the word "slient" a typo?
Would you like to read "silent" there instead?
2. Source code:
Why would another memory allocation be attempted if it could be determined quicker
that a previous one failed and this function implementation can not succeed then?
How much will it matter in general that two function calls are performed
in this use case without checking their return values immediately?
https://cwe.mitre.org/data/definitions/252.html
if (!names || !callbacks || !vqs) { …
https://cwe.mitre.org/data/definitions/754.html
Was the software development attention a bit too low as it happens occasionally?
I hope that my suggestions can improve the affected situation a bit more
also for this software module.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v10] virtio-net: add Max MTU configuration field
From: Michael S. Tsirkin @ 2016-09-13 15:58 UTC (permalink / raw)
To: Aaron Conole
Cc: Victor Kaplansky, virtio-dev, Maxime Coquelin, virtualization
In-Reply-To: <1473779892-6234-1-git-send-email-aconole@redhat.com>
On Tue, Sep 13, 2016 at 11:18:12AM -0400, Aaron Conole wrote:
> It is helpful for a host to indicate it's MTU to be set on guest NICs
> other than the assumed 1500 byte value. This helps in situations where
> the host network is using Jumbo Frames, or aiding in PMTU discovery by
> configuring a homogenous network. It is also helpful for sizing receive
> buffers correctly.
>
> The change adds a new field to configuration area of network
> devices. It will be used to pass a maximum MTU from the device to
> the driver. This will be used by the driver as a maximum value for
> packet sizes during transmission, without segmentation offloading.
>
> In addition, in order to support backward and forward compatibility,
> we introduce a new feature bit called VIRTIO_NET_F_MTU.
>
> VIRTIO-152
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Cc: Victor Kaplansky <victork@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Cornelia, could you pls ack before I start voting?
> ---
> v1:
> This is an attempt at continuing the work done by Victor Kaplansky on
> mtu negiotiation for virtio-net devices. It attempts to pick up from
> https://lists.oasis-open.org/archives/virtio-dev/201508/msg00007.html
> and is just a minor blurb from the first patch along with the 2nd patch
> from the series, and some of the feedback integrated.
>
> v2:
> Rephrase and provide a mechanism for guest->host and host->guest
> communication through a driver read-only and driver write-only field.
>
> v3:
> Converted to just support initial MTU. Guest->host and Host->guest MTU
> changes are outside the scope of this change.
>
> v4:
> Removed references to 'initial', since that condition cannot be tested.
> Simply state that if the driver will use the mtu field, it must
> negotiate the feature bit, and if not, it must not.
>
> v5:
> After feedback from Michael S. Tsirkin
>
> v6:
> Bit has to change to bit 25 due to an undocumented bit 24 being taken.
>
> v7:
> Partial rewrite with feedback from MST. Additional conformance statements
> added.
>
> v8:
> Clarified the new conformance statements.
>
> v9:
> Updated the commit log for correctness. Added ACKs from
> Michael S. Tsirkin, and Maxime Coquelin. Included the virtio
> issue id.
>
> v10:
> Update the conformance statement wordings from previous 'offered' form
> to 'succesfully negotiated' form.
>
> content.tex | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 4b45678..42c0568 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3049,6 +3049,11 @@ features.
> \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
> reconfiguration support.
>
> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> + offered by the device, device advises driver about the value of
> + MTU to be used. If negotiated, the driver uses \field{mtu} as
> + the maximum MTU value supplied to the driver.
> +
> \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>
> \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> @@ -3140,11 +3145,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
> is negotiated.
>
> +The following driver-read-only field, \field{mtu} only exists if
> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> +use.
> +
> \begin{lstlisting}
> struct virtio_net_config {
> u8 mac[6];
> le16 status;
> le16 max_virtqueue_pairs;
> + le16 mtu;
> };
> \end{lstlisting}
>
> @@ -3153,6 +3163,19 @@ struct virtio_net_config {
> The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
> if it offers VIRTIO_NET_F_MQ.
>
> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> +if it offers VIRTIO_NET_F_MTU.
> +
> +The device MUST NOT modify \field{mtu} once it has been set.
> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with \field{gso_type} NONE or ECN after VIRTIO_NET_F_MTU has been successfully
> +negotiated.
> +
> +The device MUST forward transmitted packets of up to MTU size with
> +\field{gso_type} NONE or ECN, and do so without fragmentation, after
> +VIRTIO_NET_F_MTU has been successfully negotiated.
> +
> \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>
> A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3165,6 +3188,15 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
> assume the link is active, otherwise it SHOULD read the link status from
> the bottom bit of \field{status}.
>
> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> +
> +If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive
> +buffers of size \field{mtu} to be able to receive at least one receive
> +packet with \field{gso_type} NONE or ECN.
> +
> +If the driver negotiates VIRTIO_NET_F_MTU, it MUST NOT transmit packets of
> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> +
> \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
> When using the legacy interface, transitional devices and drivers
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Michael S. Tsirkin @ 2016-09-13 15:57 UTC (permalink / raw)
To: Namhyung Kim
Cc: virtio-dev, Tony Luck, Daniel P . Berrange, Kees Cook, kvm,
Radim Krčmář, Anton Vorontsov, LKML,
Steven Rostedt, qemu-devel, Minchan Kim, Anthony Liguori,
Colin Cross, Paolo Bonzini, virtualization, Ingo Molnar
In-Reply-To: <20160820080744.10344-3-namhyung@kernel.org>
On Sat, Aug 20, 2016 at 05:07:43PM +0900, Namhyung Kim wrote:
> Add virtio pstore device to allow kernel log files saved on the host.
> It will save the log files on the directory given by pstore device
> option.
>
> $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
>
> (guest) # echo c > /proc/sysrq-trigger
>
> $ ls dir-xx
> dmesg-1.enc.z dmesg-2.enc.z
>
> The log files are usually compressed using zlib. Users can see the log
> messages directly on the host or on the guest (using pstore filesystem).
>
> The 'directory' property is required for virtio-pstore device to work.
> It also adds 'bufsize' property to set size of pstore bufer.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/virtio-pci.c | 52 ++
> hw/virtio/virtio-pci.h | 14 +
> hw/virtio/virtio-pstore.c | 699 +++++++++++++++++++++++++
> include/hw/pci/pci.h | 1 +
> include/hw/virtio/virtio-pstore.h | 36 ++
> include/standard-headers/linux/virtio_ids.h | 1 +
> include/standard-headers/linux/virtio_pstore.h | 76 +++
> qdev-monitor.c | 1 +
> 9 files changed, 881 insertions(+), 1 deletion(-)
> create mode 100644 hw/virtio/virtio-pstore.c
> create mode 100644 include/hw/virtio/virtio-pstore.h
> create mode 100644 include/standard-headers/linux/virtio_pstore.h
>
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 3e2b175..aae7082 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
> common-obj-y += virtio-mmio.o
>
> obj-y += virtio.o virtio-balloon.o
> -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 755f921..c184823 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2416,6 +2416,57 @@ static const TypeInfo virtio_host_pci_info = {
> };
> #endif
>
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> + DeviceState *vdev = DEVICE(&vps->vdev);
> + Error *err = NULL;
> +
> + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> + object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +}
> +
> +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> +
> + k->realize = virtio_pstore_pci_realize;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
> + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> + pcidev_k->class_id = PCI_CLASS_OTHERS;
> +}
> +
> +static void virtio_pstore_pci_instance_init(Object *obj)
> +{
> + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
> +
> + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> + TYPE_VIRTIO_PSTORE);
> + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
> + "directory", &error_abort);
> + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev),
> + "bufsize", &error_abort);
> +}
> +
> +static const TypeInfo virtio_pstore_pci_info = {
> + .name = TYPE_VIRTIO_PSTORE_PCI,
> + .parent = TYPE_VIRTIO_PCI,
> + .instance_size = sizeof(VirtIOPstorePCI),
> + .instance_init = virtio_pstore_pci_instance_init,
> + .class_init = virtio_pstore_pci_class_init,
> +};
> +
> /* virtio-pci-bus */
>
> static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -2485,6 +2536,7 @@ static void virtio_pci_register_types(void)
> #ifdef CONFIG_VHOST_SCSI
> type_register_static(&vhost_scsi_pci_info);
> #endif
> + type_register_static(&virtio_pstore_pci_info);
> }
>
> type_init(virtio_pci_register_types)
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 25fbf8a..354b2b7 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -31,6 +31,7 @@
> #ifdef CONFIG_VHOST_SCSI
> #include "hw/virtio/vhost-scsi.h"
> #endif
> +#include "hw/virtio/virtio-pstore.h"
>
> typedef struct VirtIOPCIProxy VirtIOPCIProxy;
> typedef struct VirtIOBlkPCI VirtIOBlkPCI;
> @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
> typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> +typedef struct VirtIOPstorePCI VirtIOPstorePCI;
>
> /* virtio-pci-bus */
>
> @@ -324,6 +326,18 @@ struct VirtIOGPUPCI {
> VirtIOGPU vdev;
> };
>
> +/*
> + * virtio-pstore-pci: This extends VirtioPCIProxy.
> + */
> +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
> +#define VIRTIO_PSTORE_PCI(obj) \
> + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
> +
> +struct VirtIOPstorePCI {
> + VirtIOPCIProxy parent_obj;
> + VirtIOPstore vdev;
> +};
> +
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> #define VIRTIO_PCI_ABI_VERSION 0
>
> diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> new file mode 100644
> index 0000000..b8fb4be
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,699 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016 LG Electronics
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "qemu-common.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qapi/visitor.h"
> +#include "qapi-event.h"
> +#include "io/channel-util.h"
> +#include "trace.h"
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "hw/virtio/virtio-pstore.h"
> +
> +#define PSTORE_DEFAULT_BUFSIZE (16 * 1024)
> +#define PSTORE_DEFAULT_FILE_MAX 5
> +
> +/* the index should match to the type value */
> +static const char *virtio_pstore_file_prefix[] = {
> + "unknown-", /* VIRTIO_PSTORE_TYPE_UNKNOWN */
Is there value in treating everything unexpected as "unknown"
and rotating them as if they were logs?
It might be better to treat everything that's not known
as guest error.
> + "dmesg-", /* VIRTIO_PSTORE_TYPE_DMESG */
use named initializers for this instead of comments.
> +};
> +
> +static char *virtio_pstore_to_filename(VirtIOPstore *s,
> + struct virtio_pstore_req *req)
> +{
> + const char *basename;
> + unsigned long long id;
> + unsigned int type = le16_to_cpu(req->type);
> + unsigned int flags = le32_to_cpu(req->flags);
> +
> + if (type < ARRAY_SIZE(virtio_pstore_file_prefix)) {
> + basename = virtio_pstore_file_prefix[type];
> + } else {
> + basename = "unknown-";
> + }
> +
> + id = s->id++;
> + return g_strdup_printf("%s/%s%llu%s", s->directory, basename, id,
> + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
> +}
> +
> +static char *virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> + struct virtio_pstore_fileinfo *info)
> +{
> + char *filename;
> + unsigned int idx;
> +
> + filename = g_strdup_printf("%s/%s", s->directory, name);
> + if (filename == NULL)
> + return NULL;
> +
> + for (idx = 0; idx < ARRAY_SIZE(virtio_pstore_file_prefix); idx++) {
> + if (g_str_has_prefix(name, virtio_pstore_file_prefix[idx])) {
> + info->type = idx;
> + name += strlen(virtio_pstore_file_prefix[idx]);
> + break;
> + }
> + }
> +
> + if (idx == ARRAY_SIZE(virtio_pstore_file_prefix)) {
> + g_free(filename);
> + return NULL;
> + }
> +
> + qemu_strtoull(name, NULL, 0, &info->id);
What if this fails?
> +
> + info->flags = 0;
> + if (g_str_has_suffix(name, ".enc.z")) {
> + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> + }
> +
> + return filename;
> +}
> +
> +static int prefix_idx;
> +static int prefix_count;
> +static int prefix_len;
This does not work properly if there are multiple instances
of it. Pls move everything into device state.
> +
> +static int filter_pstore(const struct dirent *de)
> +{
> + int i;
> +
> + for (i = 0; i < prefix_count; i++) {
> + const char *prefix = virtio_pstore_file_prefix[prefix_idx + i];
> +
> + if (g_str_has_prefix(de->d_name, prefix)) {
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> +static int sort_pstore(const struct dirent **a, const struct dirent **b)
> +{
> + uint64_t id_a, id_b;
> +
> + qemu_strtoull((*a)->d_name + prefix_len, NULL, 0, &id_a);
> + qemu_strtoull((*b)->d_name + prefix_len, NULL, 0, &id_b);
> +
> + return id_a - id_b;
> +}
> +
> +static int rotate_pstore_file(VirtIOPstore *s, unsigned short type)
> +{
> + int ret = 0;
> + int i, num;
> + char *filename;
> + struct dirent **files;
> +
> + if (type >= ARRAY_SIZE(virtio_pstore_file_prefix)) {
> + type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> + }
> +
> + prefix_idx = type;
> + prefix_len = strlen(virtio_pstore_file_prefix[type]);
> + prefix_count = 1; /* only scan current type */
> +
> + /* delete the oldest file in the same type */
> + num = scandir(s->directory, &files, filter_pstore, sort_pstore);
> + if (num < 0)
> + return num;
> + if (num < (int)s->file_max)
> + goto out;
> +
> + filename = g_strdup_printf("%s/%s", s->directory, files[0]->d_name);
> + if (filename == NULL) {
> + ret = -1;
> + goto out;
> + }
> +
> + ret = unlink(filename);
> +
> +out:
> + for (i = 0; i < num; i++) {
> + g_free(files[i]);
> + }
> + g_free(files);
> +
> + return ret;
> +}
Pls prefix everything with virtio_pstore or another
unique prefix. also below.
> +
> +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> +{
> + /* scan all pstore files */
> + prefix_idx = 0;
> + prefix_count = ARRAY_SIZE(virtio_pstore_file_prefix);
> +
> + s->file_idx = 0;
> + s->num_file = scandir(s->directory, &s->files, filter_pstore, alphasort);
> +
> + return s->num_file >= 0 ? 0 : -1;
> +}
> +
> +static ssize_t virtio_pstore_do_close(VirtIOPstore *s)
> +{
> + int i;
> +
> + for (i = 0; i < s->num_file; i++) {
> + g_free(s->files[i]);
> + }
> + g_free(s->files);
> + s->files = NULL;
> +
> + s->num_file = 0;
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s,
> + struct virtio_pstore_req *req)
> +{
> + char *filename;
> + int ret;
> +
> + filename = virtio_pstore_to_filename(s, req);
> + if (filename == NULL)
> + return -1;
this can't happen.
also this is a coding style violation.
> +
> + ret = unlink(filename);
> +
> + g_free(filename);
> + return ret;
> +}
> +
> +struct pstore_read_arg {
> + VirtIOPstore *vps;
> + VirtQueueElement *elem;
> + struct virtio_pstore_fileinfo info;
> + QIOChannel *ioc;
> +};
> +
> +static gboolean pstore_async_read_fn(QIOChannel *ioc, GIOCondition condition,
> + gpointer data)
> +{
> + struct pstore_read_arg *rarg = data;
> + struct virtio_pstore_fileinfo *info = &rarg->info;
> + VirtIOPstore *vps = rarg->vps;
> + VirtQueueElement *elem = rarg->elem;
> + struct virtio_pstore_res res;
> + size_t offset = sizeof(res) + sizeof(*info);
> + struct iovec *sg = elem->in_sg;
> + unsigned int sg_num = elem->in_num;
> + Error *err = NULL;
> + ssize_t len;
> + int ret;
> +
> + /* skip res and fileinfo */
> + iov_discard_front(&sg, &sg_num, sizeof(res) + sizeof(*info));
> +
> + len = qio_channel_readv(rarg->ioc, sg, sg_num, &err);
> + if (len < 0) {
> + if (errno == EAGAIN) {
> + len = 0;
> + }
> + ret = -1;
> + } else {
> + info->len = cpu_to_le32(len);
> + ret = 0;
> + }
> +
> + res.cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_READ);
> + res.type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> + res.ret = cpu_to_le32(ret);
> +
> + /* now copy res and fileinfo */
> + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res));
> + iov_from_buf(elem->in_sg, elem->in_num, sizeof(res), info, sizeof(*info));
> +
> + len += offset;
> + virtqueue_push(vps->rvq, elem, len);
> + virtio_notify(VIRTIO_DEVICE(vps), vps->rvq);
> +
> + return G_SOURCE_REMOVE;
> +}
> +
> +static void free_rarg_fn(gpointer data)
> +{
> + struct pstore_read_arg *rarg = data;
> +
> + qio_channel_close(rarg->ioc, NULL);
> +
> + g_free(rarg->elem);
> + g_free(rarg);
> +}
> +
> +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, VirtQueueElement *elem)
> +{
> + char *filename = NULL;
> + int fd, idx;
> + struct stat stbuf;
> + struct pstore_read_arg *rarg = NULL;
> + Error *err = NULL;
> + int ret = -1;
> +
> + if (s->file_idx >= s->num_file) {
> + return 0;
> + }
> +
> + rarg = g_malloc(sizeof(*rarg));
> + if (rarg == NULL) {
> + return -1;
> + }
> +
> + idx = s->file_idx++;
> + filename = virtio_pstore_from_filename(s, s->files[idx]->d_name,
> + &rarg->info);
> + if (filename == NULL) {
> + goto out;
> + }
> +
> + fd = open(filename, O_RDONLY);
> + if (fd < 0) {
> + error_report("cannot open %s", filename);
> + goto out;
> + }
I see open here but close nowhere. Does this leak fds?
> +
> + if (fstat(fd, &stbuf) < 0) {
So we can stat, but can we e.g. read?
> + goto out;
> + }
> +
> + rarg->vps = s;
> + rarg->elem = elem;
> + rarg->info.id = cpu_to_le64(rarg->info.id);
> + rarg->info.type = cpu_to_le16(rarg->info.type);
> + rarg->info.flags = cpu_to_le32(rarg->info.flags);
> + rarg->info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
Is this seconds since epoch?
Why ctim specifically?
Pls add comments.
> + rarg->info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
Not all hosts support nanosecond precision.
Do we need some way to tell guest what's reliable?
Unless you limit this to linux host, you should care about things
like this (in man fstat)
Since kernel 2.5.48, the stat structure supports nanosecond
resolution for the three file timestamp fields. The nanosecond compo‐
nents of each timestamp are available via names of the form
st_atim.tv_nsec if the _BSD_SOURCE or _SVID_SOURCE feature test macro
is defined. Nanosecond timestamps are nowadays standardized,
starting with POSIX.1-2008, and, starting with version 2.12, glibc also
exposes the nanosecond component names if _POSIX_C_SOURCE is defined
with the value 200809L or greater, or _XOPEN_SOURCE is defined with
the value 700 or greater. If none of the aforementioned macros are
defined, then the nanosecond values are exposed with names of the form
st_atimensec.
> +
> + rarg->ioc = qio_channel_new_fd(fd, &err);
> + if (err) {
> + error_reportf_err(err, "cannot create io channel: ");
> + goto out;
> + }
> +
> + qio_channel_set_blocking(rarg->ioc, false, &err);
> + qio_channel_add_watch(rarg->ioc, G_IO_IN, pstore_async_read_fn, rarg,
> + free_rarg_fn);
> + g_free(filename);
> + return 1;
> +
> +out:
> + g_free(filename);
> + g_free(rarg);
> +
> + return ret;
> +}
> +
> +struct pstore_write_arg {
> + VirtIOPstore *vps;
> + VirtQueueElement *elem;
> + struct virtio_pstore_req *req;
> + QIOChannel *ioc;
> +};
> +
> +static gboolean pstore_async_write_fn(QIOChannel *ioc, GIOCondition condition,
> + gpointer data)
> +{
> + struct pstore_write_arg *warg = data;
> + VirtIOPstore *vps = warg->vps;
> + VirtQueueElement *elem = warg->elem;
> + struct iovec *sg = elem->out_sg;
> + unsigned int sg_num = elem->out_num;
> + struct virtio_pstore_res res;
> + Error *err = NULL;
> + ssize_t len;
> + int ret;
> +
> + /* we already consumed the req */
> + iov_discard_front(&sg, &sg_num, sizeof(*warg->req));
> +
> + len = qio_channel_writev(warg->ioc, sg, sg_num, &err);
> + if (len < 0) {
> + ret = -1;
> + } else {
> + ret = 0;
> + }
This can discard part of the data written.
Don't we care?
> +
> + res.cmd = cpu_to_le16(VIRTIO_PSTORE_CMD_WRITE);
> + res.type = warg->req->type;
> + res.ret = cpu_to_le32(ret);
> +
> + /* tell the result to guest */
> + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res));
> +
> + virtqueue_push(vps->wvq, elem, sizeof(res));
> + virtio_notify(VIRTIO_DEVICE(vps), vps->wvq);
> +
> + return G_SOURCE_REMOVE;
> +}
> +
> +static void free_warg_fn(gpointer data)
> +{
> + struct pstore_write_arg *warg = data;
> +
> + qio_channel_close(warg->ioc, NULL);
> +
> + g_free(warg->elem);
> + g_free(warg);
> +}
> +
> +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, VirtQueueElement *elem,
> + struct virtio_pstore_req *req)
> +{
> + unsigned short type = le16_to_cpu(req->type);
> + char *filename = NULL;
> + int fd;
> + int flags = O_WRONLY | O_CREAT | O_TRUNC;
> + struct pstore_write_arg *warg = NULL;
> + Error *err = NULL;
> + int ret = -1;
> +
> + /* do not keep same type of files more than 'file-max' */
> + rotate_pstore_file(s, type);
If you don't care about failures, should this function
return a value? How about reporting it to the user?
> +
> + filename = virtio_pstore_to_filename(s, req);
> + if (filename == NULL) {
> + return -1;
> + }
this can't happen
> +
> + warg = g_malloc(sizeof(*warg));
> + if (warg == NULL) {
> + goto out;
> + }
> +
> + fd = open(filename, flags, 0644);
> + if (fd < 0) {
> + error_report("cannot open %s", filename);
> + ret = fd;
> + goto out;
> + }
> +
> + warg->vps = s;
> + warg->elem = elem;
> + warg->req = req;
> +
> + warg->ioc = qio_channel_new_fd(fd, &err);
> + if (err) {
> + error_reportf_err(err, "cannot create io channel: ");
> + goto out;
> + }
> +
> + qio_channel_set_blocking(warg->ioc, false, &err);
> + qio_channel_add_watch(warg->ioc, G_IO_OUT, pstore_async_write_fn, warg,
> + free_warg_fn);
> + g_free(filename);
> + return 1;
> +
> +out:
> + g_free(filename);
> + g_free(warg);
> + return ret;
> +}
> +
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> + VirtQueueElement *elem;
> + struct virtio_pstore_req req;
> + struct virtio_pstore_res res;
> + ssize_t len = 0;
> + int ret;
> +
> + for (;;) {
> + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> + if (!elem) {
> + return;
> + }
> +
> + if (elem->out_num < 1 || elem->in_num < 1) {
> + error_report("request or response buffer is missing");
> + exit(1);
> + }
> +
> + if (elem->out_num > 2 || elem->in_num > 3) {
> + error_report("invalid number of input/output buffer");
> + exit(1);
> + }
> +
> + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req));
> + if (len != (ssize_t)sizeof(req)) {
> + error_report("invalid request size: %ld", (long)len);
> + exit(1);
> + }
> + res.cmd = req.cmd;
> + res.type = req.type;
> +
> + switch (le16_to_cpu(req.cmd)) {
> + case VIRTIO_PSTORE_CMD_OPEN:
> + ret = virtio_pstore_do_open(s);
> + break;
> + case VIRTIO_PSTORE_CMD_CLOSE:
> + ret = virtio_pstore_do_close(s);
> + break;
> + case VIRTIO_PSTORE_CMD_ERASE:
> + ret = virtio_pstore_do_erase(s, &req);
> + break;
> + case VIRTIO_PSTORE_CMD_READ:
> + ret = virtio_pstore_do_read(s, elem);
> + if (ret == 1) {
> + /* async channel io */
> + continue;
> + }
> + break;
> + case VIRTIO_PSTORE_CMD_WRITE:
> + ret = virtio_pstore_do_write(s, elem, &req);
> + if (ret == 1) {
> + /* async channel io */
> + continue;
> + }
> + break;
> + default:
> + ret = -1;
> + break;
> + }
> +
> + res.ret = ret;
> +
> + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res));
> + virtqueue_push(vq, elem, sizeof(res) + len);
> +
> + virtio_notify(vdev, vq);
> + g_free(elem);
> +
> + if (ret < 0) {
> + return;
what does this do?
> + }
> + }
> +}
> +
> +static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> + VirtIOPstore *s = VIRTIO_PSTORE(dev);
> +
> + virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE,
> + sizeof(struct virtio_pstore_config));
> +
> + s->id = 1;
> +
> + if (!s->bufsize)
> + s->bufsize = PSTORE_DEFAULT_BUFSIZE;
> + if (!s->file_max)
> + s->file_max = PSTORE_DEFAULT_FILE_MAX;
> +
> + s->rvq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
> + s->wvq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
> +}
> +
> +static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> + virtio_cleanup(vdev);
> +}
> +
> +static void virtio_pstore_get_config(VirtIODevice *vdev, uint8_t *config_data)
> +{
> + VirtIOPstore *dev = VIRTIO_PSTORE(vdev);
> + struct virtio_pstore_config config;
Add {} here - you want all fields initialized
if you add them, to avoid leaking them to guest.
> +
> + config.bufsize = cpu_to_le32(dev->bufsize);
> +
> + memcpy(config_data, &config, sizeof(struct virtio_pstore_config));
> +}
> +
> +static void virtio_pstore_set_config(VirtIODevice *vdev,
> + const uint8_t *config_data)
> +{
> + VirtIOPstore *dev = VIRTIO_PSTORE(vdev);
> + struct virtio_pstore_config config;
> +
> + memcpy(&config, config_data, sizeof(struct virtio_pstore_config));
> +
> + dev->bufsize = le32_to_cpu(config.bufsize);
> +}
> +
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
> +{
> + return f;
> +}
> +
> +static void pstore_get_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> +
> + visit_type_str(v, name, &s->directory, errp);
> +}
> +
> +static void pstore_set_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *local_err = NULL;
> + char *value;
> +
> + visit_type_str(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + g_free(s->directory);
> + s->directory = value;
> +}
> +
> +static void pstore_release_directory(Object *obj, const char *name,
> + void *opaque)
> +{
> + VirtIOPstore *s = opaque;
> +
> + g_free(s->directory);
> + s->directory = NULL;
> +}
> +
> +static void pstore_get_bufsize(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + uint64_t value = s->bufsize;
> +
> + visit_type_size(v, name, &value, errp);
> +}
> +
> +static void pstore_set_bufsize(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *error = NULL;
> + uint64_t value;
> +
> + visit_type_size(v, name, &value, &error);
> + if (error) {
> + error_propagate(errp, error);
> + return;
> + }
> +
> + if (value < 4096) {
> + error_setg(&error, "Warning: too small buffer size: %"PRIu64, value);
> + error_propagate(errp, error);
> + return;
> + }
> +
> + s->bufsize = value;
> +}
> +
> +static void pstore_get_file_max(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + int64_t value = s->file_max;
> +
> + visit_type_int(v, name, &value, errp);
> +}
> +
> +static void pstore_set_file_max(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *error = NULL;
> + int64_t value;
> +
> + visit_type_int(v, name, &value, &error);
> + if (error) {
> + error_propagate(errp, error);
> + return;
> + }
> +
> + s->file_max = value;
> +}
Do you need dynamic properties? There are easier ways
to define an int property. Same for others.
> +
> +static Property virtio_pstore_properties[] = {
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virtio_pstore_instance_init(Object *obj)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(obj);
> +
> + object_property_add(obj, "directory", "str",
> + pstore_get_directory, pstore_set_directory,
> + pstore_release_directory, s, NULL);
> + object_property_add(obj, "bufsize", "size",
> + pstore_get_bufsize, pstore_set_bufsize, NULL, s, NULL);
> + object_property_add(obj, "file-max", "int",
> + pstore_get_file_max, pstore_set_file_max, NULL, s, NULL);
> +}
> +
> +static void virtio_pstore_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> + dc->props = virtio_pstore_properties;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + vdc->realize = virtio_pstore_device_realize;
> + vdc->unrealize = virtio_pstore_device_unrealize;
> + vdc->get_config = virtio_pstore_get_config;
> + vdc->set_config = virtio_pstore_set_config;
> + vdc->get_features = get_features;
> +}
> +
> +static const TypeInfo virtio_pstore_info = {
> + .name = TYPE_VIRTIO_PSTORE,
> + .parent = TYPE_VIRTIO_DEVICE,
> + .instance_size = sizeof(VirtIOPstore),
> + .instance_init = virtio_pstore_instance_init,
> + .class_init = virtio_pstore_class_init,
> +};
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_pstore_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 929ec2f..b31774a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -79,6 +79,7 @@
> #define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
> #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
> #define PCI_DEVICE_ID_VIRTIO_9P 0x1009
> +#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
>
> #define PCI_VENDOR_ID_REDHAT 0x1b36
> #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
> diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h
> new file mode 100644
> index 0000000..85b1828
> --- /dev/null
> +++ b/include/hw/virtio/virtio-pstore.h
> @@ -0,0 +1,36 @@
> +/*
> + * Virtio Pstore Support
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_VIRTIO_PSTORE_H
> +#define _QEMU_VIRTIO_PSTORE_H
> +
> +#include "standard-headers/linux/virtio_pstore.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/pci/pci.h"
> +
> +#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
> +#define VIRTIO_PSTORE(obj) \
> + OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
> +
> +typedef struct VirtIOPstore {
> + VirtIODevice parent_obj;
> + VirtQueue *rvq;
> + VirtQueue *wvq;
> + char *directory;
> + int file_idx;
> + int num_file;
> + struct dirent **files;
> + uint64_t id;
> + uint64_t bufsize;
> + uint64_t file_max;
> +} VirtIOPstore;
> +
> +#endif
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 77925f5..c72a9ab 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 22 /* virtio pstore */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/standard-headers/linux/virtio_pstore.h b/include/standard-headers/linux/virtio_pstore.h
> new file mode 100644
> index 0000000..2f91839
> --- /dev/null
> +++ b/include/standard-headers/linux/virtio_pstore.h
> @@ -0,0 +1,76 @@
> +#ifndef _LINUX_VIRTIO_PSTORE_H
> +#define _LINUX_VIRTIO_PSTORE_H
> +/* This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + * may be used to endorse or promote products derived from this software
> + * without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE. */
> +#include "standard-headers/linux/types.h"
> +#include "standard-headers/linux/virtio_types.h"
> +#include "standard-headers/linux/virtio_ids.h"
> +#include "standard-headers/linux/virtio_config.h"
> +
> +#define VIRTIO_PSTORE_CMD_NULL 0
> +#define VIRTIO_PSTORE_CMD_OPEN 1
> +#define VIRTIO_PSTORE_CMD_READ 2
> +#define VIRTIO_PSTORE_CMD_WRITE 3
> +#define VIRTIO_PSTORE_CMD_ERASE 4
> +#define VIRTIO_PSTORE_CMD_CLOSE 5
> +
> +#define VIRTIO_PSTORE_TYPE_UNKNOWN 0
> +#define VIRTIO_PSTORE_TYPE_DMESG 1
> +
> +#define VIRTIO_PSTORE_FL_COMPRESSED 1
> +
> +struct virtio_pstore_req {
> + __virtio16 cmd;
> + __virtio16 type;
> + __virtio32 flags;
> + __virtio64 id;
> + __virtio32 count;
> + __virtio32 reserved;
> +};
> +
> +struct virtio_pstore_res {
> + __virtio16 cmd;
> + __virtio16 type;
> + __virtio32 ret;
> +};
> +
> +struct virtio_pstore_fileinfo {
> + __virtio64 id;
> + __virtio32 count;
> + __virtio16 type;
> + __virtio16 unused;
> + __virtio32 flags;
> + __virtio32 len;
> + __virtio64 time_sec;
> + __virtio32 time_nsec;
> + __virtio32 reserved;
> +};
> +
> +struct virtio_pstore_config {
> + __virtio32 bufsize;
> +};
> +
> +#endif /* _LINUX_VIRTIO_PSTORE_H */
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index e19617f..e1df5a9 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -73,6 +73,7 @@ static const QDevAlias qdev_alias_table[] = {
> { "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
> { "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
> { "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
> + { "virtio-pstore-pci", "virtio-pstore" },
> { }
> };
>
> --
> 2.9.3
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Michael S. Tsirkin @ 2016-09-13 15:19 UTC (permalink / raw)
To: Namhyung Kim
Cc: virtio-dev, Tony Luck, Kees Cook, kvm,
Radim Krčmář, Anton Vorontsov, LKML,
Steven Rostedt, qemu-devel, Minchan Kim, Anthony Liguori,
Colin Cross, Paolo Bonzini, virtualization, Ingo Molnar
In-Reply-To: <20160820080744.10344-2-namhyung@kernel.org>
On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine. Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem. It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
>
> It supports legacy PCI device using single order-2 page buffer. It uses
> two virtqueues - one for (sync) read and another for (async) write.
> Since it cannot wait for write finished, it supports up to 128
> concurrent IO. The buffer size is configurable now.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> drivers/virtio/Kconfig | 10 +
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pstore.h | 74 +++++++
> 6 files changed, 504 insertions(+)
> create mode 100644 drivers/virtio/virtio_pstore.c
> create mode 100644 include/uapi/linux/virtio_pstore.h
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>
> If unsure, say M.
>
> +config VIRTIO_PSTORE
> + tristate "Virtio pstore driver"
> + depends on VIRTIO
> + depends on PSTORE
> + ---help---
> + This driver supports virtio pstore devices to save/restore
> + panic and oops messages on the host.
> +
> + If unsure, say M.
> +
> config VIRTIO_MMIO
> tristate "Platform bus driver for memory mapped virtio devices"
> depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index 000000000000..0a63c7db4278
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,417 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_pstore.h>
> +
> +#define VIRT_PSTORE_ORDER 2
> +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> +#define VIRT_PSTORE_NR_REQ 128
where are these numbers from?
> +
> +struct virtio_pstore {
> + struct virtio_device *vdev;
> + struct virtqueue *vq[2];
> + struct pstore_info pstore;
> + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> + unsigned int req_id;
> +
> + /* Waiting for host to ack */
> + wait_queue_head_t acked;
> + int failed;
> +};
> +
> +#define TYPE_TABLE_ENTRY(_entry) \
> + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> +
> +struct type_table {
> + int pstore;
> + u16 virtio;
> +} type_table[] = {
> + TYPE_TABLE_ENTRY(DMESG),
> +};
> +
> +#undef TYPE_TABLE_ENTRY
Let's not play preprocessor games until this becomes
a big issue. Simple
{ PSTORE_TYPE_DMESG, VIRTIO_PSTORE_TYPE_DMESG}
does the trick just as well for now.
Also see below.
> +
> +
single empty line pls.
> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (type == type_table[i].pstore)
> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
> + }
Rather complex for something that always returns a single value.
why do we need a table at all?
How about a switch statement?
static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
{
switch (type) {
case PSTORE_TYPE_DMESG:
return VIRTIO_PSTORE_TYPE_DMESG;
default:
return VIRTIO_PSTORE_TYPE_UNKNOWN;
}
}
> +
> + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> +}
This returns an incorrect type.
> +
> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio)
> + return type_table[i].pstore;
> + }
> +
> + return PSTORE_TYPE_UNKNOWN;
> +}
> +
> +static void virtpstore_ack(struct virtqueue *vq)
> +{
> + struct virtio_pstore *vps = vq->vdev->priv;
> +
> + wake_up(&vps->acked);
> +}
> +
> +static void virtpstore_check(struct virtqueue *vq)
> +{
> + struct virtio_pstore *vps = vq->vdev->priv;
> + struct virtio_pstore_res *res;
> + unsigned int len;
> +
> + res = virtqueue_get_buf(vq, &len);
> + if (res == NULL)
> + return;
> +
> + if (virtio32_to_cpu(vq->vdev, res->ret) < 0)
> + vps->failed = 1;
> +}
> +
> +static void virt_pstore_get_reqs(struct virtio_pstore *vps,
> + struct virtio_pstore_req **preq,
> + struct virtio_pstore_res **pres)
> +{
> + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ;
> +
> + *preq = &vps->req[idx];
> + *pres = &vps->res[idx];
> +
> + memset(*preq, 0, sizeof(**preq));
> + memset(*pres, 0, sizeof(**pres));
> +}
> +
> +static int virt_pstore_open(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return virtio32_to_cpu(vps->vdev, res->ret);
> +}
> +
> +static int virt_pstore_close(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req = &vps->req[vps->req_id];
> + struct virtio_pstore_res *res = &vps->res[vps->req_id];
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return virtio32_to_cpu(vps->vdev, res->ret);
> +}
> +
> +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> + int *count, struct timespec *time,
> + char **buf, bool *compressed,
> + ssize_t *ecc_notice_size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct virtio_pstore_fileinfo info;
> + struct scatterlist sgo[1], sgi[3];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> + unsigned int flags;
> + int ret;
> + void *bf;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_table(sgi, 3);
> + sg_set_buf(&sgi[0], res, sizeof(*res));
> + sg_set_buf(&sgi[1], &info, sizeof(info));
> + sg_set_buf(&sgi[2], psi->buf, psi->bufsize);
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + if (len < sizeof(*res) + sizeof(info))
> + return -1;
> +
> + ret = virtio32_to_cpu(vps->vdev, res->ret);
> + if (ret < 0)
> + return ret;
> +
> + len = virtio32_to_cpu(vps->vdev, info.len);
> +
> + bf = kmalloc(len, GFP_KERNEL);
> + if (bf == NULL)
> + return -ENOMEM;
> +
> + *id = virtio64_to_cpu(vps->vdev, info.id);
> + *type = from_virtio_type(vps, info.type);
> + *count = virtio32_to_cpu(vps->vdev, info.count);
> +
> + flags = virtio32_to_cpu(vps->vdev, info.flags);
> + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
> +
> + time->tv_sec = virtio64_to_cpu(vps->vdev, info.time_sec);
> + time->tv_nsec = virtio32_to_cpu(vps->vdev, info.time_nsec);
> +
> + memcpy(bf, psi->buf, len);
> + *buf = bf;
> +
> + return len;
> +}
> +
> +static int notrace virt_pstore_write(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id, unsigned int part, int count,
> + bool compressed, size_t size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[2], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
> +
> + if (vps->failed)
> + return -1;
> +
> + *id = vps->req_id;
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
> + req->type = to_virtio_type(vps, type);
> + req->flags = cpu_to_virtio32(vps->vdev, flags);
> +
> + sg_init_table(sgo, 2);
> + sg_set_buf(&sgo[0], req, sizeof(*req));
> + sg_set_buf(&sgo[1], pstore_get_buf(psi), size);
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC);
> + virtqueue_kick(vps->vq[1]);
> +
> + return 0;
> +}
> +
> +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
> + struct timespec time, struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_req *req;
> + struct virtio_pstore_res *res;
> + struct scatterlist sgo[1], sgi[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> +
> + virt_pstore_get_reqs(vps, &req, &res);
> +
> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE);
> + req->type = to_virtio_type(vps, type);
> + req->id = cpu_to_virtio64(vps->vdev, id);
> + req->count = cpu_to_virtio32(vps->vdev, count);
> +
> + sg_init_one(sgo, req, sizeof(*req));
> + sg_init_one(sgi, res, sizeof(*res));
> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq[0]);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
> + return virtio32_to_cpu(vps->vdev, res->ret);
> +}
> +
> +static int virt_pstore_init(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> + int err;
> +
> + if (!psinfo->bufsize)
> + psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
> +
> + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL);
> + if (!psinfo->buf) {
> + pr_err("cannot allocate pstore buffer\n");
> + return -ENOMEM;
> + }
> +
> + psinfo->owner = THIS_MODULE;
> + psinfo->name = "virtio";
> + psinfo->open = virt_pstore_open;
> + psinfo->close = virt_pstore_close;
> + psinfo->read = virt_pstore_read;
> + psinfo->erase = virt_pstore_erase;
> + psinfo->write = virt_pstore_write;
> + psinfo->flags = PSTORE_FLAGS_DMESG;
> +
> + psinfo->data = vps;
> + spin_lock_init(&psinfo->buf_lock);
> +
> + err = pstore_register(psinfo);
> + if (err)
> + kfree(psinfo->buf);
> +
> + return err;
> +}
> +
> +static int virt_pstore_exit(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> +
> + pstore_unregister(psinfo);
I don't know enough about pstore - does this
actually ensure that
1. all existing users close the device
2. no new users can open it
somehow?
> +
> + free_pages_exact(psinfo->buf, psinfo->bufsize);
> + psinfo->buf = NULL;
> + psinfo->bufsize = 0;
> +
> + return 0;
> +}
> +
> +static int virtpstore_init_vqs(struct virtio_pstore *vps)
> +{
> + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check };
> + const char *names[] = { "pstore_read", "pstore_write" };
> +
> + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq,
> + callbacks, names);
> +}
> +
> +static void virtpstore_init_config(struct virtio_pstore *vps)
> +{
> + u32 bufsize;
> +
> + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize);
> +
> + vps->pstore.bufsize = PAGE_ALIGN(bufsize);
> +}
> +
> +static void virtpstore_confirm_config(struct virtio_pstore *vps)
> +{
> + u32 bufsize = vps->pstore.bufsize;
> +
> + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize,
> + &bufsize);
> +}
> +
> +static int virtpstore_probe(struct virtio_device *vdev)
> +{
> + struct virtio_pstore *vps;
> + int err;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "driver init: config access disabled\n");
> + return -EINVAL;
> + }
> +
> + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL);
> + if (!vps) {
> + err = -ENOMEM;
> + goto out;
> + }
> + vps->vdev = vdev;
> +
> + err = virtpstore_init_vqs(vps);
> + if (err < 0)
> + goto out_free;
> +
> + virtpstore_init_config(vps);
> +
> + err = virt_pstore_init(vps);
> + if (err)
> + goto out_del_vq;
> +
> + virtpstore_confirm_config(vps);
> +
> + init_waitqueue_head(&vps->acked);
> +
> + virtio_device_ready(vdev);
> +
> + dev_info(&vdev->dev, "driver init: ok (bufsize = %luK, flags = %x)\n",
> + vps->pstore.bufsize >> 10, vps->pstore.flags);
> +
> + return 0;
> +
> +out_del_vq:
> + vdev->config->del_vqs(vdev);
> +out_free:
> + kfree(vps);
> +out:
> + dev_err(&vdev->dev, "driver init: failed with %d\n", err);
> + return err;
> +}
> +
> +static void virtpstore_remove(struct virtio_device *vdev)
> +{
> + struct virtio_pstore *vps = vdev->priv;
> +
> + virt_pstore_exit(vps);
> +
> + /* Now we reset the device so we can clean up the queues. */
> + vdev->config->reset(vdev);
> +
> + vdev->config->del_vqs(vdev);
> +
> + kfree(vps);
> +}
> +
> +static unsigned int features[] = {
> +};
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_PSTORE, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_pstore_driver = {
We need some way to avoid trying to load this
as a legacy device. There isn't a way to do it yet
so I won't block your patch on this but pls try to
come up with something, and I'll do, too.
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .id_table = id_table,
> + .probe = virtpstore_probe,
> + .remove = virtpstore_remove,
Won't this need freeze/restore callbacks?
> +};
> +
> +module_virtio_driver(virtio_pstore_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Namhyung Kim <namhyung@kernel.org>");
> +MODULE_DESCRIPTION("Virtio pstore driver");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 6d4e92ccdc91..9bbb1554d8b2 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -449,6 +449,7 @@ header-y += virtio_ids.h
> header-y += virtio_input.h
> header-y += virtio_net.h
> header-y += virtio_pci.h
> +header-y += virtio_pstore.h
> header-y += virtio_ring.h
> header-y += virtio_rng.h
> header-y += virtio_scsi.h
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 77925f587b15..c72a9ab588c0 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 22 /* virtio pstore */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_pstore.h b/include/uapi/linux/virtio_pstore.h
> new file mode 100644
> index 000000000000..f4b0d204d8ae
> --- /dev/null
> +++ b/include/uapi/linux/virtio_pstore.h
> @@ -0,0 +1,74 @@
> +#ifndef _LINUX_VIRTIO_PSTORE_H
> +#define _LINUX_VIRTIO_PSTORE_H
> +/* This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of IBM nor the names of its contributors
> + * may be used to endorse or promote products derived from this software
> + * without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE. */
> +#include <linux/types.h>
> +#include <linux/virtio_types.h>
> +
> +#define VIRTIO_PSTORE_CMD_NULL 0
> +#define VIRTIO_PSTORE_CMD_OPEN 1
> +#define VIRTIO_PSTORE_CMD_READ 2
> +#define VIRTIO_PSTORE_CMD_WRITE 3
> +#define VIRTIO_PSTORE_CMD_ERASE 4
> +#define VIRTIO_PSTORE_CMD_CLOSE 5
> +
> +#define VIRTIO_PSTORE_TYPE_UNKNOWN 0
> +#define VIRTIO_PSTORE_TYPE_DMESG 1
> +
> +#define VIRTIO_PSTORE_FL_COMPRESSED 1
Most other headers use _F_ and not _FL_
Also, we specify bit number and not the
bitmask. So:
#define VIRTIO_PSTORE_F_COMPRESSED 0
and
(0x1 << VIRTIO_PSTORE_F_COMPRESSED)
> +
> +struct virtio_pstore_req {
> + __virtio16 cmd;
> + __virtio16 type;
> + __virtio32 flags;
> + __virtio64 id;
> + __virtio32 count;
> + __virtio32 reserved;
> +};
> +
> +struct virtio_pstore_res {
> + __virtio16 cmd;
> + __virtio16 type;
> + __virtio32 ret;
> +};
> +
> +struct virtio_pstore_fileinfo {
> + __virtio64 id;
> + __virtio32 count;
> + __virtio16 type;
> + __virtio16 unused;
> + __virtio32 flags;
> + __virtio32 len;
> + __virtio64 time_sec;
> + __virtio32 time_nsec;
> + __virtio32 reserved;
Any reason one is reserved the other is unused?
If not just calls them pad1, pad2?
> +};
> +
> +struct virtio_pstore_config {
> + __virtio32 bufsize;
> +};
> +
__virtio things are for compatibility things.
New devices should just use __le everywhere.
Let me post a patch that adds config space accessors
so you can do this.
> +#endif /* _LINUX_VIRTIO_PSTORE_H */
> --
> 2.9.3
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH v10] virtio-net: add Max MTU configuration field
From: Aaron Conole @ 2016-09-13 15:18 UTC (permalink / raw)
To: virtio-dev, virtualization
Cc: Victor Kaplansky, Maxime Coquelin, Michael S. Tsirkin
It is helpful for a host to indicate it's MTU to be set on guest NICs
other than the assumed 1500 byte value. This helps in situations where
the host network is using Jumbo Frames, or aiding in PMTU discovery by
configuring a homogenous network. It is also helpful for sizing receive
buffers correctly.
The change adds a new field to configuration area of network
devices. It will be used to pass a maximum MTU from the device to
the driver. This will be used by the driver as a maximum value for
packet sizes during transmission, without segmentation offloading.
In addition, in order to support backward and forward compatibility,
we introduce a new feature bit called VIRTIO_NET_F_MTU.
VIRTIO-152
Signed-off-by: Aaron Conole <aconole@redhat.com>
Cc: Victor Kaplansky <victork@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
v1:
This is an attempt at continuing the work done by Victor Kaplansky on
mtu negiotiation for virtio-net devices. It attempts to pick up from
https://lists.oasis-open.org/archives/virtio-dev/201508/msg00007.html
and is just a minor blurb from the first patch along with the 2nd patch
from the series, and some of the feedback integrated.
v2:
Rephrase and provide a mechanism for guest->host and host->guest
communication through a driver read-only and driver write-only field.
v3:
Converted to just support initial MTU. Guest->host and Host->guest MTU
changes are outside the scope of this change.
v4:
Removed references to 'initial', since that condition cannot be tested.
Simply state that if the driver will use the mtu field, it must
negotiate the feature bit, and if not, it must not.
v5:
After feedback from Michael S. Tsirkin
v6:
Bit has to change to bit 25 due to an undocumented bit 24 being taken.
v7:
Partial rewrite with feedback from MST. Additional conformance statements
added.
v8:
Clarified the new conformance statements.
v9:
Updated the commit log for correctness. Added ACKs from
Michael S. Tsirkin, and Maxime Coquelin. Included the virtio
issue id.
v10:
Update the conformance statement wordings from previous 'offered' form
to 'succesfully negotiated' form.
content.tex | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/content.tex b/content.tex
index 4b45678..42c0568 100644
--- a/content.tex
+++ b/content.tex
@@ -3049,6 +3049,11 @@ features.
\item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
reconfiguration support.
+\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
+ offered by the device, device advises driver about the value of
+ MTU to be used. If negotiated, the driver uses \field{mtu} as
+ the maximum MTU value supplied to the driver.
+
\item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
\item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
@@ -3140,11 +3145,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
is negotiated.
+The following driver-read-only field, \field{mtu} only exists if
+VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
+use.
+
\begin{lstlisting}
struct virtio_net_config {
u8 mac[6];
le16 status;
le16 max_virtqueue_pairs;
+ le16 mtu;
};
\end{lstlisting}
@@ -3153,6 +3163,19 @@ struct virtio_net_config {
The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
if it offers VIRTIO_NET_F_MQ.
+The device MUST set \field{mtu} to between 68 and 65535 inclusive,
+if it offers VIRTIO_NET_F_MTU.
+
+The device MUST NOT modify \field{mtu} once it has been set.
+
+The device MUST NOT pass received packets that exceed \field{mtu} size
+with \field{gso_type} NONE or ECN after VIRTIO_NET_F_MTU has been successfully
+negotiated.
+
+The device MUST forward transmitted packets of up to MTU size with
+\field{gso_type} NONE or ECN, and do so without fragmentation, after
+VIRTIO_NET_F_MTU has been successfully negotiated.
+
\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
@@ -3165,6 +3188,15 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
assume the link is active, otherwise it SHOULD read the link status from
the bottom bit of \field{status}.
+A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
+
+If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive
+buffers of size \field{mtu} to be able to receive at least one receive
+packet with \field{gso_type} NONE or ECN.
+
+If the driver negotiates VIRTIO_NET_F_MTU, it MUST NOT transmit packets of
+size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
+
\subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
\label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
When using the legacy interface, transitional devices and drivers
--
2.7.4
^ permalink raw reply related
* Re: virtio_blk: Less function calls in init_vq() after error detection
From: SF Markus Elfring @ 2016-09-13 14:33 UTC (permalink / raw)
To: Christian Bornträger, virtualization, Michael S. Tsirkin
Cc: Julia Lawall, kernel-janitors, LKML, Minfei Huang, linux-doc
In-Reply-To: <e918e655-cd86-c3c8-d911-9dfc03b03e19@de.ibm.com>
>> drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> Can't you see from this diffstat that the patch actually seems to makes
> the code more complex?
I find that the repeated usage of a bit more error handling code is almost
unavoidable if you would like to handle allocation failures more directly
as I dared to propose again here.
> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
> virtio_blk: Fix a slient kernel panic
>
> which did the opposite of your patch.
This software update adjusted also the jump targets. This approach
triggered another update suggestion (in addition to improvements around
the function "kmalloc_array").
Such a software development shows different views on the implementation
for correct exception handling. I am not so "silent" on this development topic
for years.
> And in fact it fixed a bug.
I get the impression from Minfei's contribution that the statement "err = -ENOMEM;"
was added behind memory allocations.
It was also chosen to restructure this function implementation so that
the single label "out" was used there for a while.
* Is this detail worth for another look?
* How does this name selection fit to the current Linux coding style convention?
> Quite obviously multiple labels are harder to read
I do not agree agree completely to your opinion.
> and harder to get right.
These identifiers can generate their own kind of software development
challenges as usual.
> For error handling with just kfree one label is just the right thing to.
This approach can look convenient at first glance.
Does the correctness aspect need any further considerations?
Regards,
Markus
^ 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