Hi Michal, >Do we really need three different names for one thing? SLICTable, CustomTable, CustomTable0 for the same thing seems like an overkill.
I agree. Ideally it would just be CustomTable0, CustomTable1, CustomTable2.....and the code currently supports that. I had kept SLICTable/CustomTable as aliases for CustomTable0 to maintain backward compatibility, expecting that at some point in the future they would likely be removed. It's easy enough to remove the SLICTable/CustomTable aliases now if preferred. I would leave that decision to the devs, since it would be a breaking change for users currently using those config params. Regards, On Tue, Sep 18, 2018 at 3:56 PM Michal Necasek <[email protected]> wrote: > > Hi, > > Sorry for the delay. > > You're absolutely right, the acpiR3SetupCust() function does a lot of > things but acpiR3PhysCopy() is the only useful bit it does. > > The only objection I have then is the following: Do we really need > three different names for one thing? SLICTable, CustomTable, > CustomTable0 for the same thing seems like an overkill. > > Cheers, > Michal > > On 9/7/2018 10:22 AM, Canardos . wrote: > > Hi Michal, > > > > Thanks for taking a look. > > > > I agree that the behavior should not change and I don't believe it does. > > I removed the call to acpiR3PrepareHeader (as well as the surrounding > > lines) because it was dead code. Looking at the prior acpiR3SetupCust > > function: > > > > /** Custom Description Table */ > > static void acpiR3SetupCust(ACPIState *pThis, RTGCPHYS32 addr) > > { > > ACPITBLCUST cust; > > > > /* First the ACPI version 1 version of the structure. */ > > memset(&cust, 0, sizeof(cust)); > > acpiR3PrepareHeader(pThis, &cust.header, "CUST", sizeof(cust), 1); > > > > memcpy(cust.header.au8OemTabId, pThis->au8OemTabId, 8); > > cust.header.u32OemRevision = RT_H2LE_U32(pThis->u32OemRevision); > > cust.header.u8Checksum = acpiR3Checksum((uint8_t *)&cust, > > sizeof(cust)); > > > > acpiR3PhysCopy(pThis, addr, pThis->pu8CustBin, pThis->cbCustBin); > > } > > > > One can see that the cust structure is allocated locally on the first > > line, setup, but then discarded when the function exits without ever > > being used. The only code in the function that does anything is the > > final line. > > > > The custom table header that is actually used is the one provided by the > > user, which is loaded and present in the table itself > > (pThis->pu8CustBin). This behavior has been retained in the updated > > function: > > > > /** Custom Description Table */ > > static void acpiR3SetupCust(ACPIState *pThis, RTGCPHYS32 addr, uint8_t > > custTableIdx) > > { > > Assert(custTableIdx < MAX_CUST_TABLES); > > acpiR3PhysCopy(pThis, addr, pThis->pu8CustBin[custTableIdx], > > pThis->cbCustBin[custTableIdx]); > > } > > > > With respect to the incoming data, which part did you notice was copied > > differently? The intention is definitely to retain the existing behavior > > so if there's a difference there, it's unintended. > > > > In regard to use cases, one example is the requirement to now pass > > through both SLIC and MSDM tables when using (legally) using a Windows > > 10 OEM edition as a guest on the machine that it was licensed on (this > > was the motivation for the original custom table mechanism for SLIC > > passthrough for Windows 7/8 - see > > https://www.virtualbox.org/ticket/9231). Reducing the detection surface > > for dynamic analysis of malware that actively detects sandboxes is > another. > > > > Regards, > > > > On Tue, Sep 4, 2018 at 10:45 PM Michal Necasek > > <[email protected] <mailto:[email protected]>> wrote: > > > > > > Hi, > > > > Thanks for the patch! I quickly looked over it and it looks to me > > that > > the behavior of existing VMs with the > > VBoxInternal/Devices/acpi/0/Config/CustomTable keys set will subtly > > change because the call to > > > > acpiR3PrepareHeader(pThis, &cust.header, "CUST", sizeof(cust), 1); > > > > is no longer done and the incoming data is copied a bit differently. > > Can > > you explain why that's not done or why it can't cause any problems? > > > > Wouldn't it be better to keep the existing behavior for > CustomTable > > and just add CustomTable1/2/3 with slightly different behavior? > > > > Also, do you have a concrete example of ACPI tables that one might > > want to pass to a VM this way? > > > > > > Regards, > > Michal > > > > On 8/24/2018 9:14 PM, Canardos . wrote: > > > Hi Devs, > > > > > > This patch expands on work done several years ago, extending > > support for > > > custom ACPI tables from a single table to four tables, and > > potentially N > > > tables in future. > > > > > > The changes assist malware researchers to better emulate a > physical > > > machine (although increasing the aggregate allowable table size > will > > > assist further) as well as those with valid OEM licenses having > > issues > > > property passing through both SLIC and license tables. > > > > > > I limited the implementation to four custom tables given current > > VBox > > > limitations on the aggregate size of ACPI tables, but the code > > changes > > > support N tables. > > > > > > The new configuration keys are "CustomTable0..3". Legacy behavior > > has > > > been maintained, with any existing config entries for > > "CustomTable", or > > > "SLICTable" being used in the absence of a "CustomTable0" entry. > > > > > > The changes have been tested on Debian 9.5 (kernel 4.9.0-7/amd64) > > and > > > Manjaro (kernel 4.14.65-1/amd64) hosts with 32 and 64-bit Linux > > guests. > > > > > > I've endeavored to reflect the code style of the file I was > > working in, > > > please let me know if anything is amiss. > > > > > > All attached code is released under the MIT License. > > > > > > > > > > > > _______________________________________________ > > > vbox-dev mailing list > > > [email protected] <mailto:[email protected]> > > > https://www.virtualbox.org/mailman/listinfo/vbox-dev > > > > > > > _______________________________________________ > > vbox-dev mailing list > > [email protected] <mailto:[email protected]> > > https://www.virtualbox.org/mailman/listinfo/vbox-dev > > > >
_______________________________________________ vbox-dev mailing list [email protected] https://www.virtualbox.org/mailman/listinfo/vbox-dev
