Re: [PATCH V5 4/6] hw/block/nvme: support for multi-controller in subsystem

From: Minwoo Im <minwoo.im.dev@gmail.com>
To: Keith Busch <kbusch@kernel.org>
Cc: Klaus Jensen <its@irrelevant.dk>, Kevin Wolf <kwolf@redhat.com>,
	qemu-devel@nongnu.org, qemu-block@nongnu.org,
	Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH V5 4/6] hw/block/nvme: support for multi-controller in subsystem
Date: Sat, 23 Jan 2021 10:02:27 +0900	[thread overview]
Message-ID: <20210123010227.GA2893@localhost.localdomain> (raw)
In-Reply-To: <20210122184236.GA1728586@dhcp-10-100-145-180.wdc.com>

On 21-01-22 10:42:36, Keith Busch wrote:
> On Fri, Jan 22, 2021 at 09:07:34PM +0900, Minwoo Im wrote:
> > index b525fca14103..3dedefb8ebba 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -4435,6 +4435,9 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> >      strpadcpy((char *)id->mn, sizeof(id->mn), "QEMU NVMe Ctrl", ' ');
> >      strpadcpy((char *)id->fr, sizeof(id->fr), "1.0", ' ');
> >      strpadcpy((char *)id->sn, sizeof(id->sn), n->params.serial, ' ');
> > +
> > +    id->cntlid = n->cntlid;
> 
> cpu_to_le16()? It might be okay to not do that since the only
> requirement is that this is a unique value, but it would be confusing
> for decoding commands that have a controller id field.

Agreed.

Yes, cntlids are allocated in unique values so that functionality has no
problem here.  But, even if so, we should make it have proper value in
Identify data structure with the policy it has to avoid confusing.

Thanks Keith! will fix it :)