Re: [PATCH V2 2/9] iommufd: generalize iopt_pages address

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Steve,

Some nitpickings.

On Tue, Sep 24, 2024 at 08:05:31AM -0700, Steve Sistare wrote:
> +static int iopt_map_common(struct iommufd_ctx *ictx,
> +			     struct io_pagetable *iopt,
> +                          struct iopt_pages *pages,
> +                          unsigned long *iova,
> +                          unsigned long length,
> +                          unsigned long start_byte,
> +                          int iommu_prot,
> +                          unsigned int flags)

I think we should follow the existing coding style by combining
lines that could fit within 80 characters each.

>  /**
>   * iopt_map_user_pages() - Map a user VA to an iova in the io page table
>   * @ictx: iommufd_ctx the iopt is part of
> @@ -399,29 +431,15 @@ int iopt_map_user_pages(struct iommufd_ctx *ictx, struct io_pagetable *iopt,
>                         unsigned long length, int iommu_prot,
>                         unsigned int flags)
>  {
[...]
> -       return 0;
> +       return iopt_map_common(ictx, iopt, pages, iova, length,
> +                              uptr - pages->uptr,
> +                              iommu_prot, flags);

The last line could fit into the previous one.

> +enum iopt_address_type {
> +       IOPT_ADDRESS_USER = 0,
> +};
> +
>  /*
>   * This holds a pinned page list for multiple areas of IO address space. The
>   * pages always originate from a linear chunk of userspace VA. Multiple
> @@ -195,7 +199,10 @@ struct iopt_pages {
>         struct task_struct *source_task;
>         struct mm_struct *source_mm;
>         struct user_struct *source_user;
> -       void __user *uptr;
> +       enum iopt_address_type type;
> +       union {
> +               void __user *uptr;              /* IOPT_ADDRESS_USER */
> +       };
>         bool writable:1;
>         u8 account_mode;

We only have one line in the kdoc mentioning the "address" word.
So, I personally prefer "iopt_pages_type" and "IOPT_PAGES_USER",
matching with existing functions iopt_alloc/map_user_pages also.

> +struct iopt_pages *iopt_alloc_user_pages(void __user *uptr,
> +                                        unsigned long length, bool writable)
> +{
> +       struct iopt_pages *pages;
> +       unsigned long end;
> +       void __user *uptr_down = (void __user *) ALIGN_DOWN((uintptr_t)uptr, PAGE_SIZE);

+	void __user *uptr_aligned =
+		(void __user *) ALIGN_DOWN((uintptr_t)uptr, PAGE_SIZE);
+       struct iopt_pages *pages;
+       unsigned long end;

Thanks
Nicolin