Re: [PATCH] drm: Explicitly include correct DT includes
On Sun, Jul 16, 2023 at 3:26 AM Heiko Stuebner wrote: > > Am Freitag, 14. Juli 2023, 19:45:34 CEST schrieb Rob Herring: > > The DT of_device.h and of_platform.h date back to the separate > > of_platform_bus_type before it as merged into the regular platform bus. > > As part of that merge prepping Arm DT support 13 years ago, they > > "temporarily" include each other. They also include platform_device.h > > and of.h. As a result, there's a pretty much random mix of those include > > files used throughout the tree. In order to detangle these headers and > > replace the implicit includes with struct declarations, users need to > > explicitly include the correct includes. > > > > Signed-off-by: Rob Herring > > --- > > [...] > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > > b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > > index 917e79951aac..2744d8f4a6fa 100644 > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c > > @@ -12,7 +12,9 @@ > > #include > > #include > > #include > > +#include > > #include > > +#include > > #include > > #include > > I'm not sure if I'm just misreading something, but in all other places > of_device.h gets removed while here is stays as an include. Is this > correct this way? Yes, because of_match_device() is used. Rob
[PATCH] drm: Explicitly include correct DT includes
The DT of_device.h and of_platform.h date back to the separate of_platform_bus_type before it as merged into the regular platform bus. As part of that merge prepping Arm DT support 13 years ago, they "temporarily" include each other. They also include platform_device.h and of.h. As a result, there's a pretty much random mix of those include files used throughout the tree. In order to detangle these headers and replace the implicit includes with struct declarations, users need to explicitly include the correct includes. Signed-off-by: Rob Herring --- drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 2 +- drivers/gpu/drm/arm/malidp_drv.c | 1 + drivers/gpu/drm/bridge/adv7511/adv7511_cec.c | 1 - drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 +- drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c| 3 +-- drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 1 - drivers/gpu/drm/bridge/chipone-icn6211.c | 2 +- drivers/gpu/drm/bridge/display-connector.c| 1 - drivers/gpu/drm/bridge/fsl-ldb.c | 1 - drivers/gpu/drm/bridge/imx/imx8qm-ldb.c | 2 +- drivers/gpu/drm/bridge/imx/imx8qxp-ldb.c | 1 + drivers/gpu/drm/bridge/lontium-lt9211.c | 1 - drivers/gpu/drm/bridge/lvds-codec.c | 1 - drivers/gpu/drm/bridge/nwl-dsi.c | 2 +- drivers/gpu/drm/bridge/parade-ps8622.c| 1 - drivers/gpu/drm/bridge/samsung-dsim.c | 3 ++- drivers/gpu/drm/bridge/simple-bridge.c| 3 ++- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 2 +- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- drivers/gpu/drm/drm_mipi_dsi.c| 1 + drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 +- drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 2 +- drivers/gpu/drm/exynos/exynos7_drm_decon.c| 1 - drivers/gpu/drm/exynos/exynos_drm_dsi.c | 3 ++- drivers/gpu/drm/exynos/exynos_drm_fimd.c | 1 - drivers/gpu/drm/exynos/exynos_drm_rotator.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_scaler.c| 2 +- drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/exynos/exynos_mixer.c | 1 - drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 2 +- drivers/gpu/drm/imx/dcss/dcss-dev.c | 5 +++-- drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 +- drivers/gpu/drm/ingenic/ingenic-ipu.c | 2 +- drivers/gpu/drm/lima/lima_drv.c | 3 ++- drivers/gpu/drm/logicvc/logicvc_drm.c | 2 +- drivers/gpu/drm/mcde/mcde_drv.c | 2 +- drivers/gpu/drm/mediatek/mtk_disp_aal.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_color.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_merge.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 3 +-- drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 3 ++- drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 3 +-- drivers/gpu/drm/mediatek/mtk_dpi.c| 1 - drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.c| 3 ++- drivers/gpu/drm/mediatek/mtk_ethdr.c | 2 +- drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 3 +-- drivers/gpu/drm/meson/meson_drv.h | 1 - drivers/gpu/drm/meson/meson_dw_hdmi.c | 3 ++- drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 3 ++- drivers/gpu/drm/meson/meson_encoder_dsi.c | 1 - drivers/gpu/drm/meson/meson_encoder_hdmi.c| 4 +++- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 ++ drivers/gpu/drm/msm/dp/dp_audio.c | 2 +- drivers/gpu/drm/msm/dsi/dsi_host.c| 2 +- drivers/gpu/drm/msm/hdmi/hdmi.c | 2 ++ drivers/gpu/drm/msm/hdmi/hdmi_phy.c | 3 ++- drivers/gpu/drm/msm/msm_mdss.c| 2 ++ drivers/gpu/drm/mxsfb/lcdif_drv.c | 1 - drivers/gpu/drm/panel/panel-abt-y030xx067a.c | 3 ++- drivers/gpu/drm/panel/panel-auo-a030jtn01.c | 2 +- drivers/gpu/drm/panel/panel-boe-himax8279d.c | 1 - drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c| 1 - drivers/gpu/drm/panel/panel-dsi-cm.c | 2 +- drivers/gpu/drm/panel/panel-feixin-k101-im2ba02.c | 1 - drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c | 2 +- drivers/gpu/drm/panel/panel-himax-hx8394.c| 2 +- drivers/gpu/drm/pan
Re: [PATCH V4 5/8] dt-bindings: Add xen,grant-dma IOMMU description for xen-grant DMA ops
On Thu, 02 Jun 2022 22:23:50 +0300, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > The main purpose of this binding is to communicate Xen specific > information using generic IOMMU device tree bindings (which is > a good fit here) rather than introducing a custom property. > > Introduce Xen specific IOMMU for the virtualized device (e.g. virtio) > to be used by Xen grant DMA-mapping layer in the subsequent commit. > > The reference to Xen specific IOMMU node using "iommus" property > indicates that Xen grant mappings need to be enabled for the device, > and it specifies the ID of the domain where the corresponding backend > resides. The domid (domain ID) is used as an argument to the Xen grant > mapping APIs. > > This is needed for the option to restrict memory access using Xen grant > mappings to work which primary goal is to enable using virtio devices > in Xen guests. > > Signed-off-by: Oleksandr Tyshchenko > Reviewed-by: Stefano Stabellini > --- > Changes RFC -> V1: >- update commit subject/description and text in description >- move to devicetree/bindings/arm/ > > Changes V1 -> V2: >- update text in description >- change the maintainer of the binding >- fix validation issue >- reference xen,dev-domid.yaml schema from virtio/mmio.yaml > > Change V2 -> V3: >- Stefano already gave his Reviewed-by, I dropped it due to the changes > (significant) >- use generic IOMMU device tree bindings instead of custom property > "xen,dev-domid" >- change commit subject and description, was > "dt-bindings: Add xen,dev-domid property description for xen-grant DMA > ops" > > Changes V3 -> V4: >- add Stefano's R-b >- remove underscore in iommu node name >- remove consumer example virtio@3000 >- update text for two descriptions > --- > .../devicetree/bindings/iommu/xen,grant-dma.yaml | 39 > ++ > 1 file changed, 39 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iommu/xen,grant-dma.yaml > Reviewed-by: Rob Herring
Re: [PATCH V2 5/7] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops
+Saravana
On Mon, May 23, 2022 at 06:58:13PM -0700, Stefano Stabellini wrote:
> On Mon, 23 May 2022, Oleksandr wrote:
> > > > On Thu, 19 May 2022, Oleksandr wrote:
> > > > > > On Wed, May 18, 2022 at 5:06 PM Oleksandr
> > > > > > wrote:
> > > > > > > On 18.05.22 17:32, Arnd Bergmann wrote:
> > > > > > > > On Sat, May 7, 2022 at 7:19 PM Oleksandr Tyshchenko
> > > > > > > > wrote:
> > > > > > > > This would mean having a device
> > > > > > > > node for the grant-table mechanism that can be referred to using
> > > > > > > > the
> > > > > > > > 'iommus'
> > > > > > > > phandle property, with the domid as an additional argument.
> > > > > > > I assume, you are speaking about something like the following?
> > > > > > >
> > > > > > >
> > > > > > > xen_dummy_iommu {
> > > > > > > compatible = "xen,dummy-iommu";
> > > > > > > #iommu-cells = <1>;
> > > > > > > };
> > > > > > >
> > > > > > > virtio@3000 {
> > > > > > > compatible = "virtio,mmio";
> > > > > > > reg = <0x3000 0x100>;
> > > > > > > interrupts = <41>;
> > > > > > >
> > > > > > > /* The device is located in Xen domain with ID 1 */
> > > > > > > iommus = <&xen_dummy_iommu 1>;
> > > > > > > };
> > > > > > Right, that's that's the idea,
> > > > > thank you for the confirmation
> > > > >
> > > > >
> > > > >
> > > > > > except I would not call it a 'dummy'.
> > > > > > From the perspective of the DT, this behaves just like an IOMMU,
> > > > > > even if the exact mechanism is different from most hardware IOMMU
> > > > > > implementations.
> > > > > well, agree
> > > > >
> > > > >
> > > > > > > > It does not quite fit the model that Linux currently uses for
> > > > > > > > iommus,
> > > > > > > > as that has an allocator for dma_addr_t space
> > > > > > > yes (# 3/7 adds grant-table based allocator)
> > > > > > >
> > > > > > >
> > > > > > > > , but it would think it's
> > > > > > > > conceptually close enough that it makes sense for the binding.
> > > > > > > Interesting idea. I am wondering, do we need an extra actions for
> > > > > > > this
> > > > > > > to work in Linux guest (dummy IOMMU driver, etc)?
> > > > > > It depends on how closely the guest implementation can be made to
> > > > > > resemble a normal iommu. If you do allocate dma_addr_t addresses,
> > > > > > it may actually be close enough that you can just turn the
> > > > > > grant-table
> > > > > > code into a normal iommu driver and change nothing else.
> > > > > Unfortunately, I failed to find a way how use grant references at the
> > > > > iommu_ops level (I mean to fully pretend that we are an IOMMU
> > > > > driver). I
> > > > > am
> > > > > not too familiar with that, so what is written below might be wrong or
> > > > > at
> > > > > least not precise.
> > > > >
> > > > > The normal IOMMU driver in Linux doesn’t allocate DMA addresses by
> > > > > itself, it
> > > > > just maps (IOVA-PA) what was requested to be mapped by the upper
> > > > > layer.
> > > > > The
> > > > > DMA address allocation is done by the upper layer (DMA-IOMMU which is
> > > > > the glue
> > > > > layer between DMA API and IOMMU API allocates IOVA for PA?). But, all
> > > > > what we
> > > > > need here is just to allocate our specific grant-table based DMA
> > > > > addresses
> > > > > (DMA address = grant reference + offset in the page), so let’s say we
> > > > > need an
> > > > > entity to take a physical address as parameter and return a DMA
> > > > > address
> > > > > (what
> > > > > actually commit #3/7 is doing), and that’s all. So working at the
> > > > > dma_ops
> > > > > layer we get exactly what we need, with the minimal changes to guest
> > > > > infrastructure. In our case the Xen itself acts as an IOMMU.
> > > > >
> > > > > Assuming that we want to reuse the IOMMU infrastructure somehow for
> > > > > our
> > > > > needs.
> > > > > I think, in that case we will likely need to introduce a new specific
> > > > > IOVA
> > > > > allocator (alongside with a generic one) to be hooked up by the
> > > > > DMA-IOMMU
> > > > > layer if we run on top of Xen. But, even having the specific IOVA
> > > > > allocator to
> > > > > return what we indeed need (DMA address = grant reference + offset in
> > > > > the
> > > > > page) we will still need the specific minimal required IOMMU driver to
> > > > > be
> > > > > present in the system anyway in order to track the mappings(?) and do
> > > > > nothing
> > > > > with them, returning a success (this specific IOMMU driver should have
> > > > > all
> > > > > mandatory callbacks implemented).
> > > > >
> > > > > I completely agree, it would be really nice to reuse generic IOMMU
> > > > > bindings
> > > > > rather than introducing Xen specific property if what we are trying to
> > > > > implement in current patch series fits in the usage of "iommus" in
> > > > > Linux
> > > > > more-less. But, if we will have to add more complexity/more components
> > > > > to the
> > > > > code for the sake of reusing devi
Re: [PATCH V2 5/7] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops
On Wed, May 18, 2022 at 03:32:27PM +0100, Arnd Bergmann wrote: > On Sat, May 7, 2022 at 7:19 PM Oleksandr Tyshchenko > wrote: > > > > diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml > > b/Documentation/devicetree/bindings/virtio/mmio.yaml > > index 10c22b5..29a0932 100644 > > --- a/Documentation/devicetree/bindings/virtio/mmio.yaml > > +++ b/Documentation/devicetree/bindings/virtio/mmio.yaml > > @@ -13,6 +13,9 @@ description: > >See https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio > > for > >more details. > > > > +allOf: > > + - $ref: /schemas/arm/xen,dev-domid.yaml# > > + > > properties: > >compatible: > > const: virtio,mmio > > @@ -33,6 +36,10 @@ properties: > > description: Required for devices making accesses thru an IOMMU. > > maxItems: 1 > > > > + xen,dev-domid: > > +description: Required when Xen grant mappings need to be enabled for > > device. > > +$ref: /schemas/types.yaml#/definitions/uint32 > > + > > required: > >- compatible > >- reg > > Sorry for joining the discussion late. Have you considered using the > generic iommu > binding here instead of a custom property? This would mean having a device > node for the grant-table mechanism that can be referred to using the 'iommus' > phandle property, with the domid as an additional argument. > > It does not quite fit the model that Linux currently uses for iommus, > as that has an allocator for dma_addr_t space, but it would think it's > conceptually close enough that it makes sense for the binding. Something common is almost always better. That may also have the issue that fw_devlink will make the 'iommu' driver a dependency to probe. Rob
Re: [PATCH V2 5/7] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops
On Sat, May 07, 2022 at 09:19:06PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko
>
> Introduce Xen specific binding for the virtualized device (e.g. virtio)
> to be used by Xen grant DMA-mapping layer in the subsequent commit.
>
> This binding indicates that Xen grant mappings scheme needs to be
> enabled for the device which DT node contains that property and specifies
> the ID of Xen domain where the corresponding backend resides. The ID
> (domid) is used as an argument to the grant mapping APIs.
>
> This is needed for the option to restrict memory access using Xen grant
> mappings to work which primary goal is to enable using virtio devices
> in Xen guests.
>
> Signed-off-by: Oleksandr Tyshchenko
> ---
> Changes RFC -> V1:
>- update commit subject/description and text in description
>- move to devicetree/bindings/arm/
>
> Changes V1 -> V2:
>- update text in description
>- change the maintainer of the binding
>- fix validation issue
>- reference xen,dev-domid.yaml schema from virtio/mmio.yaml
> ---
> .../devicetree/bindings/arm/xen,dev-domid.yaml | 37
> ++
> Documentation/devicetree/bindings/virtio/mmio.yaml | 7
> 2 files changed, 44 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> new file mode 100644
> index ..750e89e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/xen,dev-domid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xen specific binding for virtualized devices (e.g. virtio)
> +
> +maintainers:
> + - Stefano Stabellini
> +
> +select: true
Omit. No need to apply this on every single node.
> +
> +description:
> + This binding indicates that Xen grant mappings need to be enabled for
> + the device, and it specifies the ID of the domain where the corresponding
> + device (backend) resides. The property is required to restrict memory
> + access using Xen grant mappings.
> +
> +properties:
> + xen,dev-domid:
I kind of think 'dev' is redundant. Is there another kind of domid
possible? Maybe xen,backend-domid or just xen,domid? I don't know Xen
too well, so ultimately up to you all.
> +$ref: /schemas/types.yaml#/definitions/uint32
> +description:
> + The domid (domain ID) of the domain where the device (backend) is
> running.
> +
> +additionalProperties: true
> +
> +examples:
> + - |
> +virtio@3000 {
> +compatible = "virtio,mmio";
> +reg = <0x3000 0x100>;
> +interrupts = <41>;
> +
> +/* The device is located in Xen domain with ID 1 */
> +xen,dev-domid = <1>;
> +};
> diff --git a/Documentation/devicetree/bindings/virtio/mmio.yaml
> b/Documentation/devicetree/bindings/virtio/mmio.yaml
> index 10c22b5..29a0932 100644
> --- a/Documentation/devicetree/bindings/virtio/mmio.yaml
> +++ b/Documentation/devicetree/bindings/virtio/mmio.yaml
> @@ -13,6 +13,9 @@ description:
>See https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio for
>more details.
>
> +allOf:
> + - $ref: /schemas/arm/xen,dev-domid.yaml#
> +
> properties:
>compatible:
> const: virtio,mmio
> @@ -33,6 +36,10 @@ properties:
> description: Required for devices making accesses thru an IOMMU.
> maxItems: 1
>
> + xen,dev-domid:
> +description: Required when Xen grant mappings need to be enabled for
> device.
> +$ref: /schemas/types.yaml#/definitions/uint32
No need to define the type again nor describe it again.
Instead, just change additionalProperties to unevaluateProperties in
this doc. The diff is the latter takes $ref's into account.
> +
> required:
>- compatible
>- reg
> --
> 2.7.4
>
>
Re: [PATCH V1 4/6] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops
On Tue, May 03, 2022 at 08:09:32PM +0300, Oleksandr wrote:
>
> On 03.05.22 00:59, Rob Herring wrote:
>
> Hello Rob
>
>
> > On Fri, Apr 22, 2022 at 07:51:01PM +0300, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko
> > >
> > > Introduce Xen specific binding for the virtualized device (e.g. virtio)
> > > to be used by Xen grant DMA-mapping layer in the subsequent commit.
> > >
> > > This binding indicates that Xen grant mappings scheme needs to be
> > > enabled for the device which DT node contains that property and specifies
> > > the ID of Xen domain where the corresponding backend resides. The ID
> > > (domid) is used as an argument to the grant mapping APIs.
> > >
> > > This is needed for the option to restrict memory access using Xen grant
> > > mappings to work which primary goal is to enable using virtio devices
> > > in Xen guests.
> > >
> > > Signed-off-by: Oleksandr Tyshchenko
> > > ---
> > > Changes RFC -> V1:
> > > - update commit subject/description and text in description
> > > - move to devicetree/bindings/arm/
> > > ---
> > > .../devicetree/bindings/arm/xen,dev-domid.yaml | 37
> > > ++
> > > 1 file changed, 37 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> > > b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> > > new file mode 100644
> > > index ..ef0f747
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> > > @@ -0,0 +1,37 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/arm/xen,dev-domid.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Xen specific binding for the virtualized device (e.g. virtio)
> > > +
> > > +maintainers:
> > > + - Oleksandr Tyshchenko
> > > +
> > > +select: true
> > Do we really need to support this property everywhere?
>
> From my understanding - yes.
>
> As, I think, any device node describing virtulized device in the guest
> device tree can have this property. Initially (in the RFC series) the
> "solution to restrict memory access using Xen grant mappings" was
> virtio-specific.
>
> Although the support of virtio is a primary target of this series, we
> decided to generalize this work and expand it to any device [1]. So the Xen
> grant mappings scheme (this property to be used for) can be theoretically
> used for any device emulated by the Xen backend.
>
>
> > > +
> > > +description:
> > > + This binding indicates that Xen grant mappings scheme needs to be
> > > enabled
> > > + for that device and specifies the ID of Xen domain where the
> > > corresponding
> > > + device (backend) resides. This is needed for the option to restrict
> > > memory
> > > + access using Xen grant mappings to work.
> > > +
> > > +properties:
> > > + xen,dev-domid:
> > > +$ref: /schemas/types.yaml#/definitions/uint32
> > > +description:
> > > + The domid (domain ID) of the domain where the device (backend) is
> > > running.
> > > +
> > > +additionalProperties: true
> > > +
> > > +examples:
> > > + - |
> > > +virtio_block@3000 {
> > virtio@3000
>
> ok, will change
>
>
> >
> > > +compatible = "virtio,mmio";
> > > +reg = <0x3000 0x100>;
> > > +interrupts = <41>;
> > > +
> > > +/* The device is located in Xen domain with ID 1 */
> > > +xen,dev-domid = <1>;
> > This fails validation:
> >
> > Documentation/devicetree/bindings/arm/xen,dev-domid.example.dtb:
> > virtio_block@3000: xen,dev-domid: [[1]] is not of type 'object'
> > From schema:
> > /home/rob/proj/git/linux-dt/Documentation/devicetree/bindings/virtio/mmio.yaml
>
> Thank you for pointing this out, my fault, I haven't "properly" checked this
> before. I think, we need to remove "compatible = "virtio,mmio"; here
Uhh, no. That just means the example is incomplete. You need to add th
Re: [PATCH V1 4/6] dt-bindings: Add xen,dev-domid property description for xen-grant DMA ops
On Fri, Apr 22, 2022 at 07:51:01PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko
>
> Introduce Xen specific binding for the virtualized device (e.g. virtio)
> to be used by Xen grant DMA-mapping layer in the subsequent commit.
>
> This binding indicates that Xen grant mappings scheme needs to be
> enabled for the device which DT node contains that property and specifies
> the ID of Xen domain where the corresponding backend resides. The ID
> (domid) is used as an argument to the grant mapping APIs.
>
> This is needed for the option to restrict memory access using Xen grant
> mappings to work which primary goal is to enable using virtio devices
> in Xen guests.
>
> Signed-off-by: Oleksandr Tyshchenko
> ---
> Changes RFC -> V1:
>- update commit subject/description and text in description
>- move to devicetree/bindings/arm/
> ---
> .../devicetree/bindings/arm/xen,dev-domid.yaml | 37
> ++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> new file mode 100644
> index ..ef0f747
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/xen,dev-domid.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/xen,dev-domid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xen specific binding for the virtualized device (e.g. virtio)
> +
> +maintainers:
> + - Oleksandr Tyshchenko
> +
> +select: true
Do we really need to support this property everywhere?
> +
> +description:
> + This binding indicates that Xen grant mappings scheme needs to be enabled
> + for that device and specifies the ID of Xen domain where the corresponding
> + device (backend) resides. This is needed for the option to restrict memory
> + access using Xen grant mappings to work.
> +
> +properties:
> + xen,dev-domid:
> +$ref: /schemas/types.yaml#/definitions/uint32
> +description:
> + The domid (domain ID) of the domain where the device (backend) is
> running.
> +
> +additionalProperties: true
> +
> +examples:
> + - |
> +virtio_block@3000 {
virtio@3000
> +compatible = "virtio,mmio";
> +reg = <0x3000 0x100>;
> +interrupts = <41>;
> +
> +/* The device is located in Xen domain with ID 1 */
> +xen,dev-domid = <1>;
This fails validation:
Documentation/devicetree/bindings/arm/xen,dev-domid.example.dtb:
virtio_block@3000: xen,dev-domid: [[1]] is not of type 'object'
From schema:
/home/rob/proj/git/linux-dt/Documentation/devicetree/bindings/virtio/mmio.yaml
The property has to be added to the virtio/mmio.yaml schema. If it is
not needed elsewhere, then *just* add the property there.
Rob
Re: [PATCH v2] of: of_property_read_string return -ENODATA when !length
On Fri, 15 Apr 2022 17:30:28 -0700, Stefano Stabellini wrote: > From: Stefano Stabellini > > When the length of the string is zero of_property_read_string should > return -ENODATA according to the description of the function. > > However, of_property_read_string doesn't check prop->length. If > prop->length is zero, return -ENODATA. > > Without this patch the following command in u-boot: > > fdt set /chosen/node property-name > > results in of_property_read_string returning -EILSEQ when attempting to > read property-name. With this patch, it returns -ENODATA as expected. > > Signed-off-by: Stefano Stabellini > --- > Changes in v2: > - use prop instead pp > - drop value check > - update function header documentation > --- > drivers/of/property.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Applied, thanks!
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, Apr 01, 2022 at 03:43:42PM -0700, Stefano Stabellini wrote:
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
> > wrote:
> > >
> > > On Fri, 1 Apr 2022, Rob Herring wrote:
> > > > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > > > wrote:
> > > > >
> > > > > From: Stefano Stabellini
> > > > >
> > > > > When the length of the string is zero of_property_read_string should
> > > > > return -ENODATA according to the description of the function.
> > > >
> > > > Perhaps it is a difference of:
> > > >
> > > > prop;
> > > >
> > > > vs.
> > > >
> > > > prop = "";
> > > >
> > > > Both are 0 length by some definition. The description, '-ENODATA if
> > > > property does not have a value', matches the first case.
> > > >
> > > > >
> > > > > However, of_property_read_string doesn't check pp->length. If
> > > > > pp->length
> > > > > is zero, return -ENODATA.
> > > > >
> > > > > Without this patch the following command in u-boot:
> > > > >
> > > > > fdt set /chosen/node property-name
> > > > >
> > > > > results in of_property_read_string returning -EILSEQ when attempting
> > > > > to
> > > > > read property-name. With this patch, it returns -ENODATA as expected.
> > > >
> > > > Why do you care? Do you have a user? There could be an in tree user
> > > > that doesn't like this change.
> > >
> > > During review of a Xen patch series (we have libfdt is Xen too, synced
> > > with the kernel) Julien noticed a check for -EILSEQ. I added the check
> > > so that Xen would behave correctly in cases like the u-boot example in
> > > the patch description.
> > >
> > > Looking more into it, it seemed to be a mismatch between the description
> > > of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> > > be the right return value, not -EILSEQ.)
> > >
> > > I added a printk to confirm what was going on when -EILSEQ was returned:
> > >
> > > printk("DEBUG %s %d value=%s value[0]=%d length=%u
> > > len=%lu\n",__func__,__LINE__,(char*)pp->value,
> > > *((char*)pp->value),pp->length,
> > > strlen(pp->value));
> > >
> > > This is the output:
> > > DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
> >
> > It turns out that we never set pp->value to NULL when unflattening
> > (and libfdt always returns a value). This function is assuming we do.
> > >
> > > As the description says:
> > >
> > > *
> > > * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA
> > > if
> > > * property does not have a value, and -EILSEQ if the string is not
> > > * null-terminated within the length of the property data.
> > > *
> > >
> > > It seems that this case matches "property does not have a value" which
> > > is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> > > say that length is zero, so the string cannot be null-terminated,
> > > thus -EILSEQ?
> > >
> > > I am happy to go with your interpretation but -ENODATA seems to be the
> > > best match in my opinion.
> >
> > I agree. I just think empty property should have a NULL value and 0
> > length, but we should only have to check one. I don't want check
> > length as that could be different for Sparc or non-FDT. So I think we
> > need this instead:
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index ec315b060cd5..d6b2b0d49d89 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
> >
> > pp->name = (char *)pname;
> > pp->length = sz;
> > - pp->value = (__be32 *)val;
> > + pp->value = sz ? (__be32 *)val : NULL;
> > *pprev = pp;
> > pprev = &pp->next;
> > }
> >
> >
> > It looks like setting 'value' has been like this at least since 2010.
>
> Yep, fixing old bugs nobody cares about, that's me :-)
:)
> I made the corresponding change to Xen to check that it fixes the
> original issue (I am using Xen only for convenience because I already
> have a reproducer setup for it.)
>
> Unfortunately it breaks the boot: the reason is that of_get_property is
> implemented as:
>
> return pp ? pp->value : NULL;
>
> and at least in Xen (maybe in Linux too) there are instances of callers
> doing:
>
> if (!of_get_property(node, "interrupt-controller", NULL))
> continue;
>
> This would now take the wrong code path because value is returned as
> NULL.
>
> So, although your patch is technically correct, it comes with higher
> risk of breaking existing code. How do you want to proceed?
We should just check 'length' is 0 and drop the !value part.
Rob
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
wrote:
>
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > wrote:
> > >
> > > From: Stefano Stabellini
> > >
> > > When the length of the string is zero of_property_read_string should
> > > return -ENODATA according to the description of the function.
> >
> > Perhaps it is a difference of:
> >
> > prop;
> >
> > vs.
> >
> > prop = "";
> >
> > Both are 0 length by some definition. The description, '-ENODATA if
> > property does not have a value', matches the first case.
> >
> > >
> > > However, of_property_read_string doesn't check pp->length. If pp->length
> > > is zero, return -ENODATA.
> > >
> > > Without this patch the following command in u-boot:
> > >
> > > fdt set /chosen/node property-name
> > >
> > > results in of_property_read_string returning -EILSEQ when attempting to
> > > read property-name. With this patch, it returns -ENODATA as expected.
> >
> > Why do you care? Do you have a user? There could be an in tree user
> > that doesn't like this change.
>
> During review of a Xen patch series (we have libfdt is Xen too, synced
> with the kernel) Julien noticed a check for -EILSEQ. I added the check
> so that Xen would behave correctly in cases like the u-boot example in
> the patch description.
>
> Looking more into it, it seemed to be a mismatch between the description
> of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> be the right return value, not -EILSEQ.)
>
> I added a printk to confirm what was going on when -EILSEQ was returned:
>
> printk("DEBUG %s %d value=%s value[0]=%d length=%u
> len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length,
> strlen(pp->value));
>
> This is the output:
> DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0
It turns out that we never set pp->value to NULL when unflattening
(and libfdt always returns a value). This function is assuming we do.
>
> As the description says:
>
> *
> * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA if
> * property does not have a value, and -EILSEQ if the string is not
> * null-terminated within the length of the property data.
> *
>
> It seems that this case matches "property does not have a value" which
> is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> say that length is zero, so the string cannot be null-terminated,
> thus -EILSEQ?
>
> I am happy to go with your interpretation but -ENODATA seems to be the
> best match in my opinion.
I agree. I just think empty property should have a NULL value and 0
length, but we should only have to check one. I don't want check
length as that could be different for Sparc or non-FDT. So I think we
need this instead:
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ec315b060cd5..d6b2b0d49d89 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -165,7 +165,7 @@ static void populate_properties(const void *blob,
pp->name = (char *)pname;
pp->length = sz;
- pp->value = (__be32 *)val;
+ pp->value = sz ? (__be32 *)val : NULL;
*pprev = pp;
pprev = &pp->next;
}
It looks like setting 'value' has been like this at least since 2010.
Rob
Re: [PATCH] of: of_property_read_string return -ENODATA when !length
On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini wrote: > > From: Stefano Stabellini > > When the length of the string is zero of_property_read_string should > return -ENODATA according to the description of the function. Perhaps it is a difference of: prop; vs. prop = ""; Both are 0 length by some definition. The description, '-ENODATA if property does not have a value', matches the first case. > > However, of_property_read_string doesn't check pp->length. If pp->length > is zero, return -ENODATA. > > Without this patch the following command in u-boot: > > fdt set /chosen/node property-name > > results in of_property_read_string returning -EILSEQ when attempting to > read property-name. With this patch, it returns -ENODATA as expected. Why do you care? Do you have a user? There could be an in tree user that doesn't like this change. > > Signed-off-by: Stefano Stabellini > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 8e90071de6ed..da0f02c98bb2 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node *np, > const char *propname, > const struct property *prop = of_find_property(np, propname, NULL); > if (!prop) > return -EINVAL; > - if (!prop->value) > + if (!prop->value || !pp->length) > return -ENODATA; > if (strnlen(prop->value, prop->length) >= prop->length) > return -EILSEQ;
Re: [PATCH v3 1/2] dt-bindings: arm: xen: document Xen iommu device
On Wed, Jan 26, 2022 at 10:56:39AM -0800, Stefano Stabellini wrote: > On Wed, 26 Jan 2022, Robin Murphy wrote: > > On 2022-01-26 15:09, Sergiy Kibrik wrote: > > > Hi Robin, > > > > > > > > > > > This could break Linux guests, since depending on the deferred probe > > > > timeout setting it could lead to drivers never probing because the > > > > "IOMMU" > > > > never becomes available. > > > > > > > > > > I've noticed no deferred probe timeouts when booting with this patch. > > > Could > > > you please explain more on how this would break guests? > > > > Right now I think it would actually require command-line intervention, e.g. > > "fw_devlink=on" or "deferred_probe_timeout=3600" (with modules enabled for > > the > > latter to take full effect), but I'm wary of the potential for future config > > options to control those behaviours by default. fw_devlink=on is now the default (for at least a couple of cycles). > > If deferred_probe_timeout=3600 was specified, we would just need an > IOMMU driver in Linux for the "xen,iommu-el2-v1" node to solve the > problem, right? I guess I am trying to say that it wouldn't be a device > tree interface problem but rather a Linux implementation discussion. You would have to add that IOMMU driver to old, existing kernels if you want compatibility with a new DT. Otherwise, that kernel would stop booting with a new DT. Rob
Re: [PATCH V4 6/6] dt-bindings: xen: Clarify "reg" purpose
On Fri, 10 Dec 2021 13:36:41 +0200, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > Xen on Arm has gained new support recently to calculate and report > extended regions (unused address space) safe to use for external > mappings. These regions are reported via "reg" property under > "hypervisor" node in the guest device-tree. As region 0 is reserved > for grant table space (always present), the indexes for extended > regions are 1...N. > > No device-tree bindings update is needed (except clarifying the text) > as guest infers the presence of extended regions from the number > of regions in "reg" property. > > Signed-off-by: Oleksandr Tyshchenko > --- > Changes V2 -> V3: >- new patch > > Changes V3 -> V4: >- add Stefano's R-b and Rob's A-b >- remove sentence about ACPI for "reg" and "interrupts" > properties > > Changes V4 -> V4.1 >- bring the mentioning of ACPI back which, as was pointed out by Julien, > fits in the context: > > https://lore.kernel.org/xen-devel/9602b019-6c20-cdc7-23f3-9e4f8fd72...@xen.org/T/#t > so technically restore V3 state >- remove Stefano's R-b and Rob's A-b as I am not 100% sure they are > happy with that > --- > Documentation/devicetree/bindings/arm/xen.txt | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > Acked-by: Rob Herring
Re: [PATCH V3 6/6] dt-bindings: xen: Clarify "reg" purpose
On Wed, 24 Nov 2021 22:53:43 +0200, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko > > Xen on Arm has gained new support recently to calculate and report > extended regions (unused address space) safe to use for external > mappings. These regions are reported via "reg" property under > "hypervisor" node in the guest device-tree. As region 0 is reserved > for grant table space (always present), the indexes for extended > regions are 1...N. > > No device-tree bindings update is needed (except clarifying the text) > as guest infers the presence of extended regions from the number > of regions in "reg" property. > > Signed-off-by: Oleksandr Tyshchenko > --- > According to the recent update to Xen's guest.txt: > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob_plain;f=docs/misc/arm/device-tree/guest.txt;hb=refs/heads/master > > Changes V2 -> V3: >- new patch > --- > Documentation/devicetree/bindings/arm/xen.txt | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > Acked-by: Rob Herring
Re: [PATCH v4 13/14] dt-bindings: of: Add restricted DMA pool
On Wed, Mar 10, 2021 at 9:08 AM Will Deacon wrote:
>
> Hi Claire,
>
> On Tue, Feb 09, 2021 at 02:21:30PM +0800, Claire Chang wrote:
> > Introduce the new compatible string, restricted-dma-pool, for restricted
> > DMA. One can specify the address and length of the restricted DMA memory
> > region by restricted-dma-pool in the reserved-memory node.
> >
> > Signed-off-by: Claire Chang
> > ---
> > .../reserved-memory/reserved-memory.txt | 24 +++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > index e8d3096d922c..fc9a12c2f679 100644
> > --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > @@ -51,6 +51,20 @@ compatible (optional) - standard definition
> >used as a shared pool of DMA buffers for a set of devices. It can
> >be used by an operating system to instantiate the necessary pool
> >management subsystem if necessary.
> > +- restricted-dma-pool: This indicates a region of memory meant to
> > be
> > + used as a pool of restricted DMA buffers for a set of devices.
> > The
> > + memory region would be the only region accessible to those
> > devices.
> > + When using this, the no-map and reusable properties must not be
> > set,
> > + so the operating system can create a virtual mapping that will
> > be used
> > + for synchronization. The main purpose for restricted DMA is to
> > + mitigate the lack of DMA access control on systems without an
> > IOMMU,
> > + which could result in the DMA accessing the system memory at
> > + unexpected times and/or unexpected addresses, possibly leading
> > to data
> > + leakage or corruption. The feature on its own provides a basic
> > level
> > + of protection against the DMA overwriting buffer contents at
> > + unexpected times. However, to protect against general data
> > leakage and
> > + system memory corruption, the system needs to provide way to
> > lock down
> > + the memory access, e.g., MPU.
>
> As far as I can tell, these pools work with both static allocations (which
> seem to match your use-case where firmware has preconfigured the DMA ranges)
> but also with dynamic allocations where a 'size' property is present instead
> of the 'reg' property and the kernel is responsible for allocating the
> reservation during boot. Am I right and, if so, is that deliberate?
I believe so. I'm not keen on having size only reservations in DT.
Yes, we allowed that already, but that's back from the days of needing
large CMA carveouts to be reserved early in boot. I've read that the
kernel is much better now at contiguous allocations, so do we really
need this in DT anymore?
> I ask because I think that would potentially be useful to us for the
> Protected KVM work, where we need to bounce virtio memory accesses via
> guest-determined windows because the guest memory is generally inaccessible
> to the host. We've been hacking this using a combination of "swiotlb=force"
> and set_memory_{decrypted,encrypted}() but it would be much better to
> leverage the stuff you have here.
>
> Also:
>
> > +
> > + restricted_dma_mem_reserved: restricted_dma_mem_reserved {
> > + compatible = "restricted-dma-pool";
> > + reg = <0x5000 0x40>;
> > + };
> > };
> >
> > /* ... */
> > @@ -138,4 +157,9 @@ one for multimedia processing (named
> > multimedia-memory@7700, 64MiB).
> > memory-region = <&multimedia_reserved>;
> > /* ... */
> > };
> > +
> > + pcie_device: pcie_device@0,0 {
> > + memory-region = <&restricted_dma_mem_reserved>;
> > + /* ... */
> > + };
>
> I find this example a bit weird, as I didn't think we usually had DT nodes
> for PCI devices; rather they are discovered as a result of probing config
> space. Is the idea that you have one reserved memory region attached to the
> RC and all the PCI devices below that share the region, or is there a need
> for a mapping mechanism?
We can have DT nodes for PCI. AIUI, IBM power systems always do. For
FDT, it's only if there are extra non-discoverable resources. It's
particularly fun when it's resources which need to be enabled for the
PCI device to be discovered. That seems to be a growing problem as PCI
becomes more common on embedded systems.
Rob
Re: Question on PCIe Device Tree bindings, Was: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
On Thu, Feb 4, 2021 at 9:51 PM Elliott Mitchell wrote:
>
> On Thu, Feb 04, 2021 at 03:52:26PM -0600, Rob Herring wrote:
> > On Thu, Feb 4, 2021 at 3:33 PM Stefano Stabellini
> > wrote:
> > >
> > > On Thu, 4 Feb 2021, Rob Herring wrote:
> > > > On Thu, Feb 4, 2021 at 2:36 PM Stefano Stabellini
> > > > wrote:
> > > > >
> > > > > On Thu, 4 Feb 2021, Rob Herring wrote:
> > > > > > On Thu, Feb 4, 2021 at 11:56 AM Stefano Stabellini
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi Rob,
> > > > > > >
> > > > > > > We have a question on the PCIe device tree bindings. In summary,
> > > > > > > we have
> > > > > > > come across the Raspberry Pi 4 PCIe description below:
> > > > > > >
> > > > > > >
> > > > > > > pcie0: pcie@7d50 {
> > > > > > >compatible = "brcm,bcm2711-pcie";
> > > > > > >reg = <0x0 0x7d50 0x0 0x9310>;
> > > > > > >device_type = "pci";
> > > > > > >#address-cells = <3>;
> > > > > > >#interrupt-cells = <1>;
> > > > > > >#size-cells = <2>;
> > > > > > >interrupts = ,
> > > > > > > ;
> > > > > > >interrupt-names = "pcie", "msi";
> > > > > > >interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> > > > > > >interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143
> > > > > > >
> > > > > > > IRQ_TYPE_LEVEL_HIGH>;
> > > > > > >msi-controller;
> > > > > > >msi-parent = <&pcie0>;
> > > > > > >
> > > > > > >ranges = <0x0200 0x0 0xc000 0x6 0x
> > > > > > > 0x0 0x4000>;
> > > > > > >/*
> > > > > > > * The wrapper around the PCIe block has a bug
> > > > > > > * preventing it from accessing beyond the first 3GB of
> > > > > > > * memory.
> > > > > > > */
> > > > > > >dma-ranges = <0x0200 0x0 0x 0x0 0x
> > > > > > > 0x0 0xc000>;
> > > > > > >brcm,enable-ssc;
> > > > > > >
> > > > > > >pci@1,0 {
> > > > > > >#address-cells = <3>;
> > > > > > >#size-cells = <2>;
> > > > > > >ranges;
> > > > > > >
> > > > > > >reg = <0 0 0 0 0>;
> > > > > > >
> > > > > > >usb@1,0 {
> > > > > > >reg = <0x1 0 0 0 0>;
> > > > > > >resets = <&reset
> > > > > > > RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> > > > > > >};
> > > > > > >};
> > > > > > > };
> > > > > > >
> > > > > > >
> > > > > > > Xen fails to parse it with an error because it tries to remap reg
> > > > > > > =
> > > > > > > <0x1 0 0 0 0> as if it was a CPU address and of course it
> > > > > > > fails.
> > > > > > >
> > > > > > > Reading the device tree description in details, I cannot tell if
> > > > > > > Xen has
> > > > > > > a bug: the ranges property under pci@1,0 means that pci@1,0 is
> > > > > > > treated
> > > > > > > like a default bus (not a PCI bus), hence, the children regs are
> > > > > > > translated using the ranges property of the parent
> > > > > > > (pcie@7d50).
> > > > > > >
> > > > > > > Is it possible that the device tree is missing device_type =
> > > > > > > "pci" under pci@1,0? Or is it just implied because pci@1,0 is a
> > > > > > > child of
> > > > > > > pcie@7d50?
> > > > > >
> > > > > > Indeed, it should have device_type. Linux (only recently due to
> > > > > > another missing device_type case) will also look at node name, but
> > > > > > only 'pcie'.
> > > > > >
> > > > > > We should be able to create (or extend pci-bus.yaml) a schema to
> > > > > > catch
> > > > > > this case.
> > > > >
> > > > > Ah, that is what I needed to know, thank you! Is Linux considering a
> > > > > node named "pcie" as if it has device_type = "pci"?
> > > >
> > > > Yes, it was added for Rockchip RK3399 to avoid a DT update and
> > > > regression.
> > > >
> > > > > In Xen, also to cover the RPi4 case, maybe I could add a check for the
> > > > > node name to be "pci" or "pcie" and if so Xen could assume
> > > > > device_type =
> > > > > "pci".
> > > >
> > > > I assume this never worked for RPi4 (and Linux will have the same
> > > > issue), so can't we just update the DT in this case?
> > >
> > > I am not sure where the DT is coming from, probably from the RPi4 kernel
> > > trees or firmware. I think it would be good if somebody got in touch to
> > > tell them they have an issue.
> >
> > So you just take whatever downstream RPi invents? Good luck.
> >
> > > Elliot, where was that device tree coming from originally?
>
> Please excuse my very weak device-tree fu...
>
> I'm unsure which section is the problem, but looking at `git blame` what
> shows is commt d5c8dc0d4c880fbde5293cc186b1ab23466254c4.
>
> This commit is present in the Linux master branch and the linux-5.10.y
> branch.
>
> You were saying?
That commit looks perfectly fine. The problem is the PCI bridge node
shown above which doesn't exist upstream.
Rob
Re: Question on PCIe Device Tree bindings, Was: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
On Thu, Feb 4, 2021 at 3:33 PM Stefano Stabellini
wrote:
>
> On Thu, 4 Feb 2021, Rob Herring wrote:
> > On Thu, Feb 4, 2021 at 2:36 PM Stefano Stabellini
> > wrote:
> > >
> > > On Thu, 4 Feb 2021, Rob Herring wrote:
> > > > On Thu, Feb 4, 2021 at 11:56 AM Stefano Stabellini
> > > > wrote:
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > We have a question on the PCIe device tree bindings. In summary, we
> > > > > have
> > > > > come across the Raspberry Pi 4 PCIe description below:
> > > > >
> > > > >
> > > > > pcie0: pcie@7d50 {
> > > > >compatible = "brcm,bcm2711-pcie";
> > > > >reg = <0x0 0x7d50 0x0 0x9310>;
> > > > >device_type = "pci";
> > > > >#address-cells = <3>;
> > > > >#interrupt-cells = <1>;
> > > > >#size-cells = <2>;
> > > > >interrupts = ,
> > > > > ;
> > > > >interrupt-names = "pcie", "msi";
> > > > >interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> > > > >interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143
> > > > >
> > > > > IRQ_TYPE_LEVEL_HIGH>;
> > > > >msi-controller;
> > > > >msi-parent = <&pcie0>;
> > > > >
> > > > >ranges = <0x0200 0x0 0xc000 0x6 0x
> > > > > 0x0 0x4000>;
> > > > >/*
> > > > > * The wrapper around the PCIe block has a bug
> > > > > * preventing it from accessing beyond the first 3GB of
> > > > > * memory.
> > > > > */
> > > > >dma-ranges = <0x0200 0x0 0x 0x0 0x
> > > > > 0x0 0xc000>;
> > > > >brcm,enable-ssc;
> > > > >
> > > > >pci@1,0 {
> > > > >#address-cells = <3>;
> > > > >#size-cells = <2>;
> > > > >ranges;
> > > > >
> > > > >reg = <0 0 0 0 0>;
> > > > >
> > > > >usb@1,0 {
> > > > >reg = <0x1 0 0 0 0>;
> > > > >resets = <&reset
> > > > > RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> > > > >};
> > > > >};
> > > > > };
> > > > >
> > > > >
> > > > > Xen fails to parse it with an error because it tries to remap reg =
> > > > > <0x1 0 0 0 0> as if it was a CPU address and of course it fails.
> > > > >
> > > > > Reading the device tree description in details, I cannot tell if Xen
> > > > > has
> > > > > a bug: the ranges property under pci@1,0 means that pci@1,0 is treated
> > > > > like a default bus (not a PCI bus), hence, the children regs are
> > > > > translated using the ranges property of the parent (pcie@7d50).
> > > > >
> > > > > Is it possible that the device tree is missing device_type =
> > > > > "pci" under pci@1,0? Or is it just implied because pci@1,0 is a child
> > > > > of
> > > > > pcie@7d50?
> > > >
> > > > Indeed, it should have device_type. Linux (only recently due to
> > > > another missing device_type case) will also look at node name, but
> > > > only 'pcie'.
> > > >
> > > > We should be able to create (or extend pci-bus.yaml) a schema to catch
> > > > this case.
> > >
> > > Ah, that is what I needed to know, thank you! Is Linux considering a
> > > node named "pcie" as if it has device_type = "pci"?
> >
> > Yes, it was added for Rockchip RK3399 to avoid a DT update and regression.
> >
> > > In Xen, also to cover the RPi4 case, maybe I could add a check for the
> > > node name to be "pci" or "pcie" and if so Xen could assume device_type =
> > > "pci".
> >
> > I assume this never worked for RPi4 (and Linux will have the same
> > issue), so can't we just update the DT in this case?
>
> I am not sure where the DT is coming from, probably from the RPi4 kernel
> trees or firmware. I think it would be good if somebody got in touch to
> tell them they have an issue.
So you just take whatever downstream RPi invents? Good luck.
> Elliot, where was that device tree coming from originally?
>
>
> From a Xen perspective, for the sake of minimizing user pains (given
> that it might take a while to update those DTs) and to introduce as few
> ties as possible with kernel versions, it might be best to add the
> "pci" name workaround maybe with a /* HACK */ comment on top.
There is some possibility of that causing a regression on another
platform. That's why we limited Linux as much as possible and also
print a warning. But we have to worry about 20 year old PowerMacs and
such.
Rob
Re: Question on PCIe Device Tree bindings, Was: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
On Thu, Feb 4, 2021 at 2:36 PM Stefano Stabellini
wrote:
>
> On Thu, 4 Feb 2021, Rob Herring wrote:
> > On Thu, Feb 4, 2021 at 11:56 AM Stefano Stabellini
> > wrote:
> > >
> > > Hi Rob,
> > >
> > > We have a question on the PCIe device tree bindings. In summary, we have
> > > come across the Raspberry Pi 4 PCIe description below:
> > >
> > >
> > > pcie0: pcie@7d50 {
> > >compatible = "brcm,bcm2711-pcie";
> > >reg = <0x0 0x7d50 0x0 0x9310>;
> > >device_type = "pci";
> > >#address-cells = <3>;
> > >#interrupt-cells = <1>;
> > >#size-cells = <2>;
> > >interrupts = ,
> > > ;
> > >interrupt-names = "pcie", "msi";
> > >interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> > >interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143
> > > IRQ_TYPE_LEVEL_HIGH>;
> > >msi-controller;
> > >msi-parent = <&pcie0>;
> > >
> > >ranges = <0x0200 0x0 0xc000 0x6 0x
> > > 0x0 0x4000>;
> > >/*
> > > * The wrapper around the PCIe block has a bug
> > > * preventing it from accessing beyond the first 3GB of
> > > * memory.
> > > */
> > >dma-ranges = <0x0200 0x0 0x 0x0 0x
> > > 0x0 0xc000>;
> > >brcm,enable-ssc;
> > >
> > >pci@1,0 {
> > >#address-cells = <3>;
> > >#size-cells = <2>;
> > >ranges;
> > >
> > >reg = <0 0 0 0 0>;
> > >
> > >usb@1,0 {
> > >reg = <0x1 0 0 0 0>;
> > >resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> > >};
> > >};
> > > };
> > >
> > >
> > > Xen fails to parse it with an error because it tries to remap reg =
> > > <0x1 0 0 0 0> as if it was a CPU address and of course it fails.
> > >
> > > Reading the device tree description in details, I cannot tell if Xen has
> > > a bug: the ranges property under pci@1,0 means that pci@1,0 is treated
> > > like a default bus (not a PCI bus), hence, the children regs are
> > > translated using the ranges property of the parent (pcie@7d50).
> > >
> > > Is it possible that the device tree is missing device_type =
> > > "pci" under pci@1,0? Or is it just implied because pci@1,0 is a child of
> > > pcie@7d50?
> >
> > Indeed, it should have device_type. Linux (only recently due to
> > another missing device_type case) will also look at node name, but
> > only 'pcie'.
> >
> > We should be able to create (or extend pci-bus.yaml) a schema to catch
> > this case.
>
> Ah, that is what I needed to know, thank you! Is Linux considering a
> node named "pcie" as if it has device_type = "pci"?
Yes, it was added for Rockchip RK3399 to avoid a DT update and regression.
> In Xen, also to cover the RPi4 case, maybe I could add a check for the
> node name to be "pci" or "pcie" and if so Xen could assume device_type =
> "pci".
I assume this never worked for RPi4 (and Linux will have the same
issue), so can't we just update the DT in this case?
Rob
Re: Question on PCIe Device Tree bindings, Was: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
On Thu, Feb 4, 2021 at 11:56 AM Stefano Stabellini
wrote:
>
> Hi Rob,
>
> We have a question on the PCIe device tree bindings. In summary, we have
> come across the Raspberry Pi 4 PCIe description below:
>
>
> pcie0: pcie@7d50 {
>compatible = "brcm,bcm2711-pcie";
>reg = <0x0 0x7d50 0x0 0x9310>;
>device_type = "pci";
>#address-cells = <3>;
>#interrupt-cells = <1>;
>#size-cells = <2>;
>interrupts = ,
> ;
>interrupt-names = "pcie", "msi";
>interrupt-map-mask = <0x0 0x0 0x0 0x7>;
>interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143
> IRQ_TYPE_LEVEL_HIGH>;
>msi-controller;
>msi-parent = <&pcie0>;
>
>ranges = <0x0200 0x0 0xc000 0x6 0x
> 0x0 0x4000>;
>/*
> * The wrapper around the PCIe block has a bug
> * preventing it from accessing beyond the first 3GB of
> * memory.
> */
>dma-ranges = <0x0200 0x0 0x 0x0 0x
> 0x0 0xc000>;
>brcm,enable-ssc;
>
>pci@1,0 {
>#address-cells = <3>;
>#size-cells = <2>;
>ranges;
>
>reg = <0 0 0 0 0>;
>
>usb@1,0 {
>reg = <0x1 0 0 0 0>;
>resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
>};
>};
> };
>
>
> Xen fails to parse it with an error because it tries to remap reg =
> <0x1 0 0 0 0> as if it was a CPU address and of course it fails.
>
> Reading the device tree description in details, I cannot tell if Xen has
> a bug: the ranges property under pci@1,0 means that pci@1,0 is treated
> like a default bus (not a PCI bus), hence, the children regs are
> translated using the ranges property of the parent (pcie@7d50).
>
> Is it possible that the device tree is missing device_type =
> "pci" under pci@1,0? Or is it just implied because pci@1,0 is a child of
> pcie@7d50?
Indeed, it should have device_type. Linux (only recently due to
another missing device_type case) will also look at node name, but
only 'pcie'.
We should be able to create (or extend pci-bus.yaml) a schema to catch
this case.
Rob
> I'd like to make Xen able to parse this device tree without errors but I
> am not sure what is the best way to fix it.
>
> Thanks for any help you can provide!
>
> Cheers,
>
> Stefano
>
>
>
> On Thu, 4 Feb 2021, Julien Grall wrote:
> > On 04/02/2021 00:13, Stefano Stabellini wrote:
> > > On Wed, 3 Feb 2021, Julien Grall wrote:
> > > > On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini
> > > > wrote:
> > > > > > > But aside from PCIe, let's say that we know of a few nodes for
> > > > > > > which
> > > > > > > "reg" needs a special treatment. I am not sure it makes sense to
> > > > > > > proceed
> > > > > > > with parsing those nodes without knowing how to deal with that.
> > > > > >
> > > > > > I believe that most of the time the "special" treatment would be to
> > > > > > ignore the
> > > > > > property "regs" as it will not be an CPU memory address.
> > > > > >
> > > > > > > So maybe
> > > > > > > we should add those nodes to skip_matches until we know what to do
> > > > > > > with
> > > > > > > them. At that point, I would imagine we would introduce a special
> > > > > > > handle_device function that knows what to do. In the case of PCIe,
> > > > > > > something like "handle_device_pcie".
> > > > > > Could you outline how "handle_device_pcie()" will differ with
> > > > > > handle_node()?
> > > > > >
> > > > > > In fact, the problem is not the PCIe node directly. Instead, it is
> > > > > > the
> > > > > > second
> > > > > > level of nodes below it (i.e usb@...).
> > > > > >
> > > > > > The current implementation of dt_number_of_address() only look at
> > > > > > the
> > > > > > bus type
> > > > > > of the parent. As the parent has no bus type and "ranges" then it
> > > > > > thinks this
> > > > > > is something we can translate to a CPU address.
> > > > > >
> > > > > > However, this is below a PCI bus so the meaning of "reg" is
> > > > > > completely
> > > > > > different. In this case, we only need to ignore "reg".
> > > > >
> > > > > I see what you are saying and I agree: if we had to introduce a
> > > > > special
> > > > > case for PCI, then dt_number_of_address() seems to be a good place.
> > > > > In
> > > > > fact, we already have special PCI handling, see our
> > > > > __dt_translate_address function and
> > > > > xen/common/device_tree.c:dt_busses.
> > > > >
> > > > > Which brings the question: why is this actually failing?
> > > >
> > > > I already hinted at the reason in my previous e-mail :). Let me expand
> > > > a bit more.
> > > >
> > > > >
> > > > > pcie0 {
> > > > > ranges = <0x0200 0x0 0xc000 0x6 0x 0x0
> > > > > 0x4000>;
> > > > >
> > > > > Which means that PCI addresses 0xc000-0x1 become
> > > > > 0x6-0x7.
> > > > >
> > > > > The offending DT is:
> > > > >
> >
Re: [RFC PATCH v3 5/6] dt-bindings: of: Add restricted DMA pool
On Wed, Jan 20, 2021 at 7:10 PM Robin Murphy wrote:
>
> On 2021-01-20 21:31, Rob Herring wrote:
> > On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy wrote:
> >>
> >> On 2021-01-20 16:53, Rob Herring wrote:
> >>> On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
> >>>> Introduce the new compatible string, restricted-dma-pool, for restricted
> >>>> DMA. One can specify the address and length of the restricted DMA memory
> >>>> region by restricted-dma-pool in the device tree.
> >>>
> >>> If this goes into DT, I think we should be able to use dma-ranges for
> >>> this purpose instead. Normally, 'dma-ranges' is for physical bus
> >>> restrictions, but there's no reason it can't be used for policy or to
> >>> express restrictions the firmware has enabled.
> >>
> >> There would still need to be some way to tell SWIOTLB to pick up the
> >> corresponding chunk of memory and to prevent the kernel from using it
> >> for anything else, though.
> >
> > Don't we already have that problem if dma-ranges had a very small
> > range? We just get lucky because the restriction is generally much
> > more RAM than needed.
>
> Not really - if a device has a naturally tiny addressing capability that
> doesn't even cover ZONE_DMA32 where the regular SWIOTLB buffer will be
> allocated then it's unlikely to work well, but that's just crap system
> design. Yes, memory pressure in ZONE_DMA{32} is particularly problematic
> for such limited devices, but it's irrelevant to the issue at hand here.
Yesterday's crap system design is today's security feature. Couldn't
this feature make crap system design work better?
> What we have here is a device that's not allowed to see *kernel* memory
> at all. It's been artificially constrained to a particular region by a
> TZASC or similar, and the only data which should ever be placed in that
May have been constrained, but that's entirely optional.
In the optional case where the setup is entirely up to the OS, I don't
think this belongs in the DT at all. Perhaps that should be solved
first.
> region is data intended for that device to see. That way if it tries to
> go rogue it physically can't start slurping data intended for other
> devices or not mapped for DMA at all. The bouncing is an important part
> of this - I forget the title off-hand but there was an interesting paper
> a few years ago which demonstrated that even with an IOMMU, streaming
> DMA of in-place buffers could reveal enough adjacent data from the same
> page to mount an attack on the system. Memory pressure should be
> immaterial since the size of each bounce pool carveout will presumably
> be tuned for the needs of the given device.
>
> > In any case, wouldn't finding all the dma-ranges do this? We're
> > already walking the tree to find the max DMA address now.
>
> If all you can see are two "dma-ranges" properties, how do you propose
> to tell that one means "this is the extent of what I can address, please
> set my masks and dma-range-map accordingly and try to allocate things
> where I can reach them" while the other means "take this output range
> away from the page allocator and hook it up as my dedicated bounce pool,
> because it is Serious Security Time"? Especially since getting that
> choice wrong either way would be a Bad Thing.
Either we have some heuristic based on the size or we add some hint.
The point is let's build on what we already have for defining DMA
accessible memory in DT rather than some parallel mechanism.
Rob
Re: [RFC PATCH v3 5/6] dt-bindings: of: Add restricted DMA pool
On Wed, Jan 20, 2021 at 11:30 AM Robin Murphy wrote:
>
> On 2021-01-20 16:53, Rob Herring wrote:
> > On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
> >> Introduce the new compatible string, restricted-dma-pool, for restricted
> >> DMA. One can specify the address and length of the restricted DMA memory
> >> region by restricted-dma-pool in the device tree.
> >
> > If this goes into DT, I think we should be able to use dma-ranges for
> > this purpose instead. Normally, 'dma-ranges' is for physical bus
> > restrictions, but there's no reason it can't be used for policy or to
> > express restrictions the firmware has enabled.
>
> There would still need to be some way to tell SWIOTLB to pick up the
> corresponding chunk of memory and to prevent the kernel from using it
> for anything else, though.
Don't we already have that problem if dma-ranges had a very small
range? We just get lucky because the restriction is generally much
more RAM than needed.
In any case, wouldn't finding all the dma-ranges do this? We're
already walking the tree to find the max DMA address now.
> >> Signed-off-by: Claire Chang
> >> ---
> >> .../reserved-memory/reserved-memory.txt | 24 +++
> >> 1 file changed, 24 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >> index e8d3096d922c..44975e2a1fd2 100644
> >> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> >> @@ -51,6 +51,20 @@ compatible (optional) - standard definition
> >> used as a shared pool of DMA buffers for a set of devices. It
> >> can
> >> be used by an operating system to instantiate the necessary
> >> pool
> >> management subsystem if necessary.
> >> +- restricted-dma-pool: This indicates a region of memory meant to
> >> be
> >> + used as a pool of restricted DMA buffers for a set of devices.
> >> The
> >> + memory region would be the only region accessible to those
> >> devices.
> >> + When using this, the no-map and reusable properties must not be
> >> set,
> >> + so the operating system can create a virtual mapping that will
> >> be used
> >> + for synchronization. The main purpose for restricted DMA is to
> >> + mitigate the lack of DMA access control on systems without an
> >> IOMMU,
> >> + which could result in the DMA accessing the system memory at
> >> + unexpected times and/or unexpected addresses, possibly leading
> >> to data
> >> + leakage or corruption. The feature on its own provides a basic
> >> level
> >> + of protection against the DMA overwriting buffer contents at
> >> + unexpected times. However, to protect against general data
> >> leakage and
> >> + system memory corruption, the system needs to provide way to
> >> restrict
> >> + the DMA to a predefined memory region.
> >> - vendor specific string in the form ,[-]
> >> no-map (optional) - empty property
> >> - Indicates the operating system must not create a virtual mapping
> >> @@ -120,6 +134,11 @@ one for multimedia processing (named
> >> multimedia-memory@7700, 64MiB).
> >> compatible = "acme,multimedia-memory";
> >> reg = <0x7700 0x400>;
> >> };
> >> +
> >> +restricted_dma_mem_reserved: restricted_dma_mem_reserved {
> >> +compatible = "restricted-dma-pool";
> >> +reg = <0x5000 0x40>;
> >> +};
> >> };
> >>
> >> /* ... */
> >> @@ -138,4 +157,9 @@ one for multimedia processing (named
> >> multimedia-memory@7700, 64MiB).
> >> memory-region = <&multimedia_reserved>;
> >> /* ... */
> >> };
> >> +
> >> +pcie_device: pcie_device@0,0 {
> >> +memory-region = <&restricted_dma_mem_reserved>;
> >
> > PCI hosts often have inbound window configurations that limit the
> > addre
Re: [RFC PATCH v3 5/6] dt-bindings: of: Add restricted DMA pool
On Wed, Jan 06, 2021 at 11:41:23AM +0800, Claire Chang wrote:
> Introduce the new compatible string, restricted-dma-pool, for restricted
> DMA. One can specify the address and length of the restricted DMA memory
> region by restricted-dma-pool in the device tree.
If this goes into DT, I think we should be able to use dma-ranges for
this purpose instead. Normally, 'dma-ranges' is for physical bus
restrictions, but there's no reason it can't be used for policy or to
express restrictions the firmware has enabled.
> Signed-off-by: Claire Chang
> ---
> .../reserved-memory/reserved-memory.txt | 24 +++
> 1 file changed, 24 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> index e8d3096d922c..44975e2a1fd2 100644
> --- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> @@ -51,6 +51,20 @@ compatible (optional) - standard definition
>used as a shared pool of DMA buffers for a set of devices. It can
>be used by an operating system to instantiate the necessary pool
>management subsystem if necessary.
> +- restricted-dma-pool: This indicates a region of memory meant to be
> + used as a pool of restricted DMA buffers for a set of devices. The
> + memory region would be the only region accessible to those devices.
> + When using this, the no-map and reusable properties must not be
> set,
> + so the operating system can create a virtual mapping that will be
> used
> + for synchronization. The main purpose for restricted DMA is to
> + mitigate the lack of DMA access control on systems without an
> IOMMU,
> + which could result in the DMA accessing the system memory at
> + unexpected times and/or unexpected addresses, possibly leading to
> data
> + leakage or corruption. The feature on its own provides a basic
> level
> + of protection against the DMA overwriting buffer contents at
> + unexpected times. However, to protect against general data leakage
> and
> + system memory corruption, the system needs to provide way to
> restrict
> + the DMA to a predefined memory region.
> - vendor specific string in the form ,[-]
> no-map (optional) - empty property
> - Indicates the operating system must not create a virtual mapping
> @@ -120,6 +134,11 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> compatible = "acme,multimedia-memory";
> reg = <0x7700 0x400>;
> };
> +
> + restricted_dma_mem_reserved: restricted_dma_mem_reserved {
> + compatible = "restricted-dma-pool";
> + reg = <0x5000 0x40>;
> + };
> };
>
> /* ... */
> @@ -138,4 +157,9 @@ one for multimedia processing (named
> multimedia-memory@7700, 64MiB).
> memory-region = <&multimedia_reserved>;
> /* ... */
> };
> +
> + pcie_device: pcie_device@0,0 {
> + memory-region = <&restricted_dma_mem_reserved>;
PCI hosts often have inbound window configurations that limit the
address range and translate PCI to bus addresses. Those windows happen
to be configured by dma-ranges. In any case, wouldn't you want to put
the configuration in the PCI host node? Is there a usecase of
restricting one PCIe device and not another?
Rob
Re: [patch 19/30] PCI: mobiveil: Use irq_data_get_irq_chip_data()
On Thu, Dec 10, 2020 at 1:42 PM Thomas Gleixner wrote: > > Going through a full irq descriptor lookup instead of just using the proper > helper function which provides direct access is suboptimal. > > In fact it _is_ wrong because the chip callback needs to get the chip data > which is relevant for the chip while using the irq descriptor variant > returns the irq chip data of the top level chip of a hierarchy. It does not > matter in this case because the chip is the top level chip, but that > doesn't make it more correct. > > Signed-off-by: Thomas Gleixner > Cc: Karthikeyan Mitran > Cc: Hou Zhiqiang > Cc: Lorenzo Pieralisi > Cc: Rob Herring > Cc: Bjorn Helgaas > Cc: linux-...@vger.kernel.org > --- > drivers/pci/controller/mobiveil/pcie-mobiveil-host.c |8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Rob Herring
Re: [patch 18/30] PCI: xilinx-nwl: Use irq_data_get_irq_chip_data()
On Thu, Dec 10, 2020 at 1:42 PM Thomas Gleixner wrote: > > Going through a full irq descriptor lookup instead of just using the proper > helper function which provides direct access is suboptimal. > > In fact it _is_ wrong because the chip callback needs to get the chip data > which is relevant for the chip while using the irq descriptor variant > returns the irq chip data of the top level chip of a hierarchy. It does not > matter in this case because the chip is the top level chip, but that > doesn't make it more correct. > > Signed-off-by: Thomas Gleixner > Cc: Lorenzo Pieralisi > Cc: Rob Herring > Cc: Bjorn Helgaas > Cc: Michal Simek > Cc: linux-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > --- > drivers/pci/controller/pcie-xilinx-nwl.c |8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Rob Herring
Re: RFC: PCI devices passthrough on Arm design proposal
On Mon, Jul 20, 2020 at 5:24 PM Stefano Stabellini wrote: > > + Rob Herring > > On Fri, 17 Jul 2020, Bertrand Marquis wrote: > > >> Regarding the DT entry, this is not coming from us and this is already > > >> defined this way in existing DTBs, we just reuse the existing entry. > > > > > > Is it possible to standardize the property and drop the linux prefix? > > > > Honestly i do not know. This was there in the DT examples we checked so > > we planned to use that. But it might be possible to standardize this. > > We could certainly start a discussion about it. It looks like > linux,pci-domain is used beyond purely the Linux kernel. I think that it > is worth getting Rob's advice on this. > > > Rob, for context we are trying to get Linux and Xen to agree on a > numbering scheme to identify PCI host bridges correctly. We already have > an existing hypercall from the old x86 days that passes a segment number > to Xen as a parameter, see drivers/xen/pci.c:xen_add_device. > (xen_add_device assumes that a Linux domain and a PCI segment are the > same thing which I understand is not the case.) > > > There is an existing device tree property called "linux,pci-domain" > which would solve the problem (ignoring the difference in the definition > of domain and segment) but it is clearly marked as a Linux-specific > property. Is there anything more "standard" that we can use? > > I can find PCI domains being mentioned a few times in the Device Tree > PCI specification but can't find any associated IDs, and I couldn't find > segments at all. > > What's your take on this? In general, what's your suggestion on getting > Xen and Linux (and other OSes which could be used as dom0 one day like > Zephyr) to agree on a simple numbering scheme to identify PCI host > bridges? > > Should we just use "linux,pci-domain" as-is because it is already the de > facto standard? It looks like the property appears in both QEMU and > UBoot already. Sounds good to me. We could drop the 'linux' part, but based on other places that has happened it just means we end up supporting both strings forever. Rob
Re: [Xen-devel] [PATCH v2] xen: Stop abusing DT of_dma_configure API
On Thu, Oct 10, 2019 at 9:00 AM Boris Ostrovsky
wrote:
>
> On 10/9/19 7:42 AM, Oleksandr Andrushchenko wrote:
> > On 10/8/19 10:41 PM, Rob Herring wrote:
> >> As the removed comments say, these aren't DT based devices.
> >> of_dma_configure() is going to stop allowing a NULL DT node and calling
> >> it will no longer work.
> >>
> >> The comment is also now out of date as of commit 9ab91e7c5c51 ("arm64:
> >> default to the direct mapping in get_arch_dma_ops"). Direct mapping
> >> is now the default rather than dma_dummy_ops.
> >>
> >> According to Stefano and Oleksandr, the only other part needed is
> >> setting the DMA masks and there's no reason to restrict the masks to
> >> 32-bits. So set the masks to 64 bits.
> >>
> >> Cc: Robin Murphy
> >> Cc: Julien Grall
> >> Cc: Nicolas Saenz Julienne
> >> Cc: Oleksandr Andrushchenko
> >> Cc: Boris Ostrovsky
> >> Cc: Juergen Gross
> >> Cc: Stefano Stabellini
> >> Cc: Christoph Hellwig
> >> Cc: xen-devel@lists.xenproject.org
> >> Signed-off-by: Rob Herring
> > Acked-by: Oleksandr Andrushchenko
>
>
> Is this going to go via drm tree or should I pick it up for Xen tree?
Please apply to the Xen tree.
Rob
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] xen: Stop abusing DT of_dma_configure API
As the removed comments say, these aren't DT based devices.
of_dma_configure() is going to stop allowing a NULL DT node and calling
it will no longer work.
The comment is also now out of date as of commit 9ab91e7c5c51 ("arm64:
default to the direct mapping in get_arch_dma_ops"). Direct mapping
is now the default rather than dma_dummy_ops.
According to Stefano and Oleksandr, the only other part needed is
setting the DMA masks and there's no reason to restrict the masks to
32-bits. So set the masks to 64 bits.
Cc: Robin Murphy
Cc: Julien Grall
Cc: Nicolas Saenz Julienne
Cc: Oleksandr Andrushchenko
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: Christoph Hellwig
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Rob Herring
---
v2:
- Setup dma masks
- Also fix xen_drm_front.c
This can now be applied to the Xen tree independent of the coming
of_dma_configure() changes.
Rob
drivers/gpu/drm/xen/xen_drm_front.c | 12 ++--
drivers/xen/gntdev.c| 13 ++---
2 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/xen/xen_drm_front.c
b/drivers/gpu/drm/xen/xen_drm_front.c
index ba1828acd8c9..4be49c1aef51 100644
--- a/drivers/gpu/drm/xen/xen_drm_front.c
+++ b/drivers/gpu/drm/xen/xen_drm_front.c
@@ -718,17 +718,9 @@ static int xen_drv_probe(struct xenbus_device *xb_dev,
struct device *dev = &xb_dev->dev;
int ret;
- /*
-* The device is not spawn from a device tree, so arch_setup_dma_ops
-* is not called, thus leaving the device with dummy DMA ops.
-* This makes the device return error on PRIME buffer import, which
-* is not correct: to fix this call of_dma_configure() with a NULL
-* node to set default DMA ops.
-*/
- dev->coherent_dma_mask = DMA_BIT_MASK(32);
- ret = of_dma_configure(dev, NULL, true);
+ ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
if (ret < 0) {
- DRM_ERROR("Cannot setup DMA ops, ret %d", ret);
+ DRM_ERROR("Cannot setup DMA mask, ret %d", ret);
return ret;
}
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index a446a7221e13..81401f386c9c 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -22,6 +22,7 @@
#define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
+#include
#include
#include
#include
@@ -34,9 +35,6 @@
#include
#include
#include
-#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
-#include
-#endif
#include
#include
@@ -625,14 +623,7 @@ static int gntdev_open(struct inode *inode, struct file
*flip)
flip->private_data = priv;
#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
priv->dma_dev = gntdev_miscdev.this_device;
-
- /*
-* The device is not spawn from a device tree, so arch_setup_dma_ops
-* is not called, thus leaving the device with dummy DMA ops.
-* Fix this by calling of_dma_configure() with a NULL node to set
-* default DMA ops.
-*/
- of_dma_configure(priv->dma_dev, NULL, true);
+ dma_coerce_mask_and_coherent(priv->dma_dev, DMA_BIT_MASK(64));
#endif
pr_debug("priv %p\n", priv);
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API
On Thu, Sep 26, 2019 at 6:16 AM Oleksandr Andrushchenko wrote: > > On 9/26/19 1:46 PM, Robin Murphy wrote: > > On 2019-09-26 11:17 am, Oleksandr Andrushchenko wrote: > >> > >> On 9/26/19 12:49 PM, Julien Grall wrote: > >>> Hi Rob, > >>> > >>> > >>> On 9/25/19 10:50 PM, Rob Herring wrote: > >>>> As the comment says, this isn't a DT based device. of_dma_configure() > >>>> is going to stop allowing a NULL DT node, so this needs to be fixed. > >>> > >>> And this can't work on arch not selecting CONFIG_OF and can select > >>> CONFIG_XEN_GRANT_DMA_ALLOC. > >>> > >>> We are lucky enough on x86 because, AFAICT, arch_setup_dma_ops is just > >>> a nop. > >>> > >> No luck is needed as [1] does nothing for those platforms not using > >> CONFIG_OF > >>>> > >>>> Not sure exactly what setup besides arch_setup_dma_ops is needed... > >>> > >>> We probably want to update dma_mask, coherent_dma_mask and > >>> dma_pfn_offset. > >>> > >>> Also, while look at of_configure_dma, I noticed that we consider the > >>> DMA will not be coherent for the grant-table. Oleksandr, do you know > >>> why they can't be coherent? > >> The main and the only reason to use of_configure_dma is that if we don't > >> then we > >> are about to stay with dma_dummy_ops [2]. It effectively means that > >> operations on dma-bufs > >> will end up returning errors, like [3], [4], thus not making it possible > >> for Xen PV DRM and DMA > >> part of gntdev driver to do what we need (dma-bufs in our use-cases > >> allow zero-copying > >> while using graphics buffers and many more). > >> > >> I didn't find any better way of achieving that, but of_configure_dma... > >> If there is any better solution which will not break the existing > >> functionality then > >> I will definitely change the drivers so we do not abuse DT ) > >> Before that, please keep in mind that merging this RFC will break Xen PV > >> DRM + > >> DMA buf support in gntdev... > >> Hope we can work out some acceptable solution, so everyone is happy > > > > As I mentioned elsewhere, the recent dma-direct rework means that > > dma_dummy_ops are now only explicitly installed for the ACPI error > > case, so - much as I may dislike it - you should get regular > > (direct/SWIOTLB) ops by default again. > Ah, my bad, I missed that change. So, if no dummy dma ops are to be used > then > I believe we can apply both changes, e.g. remove of_dma_configure from > both of the drivers. What about the dma masks? I think there's a default setup, but it is considered a driver bug to not set its mask. xen_drm_front sets the coherent_dma_mask (why only 32-bits though?), but not the dma_mask. gntdev is doing neither. I could copy out what of_dma_configure does but better for the Xen folks to decide what is needed or not and test the change. I'm not setup to test any of this. Rob ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [RFC PATCH] xen/gntdev: Stop abusing DT of_dma_configure API
As the comment says, this isn't a DT based device. of_dma_configure()
is going to stop allowing a NULL DT node, so this needs to be fixed.
Not sure exactly what setup besides arch_setup_dma_ops is needed...
Cc: Robin Murphy
Cc: Julien Grall
Cc: Nicolas Saenz Julienne
Cc: Oleksandr Andrushchenko
Cc: Boris Ostrovsky
Cc: Juergen Gross
Cc: Stefano Stabellini
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Rob Herring
---
drivers/xen/gntdev.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index a446a7221e13..59906f9a40e4 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -34,9 +34,6 @@
#include
#include
#include
-#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
-#include
-#endif
#include
#include
@@ -625,14 +622,6 @@ static int gntdev_open(struct inode *inode, struct file
*flip)
flip->private_data = priv;
#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
priv->dma_dev = gntdev_miscdev.this_device;
-
- /*
-* The device is not spawn from a device tree, so arch_setup_dma_ops
-* is not called, thus leaving the device with dummy DMA ops.
-* Fix this by calling of_dma_configure() with a NULL node to set
-* default DMA ops.
-*/
- of_dma_configure(priv->dma_dev, NULL, true);
#endif
pr_debug("priv %p\n", priv);
--
2.20.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 00/11] of: Fix DMA configuration for non-DT masters
On Wed, Sep 25, 2019 at 11:52 AM Robin Murphy wrote: > > On 25/09/2019 17:16, Rob Herring wrote: > > On Wed, Sep 25, 2019 at 10:30 AM Nicolas Saenz Julienne > > wrote: > >> > >> On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote: > >>> On 25/09/2019 15:52, Nicolas Saenz Julienne wrote: > >>>> On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote: > >>>>> On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne > >>>>> wrote: > >>>>>> Hi All, > >>>>>> this series tries to address one of the issues blocking us from > >>>>>> upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that > >>>>>> devices not represented in DT which sit behind a PCI bus fail to get > >>>>>> the > >>>>>> bus' DMA addressing constraints. > >>>>>> > >>>>>> This is due to the fact that of_dma_configure() assumes it's receiving > >>>>>> a > >>>>>> DT node representing the device being configured, as opposed to the > >>>>>> PCIe > >>>>>> bridge node we currently pass. This causes the code to directly jump > >>>>>> into PCI's parent node when checking for 'dma-ranges' and misses > >>>>>> whatever was set there. > >>>>>> > >>>>>> To address this I create a new API in OF - inspired from Robin Murphys > >>>>>> original proposal[2] - which accepts a bus DT node as it's input in > >>>>>> order to configure a device's DMA constraints. The changes go deep into > >>>>>> of/address.c's implementation, as a device being having a DT node > >>>>>> assumption was pretty strong. > >>>>>> > >>>>>> On top of this work, I also cleaned up of_dma_configure() removing its > >>>>>> redundant arguments and creating an alternative function for the > >>>>>> special > >>>>>> cases > >>>>>> not applicable to either the above case or the default usage. > >>>>>> > >>>>>> IMO the resulting functions are more explicit. They will probably > >>>>>> surface some hacky usages that can be properly fixed as I show with the > >>>>>> DT fixes on the Layerscape platform. > >>>>>> > >>>>>> This was also tested on a Raspberry Pi 4 with a custom PCIe driver and > >>>>>> on a Seattle AMD board. > >>>>> > >>>>> Humm, I've been working on this issue too. Looks similar though yours > >>>>> has a lot more churn and there's some other bugs I've found. > >>>> > >>>> That's good news, and yes now that I see it, some stuff on my series is > >>>> overly > >>>> complicated. Specially around of_translate_*(). > >>>> > >>>> On top of that, you removed in of_dma_get_range(): > >>>> > >>>> - /* > >>>> -* At least empty ranges has to be defined for parent node if > >>>> -* DMA is supported > >>>> -*/ > >>>> - if (!ranges) > >>>> - break; > >>>> > >>>> Which I assumed was bound to the standard and makes things easier. > >>>> > >>>>> Can you test out this branch[1]. I don't have any h/w needing this, > >>>>> but wrote a unittest and tested with modified QEMU. > >>>> > >>>> I reviewed everything, I did find a minor issue, see the patch attached. > >>> > >>> WRT that patch, the original intent of "force_dma" was purely to > >>> consider a device DMA-capable regardless of the presence of > >>> "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF > >>> device has always been bogus - magic paravirt devices which appear out > >>> of nowhere and expect to be treated as genuine DMA masters are a > >>> separate problem that we haven't really approached yet. > >> > >> I agree it's clearly abusing the function. I have no problem with the > >> behaviour > >> change if it's OK with you. > > Thinking about it, you could probably just remove that call from the Xen > DRM driver now anyway - s
Re: [Xen-devel] [PATCH 00/11] of: Fix DMA configuration for non-DT masters
On Wed, Sep 25, 2019 at 10:30 AM Nicolas Saenz Julienne wrote: > > On Wed, 2019-09-25 at 16:09 +0100, Robin Murphy wrote: > > On 25/09/2019 15:52, Nicolas Saenz Julienne wrote: > > > On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote: > > > > On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne > > > > wrote: > > > > > Hi All, > > > > > this series tries to address one of the issues blocking us from > > > > > upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that > > > > > devices not represented in DT which sit behind a PCI bus fail to get > > > > > the > > > > > bus' DMA addressing constraints. > > > > > > > > > > This is due to the fact that of_dma_configure() assumes it's > > > > > receiving a > > > > > DT node representing the device being configured, as opposed to the > > > > > PCIe > > > > > bridge node we currently pass. This causes the code to directly jump > > > > > into PCI's parent node when checking for 'dma-ranges' and misses > > > > > whatever was set there. > > > > > > > > > > To address this I create a new API in OF - inspired from Robin Murphys > > > > > original proposal[2] - which accepts a bus DT node as it's input in > > > > > order to configure a device's DMA constraints. The changes go deep > > > > > into > > > > > of/address.c's implementation, as a device being having a DT node > > > > > assumption was pretty strong. > > > > > > > > > > On top of this work, I also cleaned up of_dma_configure() removing its > > > > > redundant arguments and creating an alternative function for the > > > > > special > > > > > cases > > > > > not applicable to either the above case or the default usage. > > > > > > > > > > IMO the resulting functions are more explicit. They will probably > > > > > surface some hacky usages that can be properly fixed as I show with > > > > > the > > > > > DT fixes on the Layerscape platform. > > > > > > > > > > This was also tested on a Raspberry Pi 4 with a custom PCIe driver and > > > > > on a Seattle AMD board. > > > > > > > > Humm, I've been working on this issue too. Looks similar though yours > > > > has a lot more churn and there's some other bugs I've found. > > > > > > That's good news, and yes now that I see it, some stuff on my series is > > > overly > > > complicated. Specially around of_translate_*(). > > > > > > On top of that, you removed in of_dma_get_range(): > > > > > > - /* > > > -* At least empty ranges has to be defined for parent node if > > > -* DMA is supported > > > -*/ > > > - if (!ranges) > > > - break; > > > > > > Which I assumed was bound to the standard and makes things easier. > > > > > > > Can you test out this branch[1]. I don't have any h/w needing this, > > > > but wrote a unittest and tested with modified QEMU. > > > > > > I reviewed everything, I did find a minor issue, see the patch attached. > > > > WRT that patch, the original intent of "force_dma" was purely to > > consider a device DMA-capable regardless of the presence of > > "dma-ranges". Expecting of_dma_configure() to do anything for a non-OF > > device has always been bogus - magic paravirt devices which appear out > > of nowhere and expect to be treated as genuine DMA masters are a > > separate problem that we haven't really approached yet. > > I agree it's clearly abusing the function. I have no problem with the > behaviour > change if it's OK with you. > > Robin, have you looked into supporting multiple dma-ranges? It's the next > thing > we need for BCM STB's PCIe. I'll have a go at it myself if nothing is in the > works already. Multiple dma-ranges as far as configuring inbound windows should work already other than the bug when there's any parent translation. But if you mean supporting multiple DMA offsets and masks per device in the DMA API, there's nothing in the works yet. Rob ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 00/11] of: Fix DMA configuration for non-DT masters
On Wed, Sep 25, 2019 at 9:53 AM Nicolas Saenz Julienne wrote: > > On Tue, 2019-09-24 at 16:59 -0500, Rob Herring wrote: > > On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne > > wrote: > > > Hi All, > > > this series tries to address one of the issues blocking us from > > > upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that > > > devices not represented in DT which sit behind a PCI bus fail to get the > > > bus' DMA addressing constraints. > > > > > > This is due to the fact that of_dma_configure() assumes it's receiving a > > > DT node representing the device being configured, as opposed to the PCIe > > > bridge node we currently pass. This causes the code to directly jump > > > into PCI's parent node when checking for 'dma-ranges' and misses > > > whatever was set there. > > > > > > To address this I create a new API in OF - inspired from Robin Murphys > > > original proposal[2] - which accepts a bus DT node as it's input in > > > order to configure a device's DMA constraints. The changes go deep into > > > of/address.c's implementation, as a device being having a DT node > > > assumption was pretty strong. > > > > > > On top of this work, I also cleaned up of_dma_configure() removing its > > > redundant arguments and creating an alternative function for the special > > > cases > > > not applicable to either the above case or the default usage. > > > > > > IMO the resulting functions are more explicit. They will probably > > > surface some hacky usages that can be properly fixed as I show with the > > > DT fixes on the Layerscape platform. > > > > > > This was also tested on a Raspberry Pi 4 with a custom PCIe driver and > > > on a Seattle AMD board. > > > > Humm, I've been working on this issue too. Looks similar though yours > > has a lot more churn and there's some other bugs I've found. > > That's good news, and yes now that I see it, some stuff on my series is overly > complicated. Specially around of_translate_*(). > > On top of that, you removed in of_dma_get_range(): > > - /* > -* At least empty ranges has to be defined for parent node if > -* DMA is supported > -*/ > - if (!ranges) > - break; > > Which I assumed was bound to the standard and makes things easier. The standard is whatever we say it is and what exists in the wild... Probably better for me to get the series posted for context, but the above is removed because we could be passing in the bus device/child node and checking for 'dma-ranges' rather than only the bus node. While this does mean 'dma-ranges' could be in a child node which is wrong, it simplifies the only caller of_dma_configure(). And really, there's no way to detect that error. Someone could call of_dma_configure(NULL, child, ...). Perhaps we could assert that 'ranges' is present whenever 'dma-ranges' is. Back to the standard, I think it can be summarized as a device's immediate parent (a bus node) must contain 'dma-ranges'. All the parent nodes of the bus node should also have 'dma-ranges', but missing is treated as empty (1:1 translation). 'dma-ranges' missing in all the parent nodes is also treated as 1:1 translation and no addressing restrictions. > > Can you test out this branch[1]. I don't have any h/w needing this, > > but wrote a unittest and tested with modified QEMU. > > I reviewed everything, I did find a minor issue, see the patch attached. > > Also I tested your branch both on an RPi4, with a PCI device that depends on > these changes and by comparing the OF debugs logs on a Layerscape board which > uses dma-ranges, dma-coherent and IOMMU. All works as expected. > > Will you send this series for v5.5? Please keep me in the loop, I'll review > and > test the final version. Yes, sending it out soon. Rob ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 00/11] of: Fix DMA configuration for non-DT masters
On Tue, Sep 24, 2019 at 1:12 PM Nicolas Saenz Julienne wrote: > > Hi All, > this series tries to address one of the issues blocking us from > upstreaming Broadcom's STB PCIe controller[1]. Namely, the fact that > devices not represented in DT which sit behind a PCI bus fail to get the > bus' DMA addressing constraints. > > This is due to the fact that of_dma_configure() assumes it's receiving a > DT node representing the device being configured, as opposed to the PCIe > bridge node we currently pass. This causes the code to directly jump > into PCI's parent node when checking for 'dma-ranges' and misses > whatever was set there. > > To address this I create a new API in OF - inspired from Robin Murphys > original proposal[2] - which accepts a bus DT node as it's input in > order to configure a device's DMA constraints. The changes go deep into > of/address.c's implementation, as a device being having a DT node > assumption was pretty strong. > > On top of this work, I also cleaned up of_dma_configure() removing its > redundant arguments and creating an alternative function for the special cases > not applicable to either the above case or the default usage. > > IMO the resulting functions are more explicit. They will probably > surface some hacky usages that can be properly fixed as I show with the > DT fixes on the Layerscape platform. > > This was also tested on a Raspberry Pi 4 with a custom PCIe driver and > on a Seattle AMD board. Humm, I've been working on this issue too. Looks similar though yours has a lot more churn and there's some other bugs I've found. Can you test out this branch[1]. I don't have any h/w needing this, but wrote a unittest and tested with modified QEMU. Rob [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 19/21] treewide: add checks for the return value of memblock_alloc*()
On Mon, Jan 21, 2019 at 10:04:06AM +0200, Mike Rapoport wrote:
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> size, align);
>
> Signed-off-by: Mike Rapoport
> Reviewed-by: Guo Ren # c-sky
> Acked-by: Paul Burton# MIPS
> Acked-by: Heiko Carstens # s390
> Reviewed-by: Juergen Gross # Xen
> ---
> arch/alpha/kernel/core_cia.c | 3 +++
> arch/alpha/kernel/core_marvel.c | 6 ++
> arch/alpha/kernel/pci-noop.c | 13 +++--
> arch/alpha/kernel/pci.c | 11 ++-
> arch/alpha/kernel/pci_iommu.c | 12
> arch/arc/mm/highmem.c | 4
> arch/arm/kernel/setup.c | 6 ++
> arch/arm/mm/mmu.c | 14 +-
> arch/arm64/kernel/setup.c | 8 +---
> arch/arm64/mm/kasan_init.c| 10 ++
> arch/c6x/mm/dma-coherent.c| 4
> arch/c6x/mm/init.c| 3 +++
> arch/csky/mm/highmem.c| 5 +
> arch/h8300/mm/init.c | 3 +++
> arch/m68k/atari/stram.c | 4
> arch/m68k/mm/init.c | 3 +++
> arch/m68k/mm/mcfmmu.c | 6 ++
> arch/m68k/mm/motorola.c | 9 +
> arch/m68k/mm/sun3mmu.c| 6 ++
> arch/m68k/sun3/sun3dvma.c | 3 +++
> arch/microblaze/mm/init.c | 8 ++--
> arch/mips/cavium-octeon/dma-octeon.c | 3 +++
> arch/mips/kernel/setup.c | 3 +++
> arch/mips/kernel/traps.c | 3 +++
> arch/mips/mm/init.c | 5 +
> arch/nds32/mm/init.c | 12
> arch/openrisc/mm/ioremap.c| 8 ++--
> arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +
> arch/powerpc/kernel/pci_32.c | 3 +++
> arch/powerpc/kernel/setup-common.c| 3 +++
> arch/powerpc/kernel/setup_64.c| 4
> arch/powerpc/lib/alloc.c | 3 +++
> arch/powerpc/mm/hash_utils_64.c | 3 +++
> arch/powerpc/mm/mmu_context_nohash.c | 9 +
> arch/powerpc/mm/pgtable-book3e.c | 12 ++--
> arch/powerpc/mm/pgtable-book3s64.c| 3 +++
> arch/powerpc/mm/pgtable-radix.c | 9 -
> arch/powerpc/mm/ppc_mmu_32.c | 3 +++
> arch/powerpc/platforms/pasemi/iommu.c | 3 +++
> arch/powerpc/platforms/powermac/nvram.c | 3 +++
> arch/powerpc/platforms/powernv/opal.c | 3 +++
> arch/powerpc/platforms/powernv/pci-ioda.c | 8
> arch/powerpc/platforms/ps3/setup.c| 3 +++
> arch/powerpc/sysdev/msi_bitmap.c | 3 +++
> arch/s390/kernel/setup.c | 13 +
> arch/s390/kernel/smp.c| 5 -
> arch/s390/kernel/topology.c | 6 ++
> arch/s390/numa/mode_emu.c | 3 +++
> arch/s390/numa/numa.c | 6 +-
> arch/sh/mm/init.c | 6 ++
> arch/sh/mm/numa.c | 4
> arch/um/drivers/net_kern.c| 3 +++
> arch/um/drivers/vector_kern.c | 3 +++
> arch/um/kernel/initrd.c | 2 ++
> arch/um/kernel/mem.c | 16
> arch/unicore32/kernel/setup.c | 4
> arch/unicore32/mm/mmu.c | 15 +--
> arch/x86/kernel/acpi/boot.c | 3 +++
> arch/x86/kernel/apic/io_apic.c| 5 +
> arch/x86/kernel/e820.c| 3 +++
> arch/x86/platform/olpc/olpc_dt.c | 3 +++
> arch/x86/xen/p2m.c| 11 +--
> arch/xtensa/mm/kasan_init.c | 4
> arch/xtensa/mm/mmu.c | 3 +++
> drivers/clk/ti/clk.c | 3 +++
> drivers/macintosh/smu.c | 3 +++
> drivers/of/fdt.c | 8 +++-
> drivers/of/unittest.c | 8 +++-
Acked-by: Rob Herrin
Re: [Xen-devel] [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*()
On Wed, Jan 16, 2019 at 7:46 AM Mike Rapoport wrote:
>
> Add check for the return value of memblock_alloc*() functions and call
> panic() in case of error.
> The panic message repeats the one used by panicing memblock allocators with
> adjustment of parameters to include only relevant ones.
>
> The replacement was mostly automated with semantic patches like the one
> below with manual massaging of format strings.
>
> @@
> expression ptr, size, align;
> @@
> ptr = memblock_alloc(size, align);
> + if (!ptr)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__,
> size, align);
>
> Signed-off-by: Mike Rapoport
> ---
> arch/alpha/kernel/core_cia.c | 3 +++
> arch/alpha/kernel/core_marvel.c | 6 ++
> arch/alpha/kernel/pci-noop.c | 11 ++-
> arch/alpha/kernel/pci.c | 11 ++-
> arch/alpha/kernel/pci_iommu.c | 12
> arch/arc/mm/highmem.c | 4
> arch/arm/kernel/setup.c | 6 ++
> arch/arm/mm/mmu.c | 14 +-
> arch/arm64/kernel/setup.c | 9 ++---
> arch/arm64/mm/kasan_init.c| 10 ++
> arch/c6x/mm/dma-coherent.c| 4
> arch/c6x/mm/init.c| 3 +++
> arch/csky/mm/highmem.c| 5 +
> arch/h8300/mm/init.c | 3 +++
> arch/m68k/atari/stram.c | 4
> arch/m68k/mm/init.c | 3 +++
> arch/m68k/mm/mcfmmu.c | 6 ++
> arch/m68k/mm/motorola.c | 9 +
> arch/m68k/mm/sun3mmu.c| 6 ++
> arch/m68k/sun3/sun3dvma.c | 3 +++
> arch/microblaze/mm/init.c | 8 ++--
> arch/mips/cavium-octeon/dma-octeon.c | 3 +++
> arch/mips/kernel/setup.c | 3 +++
> arch/mips/kernel/traps.c | 3 +++
> arch/mips/mm/init.c | 5 +
> arch/nds32/mm/init.c | 12
> arch/openrisc/mm/ioremap.c| 8 ++--
> arch/powerpc/kernel/dt_cpu_ftrs.c | 5 +
> arch/powerpc/kernel/pci_32.c | 3 +++
> arch/powerpc/kernel/setup-common.c| 3 +++
> arch/powerpc/kernel/setup_64.c| 4
> arch/powerpc/lib/alloc.c | 3 +++
> arch/powerpc/mm/hash_utils_64.c | 3 +++
> arch/powerpc/mm/mmu_context_nohash.c | 9 +
> arch/powerpc/mm/pgtable-book3e.c | 12 ++--
> arch/powerpc/mm/pgtable-book3s64.c| 3 +++
> arch/powerpc/mm/pgtable-radix.c | 9 -
> arch/powerpc/mm/ppc_mmu_32.c | 3 +++
> arch/powerpc/platforms/pasemi/iommu.c | 3 +++
> arch/powerpc/platforms/powermac/nvram.c | 3 +++
> arch/powerpc/platforms/powernv/opal.c | 3 +++
> arch/powerpc/platforms/powernv/pci-ioda.c | 8
> arch/powerpc/platforms/ps3/setup.c| 3 +++
> arch/powerpc/sysdev/msi_bitmap.c | 3 +++
> arch/s390/kernel/setup.c | 13 +
> arch/s390/kernel/smp.c| 5 -
> arch/s390/kernel/topology.c | 6 ++
> arch/s390/numa/mode_emu.c | 3 +++
> arch/s390/numa/numa.c | 6 +-
> arch/s390/numa/toptree.c | 8 ++--
> arch/sh/mm/init.c | 6 ++
> arch/sh/mm/numa.c | 4
> arch/um/drivers/net_kern.c| 3 +++
> arch/um/drivers/vector_kern.c | 3 +++
> arch/um/kernel/initrd.c | 2 ++
> arch/um/kernel/mem.c | 16
> arch/unicore32/kernel/setup.c | 4
> arch/unicore32/mm/mmu.c | 15 +--
> arch/x86/kernel/acpi/boot.c | 3 +++
> arch/x86/kernel/apic/io_apic.c| 5 +
> arch/x86/kernel/e820.c| 3 +++
> arch/x86/platform/olpc/olpc_dt.c | 3 +++
> arch/x86/xen/p2m.c| 11 +--
> arch/xtensa/mm/kasan_init.c | 4
> arch/xtensa/mm/mmu.c | 3 +++
> drivers/clk/ti/clk.c | 3 +++
> drivers/macintosh/smu.c | 3 +++
> drivers/of/fdt.c | 8 +++-
> drivers/of/unittest.c | 8 +++-
Acked-by: Rob Herring
> drivers/xen/swiotlb-xen.c | 7 +--
> kernel/power/snapshot.c | 3 +++
> lib/cpu
Re: [Xen-devel] [PATCH 08/21] memblock: drop __memblock_alloc_base()
On Wed, Jan 16, 2019 at 7:45 AM Mike Rapoport wrote: > > The __memblock_alloc_base() function tries to allocate a memory up to the > limit specified by its max_addr parameter. Depending on the value of this > parameter, the __memblock_alloc_base() can is replaced with the appropriate > memblock_phys_alloc*() variant. > > Signed-off-by: Mike Rapoport > --- > arch/sh/kernel/machine_kexec.c | 3 ++- > arch/x86/kernel/e820.c | 2 +- > arch/x86/mm/numa.c | 12 > drivers/of/of_reserved_mem.c | 7 ++- > include/linux/memblock.h | 2 -- > mm/memblock.c | 9 ++--- > 6 files changed, 11 insertions(+), 24 deletions(-) Acked-by: Rob Herring ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5] devicetree, xen: add xen, shared-memory binding
On Mon, Dec 03, 2018 at 02:26:09PM -0800, Stefano Stabellini wrote: > From: Stefano Stabellini > > Introduce a device tree binding for Xen reserved-memory regions. They > are used to share memory across VMs from the VM config files. (See > static_shm config option.) > > Signed-off-by: Stefano Stabellini > Cc: julien.gr...@arm.com > Cc: devicet...@vger.kernel.org > Cc: robh...@kernel.org > Cc: mark.rutl...@arm.com > Cc: xen-de...@lists.xen.org > --- > Changes in v5: > -rename offset to xen,offset > > Changes in v4: > - add offset property > > Changes in v3: > - remove fallback version > > Changes in v2: > - fix Author line > - add versioning > - xen,id instead of id > --- > .../bindings/reserved-memory/xen,shared-memory.txt | 24 > ++ > 1 file changed, 24 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt Applied, thanks. Rob ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4] devicetree, xen: add xen, shared-memory binding
Missing the DT list... On Mon, Dec 3, 2018 at 3:43 PM Stefano Stabellini wrote: > > From: Stefano Stabellini > > Introduce a device tree binding for Xen reserved-memory regions. They > are used to share memory across VMs from the VM config files. (See > static_shm config option.) > > Signed-off-by: Stefano Stabellini > Cc: julien.gr...@arm.com > --- > Changes in v4: > - add offset property > > Changes in v3: > - remove fallback version > > Changes in v2: > - fix Author line > - add versioning > - xen,id instead of id > --- > .../bindings/reserved-memory/xen,shared-memory.txt | 24 > ++ > 1 file changed, 24 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > > diff --git > a/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > new file mode 100644 > index 000..0a35ffc > --- /dev/null > +++ b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > @@ -0,0 +1,24 @@ > +* Xen hypervisor reserved-memory binding > + > +Expose one or more memory regions as reserved-memory to the guest > +virtual machine. Typically, a region is configured at VM creation time > +to be a shared memory area across multiple virtual machines for > +communication among them. > + > +For each of these pre-shared memory regions, a range is exposed under > +the /reserved-memory node as a child node. Each range sub-node is named > +xen-shmem@ and has the following properties: > + > +- compatible: > + compatible = "xen,shared-memory-v1" > + > +- reg: > + the base guest physical address and size of the shared memory region > + > +- offset: (borrower VMs only) > + 64 bit integer offset within the owner virtual machine's shared > + memory region used for the mapping in the borrower VM. xen,offset. 'offset' is already used in some other bindings to mean register bit offset. > + > +- xen,id: > + a string that identifies the shared memory region as specified in > + the VM config file > -- > 1.9.1 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] devicetree, xen: add xen, shared-memory binding
On Mon, Oct 22, 2018 at 11:27:23AM +0100, Julien Grall wrote: > Hi Stefano, > > On 18/10/2018 23:10, Stefano Stabellini wrote: > > From: Stefano Stabellini > > > > Introduce a device tree binding for Xen reserved-memory regions. They > > are used to share memory across VMs from the VM config files. (See > > static_shm config option.) > > > > Signed-off-by: Stefano Stabellini > > --- > > Changes in v2: > > - fix Author line > > - add versioning > > - xen,id instead of id > > --- > > .../bindings/reserved-memory/xen,shared-memory.txt | 20 > > > > 1 file changed, 20 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > > > > diff --git > > a/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > > b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > > new file mode 100644 > > index 000..9078fb7 > > --- /dev/null > > +++ > > b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > > @@ -0,0 +1,20 @@ > > +* Xen hypervisor reserved-memory binding > > + > > +Expose one or more memory regions as reserved-memory to the guest > > +virtual machine. Typically, a region is configured at VM creation time > > +to be a shared memory area across multiple virtual machines for > > +communication among them. > > + > > +For each of these pre-shared memory regions, a range is exposed under > > +the /reserved-memory node as a child node. Each range sub-node is named > > +xen-shmem@ and has the following properties: > > + > > +- compatible: > > + compatible = "xen,shared-memory-v1", "xen,shared-memory" > > Do we need to specify the two compatibles? I'd just drop the fallback as version seems to be just in case. Rob ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] devicetree, xen: add xen, shared-memory binding
On Mon, Oct 08, 2018 at 04:03:54PM -0700, Stefano Stabellini wrote: > Introduce a device tree binding for Xen reserved-memory regions. They > are used to share memory across VMs from the VM config files. (See > static_shm config option.) > > Signed-off-by: Stefano Stabellini checkpatch.pl complains that the author and S-o-b don't match. > Cc: julien.gr...@arm.com > > diff --git > a/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > new file mode 100644 > index 000..a927a94 > --- /dev/null > +++ b/Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt > @@ -0,0 +1,20 @@ > +* Xen hypervisor reserved-memory binding > + > +Expose one or more memory regions as reserved-memory to the guest > +virtual machine. Typically, a region is configured at VM creation time > +to be a shared memory area across multiple virtual machines for > +communication among them. > + > +For each of these pre-shared memory regions, a range is exposed under > +the /reserved-memory node as a child node. Each range sub-node is named > +xen-shmem@ and has the following properties: > + > +- compatible: > + compatible = xen,shared-memory" Any need for versioning? > + > +- reg: > + the base guest physical address and size of the shared memory region > + > +- id: xen,id > + a string that identifies the shared memory region as specified in > + the VM config file ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
