Re: [Qemu-devel] [edk2] NVMe question

Re: [Qemu-devel] [edk2] NVMe question

[Laszlo Ersek]

On 06/02/15 09:19, Tian, Feng wrote:
> Thanks for the info.
> 
>  
> 
> I didn’t test it on NVMe-Qemu, I just run the change on a real
> production. I will do bigger scope test to see if it’s valuable to add
> it back.

This email thread is a perfect example why top-posting is a horrible
practice.

The NVMe device model was developed for QEMU by Intel's own Keith Busch
(added to the address list). Now Keith will have to read this email in
reverse. (A good portion of which consists of confidentiality notices,
posted to a public mailing list.) Good luck.

Keith, here's the thread, if it helps:
http://thread.gmane.org/gmane.comp.bios.tianocore.devel/15073

The issue seems to be that

https://github.com/tianocore/edk2/commit/d6c55989

removed the nonzero initialization of Cc.Iosqes (submission queue size?)
and Cc.Iocqes (completion queue size?) in function
NvmeEnableController(). And the removal of these field initializations
seems to cause the early sanity check in QEMU's nvme_start_ctrl() to
fail -- my guess at least.

The question is now if QEMU is right (according to the NVMe spec) to
require those fields, or if edk2 is right not to initialize them.

(On the side, let me point out that even if this edk2 change was
justified, it definitely should have been a separate patch, because it
has nothing to do with "Expose EFI_NVM_EXPRESS_PASS_THRU protocol".)

Let me break it down for the 1000th time:
- don't top post
- write focused patches

PLEASE! It's not 1980 any longer.

Thanks
Laszlo

> 
> Thanks
> 
> Feng
> 
>  
> 
> *From:* Anbazhagan, Baraneedharan [mailto:anbazhagan@hp.com]
> *Sent:* Tuesday, June 02, 2015 06:22
> *To:* Tian, Feng; edk2-devel@lists.sourceforge.net
> *Subject:* RE: NVMe question
> 
>  
> 
> Thanks for the update. NVMe used to work in QEMU and started failing now
> with this change. May be NVMe emulation issue in QEMU.
> 
>  
> 
> -Baranee
> 
>  
> 
> CONFIDENTIALITY NOTICE: The information contained in this e-mail and any
> accompanying documents may contain information which is HP confidential
> or otherwise protected from disclosure. This transmission may also be
> protected by the attorney-client privilege, the attorney work-product
> privilege, or both. If you are not the intended recipient of this
> message, or if this message has been addressed to you in error, please
> immediately alert the sender by reply e-mail and then delete this
> message, including any attachments. Any dissemination, distribution or
> other use of the contents of this message by anyone other than the
> intended recipient is strictly prohibited.
> 
>  
> 
> *From:* Tian, Feng [mailto:feng.tian@intel.com]
> *Sent:* Sunday, May 31, 2015 9:54 PM
> *To:* Anbazhagan, Baraneedharan; edk2-devel@lists.sourceforge.net
> <mailto:edk2-devel@lists.sourceforge.net>
> *Cc:* Tian, Feng
> *Subject:* RE: NVMe question
> 
>  
> 
> Hi, Baraneedharan
> 
>  
> 
> Why I removed these two fields initialization is because NVMe spec 7.6.1
> doesn’t say it’s mandatory.
> 
>  
> 
> Do you see any real impact on this?
> 
>  
> 
> Thanks
> 
> Feng
> 
>  
> 
> *From:* Anbazhagan, Baraneedharan [mailto:anbazhagan@hp.com]
> *Sent:* Saturday, May 30, 2015 06:21
> *To:* Tian, Feng; edk2-devel@lists.sourceforge.net
> <mailto:edk2-devel@lists.sourceforge.net>
> *Subject:* NVMe question
> 
>  
> 
> With recent changes to NvmExpressDxe module, NvmeEnableController
> doesn’t set I/O completion and submission queue size. Why default values
> are removed?  
> 
>  
> 
>  
> 
> CONFIDENTIALITY NOTICE: The information contained in this e-mail and any
> accompanying documents may contain information which is HP confidential
> or otherwise protected from disclosure. This transmission may also be
> protected by the attorney-client privilege, the attorney work-product
> privilege, or both. If you are not the intended recipient of this
> message, or if this message has been addressed to you in error, please
> immediately alert the sender by reply e-mail and then delete this
> message, including any attachments. Any dissemination, distribution or
> other use of the contents of this message by anyone other than the
> intended recipient is strictly prohibited.
> 
>  
> 
> 
> 
> ------------------------------------------------------------------------------
> 
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
> 

Re: [Qemu-devel] [edk2] NVMe question

[Keith Busch]

Hi,

On Tue, 2 Jun 2015, Laszlo Ersek wrote:
> removed the nonzero initialization of Cc.Iosqes (submission queue size?)
> and Cc.Iocqes (completion queue size?) in function
> NvmeEnableController(). And the removal of these field initializations
> seems to cause the early sanity check in QEMU's nvme_start_ctrl() to
> fail -- my guess at least.
>
> The question is now if QEMU is right (according to the NVMe spec) to
> require those fields, or if edk2 is right not to initialize them.

The host driver definitely needs to initialize these for the device to
understand the queue's entry sizes. Without proper values, it would have
no idea how much memory a queue occupies. If you have a real device that
ignores these, it's breaking spec.

Re: [Qemu-devel] [edk2] NVMe question

[Tian, Feng]

Hi, Keith

I agree your explanation does make sense.

But could you let me know where speaks the host driver must initialize these two fields of CC register before any I/O operation in NVMe spec?

Thanks
Feng

-----Original Message-----
From: Busch, Keith 
Sent: Wednesday, June 03, 2015 03:11
To: Laszlo Ersek
Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; Tian, Feng; Busch, Keith; qemu devel list
Subject: Re: [edk2] NVMe question

Hi,

On Tue, 2 Jun 2015, Laszlo Ersek wrote:
> removed the nonzero initialization of Cc.Iosqes (submission queue 
> size?) and Cc.Iocqes (completion queue size?) in function 
> NvmeEnableController(). And the removal of these field initializations 
> seems to cause the early sanity check in QEMU's nvme_start_ctrl() to 
> fail -- my guess at least.
>
> The question is now if QEMU is right (according to the NVMe spec) to 
> require those fields, or if edk2 is right not to initialize them.

The host driver definitely needs to initialize these for the device to understand the queue's entry sizes. Without proper values, it would have no idea how much memory a queue occupies. If you have a real device that ignores these, it's breaking spec.

Re: [Qemu-devel] [edk2] NVMe question

[Busch, Keith]

Section 3.1.5, for IOCQES and IOSQES:

"The required and maximum values for this field are specified in the Identify
Controller data structure in Figure 90 for each I/O Command Set. The value is 
in bytes and is specified as a power of two (2^n)."


If you're not setting these values, I assume you're leaving it as 0, which is most definitely below the "required" value.

> -----Original Message-----
> From: Tian, Feng
> Sent: Tuesday, June 02, 2015 6:21 PM
> To: Busch, Keith; Laszlo Ersek
> Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; qemu devel list; Tian, Feng
> Subject: RE: [edk2] NVMe question
> 
> Hi, Keith
> 
> I agree your explanation does make sense.
> 
> But could you let me know where speaks the host driver must initialize these two fields of CC
> register before any I/O operation in NVMe spec?
> 
> Thanks
> Feng
> 
> -----Original Message-----
> From: Busch, Keith
> Sent: Wednesday, June 03, 2015 03:11
> To: Laszlo Ersek
> Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; Tian, Feng; Busch, Keith; qemu
> devel list
> Subject: Re: [edk2] NVMe question
> 
> Hi,
> 
> On Tue, 2 Jun 2015, Laszlo Ersek wrote:
> > removed the nonzero initialization of Cc.Iosqes (submission queue
> > size?) and Cc.Iocqes (completion queue size?) in function
> > NvmeEnableController(). And the removal of these field initializations
> > seems to cause the early sanity check in QEMU's nvme_start_ctrl() to
> > fail -- my guess at least.
> >
> > The question is now if QEMU is right (according to the NVMe spec) to
> > require those fields, or if edk2 is right not to initialize them.
> 
> The host driver definitely needs to initialize these for the device to understand the queue's
> entry sizes. Without proper values, it would have no idea how much memory a queue occupies. If you
> have a real device that ignores these, it's breaking spec.

Re: [Qemu-devel] [edk2] NVMe question

[Tian, Feng]

Got your point.

Ok, I will roll back the change 

Thanks
Feng

-----Original Message-----
From: Busch, Keith 
Sent: Wednesday, June 03, 2015 22:47
To: Tian, Feng; Laszlo Ersek
Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; qemu devel list
Subject: RE: [edk2] NVMe question

Section 3.1.5, for IOCQES and IOSQES:

"The required and maximum values for this field are specified in the Identify Controller data structure in Figure 90 for each I/O Command Set. The value is in bytes and is specified as a power of two (2^n)."


If you're not setting these values, I assume you're leaving it as 0, which is most definitely below the "required" value.

> -----Original Message-----
> From: Tian, Feng
> Sent: Tuesday, June 02, 2015 6:21 PM
> To: Busch, Keith; Laszlo Ersek
> Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; qemu devel list; Tian, Feng
> Subject: RE: [edk2] NVMe question
> 
> Hi, Keith
> 
> I agree your explanation does make sense.
> 
> But could you let me know where speaks the host driver must initialize these two fields of CC
> register before any I/O operation in NVMe spec?
> 
> Thanks
> Feng
> 
> -----Original Message-----
> From: Busch, Keith
> Sent: Wednesday, June 03, 2015 03:11
> To: Laszlo Ersek
> Cc: edk2-devel@lists.sourceforge.net; Anbazhagan, Baraneedharan; Tian, Feng; Busch, Keith; qemu
> devel list
> Subject: Re: [edk2] NVMe question
> 
> Hi,
> 
> On Tue, 2 Jun 2015, Laszlo Ersek wrote:
> > removed the nonzero initialization of Cc.Iosqes (submission queue
> > size?) and Cc.Iocqes (completion queue size?) in function
> > NvmeEnableController(). And the removal of these field initializations
> > seems to cause the early sanity check in QEMU's nvme_start_ctrl() to
> > fail -- my guess at least.
> >
> > The question is now if QEMU is right (according to the NVMe spec) to
> > require those fields, or if edk2 is right not to initialize them.
> 
> The host driver definitely needs to initialize these for the device to understand the queue's
> entry sizes. Without proper values, it would have no idea how much memory a queue occupies. If you
> have a real device that ignores these, it's breaking spec.