Changeset View
Standalone View
usr.sbin/bhyve/pci_nvme.c
Show First 20 Lines • Show All 239 Lines • ▼ Show 20 Lines | |||||
struct pci_nvme_softc; | struct pci_nvme_softc; | ||||
struct nvme_feature_obj; | struct nvme_feature_obj; | ||||
typedef void (*nvme_feature_cb)(struct pci_nvme_softc *, | typedef void (*nvme_feature_cb)(struct pci_nvme_softc *, | ||||
struct nvme_feature_obj *, | struct nvme_feature_obj *, | ||||
struct nvme_command *, | struct nvme_command *, | ||||
struct nvme_completion *); | struct nvme_completion *); | ||||
struct nvme_feature_obj { | struct nvme_feature_obj { | ||||
chuck: I think style(9) suggests this shouldn't be done:
> Avoid typedefs ending in “_t”, except as… | |||||
Done Inline Actions
Right, It didn't match the style(9). wanpengqian_gmail.com: > I think style(9) suggests this shouldn't be done:
>
> > Avoid typedefs ending in “_t”… | |||||
uint32_t cdw11; | uint32_t cdw11; | ||||
nvme_feature_cb set; | nvme_feature_cb set; | ||||
nvme_feature_cb get; | nvme_feature_cb get; | ||||
bool namespace_specific; | bool namespace_specific; | ||||
}; | }; | ||||
#define NVME_FID_MAX (NVME_FEAT_ENDURANCE_GROUP_EVENT_CONFIGURATION + 1) | #define NVME_FID_MAX (NVME_FEAT_ENDURANCE_GROUP_EVENT_CONFIGURATION + 1) | ||||
▲ Show 20 Lines • Show All 426 Lines • ▼ Show 20 Lines | pci_nvme_init_nsdata(struct pci_nvme_softc *sc, | ||||
/* LBA data-sz = 2^lbads */ | /* LBA data-sz = 2^lbads */ | ||||
nd->lbaf[0] = nvstore->sectsz_bits << NVME_NS_DATA_LBAF_LBADS_SHIFT; | nd->lbaf[0] = nvstore->sectsz_bits << NVME_NS_DATA_LBAF_LBADS_SHIFT; | ||||
} | } | ||||
static void | static void | ||||
pci_nvme_init_logpages(struct pci_nvme_softc *sc) | pci_nvme_init_logpages(struct pci_nvme_softc *sc) | ||||
{ | { | ||||
__uint128_t power_cycles = 1; | |||||
corvinkUnsubmitted Not Done Inline ActionsI'd prefer converting it to little endian as required by the nvme spec. Nevertheless, big endian architectures aren't supported by bhyve and as far as I can see, FreeBSD has no htole128 macro yet. So, I think it's fine. corvink: I'd prefer converting it to little endian as required by the nvme spec.
Nevertheless, big… | |||||
memset(&sc->err_log, 0, sizeof(sc->err_log)); | memset(&sc->err_log, 0, sizeof(sc->err_log)); | ||||
memset(&sc->health_log, 0, sizeof(sc->health_log)); | memset(&sc->health_log, 0, sizeof(sc->health_log)); | ||||
memset(&sc->fw_log, 0, sizeof(sc->fw_log)); | memset(&sc->fw_log, 0, sizeof(sc->fw_log)); | ||||
memset(&sc->ns_log, 0, sizeof(sc->ns_log)); | memset(&sc->ns_log, 0, sizeof(sc->ns_log)); | ||||
/* Set read/write remainder to round up according to spec */ | /* Set read/write remainder to round up according to spec */ | ||||
sc->read_dunits_remainder = 999; | sc->read_dunits_remainder = 999; | ||||
sc->write_dunits_remainder = 999; | sc->write_dunits_remainder = 999; | ||||
/* Set nominal Health values checked by implementations */ | /* Set nominal Health values checked by implementations */ | ||||
sc->health_log.temperature = NVME_TEMPERATURE; | sc->health_log.temperature = NVME_TEMPERATURE; | ||||
sc->health_log.available_spare = 100; | sc->health_log.available_spare = 100; | ||||
sc->health_log.available_spare_threshold = 10; | sc->health_log.available_spare_threshold = 10; | ||||
Not Done Inline ActionsCan you help me understand the use of memcpy here? Would a simple assignment not work? chuck: Can you help me understand the use of `memcpy` here? Would a simple assignment not work? | |||||
Done Inline Actions
The NVMe PowerCycles value is a 128-bit counter, but sys/dev/nvme/nvme.h define it as an array of 2 64-bit values. it can not simply be assigned in one line. So I assign it as other 128-bit values do. Now it changes to two line assignment, (Although the initial low value is enough, I prefer to do it whole.) wanpengqian_gmail.com: > Can you help me understand the use of `memcpy` here? Would a simple assignment not work?
The… | |||||
Not Done Inline ActionsBah! I forgot about that. Yes, your first approach was fine. chuck: Bah! I forgot about that. Yes, your first approach was fine. | |||||
Not Done Inline ActionsLast I checked, there was no int128_t data type on 32-bit platforms. If that's changed, I can update the few uses of 2 64-bit quantities to a single 128-bit int. imp: Last I checked, there was no int128_t data type on 32-bit platforms. If that's changed, I can… | |||||
Not Done Inline ActionsHmm, then the emulation is already in trouble as it uses __uint128_t in several other places. If this doesn't exist for 32-bit platforms, what is the portable way to do math on 16-byte values? chuck: Hmm, then the emulation is already in trouble as it uses `__uint128_t` in several other places. | |||||
Not Done Inline Actionsbhyve currently only runs on amd64 (and arm64 out of tree). If at some point a 32-bit port (armv7 is the most likely candidate) grows bhyve support, then we can worry about this. However, for now I think it's fine for bhyve to assume 64-bit architectures and __uint128_t jhb: bhyve currently only runs on amd64 (and arm64 out of tree). If at some point a 32-bit port… | |||||
Not Done Inline ActionsThe problem is with the shared nvme driver which runs on i386... imp: The problem is with the shared nvme driver which runs on i386... | |||||
Not Done Inline ActionsI think we're OK as this file isn't shared with the nvme(4) chuck: I think we're OK as this file isn't shared with the `nvme(4)` | |||||
/* Set Active Firmware Info to slot 1 */ | /* Set Active Firmware Info to slot 1 */ | ||||
sc->fw_log.afi = (1 << NVME_FIRMWARE_PAGE_AFI_SLOT_SHIFT); | sc->fw_log.afi = (1 << NVME_FIRMWARE_PAGE_AFI_SLOT_SHIFT); | ||||
memcpy(&sc->fw_log.revision[0], sc->ctrldata.fr, | memcpy(&sc->fw_log.revision[0], sc->ctrldata.fr, | ||||
sizeof(sc->fw_log.revision[0])); | sizeof(sc->fw_log.revision[0])); | ||||
memcpy(&sc->health_log.power_cycles, &power_cycles, | |||||
sizeof(sc->health_log.power_cycles)); | |||||
} | } | ||||
static void | static void | ||||
pci_nvme_init_features(struct pci_nvme_softc *sc) | pci_nvme_init_features(struct pci_nvme_softc *sc) | ||||
{ | { | ||||
enum nvme_feature fid; | enum nvme_feature fid; | ||||
for (fid = 0; fid < NVME_FID_MAX; fid++) { | for (fid = 0; fid < NVME_FID_MAX; fid++) { | ||||
▲ Show 20 Lines • Show All 750 Lines • ▼ Show 20 Lines | case NVME_LOG_HEALTH_INFORMATION: | ||||
memcpy(&sc->health_log.data_units_read, &sc->read_data_units, | memcpy(&sc->health_log.data_units_read, &sc->read_data_units, | ||||
sizeof(sc->health_log.data_units_read)); | sizeof(sc->health_log.data_units_read)); | ||||
memcpy(&sc->health_log.data_units_written, &sc->write_data_units, | memcpy(&sc->health_log.data_units_written, &sc->write_data_units, | ||||
sizeof(sc->health_log.data_units_written)); | sizeof(sc->health_log.data_units_written)); | ||||
memcpy(&sc->health_log.host_read_commands, &sc->read_commands, | memcpy(&sc->health_log.host_read_commands, &sc->read_commands, | ||||
sizeof(sc->health_log.host_read_commands)); | sizeof(sc->health_log.host_read_commands)); | ||||
memcpy(&sc->health_log.host_write_commands, &sc->write_commands, | memcpy(&sc->health_log.host_write_commands, &sc->write_commands, | ||||
sizeof(sc->health_log.host_write_commands)); | sizeof(sc->health_log.host_write_commands)); | ||||
pthread_mutex_unlock(&sc->mtx); | pthread_mutex_unlock(&sc->mtx); | ||||
Done Inline ActionsThis copy can be performed outside of the locked region. The IO completion updates the read/write counters in a different thread, and thus, the code here (and there) require a lock to maintain the values' consistencies. chuck: This copy can be performed outside of the locked region.
The IO completion updates the… | |||||
nvme_prp_memcpy(sc->nsc_pi->pi_vmctx, command->prp1, | nvme_prp_memcpy(sc->nsc_pi->pi_vmctx, command->prp1, | ||||
command->prp2, (uint8_t *)&sc->health_log + logoff, | command->prp2, (uint8_t *)&sc->health_log + logoff, | ||||
MIN(logsize - logoff, sizeof(sc->health_log)), | MIN(logsize - logoff, sizeof(sc->health_log)), | ||||
NVME_COPY_TO_PRP); | NVME_COPY_TO_PRP); | ||||
break; | break; | ||||
case NVME_LOG_FIRMWARE_SLOT: | case NVME_LOG_FIRMWARE_SLOT: | ||||
if (logoff >= sizeof(sc->fw_log)) { | if (logoff >= sizeof(sc->fw_log)) { | ||||
▲ Show 20 Lines • Show All 1,887 Lines • Show Last 20 Lines |
I think style(9) suggests this shouldn't be done:
but perhaps I'm misinterpreting it.