On Wed, Sep 29, 2021 at 12:17:35AM +0300, Oded Gabbay wrote:
> On Tue, Sep 28, 2021 at 8:36 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote:
> >
> > On Sun, Sep 12, 2021 at 07:53:09PM +0300, Oded Gabbay wrote:
> > > From: Tomer Tayar <ttayar(a)habana.ai>
> > >
> > > Implement the calls to the dma-buf kernel api to create a dma-buf
> > > object backed by FD.
> > >
> > > We block the option to mmap the DMA-BUF object because we don't support
> > > DIRECT_IO and implicit P2P.
> >
> > This statement doesn't make sense, you can mmap your dmabuf if you
> > like. All dmabuf mmaps are supposed to set the special bit/etc to
> > exclude them from get_user_pages() anyhow - and since this is BAR
> > memory not struct page memory this driver would be doing it anyhow.
> >
> But we block mmap the dmabuf fd from user-space.
> If you try to do it, you will get MAP_FAILED.
You do, I'm saying the above paragraph explaining *why* that was done
is not correct.
> > > We check the p2p distance using pci_p2pdma_distance_many() and refusing
> > > to map dmabuf in case the distance doesn't allow p2p.
> >
> > Does this actually allow the p2p transfer for your intended use cases?
>
> It depends on the system. If we are working bare-metal, then yes, it allows.
> If inside a VM, then no. The virtualized root complex is not
> white-listed and the kernel can't know the distance.
> But I remember you asked me to add this check, in v3 of the review IIRC.
> I don't mind removing this check if you don't object.
Yes, i tis the right code, I was curious how far along things have
gotten
> > Don't write to the kernel log from user space triggered actions
> at all ?
At all.
> It's the first time I hear about this limitation...
Oh? It is a security issue, we don't want to allow userspace to DOS
the kerne logging.
> How do you tell the user it has done something wrong ?
dev_dbg is the usual way and then users doing debugging can opt in to
the logging.
> > Why doesn't this return a sg_table * and an ERR_PTR?
> Basically I modeled this function after amdgpu_vram_mgr_alloc_sgt()
> And in that function they also return int and pass the sg_table as **
>
> If it's critical I can change.
Please follow the normal kernel style
Jason
On Tue, Sep 28, 2021 at 10:04:29AM +0300, Oded Gabbay wrote:
> On Thu, Sep 23, 2021 at 12:22 PM Oded Gabbay <ogabbay(a)kernel.org> wrote:
> >
> > On Sat, Sep 18, 2021 at 11:38 AM Oded Gabbay <ogabbay(a)kernel.org> wrote:
> > >
> > > On Fri, Sep 17, 2021 at 3:30 PM Daniel Vetter <daniel(a)ffwll.ch> wrote:
> > > >
> > > > On Thu, Sep 16, 2021 at 10:10:14AM -0300, Jason Gunthorpe wrote:
> > > > > On Thu, Sep 16, 2021 at 02:31:34PM +0200, Daniel Vetter wrote:
> > > > > > On Wed, Sep 15, 2021 at 10:45:36AM +0300, Oded Gabbay wrote:
> > > > > > > On Tue, Sep 14, 2021 at 7:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote:
> > > > > > > >
> > > > > > > > On Tue, Sep 14, 2021 at 04:18:31PM +0200, Daniel Vetter wrote:
> > > > > > > > > On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > Re-sending this patch-set following the release of our user-space TPC
> > > > > > > > > > compiler and runtime library.
> > > > > > > > > >
> > > > > > > > > > I would appreciate a review on this.
> > > > > > > > >
> > > > > > > > > I think the big open we have is the entire revoke discussions. Having the
> > > > > > > > > option to let dma-buf hang around which map to random local memory ranges,
> > > > > > > > > without clear ownership link and a way to kill it sounds bad to me.
> > > > > > > > >
> > > > > > > > > I think there's a few options:
> > > > > > > > > - We require revoke support. But I've heard rdma really doesn't like that,
> > > > > > > > > I guess because taking out an MR while holding the dma_resv_lock would
> > > > > > > > > be an inversion, so can't be done. Jason, can you recap what exactly the
> > > > > > > > > hold-up was again that makes this a no-go?
> > > > > > > >
> > > > > > > > RDMA HW can't do revoke.
> > > > > >
> > > > > > Like why? I'm assuming when the final open handle or whatever for that MR
> > > > > > is closed, you do clean up everything? Or does that MR still stick around
> > > > > > forever too?
> > > > >
> > > > > It is a combination of uAPI and HW specification.
> > > > >
> > > > > revoke here means you take a MR object and tell it to stop doing DMA
> > > > > without causing the MR object to be destructed.
> > > > >
> > > > > All the drivers can of course destruct the MR, but doing such a
> > > > > destruction without explicit synchronization with user space opens
> > > > > things up to a serious use-after potential that could be a security
> > > > > issue.
> > > > >
> > > > > When the open handle closes the userspace is synchronized with the
> > > > > kernel and we can destruct the HW objects safely.
> > > > >
> > > > > So, the special HW feature required is 'stop doing DMA but keep the
> > > > > object in an error state' which isn't really implemented, and doesn't
> > > > > extend very well to other object types beyond simple MRs.
> > > >
> > > > Yeah revoke without destroying the MR doesn't work, and it sounds like
> > > > revoke by destroying the MR just moves the can of worms around to another
> > > > place.
> > > >
> > > > > > 1. User A opens gaudi device, sets up dma-buf export
> > > > > >
> > > > > > 2. User A registers that with RDMA, or anything else that doesn't support
> > > > > > revoke.
> > > > > >
> > > > > > 3. User A closes gaudi device
> > > > > >
> > > > > > 4. User B opens gaudi device, assumes that it has full control over the
> > > > > > device and uploads some secrets, which happen to end up in the dma-buf
> > > > > > region user A set up
> > > > >
> > > > > I would expect this is blocked so long as the DMABUF exists - eg the
> > > > > DMABUF will hold a fget on the FD of #1 until the DMABUF is closed, so
> > > > > that #3 can't actually happen.
> > > > >
> > > > > > It's not mlocked memory, it's mlocked memory and I can exfiltrate
> > > > > > it.
> > > > >
> > > > > That's just bug, don't make buggy drivers :)
> > > >
> > > > Well yeah, but given that habanalabs hand rolled this I can't just check
> > > > for the usual things we have to enforce this in drm. And generally you can
> > > > just open chardevs arbitrarily, and multiple users fighting over each
> > > > another. The troubles only start when you have private state or memory
> > > > allocations of some kind attached to the struct file (instead of the
> > > > underlying device), or something else that requires device exclusivity.
> > > > There's no standard way to do that.
> > > >
> > > > Plus in many cases you really want revoke on top (can't get that here
> > > > unfortunately it seems), and the attempts to get towards a generic
> > > > revoke() just never went anywhere. So again it's all hand-rolled
> > > > per-subsystem. *insert lament about us not having done this through a
> > > > proper subsystem*
> > > >
> > > > Anyway it sounds like the code takes care of that.
> > > > -Daniel
> > >
> > > Daniel, Jason,
> > > Thanks for reviewing this code.
> > >
> > > Can I get an R-B / A-B from you for this patch-set ?
> > >
> > > Thanks,
> > > Oded
> >
> > A kind reminder.
> >
> > Thanks,
> > Oded
>
> Hi,
> I know last week was LPC and maybe this got lost in the inbox, so I'm
> sending it again to make sure you got my request for R-B / A-B.
I was waiting for some clarity from the maintainers summit, but that's
still about as unclear as it gets. Either way technically it sounds ok,
but I'm a bit burried so didn't look at the code.
Acked-by: Daniel Vetter <daniel.vetter(a)ffwll.ch>
But looking beyond the strict lens of dma-buf I'm still impressed by the
mess this created, to get to the same endpoint of "we open our stack" in
the same time it takes others to sort this out. I'm still looking for some
kind of plan to fix this.
Also you probably want to get Dave to ack this too, I pinged him on irc
last week about this after maintainer summit.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Sun, Sep 12, 2021 at 07:53:08PM +0300, Oded Gabbay wrote:
> /* HL_MEM_OP_* */
> __u32 op;
> - /* HL_MEM_* flags */
> + /* HL_MEM_* flags.
> + * For the HL_MEM_OP_EXPORT_DMABUF_FD opcode, this field holds the
> + * DMA-BUF file/FD flags.
> + */
> __u32 flags;
> /* Context ID - Currently not in use */
> __u32 ctx_id;
> @@ -1072,6 +1091,13 @@ struct hl_mem_out {
>
> __u32 pad;
> };
> +
> + /* Returned in HL_MEM_OP_EXPORT_DMABUF_FD. Represents the
> + * DMA-BUF object that was created to describe a memory
> + * allocation on the device's memory space. The FD should be
> + * passed to the importer driver
> + */
> + __u64 fd;
fd's should be a s32 type in a fixed width uapi.
I usually expect to see the uapi changes inside the commit that
consumes them, splitting the patch like this seems strange but
harmless.
Jason
From: Rob Clark <robdclark(a)chromium.org>
This series adds deadline awareness to fences, so realtime deadlines
such as vblank can be communicated to the fence signaller for power/
frequency management decisions.
This is partially inspired by a trick i915 does, but implemented
via dma-fence for a couple of reasons:
1) To continue to be able to use the atomic helpers
2) To support cases where display and gpu are different drivers
This iteration adds a dma-fence ioctl to set a deadline (both to
support igt-tests, and compositors which delay decisions about which
client buffer to display), and a sw_sync ioctl to read back the
deadline. IGT tests utilizing these can be found at:
https://gitlab.freedesktop.org/robclark/igt-gpu-tools/-/commits/fence-deadl…
v1: https://patchwork.freedesktop.org/series/93035/
v2: Move filtering out of later deadlines to fence implementation
to avoid increasing the size of dma_fence
v3: Add support in fence-array and fence-chain; Add some uabi to
support igt tests and userspace compositors.
Rob Clark (9):
dma-fence: Add deadline awareness
drm/vblank: Add helper to get next vblank time
drm/atomic-helper: Set fence deadline for vblank
drm/scheduler: Add fence deadline support
drm/msm: Add deadline based boost support
dma-buf/fence-array: Add fence deadline support
dma-buf/fence-chain: Add fence deadline support
dma-buf/sync_file: Add SET_DEADLINE ioctl
dma-buf/sw_sync: Add fence deadline support
drivers/dma-buf/dma-fence-array.c | 11 ++++
drivers/dma-buf/dma-fence-chain.c | 13 +++++
drivers/dma-buf/dma-fence.c | 20 +++++++
drivers/dma-buf/sw_sync.c | 58 +++++++++++++++++++
drivers/dma-buf/sync_debug.h | 2 +
drivers/dma-buf/sync_file.c | 19 +++++++
drivers/gpu/drm/drm_atomic_helper.c | 36 ++++++++++++
drivers/gpu/drm/drm_vblank.c | 32 +++++++++++
drivers/gpu/drm/msm/msm_fence.c | 76 +++++++++++++++++++++++++
drivers/gpu/drm/msm/msm_fence.h | 20 +++++++
drivers/gpu/drm/msm/msm_gpu.h | 1 +
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 20 +++++++
drivers/gpu/drm/scheduler/sched_fence.c | 34 +++++++++++
drivers/gpu/drm/scheduler/sched_main.c | 2 +-
include/drm/drm_vblank.h | 1 +
include/drm/gpu_scheduler.h | 8 +++
include/linux/dma-fence.h | 16 ++++++
include/uapi/linux/sync_file.h | 20 +++++++
18 files changed, 388 insertions(+), 1 deletion(-)
--
2.31.1
> > > +static int bcmasp_set_priv_flags(struct net_device *dev, u32 flags)
> > > +{
> > > + struct bcmasp_intf *intf = netdev_priv(dev);
> > > +
> > > + intf->wol_keep_rx_en = flags & BCMASP_WOL_KEEP_RX_EN ? 1 : 0;
> > > +
> > > + return 0;
> >
> > Please could you explain this some more. How can you disable RX and
> > still have WoL working?
>
> Wake-on-LAN using Magic Packets and network filters requires keeping the
> UniMAC's receiver turned on, and then the packets feed into the Magic Packet
> Detector (MPD) block or the network filter block. In that mode DRAM is in
> self refresh and there is local matching of frames into a tiny FIFO however
> in the case of magic packets the packets leading to a wake-up are dropped as
> there is nowhere to store them. In the case of a network filter match (e.g.:
> matching a multicast IP address plus protocol, plus source/destination
> ports) the packets are also discarded because the receive DMA was shut down.
>
> When the wol_keep_rx_en flag is set, the above happens but we also allow the
> packets that did match a network filter to reach the small FIFO (Justin
> would know how many entries are there) that is used to push the packets to
> DRAM. The packet contents are held in there until the system wakes up which
> is usually just a few hundreds of micro seconds after we received a packet
> that triggered a wake-up. Once we overflow the receive DMA FIFO capacity
> subsequent packets get dropped which is fine since we are usually talking
> about very low bit rates, and we only try to push to DRAM the packets of
> interest, that is those for which we have a network filter.
>
> This is convenient in scenarios where you want to wake-up from multicast DNS
> (e.g.: wake on Googlecast, Bonjour etc.) because then the packet that
> resulted in the system wake-up is not discarded but is then delivered to the
> network stack.
Thanks for the explanation. It would be easier for the user if you
automate this. Enable is by default for WoL types which have user
content?
> > > + /* Per ch */
> > > + intf->tx_spb_dma = priv->base + TX_SPB_DMA_OFFSET(intf);
> > > + intf->res.tx_spb_ctrl = priv->base + TX_SPB_CTRL_OFFSET(intf);
> > > + /*
> > > + * Stop gap solution. This should be removed when 72165a0 is
> > > + * deprecated
> > > + */
> >
> > Is that an internal commit?
>
> Yes this is a revision of the silicon that is not meant to see the light of
> day.
So this can all be removed?
Andrew
On Fri, 24 Sep 2021 14:44:48 -0700, Justin Chen wrote:
> The ASP 2.0 Ethernet controller uses a brcm unimac.
>
> Signed-off-by: Justin Chen <justinpopo6(a)gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli(a)gmail.com>
> ---
> Documentation/devicetree/bindings/net/brcm,unimac-mdio.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.
Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.
Full log is available here: https://patchwork.ozlabs.org/patch/1532529
mdio@e14: #address-cells:0:0: 1 was expected
arch/arm64/boot/dts/broadcom/bcm2711-rpi-400.dt.yaml
arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dt.yaml
arch/arm/boot/dts/bcm2711-rpi-400.dt.yaml
arch/arm/boot/dts/bcm2711-rpi-4-b.dt.yaml
mdio@e14: #size-cells:0:0: 0 was expected
arch/arm64/boot/dts/broadcom/bcm2711-rpi-400.dt.yaml
arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b.dt.yaml
arch/arm/boot/dts/bcm2711-rpi-400.dt.yaml
arch/arm/boot/dts/bcm2711-rpi-4-b.dt.yaml
On Fri, 24 Sep 2021 14:44:47 -0700, Justin Chen wrote:
> From: Florian Fainelli <f.fainelli(a)gmail.com>
>
> Add a binding document for the Broadcom ASP 2.0 Ethernet controller.
>
> Signed-off-by: Florian Fainelli <f.fainelli(a)gmail.com>
> Signed-off-by: Justin Chen <justinpopo6(a)gmail.com>
> ---
> .../devicetree/bindings/net/brcm,asp-v2.0.yaml | 147 +++++++++++++++++++++
> 1 file changed, 147 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml:79:10: [warning] wrong indentation: expected 10 but found 9 (indentation)
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/brcm,asp-v2.0.example.dt.yaml: asp@9c00000: 'mdio@c614', 'mdio@ce14' do not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/brcm,asp-v2.0.yaml
Documentation/devicetree/bindings/net/brcm,asp-v2.0.example.dt.yaml:0:0: /example-0/asp@9c00000/mdio@c614: failed to match any schema with compatible: ['brcm,asp-v2.0-mdio']
Documentation/devicetree/bindings/net/brcm,asp-v2.0.example.dt.yaml:0:0: /example-0/asp@9c00000/mdio@ce14: failed to match any schema with compatible: ['brcm,asp-v2.0-mdio']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1532528
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.