| Message ID | 43bc5e07ac614d0e8e740bf6007ff77b@tpw09926dag18e.domain1.systemhost.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Invert Endian bit in SPARCv9 MMU TTE | expand |
Hi Tony, On 8/16/19 8:28 AM, tony.nguyen@bt.com wrote: > This patchset implements the IE (Invert Endian) bit in SPARCv9 MMU TTE. > > v7: [...] > - Re-declared many native endian devices as little or big endian. This is why > v7 has +16 patches. Why are you doing that? What is the rational? Anyhow if this not required by your series, you should split it out of it, and send it on your principal changes are merged. I'm worried because this these new patches involve many subsystems (thus maintainers) and reviewing them will now take a fair amount of time. > For each device declared with DEVICE_NATIVE_ENDIAN, find the set of > targets from the set of target/hw/*/device.o. > > If the set of targets are all little or all big endian, re-declare > the device endianness as DEVICE_LITTLE_ENDIAN or DEVICE_BIG_ENDIAN > respectively. If only little endian targets use a device, that doesn't mean the device is designed in little endian... Then if a big endian target plan to use this device, it will require more work and you might have introduced regressions... I'm not sure this is a safe move. > This *naive* deduction may result in genuinely native endian devices > being incorrectly declared as little or big endian, but should not > introduce regressions for current targets. Regards, Phil.
Hi Phillippe, On 8/16/19 7:58 PM, Philippe Mathieu-Daudé wrote: >On 8/16/19 8:28 AM, tony.nguyen@bt.com wrote: >> This patchset implements the IE (Invert Endian) bit in SPARCv9 MMU TTE. >> >> v7: >[...] >> - Re-declared many native endian devices as little or big endian. This is why >> v7 has +16 patches. > >Why are you doing that? What is the rational? While collapsing the byte swaps, it was suggested in patch #11 of v5 that consistent use of MemOp simplified endian comparisons. This lead to the deprecation of enum device_endian by MemOp. As MO_TE is conditional upon NEED_CPU_H, the s/DEVICE_NATIVE_ENDIAN/MO_TE/ required changing some device object files from common-obj-* to obj-*. In patch #15 of v6 Paolo noted that most devices should not of been DEVICE_NATIVE_ENDIAN and hinted at a clean up. The +16 patches in v7 is the clean up effort. >Anyhow if this not required by your series, you should split it out of >it, and send it on your principal changes are merged. >I'm worried because this these new patches involve many subsystems (thus >maintainers) and reviewing them will now take a fair amount of time. Yes, lets split these patches out. They are very much a tangent to the series purpose. >> For each device declared with DEVICE_NATIVE_ENDIAN, find the set of >> targets from the set of target/hw/*/device.o. >> >> If the set of targets are all little or all big endian, re-declare >> the device endianness as DEVICE_LITTLE_ENDIAN or DEVICE_BIG_ENDIAN >> respectively. > >If only little endian targets use a device, that doesn't mean the device >is designed in little endian... > >Then if a big endian target plan to use this device, it will require >more work and you might have introduced regressions... > >I'm not sure this is a safe move. > >> This *naive* deduction may result in genuinely native endian devices >> being incorrectly declared as little or big endian, but should not >> introduce regressions for current targets. > Roger. Evidently too naive. TBH, most devices I've never heard of... Regards, Tony
On Fri, Aug 16, 2019 at 11:58:05AM +0200, Philippe Mathieu-Daudé wrote: > Hi Tony, > > On 8/16/19 8:28 AM, tony.nguyen@bt.com wrote: > > This patchset implements the IE (Invert Endian) bit in SPARCv9 MMU TTE. > > > > v7: > [...] > > - Re-declared many native endian devices as little or big endian. This is why > > v7 has +16 patches. > > Why are you doing that? What is the rational? > > Anyhow if this not required by your series, you should split it out of > it, and send it on your principal changes are merged. > I'm worried because this these new patches involve many subsystems (thus > maintainers) and reviewing them will now take a fair amount of time. > > > For each device declared with DEVICE_NATIVE_ENDIAN, find the set of > > targets from the set of target/hw/*/device.o. > > > > If the set of targets are all little or all big endian, re-declare > > the device endianness as DEVICE_LITTLE_ENDIAN or DEVICE_BIG_ENDIAN > > respectively. > > If only little endian targets use a device, that doesn't mean the device > is designed in little endian... > > Then if a big endian target plan to use this device, it will require > more work and you might have introduced regressions... Uh.. only if they make the version of the device on a big endian target big endian. Which is a terrible idea - if you know a hardware designer planning to do this, please slap them.
On Fri, 16 Aug 2019 at 12:37, <tony.nguyen@bt.com> wrote: > > Hi Phillippe, > > On 8/16/19 7:58 PM, Philippe Mathieu-Daudé wrote: > >On 8/16/19 8:28 AM, tony.nguyen@bt.com wrote: > >> For each device declared with DEVICE_NATIVE_ENDIAN, find the set of > >> targets from the set of target/hw/*/device.o. > >> > >> If the set of targets are all little or all big endian, re-declare > >> the device endianness as DEVICE_LITTLE_ENDIAN or DEVICE_BIG_ENDIAN > >> respectively. > > > >If only little endian targets use a device, that doesn't mean the device > >is designed in little endian... > > > >Then if a big endian target plan to use this device, it will require > >more work and you might have introduced regressions... > > > >I'm not sure this is a safe move. > > > >> This *naive* deduction may result in genuinely native endian devices > >> being incorrectly declared as little or big endian, but should not > >> introduce regressions for current targets. > > > > Roger. Evidently too naive. TBH, most devices I've never heard of... OTOH it's worth noting that it's quite likely that most of the implementations of these DEVICE_NATIVE_ENDIAN devices picked it in an equally naive way, by just copying some other device's code... thanks -- PMM
On 8/16/19 7:28 AM, tony.nguyen@bt.com wrote: > Tony Nguyen (42): > configure: Define TARGET_ALIGNED_ONLY in configure > tcg: TCGMemOp is now accelerator independent MemOp > memory: Introduce size_memop > target/mips: Access MemoryRegion with MemOp > hw/s390x: Access MemoryRegion with MemOp > hw/intc/armv7m_nic: Access MemoryRegion with MemOp > hw/virtio: Access MemoryRegion with MemOp > hw/vfio: Access MemoryRegion with MemOp > exec: Access MemoryRegion with MemOp > cputlb: Access MemoryRegion with MemOp > memory: Access MemoryRegion with MemOp > hw/s390x: Hard code size with MO_{8|16|32|64} > target/mips: Hard code size with MO_{8|16|32|64} > exec: Hard code size with MO_{8|16|32|64} I have queued these 14 patches to tcg-next: https://github.com/rth7680/qemu/tree/tcg-next I agree with the downthread conversation with Phil that the middle device patches should be split out to a different series. I have some questions on some of the last few patches, and I don't know how they would interact cherry-picking from those, so I've left them for now. I had some trouble applying your patches, as they're encoded quoted-printable, and "git am" doesn't like that. If possible, please use "git send-email" to post your next patch set. r~