diff mbox series

hw/nvme: fix invalid endian conversion

Message ID 20240222-fix-sriov-numcntl-v1-1-d60bea5e72d0@samsung.com
State New
Headers show
Series hw/nvme: fix invalid endian conversion | expand

Commit Message

Klaus Jensen Feb. 22, 2024, 9:29 a.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
hosts results in numcntl being set to 0.

Fix by dropping the endian conversion.

Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
change-id: 20240222-fix-sriov-numcntl-5ebe46a42176

Best regards,

Comments

Philippe Mathieu-Daudé Feb. 22, 2024, 10:08 a.m. UTC | #1 
Hi Klaus,

On 22/2/24 10:29, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
> hosts results in numcntl being set to 0.
> 
> Fix by dropping the endian conversion.
> 
> Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")

Isn't it commit 99f48ae7ae ("Add support for Secondary Controller
List") instead?

> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index f026245d1e9e..76fe0397045b 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -7924,7 +7924,7 @@ static void nvme_init_state(NvmeCtrl *n)
>       n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
>       QTAILQ_INIT(&n->aer_queue);
>   
> -    list->numcntl = cpu_to_le16(max_vfs);
> +    list->numcntl = max_vfs;
>       for (i = 0; i < max_vfs; i++) {
>           sctrl = &list->sec[i];
>           sctrl->pcid = cpu_to_le16(n->cntlid);
> 
> ---
> base-commit: 760b4dcdddba4a40b9fa0eb78fdfc7eda7cb83d0
> change-id: 20240222-fix-sriov-numcntl-5ebe46a42176
> 
> Best regards,
Klaus Jensen Feb. 22, 2024, 10:13 a.m. UTC | #2 
On Feb 22 11:08, Philippe Mathieu-Daudé wrote:
> Hi Klaus,
> 
> On 22/2/24 10:29, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
> > hosts results in numcntl being set to 0.
> > 
> > Fix by dropping the endian conversion.
> > 
> > Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")
> 
> Isn't it commit 99f48ae7ae ("Add support for Secondary Controller
> List") instead?
> 

Thanks Philippe,

Yes, that is the commit that had the bug originally. I'll fix it up.
Minwoo Im Feb. 24, 2024, 1:15 p.m. UTC | #3 
On 24-02-22 10:29:06, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
> hosts results in numcntl being set to 0.
> 
> Fix by dropping the endian conversion.
> 
> Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Reviewed-by: Minwoo Im <minwoo.im@samsung.com>

Thanks,
Philippe Mathieu-Daudé Feb. 26, 2024, 8:18 a.m. UTC | #4 
On 22/2/24 10:29, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
> hosts results in numcntl being set to 0.
> 
> Fix by dropping the endian conversion.
> 
> Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Hi Klaus, I'm not seeing other NVMe patches on the list,
so I'll queue this on my hw-misc tree, but feel free to
object and I'll unqueue :)

Thanks,

Phil.
Klaus Jensen Feb. 26, 2024, 8:42 a.m. UTC | #5 
On Feb 26 09:18, Philippe Mathieu-Daudé wrote:
> On 22/2/24 10:29, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > numcntl is one byte and so is max_vfs. Using cpu_to_le16 on big endian
> > hosts results in numcntl being set to 0.
> > 
> > Fix by dropping the endian conversion.
> > 
> > Fixes: 746d42b13368 ("hw/nvme: Initialize capability structures for primary/secondary controllers")
> > Reported-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Hi Klaus, I'm not seeing other NVMe patches on the list,
> so I'll queue this on my hw-misc tree, but feel free to
> object and I'll unqueue :)
> 
> Thanks,
> 

No, thats perfect! Thanks! :)
diff mbox series

Patch

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f026245d1e9e..76fe0397045b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -7924,7 +7924,7 @@  static void nvme_init_state(NvmeCtrl *n)
     n->aer_reqs = g_new0(NvmeRequest *, n->params.aerl + 1);
     QTAILQ_INIT(&n->aer_queue);
 
-    list->numcntl = cpu_to_le16(max_vfs);
+    list->numcntl = max_vfs;
     for (i = 0; i < max_vfs; i++) {
         sctrl = &list->sec[i];
         sctrl->pcid = cpu_to_le16(n->cntlid);