Index: /trunk/src/VBox/Devices/Audio/DevHDA.cpp =================================================================== --- /trunk/src/VBox/Devices/Audio/DevHDA.cpp (revision 82400) +++ /trunk/src/VBox/Devices/Audio/DevHDA.cpp (revision 82401) @@ -532,4 +532,6 @@ }; +#endif /* IN_RING3 */ + /** * 32-bit size indexed masks, i.e. g_afMasks[2 bytes] = 0xffff. @@ -540,5 +542,4 @@ }; -#endif /* IN_RING3 */ @@ -2981,5 +2982,42 @@ } -#endif /* IN_RING3 */ +#else /* !IN_RING3 */ + +/** + * Checks if a dword read starting with @a idxRegDsc is safe. + * + * We can guarentee it only standard reader callbacks are used. + * @returns true if it will always succeed, false if it may return back to + * ring-3 or we're just not sure. + * @param idxRegDsc The first register descriptor in the DWORD being read. + */ +DECLINLINE(bool) hdaIsMultiReadSafeInRZ(unsigned idxRegDsc) +{ + int32_t cbLeft = 4; /* signed on purpose */ + do + { + if ( g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU24 + || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU16 + || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadU8 + || g_aHdaRegMap[idxRegDsc].pfnRead == hdaRegReadUnimpl) + { /* okay */ } + else + { + Log4(("hdaIsMultiReadSafeInRZ: idxRegDsc=%u %s\n", idxRegDsc, g_aHdaRegMap[idxRegDsc].abbrev)); + return false; + } + + idxRegDsc++; + if (idxRegDsc < RT_ELEMENTS(g_aHdaRegMap)) + cbLeft -= g_aHdaRegMap[idxRegDsc].offset - g_aHdaRegMap[idxRegDsc - 1].offset; + else + break; + } while (cbLeft > 0); + return true; +} + + +#endif /* !IN_RING3 */ + /* MMIO callbacks */ @@ -3029,4 +3067,12 @@ STAM_COUNTER_INC(&pThis->aStatRegReads[idxRegDsc]); } +#ifndef IN_RING3 + else if (!hdaIsMultiReadSafeInRZ(idxRegDsc)) + + { + STAM_COUNTER_INC(&pThis->aStatRegReadsToR3[idxRegDsc]); + rc = VINF_IOM_R3_MMIO_READ; + } +#endif else { @@ -3035,6 +3081,6 @@ * ASSUMES that only DWORD reads have sideeffects. */ -#ifdef IN_RING3 - STAM_COUNTER_INC(&pThis->StatRegMultiReads); + STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiReads)); + Log4(("hdaMmioRead: multi read: %#x LB %#x %s\n", off, cb, g_aHdaRegMap[idxRegDsc].abbrev)); uint32_t u32Value = 0; unsigned cbLeft = 4; @@ -3045,8 +3091,12 @@ rc = g_aHdaRegMap[idxRegDsc].pfnRead(pDevIns, pThis, idxRegDsc, &u32Tmp); - Log3Func(("\tRead %s[%db] => %x (%Rrc)*\n", g_aHdaRegMap[idxRegDsc].abbrev, cbReg, u32Tmp, VBOXSTRICTRC_VAL(rc))); + Log4Func(("\tRead %s[%db] => %x (%Rrc)*\n", g_aHdaRegMap[idxRegDsc].abbrev, cbReg, u32Tmp, VBOXSTRICTRC_VAL(rc))); STAM_COUNTER_INC(&pThis->aStatRegReads[idxRegDsc]); +#ifdef IN_RING3 if (rc != VINF_SUCCESS) break; +#else + AssertMsgBreak(rc == VINF_SUCCESS, ("rc=%Rrc - impossible, we sanitized the readers!\n", VBOXSTRICTRC_VAL(rc))); +#endif u32Value |= (u32Tmp & g_afMasks[cbReg]) << ((4 - cbLeft) * 8); @@ -3060,9 +3110,4 @@ else Assert(!IOM_SUCCESS(rc)); -#else /* !IN_RING3 */ - /* Take the easy way out. */ - STAM_COUNTER_INC(&pThis->aStatRegReadsToR3[idxRegDsc]); - rc = VINF_IOM_R3_MMIO_READ; -#endif /* !IN_RING3 */ } } @@ -3197,8 +3242,4 @@ #ifdef LOG_ENABLED uint32_t const u32LogOldValue = idxRegDsc >= 0 ? pThis->au32Regs[idxRegMem] : UINT32_MAX; - if (idxRegDsc == -1) - Log3Func(("@%#05x u32=%#010x cb=%d\n", (uint32_t)off, *(uint32_t const *)pv, cb)); - else - Log3Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev)); #endif @@ -3209,6 +3250,24 @@ if (idxRegDsc >= 0 && g_aHdaRegMap[idxRegDsc].size == cb) { + Log3Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev)); rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, ""); Log3Func(("\t%#x -> %#x\n", u32LogOldValue, idxRegMem != UINT32_MAX ? pThis->au32Regs[idxRegMem] : UINT32_MAX)); + } + /* + * Sub-register access. Supply missing bits as needed. + */ + else if ( idxRegDsc >= 0 + && cb < g_aHdaRegMap[idxRegDsc].size) + { + u64Value |= pThis->au32Regs[g_aHdaRegMap[idxRegDsc].mem_idx] + & g_afMasks[g_aHdaRegMap[idxRegDsc].size] + & ~g_afMasks[cb]; + Log4Func(("@%#05x u%u=%#0*RX64 cb=%#x cbReg=%x %s\n" + "\tSupplying missing bits (%#x): %#llx -> %#llx ...\n", + (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, cb, g_aHdaRegMap[idxRegDsc].size, g_aHdaRegMap[idxRegDsc].abbrev, + g_afMasks[g_aHdaRegMap[idxRegDsc].size] & ~g_afMasks[cb], u64Value & g_afMasks[cb], u64Value)); + rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, ""); + Log4Func(("\t%#x -> %#x\n", u32LogOldValue, idxRegMem != UINT32_MAX ? pThis->au32Regs[idxRegMem] : UINT32_MAX)); + STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegSubWrite)); } /* @@ -3218,6 +3277,11 @@ { #ifdef IN_RING3 - if (idxRegDsc >= 0 && g_aHdaRegMap[idxRegDsc].size != cb) - Log3Func(("\tSize mismatch: %RU32 (reg) vs %u (access)!!\n", g_aHdaRegMap[idxRegDsc].size, cb)); + if (idxRegDsc == -1) + Log4Func(("@%#05x u32=%#010x cb=%d\n", (uint32_t)off, *(uint32_t const *)pv, cb)); + else if (g_aHdaRegMap[idxRegDsc].size == cb) + Log4Func(("@%#05x u%u=%#0*RX64 %s\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, g_aHdaRegMap[idxRegDsc].abbrev)); + else + Log4Func(("@%#05x u%u=%#0*RX64 %s - mismatch cbReg=%u\n", (uint32_t)off, cb * 8, 2 + cb * 2, u64Value, + g_aHdaRegMap[idxRegDsc].abbrev, g_aHdaRegMap[idxRegDsc].size)); /* @@ -3238,7 +3302,7 @@ u64Value <<= cbBefore * 8; u64Value |= pThis->au32Regs[idxRegMem] & g_afMasks[cbBefore]; - Log3Func(("\tWithin register, supplied %u leading bits: %#llx -> %#llx ...\n", + Log4Func(("\tWithin register, supplied %u leading bits: %#llx -> %#llx ...\n", cbBefore * 8, ~g_afMasks[cbBefore] & u64Value, u64Value)); - STAM_COUNTER_INC(&pThis->StatRegMultiWrites); + STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiWrites)); } else @@ -3246,6 +3310,8 @@ } else - STAM_COUNTER_INC(&pThis->StatRegMultiWrites); - + { + Log4(("hdaMmioWrite: multi write: %s\n", g_aHdaRegMap[idxRegDsc].abbrev)); + STAM_COUNTER_INC(&pThis->CTX_SUFF_Z(StatRegMultiWrites)); + } /* Loop thru the write area, it may cover multiple registers. */ @@ -3261,5 +3327,5 @@ { u64Value |= pThis->au32Regs[idxRegMem] & g_afMasks[cbReg] & ~g_afMasks[cb]; - Log3Func(("\tSupplying missing bits (%#x): %#llx -> %#llx ...\n", + Log4Func(("\tSupplying missing bits (%#x): %#llx -> %#llx ...\n", g_afMasks[cbReg] & ~g_afMasks[cb], u64Value & g_afMasks[cb], u64Value)); } @@ -3268,5 +3334,5 @@ # endif rc = hdaWriteReg(pDevIns, pThis, idxRegDsc, u64Value, "*"); - Log3Func(("\t%#x -> %#x\n", uLogOldVal, pThis->au32Regs[idxRegMem])); + Log4Func(("\t%#x -> %#x\n", uLogOldVal, pThis->au32Regs[idxRegMem])); } else @@ -4717,4 +4783,10 @@ AssertRCReturn(rc, rc); + /** @todo r=bird: The IOMMMIO_FLAGS_READ_DWORD flag isn't entirely optimal, + * as several frequently used registers aren't dword sized. 6.0 and earlier + * will go to ring-3 to handle accesses to any such register, where-as 6.1 and + * later will do trivial register reads in ring-0. Real optimal code would use + * IOMMMIO_FLAGS_READ_PASSTHRU and do the necessary extra work to deal with + * anything the guest may throw at us. */ rc = PDMDevHlpPCIIORegionCreateMmio(pDevIns, 0, 0x4000, PCI_ADDRESS_SPACE_MEM, hdaMmioWrite, hdaMmioRead, NULL /*pvUser*/, IOMMMIO_FLAGS_READ_DWORD | IOMMMIO_FLAGS_WRITE_PASSTHRU, "HDA", &pThis->hMmio); @@ -5077,6 +5149,10 @@ g_aHdaRegMap[i].desc, "Regs/%03x-%s-Writes-ToR3", g_aHdaRegMap[i].offset, g_aHdaRegMap[i].abbrev); } - PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReads, STAMTYPE_COUNTER, "RegMultiReads", STAMUNIT_OCCURENCES, "Register read not targeting just one register"); - PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWrites, STAMTYPE_COUNTER, "RegMultiWrites", STAMUNIT_OCCURENCES, "Register writes not targeting just one register"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReadsR3, STAMTYPE_COUNTER, "RegMultiReadsR3", STAMUNIT_OCCURENCES, "Register read not targeting just one register, handled in ring-3"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiReadsRZ, STAMTYPE_COUNTER, "RegMultiReadsRZ", STAMUNIT_OCCURENCES, "Register read not targeting just one register, handled in ring-0"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWritesR3, STAMTYPE_COUNTER, "RegMultiWritesR3", STAMUNIT_OCCURENCES, "Register writes not targeting just one register, handled in ring-3"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegMultiWritesRZ, STAMTYPE_COUNTER, "RegMultiWritesRZ", STAMUNIT_OCCURENCES, "Register writes not targeting just one register, handled in ring-0"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegSubWriteR3, STAMTYPE_COUNTER, "RegSubWritesR3", STAMUNIT_OCCURENCES, "Trucated register writes, handled in ring-3"); + PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegSubWriteRZ, STAMTYPE_COUNTER, "RegSubWritesRZ", STAMUNIT_OCCURENCES, "Trucated register writes, handled in ring-0"); PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegUnknownReads, STAMTYPE_COUNTER, "RegUnknownReads", STAMUNIT_OCCURENCES, "Reads of unknown registers."); PDMDevHlpSTAMRegister(pDevIns, &pThis->StatRegUnknownWrites, STAMTYPE_COUNTER, "RegUnknownWrites", STAMUNIT_OCCURENCES, "Writes to unknown registers."); Index: /trunk/src/VBox/Devices/Audio/DevHDA.h =================================================================== --- /trunk/src/VBox/Devices/Audio/DevHDA.h (revision 82400) +++ /trunk/src/VBox/Devices/Audio/DevHDA.h (revision 82401) @@ -209,6 +209,10 @@ STAMCOUNTER aStatRegWrites[HDA_NUM_REGS]; STAMCOUNTER aStatRegWritesToR3[HDA_NUM_REGS]; - STAMCOUNTER StatRegMultiReads; - STAMCOUNTER StatRegMultiWrites; + STAMCOUNTER StatRegMultiReadsRZ; + STAMCOUNTER StatRegMultiReadsR3; + STAMCOUNTER StatRegMultiWritesRZ; + STAMCOUNTER StatRegMultiWritesR3; + STAMCOUNTER StatRegSubWriteRZ; + STAMCOUNTER StatRegSubWriteR3; STAMCOUNTER StatRegUnknownReads; STAMCOUNTER StatRegUnknownWrites;