Re: [PATCH v2 7/7] hw/nvme: cap mdts for CMB/PMR-only

[Jesper Wendel Devantier <foss@defmacro.it>]

On 2026-08-19T17:24:09+02:00, Daniel Gomez <da.gomez@kernel.org> wrote:
> From: Daniel Gomez <da.gomez@samsung.com>
>
> Commit 53493c1f83 ("hw/nvme: cap MDTS value for internal limitation")
> capped MDTS so the worst-case PRP count would fit in IOV_MAX, leaving
> transfers limited to 2 MiB.
>
> Now that dma_blk_cb() can batch IOs up to IOV_MAX instead of limiting
> to IOV_MAX, remove it, except for CMB/PMR-only where the limit still
> applies.
>
> In addition, fix UB when mdts >= 31 by dropping the shift and making the
> cap explicit. Fixes error with ubsan:
>     ../hw/nvme/ctrl.c:8638:33: runtime error: shift exponent 32 is too
>     large for 32-bit type 'int'
>
> Suggested-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Daniel Gomez <da.gomez@samsung.com>
> ---
>  hw/nvme/ctrl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 7861d8f2521..ff7e4a055b8 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -8802,8 +8802,10 @@ static bool nvme_check_params(NvmeCtrl *n, Error **errp)
>          host_memory_backend_set_mapped(n->pmr.dev, true);
>      }
>  
> -    if (!n->params.mdts || ((1 << n->params.mdts) + 1) > IOV_MAX) {
> -        error_setg(errp, "mdts exceeds IOV_MAX");
> +    /* 2^mdts + 1 must fit IOV_MAX */
> +    if ((n->params.cmb_size_mb || n->pmr.dev) &&
> +        (!n->params.mdts || (params->mdts > 9))) {
> +        error_setg(errp, "mdts=%u is incompatible with CMB/PMR", params->mdts);
>          return false;
>      }
>  
>
> -- 
> 2.55.0
>
>
>

I generally agree except for a single nit-pick, if you will permit me.

(params->mdts > 9) -- it seems a bit indirect and could break if,
for some reason, IOV_MAX is different.

 ```c
   static inline uint64_t nvme_mdts_max_iovs(uint8_t mdts)
   {
       return mdts >= 64 ? UINT64_MAX : (1ULL << mdts) + 1;
   }
 ```

 Then the check becomes
 ```c
   if ((params->cmb_size_mb || n->pmr.dev) &&
       (!params->mdts || nvme_mdts_max_iovs(params->mdts) > IOV_MAX)) {
       error_setg(errp, "mdts=%u is incompatible with CMB/PMR", params->mdts);
       return false;
   }
 ```

?