Discussion:
[edk2] NVMe question
Anbazhagan, Baraneedharan
2015-05-29 22:20:43 UTC
Permalink
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.
Tian, Feng
2015-06-01 02:54:19 UTC
Permalink
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:***@hp.com]
Sent: Saturday, May 30, 2015 06:21
To: Tian, Feng; edk2-***@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.
Anbazhagan, Baraneedharan
2015-06-01 22:21:43 UTC
Permalink
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:***@intel.com]
Sent: Sunday, May 31, 2015 9:54 PM
To: Anbazhagan, Baraneedharan; edk2-***@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:***@hp.com]
Sent: Saturday, May 30, 2015 06:21
To: Tian, Feng; edk2-***@lists.sourceforge.net<mailto:edk2-***@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.
Tian, Feng
2015-06-02 07:19:06 UTC
Permalink
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.

Thanks
Feng

From: Anbazhagan, Baraneedharan [mailto:***@hp.com]
Sent: Tuesday, June 02, 2015 06:22
To: Tian, Feng; edk2-***@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:***@intel.com]
Sent: Sunday, May 31, 2015 9:54 PM
To: Anbazhagan, Baraneedharan; edk2-***@lists.sourceforge.net<mailto:edk2-***@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:***@hp.com]
Sent: Saturday, May 30, 2015 06:21
To: Tian, Feng; edk2-***@lists.sourceforge.net<mailto:edk2-***@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.
Laszlo Ersek
2015-06-02 07:45:58 UTC
Permalink
Post by Tian, Feng
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
Post by Tian, Feng
Thanks
Feng
*Sent:* Tuesday, June 02, 2015 06:22
*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.
*Sent:* Sunday, May 31, 2015 9:54 PM
*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
*Sent:* Saturday, May 30, 2015 06:21
*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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Tian, Feng
2015-06-03 00:21:16 UTC
Permalink
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-***@lists.sourceforge.net; Anbazhagan, Baraneedharan; Tian, Feng; Busch, Keith; qemu devel list
Subject: Re: [edk2] NVMe question

Hi,
Post by Laszlo Ersek
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.

------------------------------------------------------------------------------
Tian, Feng
2015-06-04 02:04:37 UTC
Permalink
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-***@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.
Post by Tian, Feng
-----Original Message-----
From: Tian, Feng
Sent: Tuesday, June 02, 2015 6:21 PM
To: Busch, Keith; Laszlo Ersek
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
devel list
Subject: Re: [edk2] NVMe question
Hi,
Post by Laszlo Ersek
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.
------------------------------------------------------------------------------
Tian, Feng
2015-06-03 00:24:41 UTC
Permalink
Hi, Laszlo

Most of us are using MS outlook as mail client. So top-posting is inevitable per my knowledge. If you know how to avoid top posting, please let us know.

Thanks
Feng

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: Tuesday, June 02, 2015 15:46
To: edk2-***@lists.sourceforge.net; Anbazhagan, Baraneedharan; Tian, Feng; Busch, Keith
Cc: qemu devel list
Subject: Re: [edk2] NVMe question
Post by Tian, Feng
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
Post by Tian, Feng
Thanks
Feng
*Sent:* Tuesday, June 02, 2015 06:22
*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.
*Sent:* Sunday, May 31, 2015 9:54 PM
*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
*Sent:* Saturday, May 30, 2015 06:21
*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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Laszlo Ersek
2015-06-03 09:32:40 UTC
Permalink
Post by Tian, Feng
Hi, Laszlo
Most of us are using MS outlook as mail client. So top-posting is
inevitable per my knowledge. If you know how to avoid top posting,
please let us know.
The constructive answer would be

http://home.in.tum.de/~jain/software/outlook-quotefix/

but checking the "compatibility" page, I doubt you guys use an outlook
that old.

The factually correct, but not so constructive answer would be that
microsoft outlook is irreparably broken, and should not be used for
participating in distributed, open source development.

I'll point out that MS outlook is probably the source of further
problems, not just top posting. For example, I'm fairly sure it doesn't
allow you guys to save patch emails (posted with git) in pristine local
files. It's broken quoting style also makes it impossible to produce
"context sensitive" patch reviews, where you comment on the exact line
or block of code that has a problem. (Hence, I can certainly see why
someone would consider Gerrit a step forward. When your MUA is MS
outlook, anything qualifies as an improvement.)

These shortcomings likely apply to a bunch of other MUAs, like gmail (I
assume), and most mobile clients. Same as most web apps, these tools are
optimized for social interaction and "traditional productivity", not
distributed development of technology.

I probably can't convince you guys to ditch MS outlook; maybe it's a
requirement forced upon you by your IT department, or perhaps you would
lose connectivity with internal "productivity" tools if you did. (I've
worked at a telco once where there was no way I could have ditched
outlook myself.)

So, I sincerely apologize for letting my annoyance show. In the future,
I'll just seek to ignore top-posted messages, when I can afford that.

Thanks
Laszlo
Post by Tian, Feng
-----Original Message-----
Sent: Tuesday, June 02, 2015 15:46
Cc: qemu devel list
Subject: Re: [edk2] NVMe question
Post by Tian, Feng
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.
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".)
- don't top post
- write focused patches
PLEASE! It's not 1980 any longer.
Thanks
Laszlo
Post by Tian, Feng
Thanks
Feng
*Sent:* Tuesday, June 02, 2015 06:22
*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.
*Sent:* Sunday, May 31, 2015 9:54 PM
*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
*Sent:* Saturday, May 30, 2015 06:21
*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
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Continue reading on narkive:
Loading...