I am fine with all your clarifications. If you add these comments to the patches (mainly when you define UINT64 IoTranslation|MmioTranslation) I will likely add my 'Reviewed-By'.
> -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: 17 February 2015 15:33 > To: [email protected]; [email protected]; Peter Maydell; Ard > Biesheuvel; Drew Jones; Wei Huang; Donald Dutile; Wei Liu > Subject: Re: [edk2] [PATCH 06/27] ArmVirtualizationPkg/VirtFdtDxe: > parse "pci-host-ecam-generic" properties > > On 02/17/15 15:41, Olivier Martin wrote: > > Effectively, if we follow the C standard and ensure we always use the > same > > type then the results should be the expected ones. > > Yes. > > > But it is quite confusing to read, manipulating signed numbers using > > unsigned type variables. The logic is not natural... > > I tend to think about them as "remainder classes", and as modular > addition. > > > If there is someone who 'Ack-By' I am ok to accept the patch even if > I would > > still prefer the use of INT64 to manipulate a signed offset. > > INT64 has the grave problem that you can't just write (a-b) and expect > the result to be correct. For the following reasons: > > - the input values from the DTB are UINT64's. A standalone input value > might be immediately impossible to convert to INT64 while keeping the > value. (The conversion itself would not be undefined behavior; the > result is implementation-defined, but in any case it would not carry > the > value we want.) > > - Plain (a-b) in general is unsafe. The difference can be larger than > MAX_INT64 (think (MAX_INT64 - (-1))), and also smaller than MIN_INT64 > (ie. negative and greater in magnitude, think (MIN_INT64 - 1)). If that > happens, the behavior is undefined. If you constrain both "a" and "b" > so > they are non-negative, this won't happen, but then the previous point > is > again a problem. > > So you inevitably end up with elaborate if's in signed integer > arithmetic; it's always better to avoid it if possible. > > I can add a comment above the subtractions if that improves the patch > for you. > > Also I hope someone can give the ACK you requested. > > Thanks > Laszlo > > > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:[email protected]] > >> Sent: 16 February 2015 13:40 > >> To: [email protected]; [email protected]; Peter Maydell; > Ard > >> Biesheuvel; Drew Jones; Wei Huang; Donald Dutile; Wei Liu > >> Subject: Re: [edk2] [PATCH 06/27] ArmVirtualizationPkg/VirtFdtDxe: > >> parse "pci-host-ecam-generic" properties > >> > >> On 02/16/15 13:10, Olivier Martin wrote: > >>> Actually, do you always expect IoTranslation|MmioTranslation to > >> always be > >>> positive (ie: UINT64)? > >> > >> No, I don't. > >> > >> If the mathematical result would be negative, then the translation > >> offset will underflow, which is well defined for UINT64. Namely, > >> instead > >> of the mathematically correct negative integer > >> > >> Translation := CpuBase - IoBase > >> > >> which is between -(2^64 - 1) and -1, both sides inclusive, the PCD > will > >> store the following positive integer: > >> > >> Translation := (MAX_UINT64 + 1) + (CpuBase - IoBase) > >> > >> which will be between 1 and 2^64-1, both sides inclusive. > >> > >> Then, the actual translation code in patch 11/27 is just a simple > >> addition, where both operands are UINT64 (and the result is stored > in a > >> UINT64 too). The mathematical end result is > >> > >> Translated := Address + (MAX_UINT64 + 1) + (CpuBase - IoBase) > >> > >> which yields the same C value as the math value of the non- > underflowing > >> case: > >> > >> Translated := Address + (CpuBase - IoBase) > >> > >> In other words: since we're careful to use UINT64 all the time, > which > >> has well defined modular addition/subtraction semantics, we don't > need > >> to care about the relation between CpuBase and IoBase. Adding > >> 0xFFFF_FFFF_FFFF_FFFF is the same as subtracting 1. > >> > >> This comes from the C standard; implementations have no wiggle room > in > >> it. The only implementation-defined thing we're exploiting here is > that > >> UINT64 is identical to either "long unsigned" or "long long > unsigned". > >> > >> Thanks > >> Laszlo > >> > > > > > > > > > > --------------------------------------------------------------------- > --------- > > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > > from Actuate! Instantly Supercharge Your Business Reports and > Dashboards > > with Interactivity, Sharing, Native Excel Exports, App Integration & > more > > Get technology previously reserved for billion-dollar corporations, > FREE > > > http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.c > lktrk > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > > > ----------------------------------------------------------------------- > ------- > Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server > from Actuate! Instantly Supercharge Your Business Reports and > Dashboards > with Interactivity, Sharing, Native Excel Exports, App Integration & > more > Get technology previously reserved for billion-dollar corporations, > FREE > http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.c > lktrk > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=190641631&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
