Page MenuHomeFreeBSD

bhyve AHCI controller will accept ser/rev/model/nmrr config
Needs ReviewPublic

Authored by on Mar 24 2020, 11:08 AM.


Group Reviewers

Currently AHCI has hard coded ATAPI Identify information of
Serial, Firmware Revision and Model Number. also cannot set
Nominal Media Rotation Rate field value. (this value can
indicate device as SSD)

This patch will let AHCI controller accept such values.

Test Plan

Boot from Windows/Linux/FreeBSD guests.
check the setting value.

Diff Detail

Lint OK
No Unit Test Coverage
Build Status
Buildable 30126
Build 27933: arc lint + arc unit

Event Timeline added inline comments.

I have a hard time parsing the first bit of this comment?
We process only things we can use and pass the unknown stuff to blockif?


Yes, for example, commandline is
-s 4:0,ahci,hd:/zones/vm/ahci0/disk0.img,nmrr=7200,model=SG110022,rev=07789,sectorsize=512
obviously nmrr/model/rev are settings for AHCI controller(disk), so we take these values here.
while block path and sectorsize will process by block_if.c


Perhaps reword the comment to something like:
Consume arguments valid for ahci, pass others to blockif ?


Thanks, I will correct it next update.

fix a unfree memory.
restore unused0 varible.
replace fix value defined in ata.h
fix comment.

  • fix coding style. retitled this revision from bhyve AHCI controller will accept ser/rev/model/nmrr config. to bhyve AHCI controller will accept ser/rev/model/nmrr config.Mar 25 2020, 1:28 AM edited the summary of this revision. (Show Details) edited the test plan for this revision. (Show Details)
jhb added a subscriber: jhb.Mar 26 2020, 6:43 PM
jhb added inline comments.

So I think if we are going to store this structure, we should just initialize it once when the port is created rather than redoing it each time? Either that or we should keep 'ata_ident' on the stack I think.


Nit: space before =


You don't need the cast to void * for C. Also, leave a blank line before the following comment.


Eh, you have passed the wrong size to calloc, and you still did a memset after the calloc? However, it is also a bit messier to try to do all this by hand. I would use a string builder that already does the memory management for you, either libsbuf or open_memstream(3), for example:

    FILE *fp;
    char *block_opts;
    size_t block_len;
    int comma;


    opt = strdup(opts);
    block_opts = NULL;
    fp = open_memstream(&block_opts, &block_len);
    for (opts = strtok(uopt, ","); xopts != NULL; xopts = strtok(NULL, ",")) {
          if ((config = strchr(xopts, "=")) != NULL)
              *config++ = '\0';

          if (!strcmp("nmrr", xopts) {
          } else {
             /* Pass all other options to blockif_open. */
             if (config != NULL)
                 *--config = '=';
             fprintf(fp, "%s%s", comma ? "," : "", xopts);
             comma = 1;

So what I would do instead is to set these initial values up before you parse options and then let the options override them perhaps instead of checking this. Also, space after if.

  • deal with feedback.

Yes, at first I want to do that, But it require a little more modification.
Now I am creating a ata_identify_init() function, when port initialize, this function will called.




I have corrected it.


Fixed with this suggestion.


Alright. I try my best to do as you said.
But the serial number require block filename.
So the code is a little strange now, But it works.

  • Fix a code style.
  • Inital serial/rev/model first.
  • Update bhyve manpage
  • Update bhyve manpage

Please give me any suggestion on this patch? good to go or not?

I don't have a problem with the serial, f/w rev or model number parts of this.

However, the "Nominal Media Rotation Rate" seems a roundabout way to specify an SSD. If all SSDs have that parameter as non-zero, how about naming the device model "ahci-ssd" instead ?

We have ahci-cd and ahci-hd, because cd and hdd are different a lot. while hdd and ssd, just only a little difference.
I am fine with ahci-ssd, but 'Nominal Media Rotation Rate' can also benefit when user try to specify the rotation rate, such as 5400/7200/15000 etc. since I can't image why user specify such value for vm.