Page MenuHomeFreeBSD

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

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sjorge_ml_blackdot.be added inline comments.
usr.sbin/bhyve/pci_ahci.c
2361 ↗(On Diff #69823)

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
2361 ↗(On Diff #69823)

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
2361 ↗(On Diff #69823)

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

usr.sbin/bhyve/pci_ahci.c
2361 ↗(On Diff #69823)

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 ↗(On Diff #69846)

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.

1003 ↗(On Diff #69846)

Nit: space before =

2332 ↗(On Diff #69846)

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

2356 ↗(On Diff #69846)

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);
2415 ↗(On Diff #69846)

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 ↗(On Diff #69846)

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.

1003 ↗(On Diff #69846)

done.

2332 ↗(On Diff #69846)

I have corrected it.

2356 ↗(On Diff #69846)

Fixed with this suggestion.

2415 ↗(On Diff #69846)

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.

grehan accepted this revision.Jun 17 2020, 11:06 AM

That all sounds reasonable: looks fine to me.

This revision is now accepted and ready to land.Jun 17 2020, 11:06 AM

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

This revision now requires review to proceed.Jun 22 2020, 2:06 AM
jhb accepted this revision.Jun 22 2020, 10:09 PM

Only some minor nits.

usr.sbin/bhyve/pci_ahci.c
991 ↗(On Diff #73452)

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

993 ↗(On Diff #73452)

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

1034 ↗(On Diff #73452)

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.

1134 ↗(On Diff #73452)

Same thoughts here about ahci_checksum and dropping ata_ident local variable.

2384 ↗(On Diff #73452)

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

This revision is now accepted and ready to land.Jun 22 2020, 10:09 PM
usr.sbin/bhyve/pci_ahci.c
991 ↗(On Diff #73452)

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.

993 ↗(On Diff #73452)

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

  • Fix code style as suggested.
This revision now requires review to proceed.Jun 23 2020, 1:26 AM
wanpengqian_gmail.com marked 9 inline comments as done.Jun 23 2020, 1:28 AM
jhb added inline comments.Jun 23 2020, 4:47 PM
usr.sbin/bhyve/pci_ahci.c
991 ↗(On Diff #73452)

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.

993 ↗(On Diff #73452)

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.

  • Update code as comment suggested.
wanpengqian_gmail.com marked 5 inline comments as done.Jun 24 2020, 4:28 AM

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
993 ↗(On Diff #73452)

Yes, now I understand your meaning. thanks.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 27 2020, 7:57 AM
This revision was automatically updated to reflect the committed changes.