Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: please enable NVMe boot in OVMF #48

Closed
stamerlan opened this issue Jan 26, 2016 · 21 comments
Closed

RFE: please enable NVMe boot in OVMF #48

stamerlan opened this issue Jan 26, 2016 · 21 comments

Comments

@stamerlan
Copy link

Hi, guys!

It would be nice to add NVMe boot support (to install windows on NVMe disk for example).

Our company developing SSD and we have simulator for SSD HW (to debug FW and run tests). But currently we need to gather logs from NVMe controller while windows is installing (yes, it's stupid, but it isn't my decision). Those logs will be used to create tests for SSD FW. I've modified QEMU's NVMe controller to create logs in a suitable format, but I can't install Windows on NVMe disk. That's why I ask you to add this feature, it may be useful for someone else too.
It may be useful for OS developers, who are trying to write it's own bootloader and wanna check it on virtual NVMe SSD.
I'm not familiar with OVMF, so I'll try to add NVMe support for SeaBios (I'm not bios developer, but I read SeaBios code for a week, so I more familiar with it).

QEMU cmdline:
qemu-system-x86_64
-drive file=ssd.raw,if=none,id=disk0,format=raw
-device nvme,drive=disk0,serial=1234

@lersek
Copy link
Member

lersek commented Jan 26, 2016

What does NVMe enable that virtio-blk / virtio-scsi disks don't?

(Not arguing against the feature request, I'd just like to know.)

Please also provide a reasonably short QEMU command line that exposes a drive as an NVMe device to the guest. Thanks.

@lersek
Copy link
Member

lersek commented Jan 26, 2016

The title for this entry should state "OVMF"; can you please edit it?

@stamerlan stamerlan changed the title NVMe boot OVMF Jan 26, 2016
@lersek
Copy link
Member

lersek commented Jan 26, 2016

@stamerlan haha, sorry, I was not clear enough. In more details:

This tracker is not just for OVMF, it is for the entirety of edk2. In sensible bug tracking systems, like Bugzilla, you have products, components, sub-components and such -- metadata for each bug report. When you report a bug in those sensible systems, you are expected to make an effort to categorize your bug report up-front, in order to save time for the people who read the bug reports.

In this bug tracker though, we have no such metadata, the best we can do is (apparently) "tags". So @jljusten tends to tag OVMF-related bug reports with the "OVMF" label. But, since you know up-front that you are reporting the issue for OVMF, not another platform built from edk2, you should immediately signal in the title of your report that it is for OVMF.

Therefore the title should contain both OVMF and NVMe boot.

Really, there's a huge disconnect here between users of OVMF, and edk2, the open source project. One advantage that a well-maintained Bugzilla instance has is that it forces bug reporters to at least think about metadata, plus it provides a bug report template that must be completed with important information. Github's issue tracker encourages reckless user behavior -- just dump the three words that come to your mind into a new item. (But hey at least you can format those three words with markdown!)

This open letter is completely justified: https://github.com/dear-github/dear-github

To top it all off, I personally lack the credentials to change bug titles, bug statuses, tags; anything at all beyond commenting. (Except, of course, I have the privilege of fixing bugs.)

I knew using a web-era issue tracker for edk2 would be a mistake, and I stand vindicated.

TL;DR: please change the title to: "RFE: please enable NVMe boot in OVMF".

Also, please try to answer my other questions above [1]. In any RFE (request for enhancement), you gotta come up with some justification why the developers should spend their time on the feature you'd like to have. As I said, I don't doubt this is a reasonable request, but I do care about your reasons for wanting to use NVMe.

[1] In a sane bug tracker, I would reference earlier comments by writing "comment 3" or "comment 4", and the tracker would automatically turn those words into links. In this tracker however, comments have no numbers (they don't even have human readable exact timestamps! WTF is "lersek commented an hour ago" supposed to mean?!), I can only copy and paste garbage-looking links like this: #48 (comment)

GitHub issue tracker, the social media of bug reporting, for the facebook generation.

@stamerlan
Copy link
Author

I meant qemu virtual nvme disk:
qemu-system-x86_64
-drive file=ssd.img,if=none,id=disk0,format=raw
-device nvme,drive=drive0,serial=1234

@stamerlan stamerlan changed the title OVMF RFE: please enable NVMe boot in OVMF Jan 26, 2016
@lersek
Copy link
Member

lersek commented Jan 26, 2016

I will assume that http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/10375 is related.

@stamerlan
Copy link
Author

Yup, @lersek it's my e-mail. Shell I add explanation in the 1st post or leave one more comment?

@lersek
Copy link
Member

lersek commented Jan 26, 2016

@stamerlan if you could expand the first comment with details, that would be great. Thanks!

@lersek
Copy link
Member

lersek commented Jan 26, 2016

@stamerlan thanks for explaining your use case in the first comment. It is perfectly reasonable; OVMF is regularly used for prototyping and testing otherwise hardware-oriented features.

@stamerlan
Copy link
Author

@lersek as far as I understand virtio parameters used to cooperate with hypervisor to use real HW?

@lersek
Copy link
Member

lersek commented Jan 26, 2016

@stamerlan I don't understand the question. But anyway I just wanted to be sure that NVMe was specifically needed for your use case (i.e., it couldn't be served by virtio disks).

@lersek
Copy link
Member

lersek commented Jan 26, 2016

lersek added a commit to lersek/edk2 that referenced this issue Jan 27, 2016
QEMU emulates NVMe. NvmExpressDxe seems to work well with it. The relevant
QEMU options are

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL

where the required SERIAL value sets the Serial Number (SN) field of the
"Identify Controller Data Structure". It is an ASCII string with up to 20
characters, which QEMU pads with spaces to maximum length.

(Refer to "NVME_ADMIN_CONTROLLER_DATA.Sn" in
"MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h".)

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: tianocore#48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
lersek added a commit to lersek/edk2 that referenced this issue Jan 27, 2016
This patch enables QemuBootOrderLib to parse OFW device paths formatted by
the QEMU patch

  nvme: generate OpenFirmware device path in the "bootorder" fw_cfg file

  http://thread.gmane.org/gmane.comp.emulators.qemu.block/8332

With both patches applied, OVMF will honor the bootindex=N property of the
NVMe device:

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL,bootindex=N
                                          ^^^^^^^^^^^

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: tianocore#48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
@lersek
Copy link
Member

lersek commented Jan 27, 2016

@lersek
Copy link
Member

lersek commented Jan 27, 2016

@stamerlan: can you please test my patches (QEMU and OVMF) that I linked in #48 (comment) and #48 (comment) ? I CC'd you on both sets directly, so you should have them in your email. A Tested-by from users is highly appreciated. Thanks.

@stamerlan
Copy link
Author

@hi, @lersek
I saw e-mails, but I got sick, so I'll come to office to test it tomorrow.

@lersek
Copy link
Member

lersek commented Jan 28, 2016

@stamerlan take your time, get better first. thx

@stamerlan
Copy link
Author

Hi, @lersek

Yes, your patch is working. I successfully installed windows 10 x86_64 on qemu!

Thank you a lot!

P.S. what happened with nvme_boot_issue48, today I can't checkout it?

@lersek
Copy link
Member

lersek commented Jan 29, 2016

@stamerlan Thank you very much for the testing and the feedback. I highly appreciate it!

Please don't close this item just yet though -- can you please reopen it? The patches are still under review, and have not been committed to either QEMU or OVMF. This item should only be closed when the edk2 patches are "in".

However, please do report your successful testing in both the QEMU and edk2 threads. Please locate the following messages in your mailbox:

[PATCH 0/3] OvmfPkg: enable NVMe devices
[PATCH] nvme: generate OpenFirmware device path in the "bootorder" fw_cfg file

hit Reply All, and respond with

Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>

This way reviewers will see that the patch has undergone independent testing, and your testing feedback will be captured in the commit log. Thank you!

@lersek
Copy link
Member

lersek commented Jan 29, 2016

@stamerlan

P.S. what happened with nvme_boot_issue48, today I can't checkout it?

I have no clue, it must be a github issue or a network glitch between you & github; I didn't touch this branch.

(I never touch branches that I push for review. If a new version of the series becomes necessary, I push XXXX_v2, XXXX_v3 branches. Such a branch should stay reflecting the mailing list posting.)

@jljusten jljusten reopened this Jan 29, 2016
@jljusten jljusten added the OVMF label Jan 29, 2016
@stamerlan
Copy link
Author

Sorry for my delay. I have no access to my work-mailbox from home.
I replied today in qemu and edk2 threads.

Thank you a lot!

qemu-deploy pushed a commit to qemu/qemu that referenced this issue Feb 2, 2016
Background on QEMU boot indices
-------------------------------

Normally, the "bootindex" property is configured for bootable devices
with:

  DEVICE_instance_init()
    device_add_bootindex_property(..., "bootindex", ...)
      object_property_add(..., device_get_bootindex,
                          device_set_bootindex, ...)

and when the bootindex is set on the QEMU command line, with

  -device DEVICE,...,bootindex=N

the setter that was configured above is invoked:

  device_set_bootindex()
    /* parse boot index */
    visit_type_int32()

    /* verify unicity */
    check_boot_index()

    /* store parsed boot index */
    ...

    /* insert device path to boot order */
    add_boot_device_path()

In the last step, add_boot_device_path() ensures that an OpenFirmware
device path will show up in the "bootorder" fw_cfg file, at a position
corresponding to the device's boot index. Thus guest firmware (SeaBIOS and
OVMF) can try to boot off the device with the right priority.

NVMe boot index
---------------

In QEMU commit 33739c7,

  nvma: ide: add bootindex to qom property

the following generic setters / getters:
- device_set_bootindex()
- device_get_bootindex()

were open-coded for NVMe, under the names
- nvme_set_bootindex()
- nvme_get_bootindex()

Plus nvme_instance_init() was added to configure the "bootindex" property
manually, designating the open-coded getter & setter, rather than calling
device_add_bootindex_property().

Crucially, nvme_set_bootindex() avoided the final add_boot_device_path()
call. This fact is spelled out in the message of commit 33739c7, and
it was presumably the entire reason for all of the code duplication.

Now, Vladislav filed an RFE for OVMF
<tianocore/edk2#48>; OVMF should boot off NVMe
devices. It is simple to build edk2's existent NvmExpressDxe driver into
OVMF, but the boot order matching logic in OVMF can only handle NVMe if
the "bootorder" fw_cfg file includes such devices.

Therefore this patch converts the NVMe device model to
device_set_bootindex() all the way.

Device paths
------------

device_set_bootindex() accepts an optional parameter called "suffix". When
present, it is expected to take the form of an OpenFirmware device path
node, and it gets appended as last node to the otherwise auto-generated
OFW path.

For NVMe, the auto-generated part is

  /pci@i0cf8/pci8086,5845@6[,1]
       ^     ^            ^  ^
       |     |            PCI slot and (present when nonzero)
       |     |            function of the NVMe controller, both hex
       |     "driver name" component, built from PCI vendor & device IDs
       PCI root at system bus port, PIO

to which here we append the suffix

  /namespace@1,0
             ^ ^
             | big endian (MSB at lowest address) numeric interpretation
             | of the 64-bit IEEE Extended Unique Identifier, aka EUI-64,
             | hex
             32-bit NVMe namespace identifier, aka NSID, hex

resulting in the OFW device path

  /pci@i0cf8/pci8086,5845@6[,1]/namespace@1,0

The reason for including the NSID and the EUI-64 is that an NVMe device
can in theory produce several different namespaces (distinguished by
NSID). Additionally, each of those may (optionally) have an EUI-64 value.

For now, QEMU only provides namespace 1.

Furthermore, QEMU doesn't even represent the EUI-64 as a standalone field;
it is embedded (and left unused) inside the "NvmeIdNs.res30" array, at the
last eight bytes. (Which is fine, since EUI-64 can be left zero-filled if
unsupported by the device.)

Based on the above, we set the "unit address" part of the last
("namespace") node to fixed "1,0".

OVMF will then map the above OFW device path to the following UEFI device
path fragment, for boot order processing:

  PciRoot(0x0)/Pci(0x6,0x1)/NVMe(0x1,00-00-00-00-00-00-00-00)
          ^        ^   ^    ^    ^   ^
          |        |   |    |    |   octets of the EUI-64 in address order
          |        |   |    |    NSID
          |        |   |    NVMe namespace messaging device path node
          |        PCI slot and function
          PCI root bridge

Cc: Keith Busch <keith.busch@intel.com> (supporter:nvme)
Cc: Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Cc: qemu-block@nongnu.org (open list:nvme)
Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Gonglei <arei.gonglei@huawei.com>
Acked-by: Keith Busch <keith.busch@intel.com>
Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Message-id: 1453850483-27511-1-git-send-email-lersek@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
jljusten pushed a commit that referenced this issue Feb 2, 2016
QEMU emulates NVMe. NvmExpressDxe seems to work well with it. The relevant
QEMU options are

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL

where the required SERIAL value sets the Serial Number (SN) field of the
"Identify Controller Data Structure". It is an ASCII string with up to 20
characters, which QEMU pads with spaces to maximum length.

(Refer to "NVME_ADMIN_CONTROLLER_DATA.Sn" in
"MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h".)

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: #48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@19791 6f19259b-4bc3-4df7-8a09-765794883524
jljusten pushed a commit that referenced this issue Feb 2, 2016
This patch enables QemuBootOrderLib to parse OFW device paths formatted by
QEMU commit a907ec5:

  nvme: generate OpenFirmware device path in the "bootorder" fw_cfg file

With both patches applied, OVMF will honor the bootindex=N property of the
NVMe device:

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL,bootindex=N
                                          ^^^^^^^^^^^

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: #48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@19792 6f19259b-4bc3-4df7-8a09-765794883524
@lersek
Copy link
Member

lersek commented Feb 2, 2016

Patches are upstream, both QEMU and edk2. Closing.

@lersek lersek closed this as completed Feb 2, 2016
mdkinney pushed a commit to mdkinney/edk2 that referenced this issue Mar 8, 2016
QEMU emulates NVMe. NvmExpressDxe seems to work well with it. The relevant
QEMU options are

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL

where the required SERIAL value sets the Serial Number (SN) field of the
"Identify Controller Data Structure". It is an ASCII string with up to 20
characters, which QEMU pads with spaces to maximum length.

(Refer to "NVME_ADMIN_CONTROLLER_DATA.Sn" in
"MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.h".)

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: tianocore#48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@19791 6f19259b-4bc3-4df7-8a09-765794883524
mdkinney pushed a commit to mdkinney/edk2 that referenced this issue Mar 8, 2016
This patch enables QemuBootOrderLib to parse OFW device paths formatted by
QEMU commit a907ec5:

  nvme: generate OpenFirmware device path in the "bootorder" fw_cfg file

With both patches applied, OVMF will honor the bootindex=N property of the
NVMe device:

  -drive id=drive0,if=none,format=FORMAT,file=PATHNAME \
  -device nvme,drive=drive0,serial=SERIAL,bootindex=N
                                          ^^^^^^^^^^^

Cc: Vladislav Vovchenko <vladislav.vovchenko@sk.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reference: tianocore#48
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Vladislav Vovchenko <vladislav.vovchenko@sk.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@19792 6f19259b-4bc3-4df7-8a09-765794883524
@lersek
Copy link
Member

lersek commented Jul 21, 2016

This (closed) item has been manually migrated to
https://tianocore.acgmultimedia.com/show_bug.cgi?id=79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants