* Re: [Qemu-devel] [edk2] NVMe question [not found] ` <7F1BAD85ADEA444D97065A60D2E97EE501D6AB7C@SHSMSX101.ccr.corp.intel.com> @ 2015-06-02 7:45 ` Laszlo Ersek 2015-06-02 19:11 ` Keith Busch 0 siblings, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2015-06-02 7:45 UTC (permalink / raw) To: edk2-devel, Anbazhagan, Baraneedharan, Feng Tian, Keith Busch Cc: qemu devel list 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [edk2] NVMe question 2015-06-02 7:45 ` [Qemu-devel] [edk2] NVMe question Laszlo Ersek @ 2015-06-02 19:11 ` Keith Busch 2015-06-03 0:21 ` Tian, Feng 0 siblings, 1 reply; 5+ messages in thread From: Keith Busch @ 2015-06-02 19:11 UTC (permalink / raw) To: Laszlo Ersek Cc: Keith Busch, Anbazhagan, Baraneedharan, edk2-devel, Feng Tian, qemu devel list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [edk2] NVMe question 2015-06-02 19:11 ` Keith Busch @ 2015-06-03 0:21 ` Tian, Feng 2015-06-03 14:46 ` Busch, Keith 0 siblings, 1 reply; 5+ messages in thread From: Tian, Feng @ 2015-06-03 0:21 UTC (permalink / raw) To: Busch, Keith, Laszlo Ersek Cc: Tian, Feng, Anbazhagan, Baraneedharan, edk2-devel, qemu devel list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [edk2] NVMe question 2015-06-03 0:21 ` Tian, Feng @ 2015-06-03 14:46 ` Busch, Keith 2015-06-04 2:04 ` Tian, Feng 0 siblings, 1 reply; 5+ messages in thread From: Busch, Keith @ 2015-06-03 14:46 UTC (permalink / raw) To: Tian, Feng, Laszlo Ersek Cc: Anbazhagan, Baraneedharan, edk2-devel, qemu devel list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [edk2] NVMe question 2015-06-03 14:46 ` Busch, Keith @ 2015-06-04 2:04 ` Tian, Feng 0 siblings, 0 replies; 5+ messages in thread From: Tian, Feng @ 2015-06-04 2:04 UTC (permalink / raw) To: Busch, Keith, Laszlo Ersek Cc: Tian, Feng, Anbazhagan, Baraneedharan, edk2-devel, qemu devel list 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-06-04 2:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <8530272280D33D4095B8F02BA1B57BAC3AFD753D@G9W0345.americas.hpqcorp.net> [not found] ` <7F1BAD85ADEA444D97065A60D2E97EE501D6A4BB@SHSMSX101.ccr.corp.intel.com> [not found] ` <8530272280D33D4095B8F02BA1B57BAC3AFD9AB1@G9W0345.americas.hpqcorp.net> [not found] ` <7F1BAD85ADEA444D97065A60D2E97EE501D6AB7C@SHSMSX101.ccr.corp.intel.com> 2015-06-02 7:45 ` [Qemu-devel] [edk2] NVMe question Laszlo Ersek 2015-06-02 19:11 ` Keith Busch 2015-06-03 0:21 ` Tian, Feng 2015-06-03 14:46 ` Busch, Keith 2015-06-04 2:04 ` Tian, Feng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.