VirtualBox

Changeset 81848 in vbox for trunk/src/VBox/Devices/Storage


Ignore:
Timestamp:
Nov 14, 2019 9:09:14 PM (5 years ago)
Author:
vboxsync
svn:sync-xref-src-repo-rev:
134642
Message:

DevATA: Be paranoid when using state members to index ATACONTROLLER::aIfs. bugref:9218

File:
1 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/VBox/Devices/Storage/DevATA.cpp

    r81847 r81848  
    8585/** The maxium I/O buffer size (for sanity). */
    8686#define ATA_MAX_IO_BUFFER_SIZE      (ATA_MAX_MULT_SECTORS * ATA_MAX_SECTOR_SIZE)
     87
     88/** Mask to be applied to all indexing into ATACONTROLLER::aIfs. */
     89#define ATA_SELECTED_IF_MASK        1
    8790
    8891/**
    … …  
    10771080            pCtl->BmDma.u8Status |= BM_STATUS_INT;
    10781081            /* Only actually set the IRQ line if updating the currently selected drive. */
    1079             if (s == &pCtl->aIfs[pCtl->iSelectedIf])
     1082            if (s == &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK])
    10801083            {
    10811084                /** @todo experiment with adaptive IRQ delivery: for reads it is
    … …  
    11081111            Log2(("%s: LUN#%d deasserting IRQ\n", __FUNCTION__, s->iLUN));
    11091112            /* Only actually unset the IRQ line if updating the currently selected drive. */
    1110             if (s == &pCtl->aIfs[pCtl->iSelectedIf])
     1113            if (s == &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK])
    11111114            {
    11121115                if (pCtl->irq == 16)
    … …  
    43924395static int ataIOPortWriteU8(PATACONTROLLER pCtl, uint32_t addr, uint32_t val)
    43934396{
    4394     Log2(("%s: LUN#%d write addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN, addr, val));
     4397    Log2(("%s: LUN#%d write addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].iLUN, addr, val));
    43954398    addr &= 7;
    43964399    switch (addr)
    … …  
    44424445            pCtl->aIfs[0].uATARegSelect = (val & ~0x10) | 0xa0;
    44434446            pCtl->aIfs[1].uATARegSelect = (val | 0x10) | 0xa0;
    4444             if (((val >> 4) & 1) != pCtl->iSelectedIf)
     4447            if (((val >> 4) & ATA_SELECTED_IF_MASK) != pCtl->iSelectedIf)
    44454448            {
    44464449                PPDMDEVINS pDevIns = CONTROLLER_2_DEVINS(pCtl);
    44474450
    44484451                /* select another drive */
    4449                 pCtl->iSelectedIf = (val >> 4) & 1;
     4452                uintptr_t const iSelectedIf = (val >> 4) & ATA_SELECTED_IF_MASK;
     4453                pCtl->iSelectedIf = (uint8_t)iSelectedIf;
    44504454                /* The IRQ line is multiplexed between the two drives, so
    44514455                 * update the state when switching to another drive. Only need
    44524456                 * to update interrupt line if it is enabled and there is a
    44534457                 * state change. */
    4454                 if (    !(pCtl->aIfs[pCtl->iSelectedIf].uATARegDevCtl & ATA_DEVCTL_DISABLE_IRQ)
    4455                     &&  (   pCtl->aIfs[pCtl->iSelectedIf].fIrqPending
    4456                          !=  pCtl->aIfs[pCtl->iSelectedIf ^ 1].fIrqPending))
     4458                if (    !(pCtl->aIfs[iSelectedIf].uATARegDevCtl & ATA_DEVCTL_DISABLE_IRQ)
     4459                    &&  pCtl->aIfs[iSelectedIf].fIrqPending != pCtl->aIfs[iSelectedIf ^ 1].fIrqPending)
    44574460                {
    4458                     if (pCtl->aIfs[pCtl->iSelectedIf].fIrqPending)
     4461                    if (pCtl->aIfs[iSelectedIf].fIrqPending)
    44594462                    {
    4460                         Log2(("%s: LUN#%d asserting IRQ (drive select change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN));
     4463                        Log2(("%s: LUN#%d asserting IRQ (drive select change)\n", __FUNCTION__, pCtl->aIfs[iSelectedIf].iLUN));
    44614464                        /* The BMDMA unit unconditionally sets BM_STATUS_INT if
    44624465                         * the interrupt line is asserted. It monitors the line
    … …  
    44704473                    else
    44714474                    {
    4472                         Log2(("%s: LUN#%d deasserting IRQ (drive select change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN));
     4475                        Log2(("%s: LUN#%d deasserting IRQ (drive select change)\n", __FUNCTION__, pCtl->aIfs[iSelectedIf].iLUN));
    44734476                        if (pCtl->irq == 16)
    44744477                            PDMDevHlpPCISetIrq(pDevIns, 0, 0);
    … …  
    44814484        default:
    44824485        case 7: /* command */
     4486        {
    44834487            /* ignore commands to non-existent device */
    4484             if (pCtl->iSelectedIf && !pCtl->aIfs[pCtl->iSelectedIf].pDrvMedia)
     4488            uintptr_t iSelectedIf = pCtl->iSelectedIf & ATA_SELECTED_IF_MASK;
     4489            if (iSelectedIf && !pCtl->aIfs[iSelectedIf].pDrvMedia)
    44854490                break;
    44864491#ifndef IN_RING3
    … …  
    44884493            return VINF_IOM_R3_IOPORT_WRITE;
    44894494#else /* IN_RING3 */
    4490             ataUnsetIRQ(&pCtl->aIfs[pCtl->iSelectedIf]);
    4491             ataR3ParseCmd(&pCtl->aIfs[pCtl->iSelectedIf], val);
     4495            ataUnsetIRQ(&pCtl->aIfs[iSelectedIf]);
     4496            ataR3ParseCmd(&pCtl->aIfs[iSelectedIf], val);
    44924497#endif /* !IN_RING3 */
     4498        }
    44934499    }
    44944500    return VINF_SUCCESS;
    … …  
    44984504static int ataIOPortReadU8(PATACONTROLLER pCtl, uint32_t addr, uint32_t *pu32)
    44994505{
    4500     ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     4506    ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    45014507    uint32_t    val;
    45024508    bool        fHOB;
    … …  
    46764682static uint32_t ataStatusRead(PATACONTROLLER pCtl, uint32_t addr)
    46774683{
    4678     ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     4684    ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    46794685    uint32_t val;
    46804686    RT_NOREF1(addr);
    … …  
    46854691    else
    46864692        val = s->uATARegStatus;
    4687     Log2(("%s: LUN#%d read addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN, addr, val));
     4693    Log2(("%s: LUN#%d read addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].iLUN, addr, val));
    46884694    return val;
    46894695}
    … …  
    46974703#endif /* !IN_RING3 */
    46984704
    4699     Log2(("%s: LUN#%d write addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN, addr, val));
     4705    Log2(("%s: LUN#%d write addr=%#x val=%#04x\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].iLUN, addr, val));
    47004706    /* RESET is common for both drives attached to a controller. */
    47014707    if (   !(pCtl->aIfs[0].uATARegDevCtl & ATA_DEVCTL_RESET)
    … …  
    47684774     * is pending on the current interface. */
    47694775    if (   ((val ^ pCtl->aIfs[0].uATARegDevCtl) & ATA_DEVCTL_DISABLE_IRQ)
    4770         && pCtl->aIfs[pCtl->iSelectedIf].fIrqPending)
     4776        && pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].fIrqPending)
    47714777    {
    47724778        if (!(val & ATA_DEVCTL_DISABLE_IRQ))
    47734779        {
    4774             Log2(("%s: LUN#%d asserting IRQ (interrupt disable change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN));
     4780            Log2(("%s: LUN#%d asserting IRQ (interrupt disable change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].iLUN));
    47754781            /* The BMDMA unit unconditionally sets BM_STATUS_INT if the
    47764782             * interrupt line is asserted. It monitors the line for a rising
    … …  
    47844790        else
    47854791        {
    4786             Log2(("%s: LUN#%d deasserting IRQ (interrupt disable change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf].iLUN));
     4792            Log2(("%s: LUN#%d deasserting IRQ (interrupt disable change)\n", __FUNCTION__, pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].iLUN));
    47874793            if (pCtl->irq == 16)
    47884794                PDMDevHlpPCISetIrq(CONTROLLER_2_DEVINS(pCtl), 0, 0);
    … …  
    48074813    ATADevState *s;
    48084814
    4809     s = &pCtl->aIfs[pCtl->iAIOIf];
     4815    s = &pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK];
    48104816    Log3(("%s: if=%p\n", __FUNCTION__, s));
    48114817
    … …  
    48464852
    48474853        Log2(("%s: %s tx_size=%d elem_tx_size=%d index=%d end=%d\n",
    4848              __FUNCTION__, s->uTxDir == PDMMEDIATXDIR_FROM_DEVICE ? "T2I" : "I2T",
    4849              s->cbTotalTransfer, s->cbElementaryTransfer,
    4850              s->iIOBufferCur, s->iIOBufferEnd));
     4854              __FUNCTION__, s->uTxDir == PDMMEDIATXDIR_FROM_DEVICE ? "T2I" : "I2T",
     4855              s->cbTotalTransfer, s->cbElementaryTransfer,
     4856              s->iIOBufferCur, s->iIOBufferEnd));
    48514857        ataHCPIOTransferStart(s, s->iIOBufferCur, s->cbElementaryTransfer);
    48524858        s->cbTotalTransfer -= s->cbElementaryTransfer;
    … …  
    50105016    if (rc == VINF_SUCCESS)
    50115017    {
    5012         ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     5018        ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    50135019        uint32_t const iIOBufferPIODataStart = RT_MIN(s->iIOBufferPIODataStart, sizeof(s->abIOBuffer));
    50145020        uint32_t const iIOBufferPIODataEnd   = RT_MIN(s->iIOBufferPIODataEnd,   sizeof(s->abIOBuffer));
    … …  
    50855091    if (rc == VINF_SUCCESS)
    50865092    {
    5087         ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     5093        ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    50885094
    50895095        if (s->iIOBufferPIODataStart < s->iIOBufferPIODataEnd)
    … …  
    51715177        if (rc == VINF_SUCCESS)
    51725178        {
    5173             ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     5179            ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    51745180
    51755181            uint32_t const offStart = s->iIOBufferPIODataStart;
    … …  
    52625268        if (rc == VINF_SUCCESS)
    52635269        {
    5264             ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf];
     5270            ATADevState *s = &pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK];
    52655271
    52665272            uint32_t const offStart = s->iIOBufferPIODataStart;
    … …  
    53485354{
    53495355    PPDMDEVINS pDevIns = CONTROLLER_2_DEVINS(pCtl);
    5350     ATADevState *s = &pCtl->aIfs[pCtl->iAIOIf];
     5356    ATADevState *s = &pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK];
    53515357    bool fRedo;
    53525358    RTGCPHYS32 GCPhysDesc;
    … …  
    56325638
    56335639                pCtl->iAIOIf = pReq->u.t.iIf;
    5634                 s = &pCtl->aIfs[pCtl->iAIOIf];
     5640                s = &pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK];
    56355641                s->cbTotalTransfer = pReq->u.t.cbTotalTransfer;
    56365642                s->uTxDir = pReq->u.t.uTxDir;
    … …  
    57765782            {
    57775783                BMDMAState *bm = &pCtl->BmDma;
    5778                 s = &pCtl->aIfs[pCtl->iAIOIf]; /* Do not remove or there's an instant crash after loading the saved state */
     5784                s = &pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK]; /* Do not remove or there's an instant crash after loading the saved state */
    57795785                ATAFNSS iOriginalSourceSink = (ATAFNSS)s->iSourceSink; /* Used by the hack below, but gets reset by then. */
    57805786
    … …  
    58305836
    58315837            case ATA_AIO_PIO:
    5832                 s = &pCtl->aIfs[pCtl->iAIOIf]; /* Do not remove or there's an instant crash after loading the saved state */
     5838                s = &pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK]; /* Do not remove or there's an instant crash after loading the saved state */
    58335839
    58345840                if (s->iSourceSink != ATAFN_SS_NULL)
    … …  
    59325938                 * any command activity on the other drive otherwise using
    59335939                 * one thread per controller wouldn't work at all. */
    5934                 s = &pCtl->aIfs[pReq->u.a.iIf];
     5940                s = &pCtl->aIfs[pReq->u.a.iIf & ATA_SELECTED_IF_MASK];
    59355941
    59365942                pCtl->uAsyncIOState = ATA_AIO_NEW;
    … …  
    59695975            u64TS = RTTimeNanoTS() - u64TS;
    59705976            uWait = u64TS / 1000;
     5977            uintptr_t const iAIOIf = pCtl->iAIOIf & ATA_SELECTED_IF_MASK;
    59715978            Log(("%s: Ctl#%d: LUN#%d finished I/O transaction in %d microseconds\n",
    5972                  __FUNCTION__, ATACONTROLLER_IDX(pCtl), pCtl->aIfs[pCtl->iAIOIf].iLUN, (uint32_t)(uWait)));
     5979                 __FUNCTION__, ATACONTROLLER_IDX(pCtl), pCtl->aIfs[iAIOIf].iLUN, (uint32_t)(uWait)));
    59735980            /* Mark command as finished. */
    5974             pCtl->aIfs[pCtl->iAIOIf].u64CmdTS = 0;
     5981            pCtl->aIfs[iAIOIf].u64CmdTS = 0;
    59755982
    59765983            /*
    … …  
    59795986             * spin up time etc.) so the threshold is different.
    59805987             */
    5981             if (pCtl->aIfs[pCtl->iAIOIf].uATARegCommand != ATA_PACKET)
     5988            if (pCtl->aIfs[iAIOIf].uATARegCommand != ATA_PACKET)
    59825989            {
    59835990                if (uWait > 8 * 1000 * 1000)
    … …  
    59905997                     */
    59915998                    LogRel(("PIIX3 ATA: execution time for ATA command %#04x was %d seconds\n",
    5992                             pCtl->aIfs[pCtl->iAIOIf].uATARegCommand, uWait / (1000 * 1000)));
     5999                            pCtl->aIfs[iAIOIf].uATARegCommand, uWait / (1000 * 1000)));
    59936000                }
    59946001            }
    … …  
    60046011                     */
    60056012                    LogRel(("PIIX3 ATA: execution time for ATAPI command %#04x was %d seconds\n",
    6006                             pCtl->aIfs[pCtl->iAIOIf].abATAPICmd[0], uWait / (1000 * 1000)));
     6013                            pCtl->aIfs[iAIOIf].abATAPICmd[0], uWait / (1000 * 1000)));
    60076014                }
    60086015            }
    … …  
    60746081        /* Do not start DMA transfers if there's a PIO transfer going on,
    60756082         * or if there is already a transfer started on this controller. */
    6076         if (   !pCtl->aIfs[pCtl->iSelectedIf].fDMA
     6083        if (   !pCtl->aIfs[pCtl->iSelectedIf & ATA_SELECTED_IF_MASK].fDMA
    60776084            || (uOldBmDmaStatus & BM_STATUS_DMAING))
    60786085            return;
    60796086
    6080         if (pCtl->aIfs[pCtl->iAIOIf].uATARegStatus & ATA_STAT_DRQ)
     6087        if (pCtl->aIfs[pCtl->iAIOIf & ATA_SELECTED_IF_MASK].uATARegStatus & ATA_STAT_DRQ)
    60816088        {
    60826089            Log2(("%s: Ctl#%d: message to async I/O thread, continuing DMA transfer\n", __FUNCTION__, ATACONTROLLER_IDX(pCtl)));
    … …  
    70987105        }
    70997106
    7100         pHlp->pfnSSMGetU8(pSSM, &pThis->aCts[i].iSelectedIf);
    7101         pHlp->pfnSSMGetU8(pSSM, &pThis->aCts[i].iAIOIf);
     7107        rc = pHlp->pfnSSMGetU8(pSSM, &pThis->aCts[i].iSelectedIf);
     7108        AssertRCReturn(rc, rc);
     7109        AssertLogRelMsgStmt(pThis->aCts[i].iSelectedIf == (pThis->aCts[i].iSelectedIf & ATA_SELECTED_IF_MASK),
     7110                            ("iSelectedIf = %d\n", pThis->aCts[i].iSelectedIf),
     7111                            pThis->aCts[i].iSelectedIf &= ATA_SELECTED_IF_MASK);
     7112        rc = pHlp->pfnSSMGetU8(pSSM, &pThis->aCts[i].iAIOIf);
     7113        AssertRCReturn(rc, rc);
     7114        AssertLogRelMsgStmt(pThis->aCts[i].iAIOIf == (pThis->aCts[i].iAIOIf & ATA_SELECTED_IF_MASK),
     7115                            ("iAIOIf = %d\n", pThis->aCts[i].iAIOIf),
     7116                            pThis->aCts[i].iAIOIf &= ATA_SELECTED_IF_MASK);
    71027117        pHlp->pfnSSMGetU8(pSSM, &pThis->aCts[i].uAsyncIOState);
    71037118        pHlp->pfnSSMGetBool(pSSM, &pThis->aCts[i].fChainedTransfer);
Note: See TracChangeset for help on using the changeset viewer.

© 2025 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette