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.clktrk > _______________________________________________ > 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
