Paths

Table of Contentst

Differential D59080

bhyve: fix byte order for manually set NVMe eui64
AcceptedPublic
Actions

Authored by novel on Fri, Aug 21, 1:50 PM.

Details

Reviewers

chuck

Group Reviewers

bhyve

Summary

Manually specified eui64 value gets converted to big endian twice:
first using htobe64() and then using be64enc(). On little-endian hosts
that results in a little-endian value instead of a big-endian.

Fix by removing htobe64() for a user submitted value.

Fixes: 409a80e5a434 ("bhyve: Create EUI64 for NVMe namespaces")
Relnotes: yes
Sponsored by: The FreeBSD Foundation
MFC after: 3 weeks

Test Plan

Manually specified eui64

/usr/sbin/bhyve -c 1 -m 1024 -u -H -P -s 0:0,hostbridge \
    -l bootrom,/usr/local/share/uefi-firmware/BHYVE_UEFI.fd \
    -s 1:0,lpc \
    -s 2:0,ahci,hd:/var/lib/libvirt/images/freebsd15.0.img \
    -s 5:0,nvme,/data/img/10gb_ufs.img,eui64=0xdeadbeef00000001 \
    -l com1,stdio \
    eui64-repro

Unpatched:

root@freebsd:~ # nvmecontrol identify nvme0ns1 | grep 'IEEE EUI64'                                                                                                                                                                                                                                                     
IEEE EUI64:                  01000000efbeadde                                                                                                                                                                                                                                                                          
root@freebsd:~ #

Patched:

root@freebsd:~ # nvmecontrol identify nvme0ns1 | grep 'IEEE EUI64'                                                                                                                                                                                                                                                     
IEEE EUI64:                  deadbeef00000001                                                                                                                                                                                                                                                                          
root@freebsd:~ #

Autogenerated eui64

/usr/sbin/bhyve -c 1 -m 1024 -u -H -P -s 0:0,hostbridge
    -l bootrom,/usr/local/share/uefi-firmware/BHYVE_UEFI.fd \
    -s 1:0,lpc \
    -s 2:0,ahci,hd:/var/lib/libvirt/images/freebsd15.0.img \
    -s 5:0,nvme,/data/img/10gb_ufs.img \
    -l com1,stdio \
    eui64-repro

Unpatched:

root@freebsd:~ # nvmecontrol identify nvme0ns1 | grep 'IEEE EUI64'                                                                                                                                                                                                                                                     
IEEE EUI64:                  589cfc20a0a10001                                                                                                                                                                                                                                                                          
root@freebsd:~ #

Patched:

root@freebsd:~ # nvmecontrol identify nvme0ns1 | grep 'IEEE EUI64'                                                                                                                                                                                                                                                     
IEEE EUI64:                  589cfc20a0a10001                                                                                                                                                                                                                                                                          
root@freebsd:~ #

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 75945
Build 72828: arc lint + arc unit

Event Timeline

novel created this revision.Fri, Aug 21, 1:50 PM

Herald added a reviewer: bhyve. · View Herald TranscriptFri, Aug 21, 1:50 PM

Herald added subscribers: dab, bcran, rgrimes, imp. · View Herald Transcript

novel requested review of this revision.Fri, Aug 21, 1:50 PM

Harbormaster completed remote builds in B75945: Diff 184646.Fri, Aug 21, 1:50 PM

novel edited the test plan for this revision. (Show Details)Fri, Aug 21, 1:52 PM

novel edited the test plan for this revision. (Show Details)Fri, Aug 21, 1:54 PM

I'm not very familiar with nvme, but I presume this might cause problems in existing VM images that embed this identifer somewhere?

In D59080#1354973, @markj wrote:

I'm not very familiar with nvme, but I presume this might cause problems in existing VM images that embed this identifer somewhere?

I imagine that users who specify 'eui64=` for their NVMe devices will need to reverse the value they pass to bhyve to keep the value the same inside a VM. I think users who use autogenerated value are not affected.

In D59080#1355002, @novel wrote:

In D59080#1354973, @markj wrote:

I'm not very familiar with nvme, but I presume this might cause problems in existing VM images that embed this identifer somewhere?

I imagine that users who specify 'eui64=` for their NVMe devices will need to reverse the value they pass to bhyve to keep the value the same inside a VM.

Right, this looks like a backward compatibility break. I can't tell whether it's likely to hurt anyone.

I think users who use autogenerated value are not affected.

Right.

novel edited the test plan for this revision. (Show Details)Sat, Aug 22, 4:52 AM

Thanks for finding this and fixing it.

This revision is now accepted and ready to land.Sun, Aug 23, 12:03 AM

In D59080#1355723, @chuck wrote:

Thanks for finding this and fixing it.

Thanks for review! Do you have a suggestion how to better handle the breaking change for users who already use ,eui64= in their configurations?

The EUI64 exists to uniquely identify a drive, and this fix doesn't change that. The value is reported in the Namespace data, and is not a load-bearing value from a protocol perspective. If someone had automated logging and inventory setup for their systems and were explicitly setting the EUI64, they might notice this change.

At a minimum, it would be worth noting it in the release notes and UPDATING.

Conservatively, we could decide that this breaking change stays in 16.0 and doesn't get MFC'd. That said, since the implementation has been wrong and there are no issues in Bugzilla related to this, it would probably be fine to propagate this fix to 15-stable as well.

Add an UPDATING entry.

This revision now requires review to proceed.Mon, Aug 24, 4:18 PM

Harbormaster completed remote builds in B76052: Diff 184893.Mon, Aug 24, 4:18 PM

Updated the commit metadata with all the relevant fields.

chuck accepted this revision.Tue, Aug 25, 5:15 PM

This revision is now accepted and ready to land.Tue, Aug 25, 5:15 PM