* RE: [PATCH 1/8] scsi: add a host / host template field for the virt boundary
From: Kashyap Desai @ 2019-06-20 6:17 UTC (permalink / raw)
To: Ming Lei, Christoph Hellwig
Cc: Martin K . Petersen, Sagi Grimberg, Max Gurtovoy, Bart Van Assche,
linux-rdma, Linux SCSI List, PDL,MEGARAIDLINUX,
PDL-MPT-FUSIONLINUX, linux-hyperv, Linux Kernel Mailing List,
Kashyap Desai
In-Reply-To: <CACVXFVOwCeM2JzefBpKsVZrEaWpSBR0DF8qp4oKfoHm+pwLBYw@mail.gmail.com>
> -----Original Message-----
> From: megaraidlinux.pdl@broadcom.com
> [mailto:megaraidlinux.pdl@broadcom.com] On Behalf Of Ming Lei
> Sent: Tuesday, June 18, 2019 6:05 AM
> To: Christoph Hellwig <hch@lst.de>
> Cc: Martin K . Petersen <martin.petersen@oracle.com>; Sagi Grimberg
> <sagi@grimberg.me>; Max Gurtovoy <maxg@mellanox.com>; Bart Van
> Assche <bvanassche@acm.org>; linux-rdma <linux-rdma@vger.kernel.org>;
> Linux SCSI List <linux-scsi@vger.kernel.org>;
> megaraidlinux.pdl@broadcom.com; MPT-FusionLinux.pdl@broadcom.com;
> linux-hyperv@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>
> Subject: Re: [PATCH 1/8] scsi: add a host / host template field for the
> virt
> boundary
>
> On Mon, Jun 17, 2019 at 8:21 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > This allows drivers setting it up easily instead of branching out to
> > block layer calls in slave_alloc, and ensures the upgraded
> > max_segment_size setting gets picked up by the DMA layer.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > drivers/scsi/hosts.c | 3 +++
> > drivers/scsi/scsi_lib.c | 3 ++-
> > include/scsi/scsi_host.h | 3 +++
> > 3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index
> > ff0d8c6a8d0c..55522b7162d3 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -462,6 +462,9 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
> > else
> > shost->dma_boundary = 0xffffffff;
> >
> > + if (sht->virt_boundary_mask)
> > + shost->virt_boundary_mask = sht->virt_boundary_mask;
> > +
> > device_initialize(&shost->shost_gendev);
> > dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
> > shost->shost_gendev.bus = &scsi_bus_type; diff --git
> > a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index
> > 65d0a10c76ad..d333bb6b1c59 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1775,7 +1775,8 @@ void __scsi_init_queue(struct Scsi_Host *shost,
> struct request_queue *q)
> > dma_set_seg_boundary(dev, shost->dma_boundary);
> >
> > blk_queue_max_segment_size(q, shost->max_segment_size);
> > - dma_set_max_seg_size(dev, shost->max_segment_size);
> > + blk_queue_virt_boundary(q, shost->virt_boundary_mask);
> > + dma_set_max_seg_size(dev, queue_max_segment_size(q));
>
> The patch looks fine, also suggest to make sure that max_segment_size is
> block-size aligned, and un-aligned max segment size has caused trouble on
> mmc.
I validated changes on latest and few older series controllers.
Post changes, I noticed max_segment_size is updated.
find /sys/ -name max_segment_size |grep sdb |xargs grep -r .
/sys/devices/pci0000:3a/0000:3a:00.0/0000:3b:00.0/0000:3c:04.0/0000:40:00.0/0000:41:00.0/0000:42:00.0/host0/target0:2:12/0:2:12:0/block/sdb/queue/max_segment_size:4294967295
I verify that single SGE having 1MB transfer length is working fine.
Acked-by: Kashyap Desai < kashyap.desai@broadcom.com>
>
> Thanks,
> Ming Lei
>
^ permalink raw reply
* Re: [PATCH v17 12/15] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get
From: Khalid Aziz @ 2019-06-19 20:05 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Mauro Carvalho Chehab, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky,
Christoph Hellwig, Jason Gunthorpe, Dave Martin, Evgeniy Stepanov,
Kevin Brodsky, Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman
In-Reply-To: <7fbcdbe16a2bd99e92eb4541248469738d89a122.1560339705.git.andreyknvl@google.com>
On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> videobuf_dma_contig_user_get() uses provided user pointers for vma
> lookups, which can only by done with untagged pointers.
>
> Untag the pointers in this function.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
Patch looks good, but commit log should be updated to not be specific to
arm64.
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> drivers/media/v4l2-core/videobuf-dma-contig.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> exact_copy_from_user
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index e1bf50df4c70..8a1ddd146b17 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -160,6 +160,7 @@ static void videobuf_dma_contig_user_put(struct videobuf_dma_contig_memory *mem)
> static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> struct videobuf_buffer *vb)
> {
> + unsigned long untagged_baddr = untagged_addr(vb->baddr);
> struct mm_struct *mm = current->mm;
> struct vm_area_struct *vma;
> unsigned long prev_pfn, this_pfn;
> @@ -167,22 +168,22 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
> unsigned int offset;
> int ret;
>
> - offset = vb->baddr & ~PAGE_MASK;
> + offset = untagged_baddr & ~PAGE_MASK;
> mem->size = PAGE_ALIGN(vb->size + offset);
> ret = -EINVAL;
>
> down_read(&mm->mmap_sem);
>
> - vma = find_vma(mm, vb->baddr);
> + vma = find_vma(mm, untagged_baddr);
> if (!vma)
> goto out_up;
>
> - if ((vb->baddr + mem->size) > vma->vm_end)
> + if ((untagged_baddr + mem->size) > vma->vm_end)
> goto out_up;
>
> pages_done = 0;
> prev_pfn = 0; /* kill warning */
> - user_address = vb->baddr;
> + user_address = untagged_baddr;
>
> while (pages_done < (mem->size >> PAGE_SHIFT)) {
> ret = follow_pfn(vma, user_address, &this_pfn);
>
^ permalink raw reply
* Re: [PATCH v17 07/15] fs, arm64: untag user pointers in copy_mount_options
From: Khalid Aziz @ 2019-06-19 20:01 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Catalin Marinas, Vincenzo Frascino, Will Deacon, Mark Rutland,
Andrew Morton, Greg Kroah-Hartman, Kees Cook, Yishai Hadas,
Felix Kuehling, Alexander Deucher, Christian Koenig,
Mauro Carvalho Chehab, Jens Wiklander, Alex Williamson,
Leon Romanovsky, Luc Van Oostenryck, Dave Martin, enh,
Jason Gunthorpe
In-Reply-To: <4ed871e14cc265a519c6ba8660a1827844371791.1560339705.git.andreyknvl@google.com>
On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> In copy_mount_options a user address is being subtracted from TASK_SIZE.
> If the address is lower than TASK_SIZE, the size is calculated to not
> allow the exact_copy_from_user() call to cross TASK_SIZE boundary.
> However if the address is tagged, then the size will be calculated
> incorrectly.
>
> Untag the address before subtracting.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
Please update commit log to make it not arm64 specific since this change
affects other architectures as well. Other than that,
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> fs/namespace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index b26778bdc236..2e85712a19ed 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2993,7 +2993,7 @@ void *copy_mount_options(const void __user * data)
> * the remainder of the page.
> */
> /* copy_from_user cannot cross TASK_SIZE ! */
> - size = TASK_SIZE - (unsigned long)data;
> + size = TASK_SIZE - (unsigned long)untagged_addr(data);
> if (size > PAGE_SIZE)
> size = PAGE_SIZE;
>
>
^ permalink raw reply
* Re: [PATCH iproute2 v2 2/2] uapi: update if_link.h
From: David Ahern @ 2019-06-19 19:31 UTC (permalink / raw)
To: Stephen Hemminger, Denis Kirjanov; +Cc: netdev, linux-rdma, dledford, mkubecek
In-Reply-To: <20190619104652.4c71c33b@hermes.lan>
On 6/19/19 11:47 AM, Stephen Hemminger wrote:
> On Wed, 19 Jun 2019 16:14:14 +0200
> Denis Kirjanov <kda@linux-powerpc.org> wrote:
>
>> update if_link.h to commit 75345f888f700c4ab2448287e35d48c760b202e6
>> ("ipoib: show VF broadcast address")
>>
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>
> This is only on net-next so the patches should target iproute2-next.
>
> David can update from that.
>
Make uapi changes as a separate patch; don't worry about syncing the
entire file. That patch is my hint to resync the kernel headers to top
of tree before applying the rest of the set.
^ permalink raw reply
* Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors
From: Mark Bloch @ 2019-06-19 17:52 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jianbo Liu, Saeed Mahameed, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Roi Dayan
In-Reply-To: <20190619081226.GI11611@mtr-leonro.mtl.com>
On 6/19/19 1:12 AM, Leon Romanovsky wrote:
> On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote:
>>
>>
>> On 6/19/2019 00:43, Leon Romanovsky wrote:
>>> On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
>>>>
>>>>
>>>> On 6/18/2019 23:51, Leon Romanovsky wrote:
>>>>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
>>>>>> The 06/19/2019 13:04, Leon Romanovsky wrote:
>>>>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
>>>>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
>>>>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
>>>>>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>>>>
>>>>>>>>>> If vport metadata matching is enabled in eswitch, the rule created
>>>>>>>>>> must be changed to match on the metadata, instead of source port.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
>>>>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>>>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
>>>>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>>>>>>>>>> ---
>>>>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
>>>>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
>>>>>>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------
>>>>>>>>>> 3 files changed, 63 insertions(+), 9 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> index 22e651cb5534..d4ed611de35d 100644
>>>>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
>>>>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
>>>>>>>>>> return mlx5_eswitch_vport_rep(esw, vport);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
>>>>>>>>>> +{
>>>>>>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
>>>>>>>>>> + u16 vport)
>>>>>>>>>> +{
>>>>>>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
>>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> 1. There is no need to introduce one line functions, call to that code directly.
>>>>>>>>
>>>>>>>> No. They are in IB, and we don't want them be mixed up by the original
>>>>>>>> functions in eswitch. Please ask Mark more about it.
>>>>>>>
>>>>>>> Please enlighten me.
>>>>>>
>>>>>> It was suggested by Mark in prevouis review.
>>>>>> I think it's because there are in different modules, and better to with
>>>>>> different names, so introduce there extra one line functions.
>>>>>> Please correct me if I'm wrong, Mark...
>>>>>
>>>>> mlx5_ib is full of direct function calls to mlx5_core and it is done on
>>>>> purpose for at least two reasons. First is to control in one place
>>>>> all compilation options and expose proper API interface with and without
>>>>> specific kernel config is on. Second is to emphasize that this is core
>>>>> function and save us time in refactoring and reviewing.
>>>>
>>>> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
>>>> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
>>>> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
>>>> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
>>>> should have probably done the same.
>>>
>>> This is exactly the problem, eswitch.h should provide stubs for all
>>> exported functions, so other clients of eswitch won't need to deal with
>>> various unrelated config options.
>>
>> The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly
>> but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling
>> the eswitch layer.
>>
>> If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when
>> CONFIG_MLX5_ESWITCH id defined.
>
> This simple patch + cleanup of ib_rep.h will do the trick.
>
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 67b9e7ac569a..b917ba28659e 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -59,7 +59,9 @@
> #include <linux/in.h>
> #include <linux/etherdevice.h>
> #include "mlx5_ib.h"
> +#if defined(CONFIG_MLX5_ESWITCH)
> #include "ib_rep.h"
> +#endif
> #include "cmd.h"
> #include "srq.h"
> #include <linux/mlx5/fs_helpers.h>
> @@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile pf_profile = {
> mlx5_ib_stage_delay_drop_cleanup),
> };
>
> +#if defined(CONFIG_MLX5_ESWITCH)
> const struct mlx5_ib_profile uplink_rep_profile = {
> STAGE_CREATE(MLX5_IB_STAGE_INIT,
> mlx5_ib_stage_init_init,
> @@ -6812,6 +6815,7 @@
> const struct mlx5_ib_profile uplink_rep_profile = {
> mlx5_ib_stage_post_ib_reg_umr_init,
> NULL),
> };
>
I really dislike seeing #if defined(CONFIG_MLX5_ESWITCH) inside .c files
and here it's easily avoided so I don't see a reason do it it.
Can this cleanup wait for after this series?
Mark
>>
>> so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h).
>> For me it makes it easier to follow, but I can adept if you don't like it.
>>
>> Mark
>>
>>>
>>>>
>>>> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
>>>> I want to group together stuff in separate files.
>>>
>>> Yes, it is right thing to do.
>>>
>>>>
>>>> If you prefer direct calls that's okay as well.
>>>
>>> Yes, please.
>>>
>>>>
>>>> Mark
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 2. It should be bool and not u32.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> --
>>>>>>
>>>>>> --
^ permalink raw reply
* Re: [PATCH iproute2 v2 2/2] uapi: update if_link.h
From: Stephen Hemminger @ 2019-06-19 17:47 UTC (permalink / raw)
To: Denis Kirjanov, David Ahern; +Cc: netdev, linux-rdma, dledford, mkubecek
In-Reply-To: <20190619141414.4242-2-dkirjanov@suse.com>
On Wed, 19 Jun 2019 16:14:14 +0200
Denis Kirjanov <kda@linux-powerpc.org> wrote:
> update if_link.h to commit 75345f888f700c4ab2448287e35d48c760b202e6
> ("ipoib: show VF broadcast address")
>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
This is only on net-next so the patches should target iproute2-next.
David can update from that.
^ permalink raw reply
* Re: [PATCH v17 06/15] mm, arm64: untag user pointers in get_vaddr_frames
From: Khalid Aziz @ 2019-06-19 16:48 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <4c0b9a258e794437a1c6cec97585b4b5bd2d3bba.1560339705.git.andreyknvl@google.com>
On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> get_vaddr_frames uses provided user pointers for vma lookups, which can
> only by done with untagged pointers. Instead of locating and changing
> all callers of this function, perform untagging in it.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
With the suggested change to commit log in my previous email:
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> mm/frame_vector.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> index c64dca6e27c2..c431ca81dad5 100644
> --- a/mm/frame_vector.c
> +++ b/mm/frame_vector.c
> @@ -46,6 +46,8 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
> if (WARN_ON_ONCE(nr_frames > vec->nr_allocated))
> nr_frames = vec->nr_allocated;
>
> + start = untagged_addr(start);
> +
> down_read(&mm->mmap_sem);
> locked = 1;
> vma = find_vma_intersection(mm, start, start + 1);
>
^ permalink raw reply
* Re: [PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls
From: Khalid Aziz @ 2019-06-19 16:46 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <a5e0e465-89d5-91d0-c6a4-39674269bbf2@oracle.com>
On 6/19/19 9:55 AM, Khalid Aziz wrote:
> On 6/12/19 5:43 AM, Andrey Konovalov wrote:
>> This patch is a part of a series that extends arm64 kernel ABI to allow to
>> pass tagged user pointers (with the top byte set to something else other
>> than 0x00) as syscall arguments.
>>
>> This patch allows tagged pointers to be passed to the following memory
>> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
>> mremap, msync, munlock, move_pages.
>>
>> The mmap and mremap syscalls do not currently accept tagged addresses.
>> Architectures may interpret the tag as a background colour for the
>> corresponding vma.
>>
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>
> Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
>
>
I would also recommend updating commit log for all the patches in this
series that are changing files under mm/ as opposed to arch/arm64 to not
reference arm64 kernel ABI since the change applies to every
architecture. So something along the lines of "This patch is part of a
series that extends kernel ABI to allow......."
--
Khalid
>> mm/madvise.c | 2 ++
>> mm/mempolicy.c | 3 +++
>> mm/migrate.c | 2 +-
>> mm/mincore.c | 2 ++
>> mm/mlock.c | 4 ++++
>> mm/mprotect.c | 2 ++
>> mm/mremap.c | 7 +++++++
>> mm/msync.c | 2 ++
>> 8 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 628022e674a7..39b82f8a698f 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>> size_t len;
>> struct blk_plug plug;
>>
>> + start = untagged_addr(start);
>> +
>> if (!madvise_behavior_valid(behavior))
>> return error;
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 01600d80ae01..78e0a88b2680 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned long len,
>> int err;
>> unsigned short mode_flags;
>>
>> + start = untagged_addr(start);
>> mode_flags = mode & MPOL_MODE_FLAGS;
>> mode &= ~MPOL_MODE_FLAGS;
>> if (mode >= MPOL_MAX)
>> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
>> int uninitialized_var(pval);
>> nodemask_t nodes;
>>
>> + addr = untagged_addr(addr);
>> +
>> if (nmask != NULL && maxnode < nr_node_ids)
>> return -EINVAL;
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index f2ecc2855a12..d22c45cf36b2 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>> goto out_flush;
>> if (get_user(node, nodes + i))
>> goto out_flush;
>> - addr = (unsigned long)p;
>> + addr = (unsigned long)untagged_addr(p);
>>
>> err = -ENODEV;
>> if (node < 0 || node >= MAX_NUMNODES)
>> diff --git a/mm/mincore.c b/mm/mincore.c
>> index c3f058bd0faf..64c322ed845c 100644
>> --- a/mm/mincore.c
>> +++ b/mm/mincore.c
>> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
>> unsigned long pages;
>> unsigned char *tmp;
>>
>> + start = untagged_addr(start);
>> +
>> /* Check the start address: needs to be page-aligned.. */
>> if (start & ~PAGE_MASK)
>> return -EINVAL;fixup_user_fault
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 080f3b36415b..e82609eaa428 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>> unsigned long lock_limit;
>> int error = -ENOMEM;
>>
>> + start = untagged_addr(start);
>> +
>> if (!can_do_mlock())
>> return -EPERM;
>>
>> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
>> {
>> int ret;
>>
>> + start = untagged_addr(start);
>> +
>> len = PAGE_ALIGN(len + (offset_in_page(start)));
>> start &= PAGE_MASK;
>>
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index bf38dfbbb4b4..19f981b733bc 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
>> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
>> (prot & PROT_READ);
>>
>> + start = untagged_addr(start);
>> +
>> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
>> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
>> return -EINVAL;
>> diff --git a/mm/mremap.c b/mm/mremap.c
>> index fc241d23cd97..64c9a3b8be0a 100644
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>> LIST_HEAD(uf_unmap_early);
>> LIST_HEAD(uf_unmap);
>>
>> + /*
>> + * Architectures may interpret the tag passed to mmap as a background
>> + * colour for the corresponding vma. For mremap we don't allow tagged
>> + * new_addr to preserve similar behaviour to mmap.
>> + */
>> + addr = untagged_addr(addr);
>> +
>> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
>> return ret;
>>
>> diff --git a/mm/msync.c b/mm/msync.c
>> index ef30a429623a..c3bd3e75f687 100644
>> --- a/mm/msync.c
>> +++ b/mm/msync.c
>> @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
>> int unmapped_error = 0;
>> int error = -EINVAL;
>>
>> + start = untagged_addr(start);
>> +
>> if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
>> goto out;
>> if (offset_in_page(start))
>>
>
>
^ permalink raw reply
* Re: [PATCH v17 05/15] mm, arm64: untag user pointers in mm/gup.c
From: Khalid Aziz @ 2019-06-19 16:41 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <8f65548bef8544d49980a92d221b74440d544c1e.1560339705.git.andreyknvl@google.com>
On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> mm/gup.c provides a kernel interface that accepts user addresses and
> manipulates user pages directly (for example get_user_pages, that is used
> by the futex syscall). Since a user can provided tagged addresses, we need
> to handle this case.
>
> Add untagging to gup.c functions that use user addresses for vma lookups.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> mm/gup.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index ddde097cf9e4..c37df3d455a2 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -802,6 +802,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> if (!nr_pages)
> return 0;
>
> + start = untagged_addr(start);
> +
> VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
>
> /*
> @@ -964,6 +966,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
> struct vm_area_struct *vma;
> vm_fault_t ret, major = 0;
>
> + address = untagged_addr(address);
> +
> if (unlocked)
> fault_flags |= FAULT_FLAG_ALLOW_RETRY;
>
>
^ permalink raw reply
* Re: use exact allocation for dma coherent memory
From: Jason Gunthorpe @ 2019-06-19 16:29 UTC (permalink / raw)
To: Christoph Hellwig, Potnuri Bharat Teja
Cc: Dan Carpenter, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Ian Abbott, H Hartley Sweeten, devel, linux-s390,
Intel Linux Wireless, linux-rdma, netdev, intel-gfx,
linux-wireless, linux-kernel, dri-devel, linux-mm, iommu
In-Reply-To: <20190617083342.GA7883@lst.de>
On Mon, Jun 17, 2019 at 10:33:42AM +0200, Christoph Hellwig wrote:
> > drivers/infiniband/hw/cxgb4/qp.c
> > 129 static int alloc_host_sq(struct c4iw_rdev *rdev, struct t4_sq *sq)
> > 130 {
> > 131 sq->queue = dma_alloc_coherent(&(rdev->lldi.pdev->dev), sq->memsize,
> > 132 &(sq->dma_addr), GFP_KERNEL);
> > 133 if (!sq->queue)
> > 134 return -ENOMEM;
> > 135 sq->phys_addr = virt_to_phys(sq->queue);
> > 136 dma_unmap_addr_set(sq, mapping, sq->dma_addr);
> > 137 return 0;
> > 138 }
> >
> > Is this a bug?
>
> Yes. This will blow up badly on many platforms, as sq->queue
> might be vmapped, ioremapped, come from a pool without page backing.
Gah, this addr gets fed into io_remap_pfn_range/remap_pfn_range too..
Potnuri, you should fix this..
You probably need to use dma_mmap_from_dev_coherent() in the mmap ?
Jason
^ permalink raw reply
* Re: [PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls
From: Khalid Aziz @ 2019-06-19 15:55 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <f9b50767d639b7116aa986dc67f158131b8d4169.1560339705.git.andreyknvl@google.com>
On 6/12/19 5:43 AM, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch allows tagged pointers to be passed to the following memory
> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
> mremap, msync, munlock, move_pages.
>
> The mmap and mremap syscalls do not currently accept tagged addresses.
> Architectures may interpret the tag as a background colour for the
> corresponding vma.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
> mm/madvise.c | 2 ++
> mm/mempolicy.c | 3 +++
> mm/migrate.c | 2 +-
> mm/mincore.c | 2 ++
> mm/mlock.c | 4 ++++
> mm/mprotect.c | 2 ++
> mm/mremap.c | 7 +++++++
> mm/msync.c | 2 ++
> 8 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 628022e674a7..39b82f8a698f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> size_t len;
> struct blk_plug plug;
>
> + start = untagged_addr(start);
> +
> if (!madvise_behavior_valid(behavior))
> return error;
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d80ae01..78e0a88b2680 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned long len,
> int err;
> unsigned short mode_flags;
>
> + start = untagged_addr(start);
> mode_flags = mode & MPOL_MODE_FLAGS;
> mode &= ~MPOL_MODE_FLAGS;
> if (mode >= MPOL_MAX)
> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
> int uninitialized_var(pval);
> nodemask_t nodes;
>
> + addr = untagged_addr(addr);
> +
> if (nmask != NULL && maxnode < nr_node_ids)
> return -EINVAL;
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index f2ecc2855a12..d22c45cf36b2 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1616,7 +1616,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> goto out_flush;
> if (get_user(node, nodes + i))
> goto out_flush;
> - addr = (unsigned long)p;
> + addr = (unsigned long)untagged_addr(p);
>
> err = -ENODEV;
> if (node < 0 || node >= MAX_NUMNODES)
> diff --git a/mm/mincore.c b/mm/mincore.c
> index c3f058bd0faf..64c322ed845c 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
> unsigned long pages;
> unsigned char *tmp;
>
> + start = untagged_addr(start);
> +
> /* Check the start address: needs to be page-aligned.. */
> if (start & ~PAGE_MASK)
> return -EINVAL;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 080f3b36415b..e82609eaa428 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
> unsigned long lock_limit;
> int error = -ENOMEM;
>
> + start = untagged_addr(start);
> +
> if (!can_do_mlock())
> return -EPERM;
>
> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
> {
> int ret;
>
> + start = untagged_addr(start);
> +
> len = PAGE_ALIGN(len + (offset_in_page(start)));
> start &= PAGE_MASK;
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index bf38dfbbb4b4..19f981b733bc 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
> (prot & PROT_READ);
>
> + start = untagged_addr(start);
> +
> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
> return -EINVAL;
> diff --git a/mm/mremap.c b/mm/mremap.c
> index fc241d23cd97..64c9a3b8be0a 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -606,6 +606,13 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> LIST_HEAD(uf_unmap_early);
> LIST_HEAD(uf_unmap);
>
> + /*
> + * Architectures may interpret the tag passed to mmap as a background
> + * colour for the corresponding vma. For mremap we don't allow tagged
> + * new_addr to preserve similar behaviour to mmap.
> + */
> + addr = untagged_addr(addr);
> +
> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> return ret;
>
> diff --git a/mm/msync.c b/mm/msync.c
> index ef30a429623a..c3bd3e75f687 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
> int unmapped_error = 0;
> int error = -EINVAL;
>
> + start = untagged_addr(start);
> +
> if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> goto out;
> if (offset_in_page(start))
>
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Catalin Marinas @ 2019-06-19 15:29 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Linux ARM, Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Kees Cook, Yishai Hadas, Felix Kuehling,
Alexander Deucher, Christian Koenig <Christian.Koenig@
In-Reply-To: <CAAeHK+xvtqALY9DESF048mR17Po=W++QwWOUOOeSXKgriVTC-w@mail.gmail.com>
On Wed, Jun 19, 2019 at 04:45:02PM +0200, Andrey Konovalov wrote:
> On Wed, Jun 12, 2019 at 1:43 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > From: Catalin Marinas <catalin.marinas@arm.com>
> >
> > It is not desirable to relax the ABI to allow tagged user addresses into
> > the kernel indiscriminately. This patch introduces a prctl() interface
> > for enabling or disabling the tagged ABI with a global sysctl control
> > for preventing applications from enabling the relaxed ABI (meant for
> > testing user-space prctl() return error checking without reconfiguring
> > the kernel). The ABI properties are inherited by threads of the same
> > application and fork()'ed children but cleared on execve().
> >
> > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> > MTE-specific settings like imprecise vs precise exceptions.
> >
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Catalin, would you like to do the requested changes to this patch
> yourself and send it to me or should I do that?
I'll send you an updated version this week.
--
Catalin
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Andrey Konovalov @ 2019-06-19 14:45 UTC (permalink / raw)
To: Linux ARM, Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Khalid Aziz, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Christoph Hellwig,
Jason Gunthorpe, Dave Martin, Evgeniy Stepanov, Kevin Brodsky,
Kees Cook, Ruben Ayrapetyan, Ramana Radhakrishnan,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman
In-Reply-To: <a7a2933bea5fe57e504891b7eec7e9432e5e1c1a.1560339705.git.andreyknvl@google.com>
On Wed, Jun 12, 2019 at 1:43 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> From: Catalin Marinas <catalin.marinas@arm.com>
>
> It is not desirable to relax the ABI to allow tagged user addresses into
> the kernel indiscriminately. This patch introduces a prctl() interface
> for enabling or disabling the tagged ABI with a global sysctl control
> for preventing applications from enabling the relaxed ABI (meant for
> testing user-space prctl() return error checking without reconfiguring
> the kernel). The ABI properties are inherited by threads of the same
> application and fork()'ed children but cleared on execve().
>
> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> MTE-specific settings like imprecise vs precise exceptions.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Catalin, would you like to do the requested changes to this patch
yourself and send it to me or should I do that?
^ permalink raw reply
* Re: [PATCH v17 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
From: Andrey Konovalov @ 2019-06-19 14:42 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm@vger.kernel.org, Szabolcs Nagy, Will Deacon,
dri-devel@lists.freedesktop.org, linux-mm@kvack.org, Khalid Aziz,
linux-kselftest@vger.kernel.org, Felix Kuehling,
Vincenzo Frascino, Mauro Carvalho Chehab, Jacob Bramley,
Leon Romanovsky, linux-rdma@vger.kernel.org,
amd-gfx@lists.freedesktop.org, Christoph Hellwig, Jason Gunthorpe,
Dmitry Vyukov, Dave P Martin, Evgeniy Stepanov
In-Reply-To: <20190612150040.GH28951@C02TF0J2HF1T.local>
On Wed, Jun 12, 2019 at 5:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, Jun 12, 2019 at 01:30:36PM +0100, Szabolcs Nagy wrote:
> > On 12/06/2019 12:43, Andrey Konovalov wrote:
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/arm64/tags_lib.c
> > > @@ -0,0 +1,62 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <stdlib.h>
> > > +#include <sys/prctl.h>
> > > +
> > > +#define TAG_SHIFT (56)
> > > +#define TAG_MASK (0xffUL << TAG_SHIFT)
> > > +
> > > +#define PR_SET_TAGGED_ADDR_CTRL 55
> > > +#define PR_GET_TAGGED_ADDR_CTRL 56
> > > +#define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> > > +
> > > +void *__libc_malloc(size_t size);
> > > +void __libc_free(void *ptr);
> > > +void *__libc_realloc(void *ptr, size_t size);
> > > +void *__libc_calloc(size_t nmemb, size_t size);
> >
> > this does not work on at least musl.
> >
> > the most robust solution would be to implement
> > the malloc apis with mmap/munmap/mremap, if that's
> > too cumbersome then use dlsym RTLD_NEXT (although
> > that has the slight wart that in glibc it may call
> > calloc so wrapping calloc that way is tricky).
> >
> > in simple linux tests i'd just use static or
> > stack allocations or mmap.
> >
> > if a generic preloadable lib solution is needed
> > then do it properly with pthread_once to avoid
> > races etc.
>
> Thanks for the feedback Szabolcs. I guess we can go back to the initial
> simple test that Andrey had and drop the whole LD_PRELOAD hack (I'll
> just use it for my internal testing).
OK, will do in v18.
>
> BTW, when you get some time, please review Vincenzo's ABI documentation
> patches from a user/libc perspective. Once agreed, they should become
> part of this series.
>
> --
> Catalin
^ permalink raw reply
* [PATCH iproute2 v2 2/2] uapi: update if_link.h
From: Denis Kirjanov @ 2019-06-19 14:14 UTC (permalink / raw)
To: stephen; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
In-Reply-To: <20190619141414.4242-1-dkirjanov@suse.com>
update if_link.h to commit 75345f888f700c4ab2448287e35d48c760b202e6
("ipoib: show VF broadcast address")
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
include/uapi/linux/if_link.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index bfe7f9e6..5f271d84 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -692,6 +692,7 @@ enum {
IFLA_VF_IB_NODE_GUID, /* VF Infiniband node GUID */
IFLA_VF_IB_PORT_GUID, /* VF Infiniband port GUID */
IFLA_VF_VLAN_LIST, /* nested list of vlans, option for QinQ */
+ IFLA_VF_BROADCAST, /* VF broadcast */
__IFLA_VF_MAX,
};
@@ -702,6 +703,10 @@ struct ifla_vf_mac {
__u8 mac[32]; /* MAX_ADDR_LEN */
};
+struct ifla_vf_broadcast {
+ __u8 broadcast[32];
+};
+
struct ifla_vf_vlan {
__u32 vf;
__u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
--
2.12.3
^ permalink raw reply related
* [PATCH iproute2 v2 1/2] ipaddress: correctly print a VF hw address in the IPoIB case
From: Denis Kirjanov @ 2019-06-19 14:14 UTC (permalink / raw)
To: stephen; +Cc: netdev, linux-rdma, dledford, mkubecek, Denis Kirjanov
Current code assumes that we print Etheret mac and
that doesn't work in IPoIB case with SRIOV-enabled hardware
Before:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
trust off, query_rss off
...
After:
11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
state UP mode DEFAULT group default qlen 256
link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
vf 0 link/infiniband
80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
checking off, link-state disable, trust off, query_rss off
v1->v2: updated kernel headers to uapi commit
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
ip/ipaddress.c | 42 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 5 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index b504200b..13ad76dd 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -26,6 +26,7 @@
#include <linux/netdevice.h>
#include <linux/if_arp.h>
+#include <linux/if_infiniband.h>
#include <linux/sockios.h>
#include <linux/net_namespace.h>
@@ -349,9 +350,10 @@ static void print_af_spec(FILE *fp, struct rtattr *af_spec_attr)
static void print_vf_stats64(FILE *fp, struct rtattr *vfstats);
-static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
+static void print_vfinfo(struct ifinfomsg *ifi, FILE *fp, struct rtattr *vfinfo)
{
struct ifla_vf_mac *vf_mac;
+ struct ifla_vf_broadcast *vf_broadcast;
struct ifla_vf_tx_rate *vf_tx_rate;
struct rtattr *vf[IFLA_VF_MAX + 1] = {};
@@ -365,13 +367,43 @@ static void print_vfinfo(FILE *fp, struct rtattr *vfinfo)
parse_rtattr_nested(vf, IFLA_VF_MAX, vfinfo);
vf_mac = RTA_DATA(vf[IFLA_VF_MAC]);
+ vf_broadcast = RTA_DATA(vf[IFLA_VF_BROADCAST]);
vf_tx_rate = RTA_DATA(vf[IFLA_VF_TX_RATE]);
print_string(PRINT_FP, NULL, "%s ", _SL_);
print_int(PRINT_ANY, "vf", "vf %d ", vf_mac->vf);
- print_string(PRINT_ANY, "mac", "MAC %s",
- ll_addr_n2a((unsigned char *) &vf_mac->mac,
- ETH_ALEN, 0, b1, sizeof(b1)));
+
+ print_string(PRINT_ANY,
+ "link_type",
+ " link/%s ",
+ ll_type_n2a(ifi->ifi_type, b1, sizeof(b1)));
+
+ print_color_string(PRINT_ANY,
+ COLOR_MAC,
+ "address",
+ "%s",
+ ll_addr_n2a((unsigned char *) &vf_mac->mac,
+ ifi->ifi_type == ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+
+ if (vf[IFLA_VF_BROADCAST]) {
+ if (ifi->ifi_flags&IFF_POINTOPOINT) {
+ print_string(PRINT_FP, NULL, " peer ", NULL);
+ print_bool(PRINT_JSON,
+ "link_pointtopoint", NULL, true);
+ } else {
+ print_string(PRINT_FP, NULL, " brd ", NULL);
+ }
+ print_color_string(PRINT_ANY,
+ COLOR_MAC,
+ "broadcast",
+ "%s",
+ ll_addr_n2a((unsigned char *) &vf_broadcast->broadcast,
+ ifi->ifi_type == ARPHRD_ETHER ? ETH_ALEN : INFINIBAND_ALEN,
+ ifi->ifi_type,
+ b1, sizeof(b1)));
+ }
if (vf[IFLA_VF_VLAN_LIST]) {
struct rtattr *i, *vfvlanlist = vf[IFLA_VF_VLAN_LIST];
@@ -1102,7 +1134,7 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
open_json_array(PRINT_JSON, "vfinfo_list");
for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
open_json_object(NULL);
- print_vfinfo(fp, i);
+ print_vfinfo(ifi, fp, i);
close_json_object();
}
close_json_array(PRINT_JSON, NULL);
--
2.12.3
^ permalink raw reply related
* Re: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport number in VF vports and uplink ingress ACLs
From: Jianbo Liu @ 2019-06-19 12:52 UTC (permalink / raw)
To: Parav Pandit
Cc: Saeed Mahameed, Leon Romanovsky, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Eli Britstein, Roi Dayan, Mark Bloch
In-Reply-To: <AM0PR05MB48664868E0B89E582807830BD1EA0@AM0PR05MB4866.eurprd05.prod.outlook.com>
The 06/18/2019 18:31, Parav Pandit wrote:
>
>
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> > Behalf Of Saeed Mahameed
> > Sent: Tuesday, June 18, 2019 12:53 AM
> > To: Saeed Mahameed <saeedm@mellanox.com>; Leon Romanovsky
> > <leonro@mellanox.com>
> > Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Jianbo Liu
> > <jianbol@mellanox.com>; Eli Britstein <elibr@mellanox.com>; Roi Dayan
> > <roid@mellanox.com>; Mark Bloch <markb@mellanox.com>
> > Subject: [PATCH mlx5-next 05/15] net/mlx5: E-Switch, Tag packet with vport
> > number in VF vports and uplink ingress ACLs
> >
> > From: Jianbo Liu <jianbol@mellanox.com>
> >
> > When a dual-port VHCA sends a RoCE packet on its non-native port, and the
> > packet arrives to its affiliated vport FDB, a mismatch might occur on the rules
> > that match the packet source vport as it is not represented by single VHCA only
> > in this case. So we change to match on metadata instead of source vport.
> > To do that, a rule is created in all vports and uplink ingress ACLs, to save the
> > source vport number and vhca id in the packet's metadata in order to match on
> > it later.
> > The metadata register used is the first of the 32-bit type C registers. It can be
> > used for matching and header modify operations. The higher 16 bits of this
> > register are for vhca id, and the lower 16 ones is for vport number.
> > This change is not for dual-port RoCE only. If HW and FW allow, the vport
> > metadata matching is enabled by default.
> >
> > Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> > Reviewed-by: Eli Britstein <elibr@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> > Reviewed-by: Mark Bloch <markb@mellanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/eswitch.c | 2 +
> > .../net/ethernet/mellanox/mlx5/core/eswitch.h | 9 +
> > .../mellanox/mlx5/core/eswitch_offloads.c | 183 ++++++++++++++----
> > include/linux/mlx5/eswitch.h | 3 +
> > 4 files changed, 161 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > index a42a23e505df..1235fd84ae3a 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
...
> > + err = esw_vport_egress_prio_tag_config(esw, vport);
> > + if (err)
> > + goto err_egress;
> > + }
> > }
> >
> > + if (mlx5_eswitch_vport_match_metadata_enabled(esw))
> > + esw_info(esw->dev, "Use metadata reg_c as source vport to
> > match\n");
> > +
> > return 0;
> >
> > err_egress:
> > esw_vport_disable_ingress_acl(esw, vport);
> > err_ingress:
> > - mlx5_esw_for_each_vf_vport_reverse(esw, j, vport, i - 1) {
> > + for (j = MLX5_VPORT_PF; j < i; j++) {
> Keep the reverse order as before.
The vports are independent from each other. It doesn't matter disabling
them in or out of order. I don't understand what's the benifit.
>
> > + vport = &esw->vports[j];
> > esw_vport_disable_egress_acl(esw, vport);
> > esw_vport_disable_ingress_acl(esw, vport);
> > }
> > @@ -1704,15 +1800,17 @@ static int esw_prio_tag_acls_config(struct
> > mlx5_eswitch *esw, int nvports)
> > return err;
> > }
> >
> > -static void esw_prio_tag_acls_cleanup(struct mlx5_eswitch *esw)
> > +static void esw_destroy_offloads_acl_tables(struct mlx5_eswitch *esw)
> > {
> > struct mlx5_vport *vport;
> > int i;
> >
> > - mlx5_esw_for_each_vf_vport(esw, i, vport, esw->nvports) {
> > + mlx5_esw_for_all_vports(esw, i, vport) {
> If you are changing this, please do in reverse order to keep it exact mirror of create/enable sequence.
Same...
>
> > esw_vport_disable_egress_acl(esw, vport);
> > esw_vport_disable_ingress_acl(esw, vport);
> > }
> > +
> > + esw->flags &= ~MLX5_ESWITCH_VPORT_MATCH_METADATA;
> > }
> >
> > static int esw_offloads_steering_init(struct mlx5_eswitch *esw, int nvports)
^ permalink raw reply
* Re: [PATCH v3 hmm 11/12] mm/hmm: Remove confusing comment and logic from hmm_release
From: Christoph Hellwig @ 2019-06-19 12:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Yang, Philip, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, John Hubbard,
Kuehling, Felix,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Ben Skeggs
In-Reply-To: <20190619115632.GC9360-uk2M96/98Pc@public.gmane.org>
On Wed, Jun 19, 2019 at 08:56:32AM -0300, Jason Gunthorpe wrote:
> This looks a lot like the ODP code (amdgpu_mn_node == ib_umem_odp)
>
> The interval tree is to quickly find the driver object(s) that have
> the virtual pages during invalidation:
>
> static int amdgpu_mn_sync_pagetables_gfx(struct hmm_mirror *mirror,
> const struct hmm_update *update)
> {
> it = interval_tree_iter_first(&amn->objects, start, end);
> while (it) {
> [..]
> amdgpu_mn_invalidate_node(node, start, end);
>
> And following the ODP model there should be a single hmm_mirror per-mm
> (user can fork and stuff, this is something I want to have core code
> help with).
That makes the hmm_mirror object pretty silly, though as the scope
is then exactly the same as the mmu_notifier itself.
> The hmm_mirror can either exist so long as objects exist, or it can
> exist until the chardev is closed - but never longer than the
> chardev's lifetime.
>
> Maybe we should be considering providing a mmu notifier & interval
> tree & lock abstraction since ODP & AMD are very similar here..
It defintively sounds like a good idea to move this kind of object
management into common code. Nouvea actually seems like the odd one
out here by not having a list of objects below the mirror, but then
again and interval tree with a single entry wouldn't really hurt it
either.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v3 hmm 11/12] mm/hmm: Remove confusing comment and logic from hmm_release
From: Jason Gunthorpe @ 2019-06-19 11:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrea Arcangeli, Yang, Philip, Ralph Campbell,
linux-rdma@vger.kernel.org, John Hubbard, Kuehling, Felix,
dri-devel@lists.freedesktop.org, linux-mm@kvack.org,
Jerome Glisse, amd-gfx@lists.freedesktop.org, Ben Skeggs
In-Reply-To: <20190619080705.GA5164@infradead.org>
On Wed, Jun 19, 2019 at 01:07:05AM -0700, Christoph Hellwig wrote:
> On Wed, Jun 19, 2019 at 12:53:55AM +0000, Kuehling, Felix wrote:
> > This code is derived from our old MMU notifier code. Before HMM we used
> > to register a single MMU notifier per mm_struct and look up virtual
> > address ranges that had been registered for mirroring via driver API
> > calls. The idea was to reuse a single MMU notifier for the life time of
> > the process. It would remain registered until we got a notifier_release.
> >
> > hmm_mirror took the place of that when we converted the code to HMM.
> >
> > I suppose we could destroy the mirror earlier, when we have no more
> > registered virtual address ranges, and create a new one if needed later.
>
> I didn't write the code, but if you look at hmm_mirror it already is
> a multiplexer over the mmu notifier, and the intent clearly seems that
> you register one per range that you want to mirror, and not multiplex
> it once again. In other words - I think each amdgpu_mn_node should
> probably have its own hmm_mirror. And while the amdgpu_mn_node objects
> are currently stored in an interval tree it seems like they are only
> linearly iterated anyway, so a list actually seems pretty suitable. If
> not we need to improve the core data structures instead of working
> around them.
This looks a lot like the ODP code (amdgpu_mn_node == ib_umem_odp)
The interval tree is to quickly find the driver object(s) that have
the virtual pages during invalidation:
static int amdgpu_mn_sync_pagetables_gfx(struct hmm_mirror *mirror,
const struct hmm_update *update)
{
it = interval_tree_iter_first(&amn->objects, start, end);
while (it) {
[..]
amdgpu_mn_invalidate_node(node, start, end);
And following the ODP model there should be a single hmm_mirror per-mm
(user can fork and stuff, this is something I want to have core code
help with).
The hmm_mirror can either exist so long as objects exist, or it can
exist until the chardev is closed - but never longer than the
chardev's lifetime.
Maybe we should be considering providing a mmu notifier & interval
tree & lock abstraction since ODP & AMD are very similar here..
Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 hmm 06/12] mm/hmm: Hold on to the mmget for the lifetime of the range
From: Christoph Hellwig @ 2019-06-19 11:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ben Skeggs
In-Reply-To: <20190619113452.GB9360-uk2M96/98Pc@public.gmane.org>
On Wed, Jun 19, 2019 at 08:34:52AM -0300, Jason Gunthorpe wrote:
> /**
> * list_empty_careful - tests whether a list is empty and not being modified
> * @head: the list to test
> *
> * Description:
> * tests whether a list is empty _and_ checks that no other CPU might be
> * in the process of modifying either member (next or prev)
> *
> * NOTE: using list_empty_careful() without synchronization
> * can only be safe if the only activity that can happen
> * to the list entry is list_del_init(). Eg. it cannot be used
> * if another CPU could re-list_add() it.
> */
>
> Agree it doesn't seem obvious why this is relevant when checking the
> list head..
>
> Maybe the comment is a bit misleading?
From looking at the commit log in the history tree list_empty_careful
was initially added by Linus, and then mingo added that comment later.
I don't see how list_del_init would change anything here, so I suspect
list_del_init was just used as a short hand for list_del or
list_del_init.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v3 hmm 06/12] mm/hmm: Hold on to the mmget for the lifetime of the range
From: Jason Gunthorpe @ 2019-06-19 11:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell, linux-rdma,
John Hubbard, Felix.Kuehling, dri-devel, linux-mm, Jerome Glisse,
amd-gfx, Ben Skeggs
In-Reply-To: <20190619081858.GB24900@infradead.org>
On Wed, Jun 19, 2019 at 01:18:58AM -0700, Christoph Hellwig wrote:
> > mutex_lock(&hmm->lock);
> > - list_del(&range->list);
> > + list_del_init(&range->list);
> > mutex_unlock(&hmm->lock);
>
> I don't see the point why this is a list_del_init - that just
> reinitializeѕ range->list, but doesn't change anything for the list
> head it was removed from. (and if the list_del_init was intended
> a later patch in your branch reverts it to plain list_del..)
Just following the instructions:
/**
* list_empty_careful - tests whether a list is empty and not being modified
* @head: the list to test
*
* Description:
* tests whether a list is empty _and_ checks that no other CPU might be
* in the process of modifying either member (next or prev)
*
* NOTE: using list_empty_careful() without synchronization
* can only be safe if the only activity that can happen
* to the list entry is list_del_init(). Eg. it cannot be used
* if another CPU could re-list_add() it.
*/
Agree it doesn't seem obvious why this is relevant when checking the
list head..
Maybe the comment is a bit misleading?
Jason
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v3 hmm 08/12] mm/hmm: Remove racy protection against double-unregistration
From: Christoph Hellwig @ 2019-06-19 8:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ben Skeggs
In-Reply-To: <20190618185757.GP6961-uk2M96/98Pc@public.gmane.org>
On Tue, Jun 18, 2019 at 03:57:57PM -0300, Jason Gunthorpe wrote:
> With the previous loose coupling of the mirror and the range some code
> might rance to try to create a range without a mirror, which will now
> reliably crash with the poison.
>
> It isn't so much the double unregister that worries me, but racing
> unregister with range functions.
Oh well. It was just a nitpick for the highly unusual code patterns
in the two unregister routines, probably not worth fighting over even
if I still don't see the point.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v3 hmm 06/12] mm/hmm: Hold on to the mmget for the lifetime of the range
From: Christoph Hellwig @ 2019-06-19 8:18 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ben Skeggs
In-Reply-To: <20190618151100.GI6961-uk2M96/98Pc@public.gmane.org>
> mutex_lock(&hmm->lock);
> - list_del(&range->list);
> + list_del_init(&range->list);
> mutex_unlock(&hmm->lock);
I don't see the point why this is a list_del_init - that just
reinitializeѕ range->list, but doesn't change anything for the list
head it was removed from. (and if the list_del_init was intended
a later patch in your branch reverts it to plain list_del..)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v3 hmm 02/12] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
From: Christoph Hellwig @ 2019-06-19 8:14 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrea Arcangeli, Philip Yang, Ralph Campbell,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Christoph Hellwig,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Ira Weiny, Ben Skeggs
In-Reply-To: <20190618130544.GC6961-uk2M96/98Pc@public.gmane.org>
On Tue, Jun 18, 2019 at 10:05:44AM -0300, Jason Gunthorpe wrote:
> I'm thinking to tackle that as part of the mmu notififer invlock
> idea.. Once the range looses the lock then we don't really need to
> register it at all.
Ok.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH mlx5-next 11/15] RDMA/mlx5: Add vport metadata matching for IB representors
From: Leon Romanovsky @ 2019-06-19 8:12 UTC (permalink / raw)
To: Mark Bloch
Cc: Jianbo Liu, Saeed Mahameed, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Roi Dayan
In-Reply-To: <ac23c3ea-3ea7-2a5b-5fc6-aece0aed0b54@mellanox.com>
On Wed, Jun 19, 2019 at 07:58:51AM +0000, Mark Bloch wrote:
>
>
> On 6/19/2019 00:43, Leon Romanovsky wrote:
> > On Wed, Jun 19, 2019 at 07:26:54AM +0000, Mark Bloch wrote:
> >>
> >>
> >> On 6/18/2019 23:51, Leon Romanovsky wrote:
> >>> On Wed, Jun 19, 2019 at 06:40:16AM +0000, Jianbo Liu wrote:
> >>>> The 06/19/2019 13:04, Leon Romanovsky wrote:
> >>>>> On Wed, Jun 19, 2019 at 04:44:26AM +0000, Jianbo Liu wrote:
> >>>>>> The 06/18/2019 18:19, Leon Romanovsky wrote:
> >>>>>>> On Mon, Jun 17, 2019 at 07:23:30PM +0000, Saeed Mahameed wrote:
> >>>>>>>> From: Jianbo Liu <jianbol@mellanox.com>
> >>>>>>>>
> >>>>>>>> If vport metadata matching is enabled in eswitch, the rule created
> >>>>>>>> must be changed to match on the metadata, instead of source port.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jianbo Liu <jianbol@mellanox.com>
> >>>>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
> >>>>>>>> Reviewed-by: Mark Bloch <markb@mellanox.com>
> >>>>>>>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> >>>>>>>> ---
> >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.c | 11 +++++++
> >>>>>>>> drivers/infiniband/hw/mlx5/ib_rep.h | 16 ++++++++++
> >>>>>>>> drivers/infiniband/hw/mlx5/main.c | 45 +++++++++++++++++++++++------
> >>>>>>>> 3 files changed, 63 insertions(+), 9 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/infiniband/hw/mlx5/ib_rep.c b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> index 22e651cb5534..d4ed611de35d 100644
> >>>>>>>> --- a/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> +++ b/drivers/infiniband/hw/mlx5/ib_rep.c
> >>>>>>>> @@ -131,6 +131,17 @@ struct mlx5_eswitch_rep *mlx5_ib_vport_rep(struct mlx5_eswitch *esw, int vport)
> >>>>>>>> return mlx5_eswitch_vport_rep(esw, vport);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> +u32 mlx5_ib_eswitch_vport_match_metadata_enabled(struct mlx5_eswitch *esw)
> >>>>>>>> +{
> >>>>>>>> + return mlx5_eswitch_vport_match_metadata_enabled(esw);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +u32 mlx5_ib_eswitch_get_vport_metadata_for_match(struct mlx5_eswitch *esw,
> >>>>>>>> + u16 vport)
> >>>>>>>> +{
> >>>>>>>> + return mlx5_eswitch_get_vport_metadata_for_match(esw, vport);
> >>>>>>>> +}
> >>>>>>>
> >>>>>>> 1. There is no need to introduce one line functions, call to that code directly.
> >>>>>>
> >>>>>> No. They are in IB, and we don't want them be mixed up by the original
> >>>>>> functions in eswitch. Please ask Mark more about it.
> >>>>>
> >>>>> Please enlighten me.
> >>>>
> >>>> It was suggested by Mark in prevouis review.
> >>>> I think it's because there are in different modules, and better to with
> >>>> different names, so introduce there extra one line functions.
> >>>> Please correct me if I'm wrong, Mark...
> >>>
> >>> mlx5_ib is full of direct function calls to mlx5_core and it is done on
> >>> purpose for at least two reasons. First is to control in one place
> >>> all compilation options and expose proper API interface with and without
> >>> specific kernel config is on. Second is to emphasize that this is core
> >>> function and save us time in refactoring and reviewing.
> >>
> >> This was done in order to avoid #ifdef CONFIG_MLX5_ESWITCH,
> >> I want to hide (as much as possible) the interactions with the eswitch level in ib_rep.c/ib_rep.h
> >> so ib_rep.h will provide the stubs needed in case CONFIG_MLX5_ESWITCH isn't defined.
> >> (Today include/linux/mlx5/eswitch.h) doesn't provide any stubs, mlx5_eswitch_get_encap_mode()
> >> should have probably done the same.
> >
> > This is exactly the problem, eswitch.h should provide stubs for all
> > exported functions, so other clients of eswitch won't need to deal with
> > various unrelated config options.
>
> The way it works today, code in drivers/infiniband/hw/mlx5/main.c doesn't call eswitch layer directly
> but the functions in ib_rep.{c,h} as most often there is additional logic we must do before calling
> the eswitch layer.
>
> If you look at drivers/infiniband/hw/mlx5/Makefile you will see ib_rep is complied only when
> CONFIG_MLX5_ESWITCH id defined.
This simple patch + cleanup of ib_rep.h will do the trick.
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 67b9e7ac569a..b917ba28659e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -59,7 +59,9 @@
#include <linux/in.h>
#include <linux/etherdevice.h>
#include "mlx5_ib.h"
+#if defined(CONFIG_MLX5_ESWITCH)
#include "ib_rep.h"
+#endif
#include "cmd.h"
#include "srq.h"
#include <linux/mlx5/fs_helpers.h>
@@ -6765,6 +6767,7 @@ static const struct mlx5_ib_profile pf_profile = {
mlx5_ib_stage_delay_drop_cleanup),
};
+#if defined(CONFIG_MLX5_ESWITCH)
const struct mlx5_ib_profile uplink_rep_profile = {
STAGE_CREATE(MLX5_IB_STAGE_INIT,
mlx5_ib_stage_init_init,
@@ -6812,6 +6815,7 @@
const struct mlx5_ib_profile uplink_rep_profile = {
mlx5_ib_stage_post_ib_reg_umr_init,
NULL),
};
>
> so instead of having to deal with two places that contain stubs, we need to deal with only one (ib_rep.h).
> For me it makes it easier to follow, but I can adept if you don't like it.
>
> Mark
>
> >
> >>
> >> As my long term goal is to break drivers/infiniband/hw/mlx5/main.c (that file is already 7000 LOC)
> >> I want to group together stuff in separate files.
> >
> > Yes, it is right thing to do.
> >
> >>
> >> If you prefer direct calls that's okay as well.
> >
> > Yes, please.
> >
> >>
> >> Mark
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> 2. It should be bool and not u32.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>
> >>>>>> --
> >>>>
> >>>> --
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox