Page MenuHomeFreeBSD

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

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

Details

Reviewers
jhb
grehan
Group Reviewers
bhyve
Summary

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
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 30129
Build 27936: arc lint + arc unit

Event Timeline

sjorge_ml_blackdot.be added inline comments.
usr.sbin/bhyve/pci_ahci.c
2379

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?

usr.sbin/bhyve/pci_ahci.c
2379

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

usr.sbin/bhyve/pci_ahci.c
2379

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

usr.sbin/bhyve/pci_ahci.c
2379

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.
wanpengqian_gmail.com 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
wanpengqian_gmail.com edited the summary of this revision. (Show Details)
wanpengqian_gmail.com edited the test plan for this revision. (Show Details)
jhb added a subscriber: jhb.Mar 26 2020, 6:43 PM
jhb added inline comments.
usr.sbin/bhyve/pci_ahci.c
986–1030

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.

1047

Nit: space before =

2344

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

2370

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;
          } 
     }
     free(uopts);
     fclose(fp);
2455

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.
usr.sbin/bhyve/pci_ahci.c
986–1030

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.

1047

done.

2344

I have corrected it.

2370

Fixed with this suggestion.

2455

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?