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

[wanpengqian_gmail.com]

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.

[sjorge_ml_blackdot.be]

usr.sbin/bhyve/pci_ahci.c

2380

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?

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_ahci.c

2380

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

[sjorge_ml_blackdot.be]

usr.sbin/bhyve/pci_ahci.c

2380

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

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_ahci.c

2380

Thanks, I will correct it next update.

[wanpengqian_gmail.com]

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

[wanpengqian_gmail.com]

• fix coding style.

[jhb]

usr.sbin/bhyve/pci_ahci.c

986–1031

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.

1048

Nit: space before =

2345

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

2371

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.

[wanpengqian_gmail.com]

• deal with feedback.

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_ahci.c

986–1031

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.

1048

done.

2345

I have corrected it.

2371

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.

[wanpengqian_gmail.com]

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

[wanpengqian_gmail.com]

• Update bhyve manpage

[wanpengqian_gmail.com]

• Update bhyve manpage

[wanpengqian_gmail.com]

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

[grehan]

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 ?

[wanpengqian_gmail.com]

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.

[grehan]

That all sounds reasonable: looks fine to me.

[wanpengqian_gmail.com]

Fix a manpage typo in bhyve.8
Setting serial number in command line should be ser instead of serial.

[jhb]

Only some minor nits.

usr.sbin/bhyve/pci_ahci.c

987

I think the ahci_checksum()'s can move into ata_identify_init()?

989

You could then probably just use &p->ata_ident here directly without need for the ata_ident local variable.

1030

Tiny style nit: please put the else { on the same line as the previous { here and in a few other places late run the patch.

1131

Same thoughts here about ahci_checksum and dropping ata_ident local variable.

2380

Please use an older C-style comment (/* .. */) rather than //.

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_ahci.c

987

The previous code logic is putting ahci_checksum() here. so I think is better leave as it is.
I will create a new patch for this later.

989

I just thinking create a local variable here will make code more readable.
May I know why?

[wanpengqian_gmail.com]

• Fix code style as suggested.

[jhb]

usr.sbin/bhyve/pci_ahci.c

987

I guess I view this as part of generating p->ata_idents value. The checksum won't change since the value of p->ata_ident is constant once ata_identify_init completes. This seems consistent my earlier request of generating p->ata_ident once instead of on each identify command.

989

If it's only for a single use, then I'm not sure it helps. You also don't really need the cast anymore, plus the existing version has a style bug in not having a blank line between the variable declaration and the code and removing the variable seems the simpler fix for that. That is all conditional on moving ahci_checksum however as it is only that change that changes the variable to being single-use.

[wanpengqian_gmail.com]

• Update code as comment suggested.

[wanpengqian_gmail.com]

Drop ahci_checksum() from handle_identify() and handle_atapi_identify(),
The checksum caculation will happen at the end of ata_identify_init() function.
Also drop local variables which only use one time.

usr.sbin/bhyve/pci_ahci.c

989

Yes, now I understand your meaning. thanks.