Page MenuHomeFreeBSD
Differential D24174 Diff 69846 usr.sbin/bhyve/pci_ahci.c

Changeset View

Standalone View

View Options

usr.sbin/bhyve/pci_ahci.c

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Linesstruct ahci_ioreq {
uint32_t done; uint32_t done;
int slot; int slot;
int more; int more;
}; };
struct ahci_port { struct ahci_port {
struct blockif_ctxt *bctx; struct blockif_ctxt *bctx;
struct pci_ahci_softc *pr_sc; struct pci_ahci_softc *pr_sc;
struct ata_params ata_ident;
uint8_t *cmd_lst; uint8_t *cmd_lst;
uint8_t *rfis; uint8_t *rfis;
char ident[AHCI_PORT_IDENT];
int port; int port;
int atapi; int atapi;
int reset; int reset;
int waitforclear; int waitforclear;
int mult_sectors; int mult_sectors;
uint8_t xfermode; uint8_t xfermode;
uint8_t err_cfis[20]; uint8_t err_cfis[20];
uint8_t sense_key; uint8_t sense_key;
▲ Show 20 Lines • Show All 828 Lines • ▼ Show 20 Lines
{ {
struct ahci_cmd_hdr *hdr; struct ahci_cmd_hdr *hdr;
hdr = (struct ahci_cmd_hdr *)(p->cmd_lst + slot * AHCI_CL_SIZE); hdr = (struct ahci_cmd_hdr *)(p->cmd_lst + slot * AHCI_CL_SIZE);
if (p->atapi || hdr->prdtl == 0) { if (p->atapi || hdr->prdtl == 0) {
ahci_write_fis_d2h(p, slot, cfis, ahci_write_fis_d2h(p, slot, cfis,
(ATA_E_ABORT << 8) | ATA_S_READY | ATA_S_ERROR); (ATA_E_ABORT << 8) | ATA_S_READY | ATA_S_ERROR);
} else { } else {
uint16_t buf[256]; struct ata_params *ata_ident = &p->ata_ident;
jhbUnsubmitted
Done
Inline Actions

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.

jhb: So I think if we are going to store this structure, we should just initialize it once when the…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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.

wanpengqian_gmail.com: Yes, at first I want to do that, But it require a little more modification. Now I am creating a…
uint64_t sectors; uint64_t sectors;
jhbUnsubmitted
Done
Inline Actions

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

jhb: I think the ahci_checksum()'s can move into ata_identify_init()?
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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.

wanpengqian_gmail.com: The previous code logic is putting ahci_checksum() here. so I think is better leave as it is. I…
jhbUnsubmitted
Done
Inline Actions

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.

jhb: I guess I view this as part of generating `p->ata_ident`s value. The checksum won't change…
int sectsz, psectsz, psectoff, candelete, ro; int sectsz, psectsz, psectoff, candelete, ro;
uint16_t cyl; uint16_t cyl;
jhbUnsubmitted
Done
Inline Actions

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

jhb: You could then probably just use `&p->ata_ident` here directly without need for the `ata_ident`…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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

wanpengqian_gmail.com: I just thinking create a local variable here will make code more readable. May I know why?
jhbUnsubmitted
Not Done
Inline Actions

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.

jhb: If it's only for a single use, then I'm not sure it helps. You also don't really need the cast…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

Yes, now I understand your meaning. thanks.

wanpengqian_gmail.com: Yes, now I understand your meaning. thanks.
uint8_t sech, heads; uint8_t sech, heads;
ro = blockif_is_ro(p->bctx); ro = blockif_is_ro(p->bctx);
candelete = blockif_candelete(p->bctx); candelete = blockif_candelete(p->bctx);
sectsz = blockif_sectsz(p->bctx); sectsz = blockif_sectsz(p->bctx);
sectors = blockif_size(p->bctx) / sectsz; sectors = blockif_size(p->bctx) / sectsz;
blockif_chs(p->bctx, &cyl, &heads, &sech); blockif_chs(p->bctx, &cyl, &heads, &sech);
blockif_psectsz(p->bctx, &psectsz, &psectoff); blockif_psectsz(p->bctx, &psectsz, &psectoff);
memset(buf, 0, sizeof(buf)); ata_ident->config = ATA_DRQ_FAST;
buf[0] = 0x0040; ata_ident->cylinders = cyl;
buf[1] = cyl; ata_ident->heads = heads;
buf[3] = heads; ata_ident->sectors= sech;
buf[6] = sech;
ata_string((uint8_t *)(buf+10), p->ident, 20); ata_ident->sectors_intr= (0x8000 | 128);
jhbUnsubmitted
Done
Inline Actions

Nit: space before =

jhb: Nit: space before `=`
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

done.

wanpengqian_gmail.com: done.
ata_string((uint8_t *)(buf+23), "001", 8); ata_ident->tcg = 0;
ata_string((uint8_t *)(buf+27), "BHYVE SATA DISK", 40);
buf[47] = (0x8000 | 128); ata_ident->capabilities1 = ATA_SUPPORT_DMA |
buf[48] = 0; ATA_SUPPORT_LBA | ATA_SUPPORT_IORDY;
buf[49] = (1 << 8 | 1 << 9 | 1 << 11); ata_ident->capabilities2 = (1 << 14);
buf[50] = (1 << 14); ata_ident->atavalid = ATA_FLAG_54_58 |
buf[53] = (1 << 1 | 1 << 2); ATA_FLAG_64_70;
if (p->mult_sectors) if (p->mult_sectors)
buf[59] = (0x100 | p->mult_sectors); ata_ident->multi = (ATA_MULTI_VALID | p->mult_sectors);
if (sectors <= 0x0fffffff) { if (sectors <= 0x0fffffff) {
buf[60] = sectors; ata_ident->lba_size_1 = sectors;
buf[61] = (sectors >> 16); ata_ident->lba_size_2 = (sectors >> 16);
} else { } else {
buf[60] = 0xffff; ata_ident->lba_size_1 = 0xffff;
buf[61] = 0x0fff; ata_ident->lba_size_2 = 0x0fff;
} }
buf[63] = 0x7; ata_ident->mwdmamodes = 0x7;
if (p->xfermode & ATA_WDMA0) if (p->xfermode & ATA_WDMA0)
buf[63] |= (1 << ((p->xfermode & 7) + 8)); ata_ident->mwdmamodes |= (1 << ((p->xfermode & 7) + 8));
buf[64] = 0x3; ata_ident->apiomodes = 0x3;
buf[65] = 120; ata_ident->mwdmamin = 0x0078;
buf[66] = 120; ata_ident->mwdmarec = 0x0078;
buf[67] = 120; ata_ident->pioblind = 0x0078;
buf[68] = 120; ata_ident->pioiordy = 0x0078;
buf[69] = 0; ata_ident->support3 = 0;
buf[75] = 31; ata_ident->queue = 31;
buf[76] = (ATA_SATA_GEN1 | ATA_SATA_GEN2 | ATA_SATA_GEN3 | ata_ident->satacapabilities = (ATA_SATA_GEN1 | ATA_SATA_GEN2 | ATA_SATA_GEN3 |
jhbUnsubmitted
Done
Inline Actions

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.

jhb: Tiny style nit: please put the `else {` on the same line as the previous `{` here and in a few…
ATA_SUPPORT_NCQ); ATA_SUPPORT_NCQ);
buf[77] = (ATA_SUPPORT_RCVSND_FPDMA_QUEUED | ata_ident->satacapabilities2 = (ATA_SUPPORT_RCVSND_FPDMA_QUEUED |
(p->ssts & ATA_SS_SPD_MASK) >> 3); (p->ssts & ATA_SS_SPD_MASK) >> 3);
buf[80] = 0x3f0; ata_ident->version_major = 0x3f0;
buf[81] = 0x28; ata_ident->version_minor = 0x28;
buf[82] = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_WRITECACHE| ata_ident->support.command1 = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_WRITECACHE|
ATA_SUPPORT_LOOKAHEAD | ATA_SUPPORT_NOP); ATA_SUPPORT_LOOKAHEAD | ATA_SUPPORT_NOP);
buf[83] = (ATA_SUPPORT_ADDRESS48 | ATA_SUPPORT_FLUSHCACHE | ata_ident->support.command2 = (ATA_SUPPORT_ADDRESS48 | ATA_SUPPORT_FLUSHCACHE |
ATA_SUPPORT_FLUSHCACHE48 | 1 << 14); ATA_SUPPORT_FLUSHCACHE48 | 1 << 14);
buf[84] = (1 << 14); ata_ident->support.extension = (1 << 14);
buf[85] = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_WRITECACHE| ata_ident->enabled.command1 = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_WRITECACHE|
ATA_SUPPORT_LOOKAHEAD | ATA_SUPPORT_NOP); ATA_SUPPORT_LOOKAHEAD | ATA_SUPPORT_NOP);
buf[86] = (ATA_SUPPORT_ADDRESS48 | ATA_SUPPORT_FLUSHCACHE | ata_ident->enabled.command2 = (ATA_SUPPORT_ADDRESS48 | ATA_SUPPORT_FLUSHCACHE |
ATA_SUPPORT_FLUSHCACHE48 | 1 << 15); ATA_SUPPORT_FLUSHCACHE48 | 1 << 15);
buf[87] = (1 << 14); ata_ident->enabled.extension = (1 << 14);
buf[88] = 0x7f; ata_ident->udmamodes = 0x7f;
if (p->xfermode & ATA_UDMA0) if (p->xfermode & ATA_UDMA0)
buf[88] |= (1 << ((p->xfermode & 7) + 8)); ata_ident->udmamodes |= (1 << ((p->xfermode & 7) + 8));
buf[100] = sectors; ata_ident->lba_size48_1 = sectors;
buf[101] = (sectors >> 16); ata_ident->lba_size48_2 = (sectors >> 16);
buf[102] = (sectors >> 32); ata_ident->lba_size48_3 = (sectors >> 32);
buf[103] = (sectors >> 48); ata_ident->lba_size48_4 = (sectors >> 48);
if (candelete && !ro) { if (candelete && !ro) {
buf[69] |= ATA_SUPPORT_RZAT | ATA_SUPPORT_DRAT; ata_ident->support3 |= ATA_SUPPORT_RZAT | ATA_SUPPORT_DRAT;
buf[105] = 1; ata_ident->max_dsm_blocks = 1;
buf[169] = ATA_SUPPORT_DSM_TRIM; ata_ident->support_dsm = ATA_SUPPORT_DSM_TRIM;
} }
buf[106] = 0x4000; ata_ident->pss = ATA_PSS_VALID_VALUE;
buf[209] = 0x4000; ata_ident->lsalign = 0x4000;
if (psectsz > sectsz) { if (psectsz > sectsz) {
buf[106] |= 0x2000; ata_ident->pss |= ATA_PSS_MULTLS;
buf[106] |= ffsl(psectsz / sectsz) - 1; ata_ident->pss |= ffsl(psectsz / sectsz) - 1;
buf[209] |= (psectoff / sectsz); ata_ident->lsalign |= (psectoff / sectsz);
} }
if (sectsz > 512) { if (sectsz > 512) {
buf[106] |= 0x1000; ata_ident->pss |= ATA_PSS_LSSABOVE512;
buf[117] = sectsz / 2; ata_ident->lss_1 = sectsz / 2;
buf[118] = ((sectsz / 2) >> 16); ata_ident->lss_2 = ((sectsz / 2) >> 16);
} }
buf[119] = (ATA_SUPPORT_RWLOGDMAEXT | 1 << 14); ata_ident->support2 = (ATA_SUPPORT_RWLOGDMAEXT | 1 << 14);
buf[120] = (ATA_SUPPORT_RWLOGDMAEXT | 1 << 14); ata_ident->enabled2 = (ATA_SUPPORT_RWLOGDMAEXT | 1 << 14);
buf[222] = 0x1020; ata_ident->transport_major = 0x1020;
buf[255] = 0x00a5; ata_ident->integrity = 0x00a5;
ahci_checksum((uint8_t *)buf, sizeof(buf)); ahci_checksum((uint8_t *)(ata_ident), sizeof(struct ata_params));
ahci_write_fis_piosetup(p); ahci_write_fis_piosetup(p);
write_prdt(p, slot, cfis, (void *)buf, sizeof(buf)); write_prdt(p, slot, cfis, (void *)ata_ident, sizeof(struct ata_params));
ahci_write_fis_d2h(p, slot, cfis, ATA_S_DSC | ATA_S_READY); ahci_write_fis_d2h(p, slot, cfis, ATA_S_DSC | ATA_S_READY);
} }
} }
static void static void
handle_atapi_identify(struct ahci_port *p, int slot, uint8_t *cfis) handle_atapi_identify(struct ahci_port *p, int slot, uint8_t *cfis)
{ {
if (!p->atapi) { if (!p->atapi) {
ahci_write_fis_d2h(p, slot, cfis, ahci_write_fis_d2h(p, slot, cfis,
(ATA_E_ABORT << 8) | ATA_S_READY | ATA_S_ERROR); (ATA_E_ABORT << 8) | ATA_S_READY | ATA_S_ERROR);
} else { } else {
uint16_t buf[256]; struct ata_params *ata_ident = &p->ata_ident;
memset(buf, 0, sizeof(buf)); ata_ident->config = (2 << 14 | 5 << 8 | 1 << 7 | 2 << 5);
buf[0] = (2 << 14 | 5 << 8 | 1 << 7 | 2 << 5); ata_ident->capabilities1 = ATA_SUPPORT_LBA |
ata_string((uint8_t *)(buf+10), p->ident, 20); ATA_SUPPORT_DMA;
ata_string((uint8_t *)(buf+23), "001", 8); ata_ident->capabilities2 = (1 << 14 | 1);
ata_string((uint8_t *)(buf+27), "BHYVE SATA DVD ROM", 40); ata_ident->atavalid = ATA_FLAG_54_58 | ATA_FLAG_64_70;
buf[49] = (1 << 9 | 1 << 8); ata_ident->obsolete62 = 0x3f;
buf[50] = (1 << 14 | 1); ata_ident->mwdmamodes = 7;
buf[53] = (1 << 2 | 1 << 1);
buf[62] = 0x3f;
buf[63] = 7;
if (p->xfermode & ATA_WDMA0) if (p->xfermode & ATA_WDMA0)
buf[63] |= (1 << ((p->xfermode & 7) + 8)); ata_ident->mwdmamodes |= (1 << ((p->xfermode & 7) + 8));
buf[64] = 3; ata_ident->apiomodes = 3;
buf[65] = 120; ata_ident->mwdmamin = 0x0078;
buf[66] = 120; ata_ident->mwdmarec = 0x0078;
buf[67] = 120; ata_ident->pioblind = 0x0078;
buf[68] = 120; ata_ident->pioiordy = 0x0078;
buf[76] = (ATA_SATA_GEN1 | ATA_SATA_GEN2 | ATA_SATA_GEN3); ata_ident->satacapabilities = (ATA_SATA_GEN1 | ATA_SATA_GEN2 | ATA_SATA_GEN3);
buf[77] = ((p->ssts & ATA_SS_SPD_MASK) >> 3); ata_ident->satacapabilities2 = ((p->ssts & ATA_SS_SPD_MASK) >> 3);
buf[78] = (1 << 5); ata_ident->satasupport = ATA_SUPPORT_NCQ_STREAM;
buf[80] = 0x3f0; ata_ident->version_major = 0x3f0;
buf[82] = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_PACKET | ata_ident->support.command1 = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_PACKET |
ATA_SUPPORT_RESET | ATA_SUPPORT_NOP); ATA_SUPPORT_RESET | ATA_SUPPORT_NOP);
buf[83] = (1 << 14); ata_ident->support.command2 = (1 << 14);
buf[84] = (1 << 14); ata_ident->support.extension = (1 << 14);
buf[85] = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_PACKET | ata_ident->enabled.command1 = (ATA_SUPPORT_POWERMGT | ATA_SUPPORT_PACKET |
ATA_SUPPORT_RESET | ATA_SUPPORT_NOP); ATA_SUPPORT_RESET | ATA_SUPPORT_NOP);
buf[87] = (1 << 14); ata_ident->enabled.extension = (1 << 14);
buf[88] = 0x7f; ata_ident->udmamodes = 0x7f;
if (p->xfermode & ATA_UDMA0) if (p->xfermode & ATA_UDMA0)
buf[88] |= (1 << ((p->xfermode & 7) + 8)); ata_ident->udmamodes |= (1 << ((p->xfermode & 7) + 8));
buf[222] = 0x1020; ata_ident->transport_major = 0x1020;
jhbUnsubmitted
Done
Inline Actions

Same thoughts here about ahci_checksum and dropping ata_ident local variable.

jhb: Same thoughts here about `ahci_checksum` and dropping `ata_ident` local variable.
buf[255] = 0x00a5; ata_ident->integrity = 0x00a5;
ahci_checksum((uint8_t *)buf, sizeof(buf)); ahci_checksum((uint8_t *)(ata_ident), sizeof(struct ata_params));
ahci_write_fis_piosetup(p); ahci_write_fis_piosetup(p);
write_prdt(p, slot, cfis, (void *)buf, sizeof(buf)); write_prdt(p, slot, cfis, (void *)ata_ident, sizeof(struct ata_params));
ahci_write_fis_d2h(p, slot, cfis, ATA_S_DSC | ATA_S_READY); ahci_write_fis_d2h(p, slot, cfis, ATA_S_DSC | ATA_S_READY);
} }
} }
static void static void
atapi_inquiry(struct ahci_port *p, int slot, uint8_t *cfis) atapi_inquiry(struct ahci_port *p, int slot, uint8_t *cfis)
{ {
uint8_t buf[36]; uint8_t buf[36];
▲ Show 20 Lines • Show All 1,175 Lines • ▼ Show 20 Lines
{ {
char bident[sizeof("XX:XX:XX")]; char bident[sizeof("XX:XX:XX")];
struct blockif_ctxt *bctxt; struct blockif_ctxt *bctxt;
struct pci_ahci_softc *sc; struct pci_ahci_softc *sc;
int ret, slots, p; int ret, slots, p;
MD5_CTX mdctx; MD5_CTX mdctx;
u_char digest[16]; u_char digest[16];
char *next, *next2; char *next, *next2;
char *bopt, *uopt, *xopts, *config;
ret = 0; ret = 0;
#ifdef AHCI_DEBUG #ifdef AHCI_DEBUG
dbg = fopen("/tmp/log", "w+"); dbg = fopen("/tmp/log", "w+");
#endif #endif
sc = calloc(1, sizeof(struct pci_ahci_softc)); sc = calloc(1, sizeof(struct pci_ahci_softc));
pi->pi_arg = sc; pi->pi_arg = sc;
sc->asc_pi = pi; sc->asc_pi = pi;
pthread_mutex_init(&sc->mtx, NULL); pthread_mutex_init(&sc->mtx, NULL);
sc->ports = 0; sc->ports = 0;
sc->pi = 0; sc->pi = 0;
slots = 32; slots = 32;
for (p = 0; p < MAX_PORTS && opts != NULL; p++, opts = next) { for (p = 0; p < MAX_PORTS && opts != NULL; p++, opts = next) {
memset((void *)(&sc->port[p].ata_ident), 0, sizeof(struct ata_params));
jhbUnsubmitted
Done
Inline Actions

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

jhb: You don't need the cast to `void *` for C. Also, leave a blank line before the following…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

I have corrected it.

wanpengqian_gmail.com: I have corrected it.
/* Identify and cut off type of present port. */ /* Identify and cut off type of present port. */
if (strncmp(opts, "hd:", 3) == 0) { if (strncmp(opts, "hd:", 3) == 0) {
atapi = 0; atapi = 0;
opts += 3; opts += 3;
} else if (strncmp(opts, "cd:", 3) == 0) { } else if (strncmp(opts, "cd:", 3) == 0) {
atapi = 1; atapi = 1;
opts += 3; opts += 3;
} }
/* Find and cut off the next port options. */ /* Find and cut off the next port options. */
next = strstr(opts, ",hd:"); next = strstr(opts, ",hd:");
next2 = strstr(opts, ",cd:"); next2 = strstr(opts, ",cd:");
if (next == NULL || (next2 != NULL && next2 < next)) if (next == NULL || (next2 != NULL && next2 < next))
next = next2; next = next2;
if (next != NULL) { if (next != NULL) {
next[0] = 0; next[0] = 0;
next++; next++;
} }
if (opts[0] == 0) if (opts[0] == 0)
continue; continue;
uopt = strdup(opts);
bopt = calloc(1, sizeof(*opts));
jhbUnsubmitted
Done
Inline Actions

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);
jhb: Eh, you have passed the wrong size to calloc, and you still did a memset after the calloc?
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

Fixed with this suggestion.

wanpengqian_gmail.com: Fixed with this suggestion.
memset(bopt,0,strlen(bopt));
int optpos=0;
for (xopts = strtok(uopt, ",");
xopts != NULL;
xopts = strtok(NULL, ",")) {
int concat = 1;
/* /*
* Consume arguments valid for ahci, pass others to blockif_open
sjorge_ml_blackdot.beUnsubmitted
Done
Inline Actions

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?

sjorge_ml_blackdot.be: I have a hard time parsing the first bit of this comment? We process only things we can use and…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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

wanpengqian_gmail.com: Yes, for example, commandline is -s 4:0,ahci,hd:/zones/vm/ahci0/disk0.img,nmrr=7200…
sjorge_ml_blackdot.beUnsubmitted
Done
Inline Actions

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

sjorge_ml_blackdot.be: Perhaps reword the comment to something like: `Consume arguments valid for ahci, pass others to…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

Thanks, I will correct it next update.

wanpengqian_gmail.com: Thanks, I will correct it next update.
jhbUnsubmitted
Done
Inline Actions

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

jhb: Please use an older C-style comment (/* .. */) rather than //.
*/
if ((config = strchr(xopts, '=')) != NULL) {
*config++ = '\0';
if (!strcmp("nmrr", xopts)) {
sc->port[p].ata_ident.media_rotation_rate = atoi(config);
concat = 0;
} else if(!strcmp("ser", xopts)) {
ata_string((uint8_t *)(&sc->port[p].ata_ident.serial), config, 20);
concat = 0;
} else if(!strcmp("rev", xopts)) {
ata_string((uint8_t *)(&sc->port[p].ata_ident.revision), config, 8);
concat = 0;
} else if(!strcmp("model", xopts)) {
ata_string((uint8_t *)(&sc->port[p].ata_ident.model), config, 40);
concat = 0;
}
*--config = '=';
}
if(concat) {
if(optpos) strcat(bopt, ",");
strcat(bopt, xopts);
}
optpos++;
}
free(uopt);
DPRINTF("%s\n", bopt);
/*
* Attempt to open the backing image. Use the PCI slot/func * Attempt to open the backing image. Use the PCI slot/func
* and the port number for the identifier string. * and the port number for the identifier string.
*/ */
snprintf(bident, sizeof(bident), "%d:%d:%d", pi->pi_slot, snprintf(bident, sizeof(bident), "%d:%d:%d", pi->pi_slot,
pi->pi_func, p); pi->pi_func, p);
bctxt = blockif_open(opts, bident); bctxt = blockif_open(bopt, bident);
free(bopt);
if (bctxt == NULL) { if (bctxt == NULL) {
sc->ports = p; sc->ports = p;
ret = 1; ret = 1;
goto open_fail; goto open_fail;
} }
sc->port[p].bctx = bctxt; sc->port[p].bctx = bctxt;
sc->port[p].pr_sc = sc; sc->port[p].pr_sc = sc;
sc->port[p].port = p; sc->port[p].port = p;
sc->port[p].atapi = atapi; sc->port[p].atapi = atapi;
/* /*
* Create an identifier for the backing file. * Create an identifier for the backing file.
* Use parts of the md5 sum of the filename * Use parts of the md5 sum of the filename
*/ */
if(strlen(sc->port[p].ata_ident.serial) == 0) {
jhbUnsubmitted
Done
Inline Actions

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.

jhb: So what I would do instead is to set these initial values up before you parse options and then…
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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: Alright. I try my best to do as you said. But the serial number require block filename. So the…
char ident[AHCI_PORT_IDENT];
MD5Init(&mdctx); MD5Init(&mdctx);
MD5Update(&mdctx, opts, strlen(opts)); MD5Update(&mdctx, opts, strlen(opts));
MD5Final(digest, &mdctx); MD5Final(digest, &mdctx);
snprintf(sc->port[p].ident, AHCI_PORT_IDENT, snprintf(ident, AHCI_PORT_IDENT,
"BHYVE-%02X%02X-%02X%02X-%02X%02X", "BHYVE-%02X%02X-%02X%02X-%02X%02X",
digest[0], digest[1], digest[2], digest[3], digest[4], digest[0], digest[1], digest[2], digest[3], digest[4],
digest[5]); digest[5]);
ata_string((uint8_t *)&sc->port[p].ata_ident.serial, ident, 20);
}
if(strlen(sc->port[p].ata_ident.revision) == 0) {
ata_string((uint8_t *)&sc->port[p].ata_ident.revision, "001", 8);
}
if(strlen(sc->port[p].ata_ident.model) == 0) {
if(atapi) {
ata_string((uint8_t *)&sc->port[p].ata_ident.model, "BHYVE SATA DVD ROM", 40);
} else {
ata_string((uint8_t *)&sc->port[p].ata_ident.model, "BHYVE SATA DISK", 40);
}
}
/* /*
* Allocate blockif request structures and add them * Allocate blockif request structures and add them
* to the free list * to the free list
*/ */
pci_ahci_ioreq_init(&sc->port[p]); pci_ahci_ioreq_init(&sc->port[p]);
sc->pi |= (1 << p); sc->pi |= (1 << p);
▲ Show 20 Lines • Show All 84 Lines • Show Last 20 Lines