Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On 10/14/2010 02:44 PM, Jes Sorensen wrote: On 10/14/10 20:33, Alex Williamson wrote: We can't let the compiler define the alignment for qemu_cfg data. Signed-off-by: Alex Williamsonalex.william...@redhat.com --- 0.13 stable candidate? ACK I would say so. fw_cfg interfaces are somewhat difficult to rationalize about for compatibility. 0.13.0 is tagged already so it's too late to pull it in there. If we say we don't care about compatibility at the fw_cfg level, then it doesn't matter if we pull it into stable-0.13. If we do care, then this is an ABI breaker. I don't know that the answer is obvious to me. Regards, Anthony Liguori Jes -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On Thu, 2010-10-14 at 14:48 -0500, Anthony Liguori wrote: On 10/14/2010 02:44 PM, Jes Sorensen wrote: On 10/14/10 20:33, Alex Williamson wrote: We can't let the compiler define the alignment for qemu_cfg data. Signed-off-by: Alex Williamsonalex.william...@redhat.com --- 0.13 stable candidate? ACK I would say so. fw_cfg interfaces are somewhat difficult to rationalize about for compatibility. 0.13.0 is tagged already so it's too late to pull it in there. If we say we don't care about compatibility at the fw_cfg level, then it doesn't matter if we pull it into stable-0.13. If we do care, then this is an ABI breaker. If it works anywhere (I assume it works on 32bit), then it's only because it happened to get the alignment right. This just makes 64bit hosts get it right too. I don't see any compatibility issues, non-packed + 64bit = broken. Thanks, Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On 10/14/2010 02:58 PM, Alex Williamson wrote: On Thu, 2010-10-14 at 14:48 -0500, Anthony Liguori wrote: On 10/14/2010 02:44 PM, Jes Sorensen wrote: On 10/14/10 20:33, Alex Williamson wrote: We can't let the compiler define the alignment for qemu_cfg data. Signed-off-by: Alex Williamsonalex.william...@redhat.com --- 0.13 stable candidate? ACK I would say so. fw_cfg interfaces are somewhat difficult to rationalize about for compatibility. 0.13.0 is tagged already so it's too late to pull it in there. If we say we don't care about compatibility at the fw_cfg level, then it doesn't matter if we pull it into stable-0.13. If we do care, then this is an ABI breaker. If it works anywhere (I assume it works on 32bit), then it's only because it happened to get the alignment right. This just makes 64bit hosts get it right too. I don't see any compatibility issues, non-packed + 64bit = broken. Thanks, Ok, I'll buy that argument :-) Regards, Anthony Liguori Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On Thursday 14 October 2010 21:58:08 Alex Williamson wrote: If it works anywhere (I assume it works on 32bit), then it's only because it happened to get the alignment right. This just makes 64bit hosts get it right too. I don't see any compatibility issues, non-packed + 64bit = broken. Thanks, I would actually assume that only x86-32 hosts got it right, because all 32 bit hosts I've seen other than x86 also define 8 byte alignment for uint64_t. You might however consider making it __attribute((__packed__, __aligned__(4))) instead of just packed, because otherwise you make the alignment one byte, which is not only different from what it used to be on x86-32 but also will cause inefficient compiler outpout on platforms that don't have unaligned word accesses in hardware. Arnd -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On Thu, 2010-10-14 at 22:20 +0200, Arnd Bergmann wrote: On Thursday 14 October 2010 21:58:08 Alex Williamson wrote: If it works anywhere (I assume it works on 32bit), then it's only because it happened to get the alignment right. This just makes 64bit hosts get it right too. I don't see any compatibility issues, non-packed + 64bit = broken. Thanks, I would actually assume that only x86-32 hosts got it right, because all 32 bit hosts I've seen other than x86 also define 8 byte alignment for uint64_t. You might however consider making it __attribute((__packed__, __aligned__(4))) instead of just packed, because otherwise you make the alignment one byte, which is not only different from what it used to be on x86-32 but also will cause inefficient compiler outpout on platforms that don't have unaligned word accesses in hardware. The structs in question only contain 4 8 byte elements, so there shouldn't be any change on x86-32 using one-byte aligned packing. AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone else. Performance isn't much of a consideration for this type of interface since it's only used pre-boot. In fact, the channel between qemu and the bios is only one byte wide, so wider alignment can cost extra emulated I/O accesses. Thanks, Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On Thursday 14 October 2010 22:59:04 Alex Williamson wrote: The structs in question only contain 4 8 byte elements, so there shouldn't be any change on x86-32 using one-byte aligned packing. I'm talking about the alignment of the structure, not the members within the structure. The data structure should be compatible, but not accesses to it. AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone else. You can use qemu to emulate an x86 pc on anything... Performance isn't much of a consideration for this type of interface since it's only used pre-boot. In fact, the channel between qemu and the bios is only one byte wide, so wider alignment can cost extra emulated I/O accesses. Right, the data gets passed as bytes, so it hardly matters in the end. Still the e820_add_entry assigns data to the struct members, which it either does using byte accesses and shifts or a multiple 32 bit assignment. Just because using a one byte alignment technically results in correct output doesn't make it the right solution. I don't care about the few cycles of execution time or the few bytes you waste in this particular case, but you are setting a wrong example by using smaller alignment than necessary. Arnd -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] Re: [PATCH] pc: e820 qemu_cfg tables need to be packed
On Thu, 2010-10-14 at 23:19 +0200, Arnd Bergmann wrote: On Thursday 14 October 2010 22:59:04 Alex Williamson wrote: The structs in question only contain 4 8 byte elements, so there shouldn't be any change on x86-32 using one-byte aligned packing. I'm talking about the alignment of the structure, not the members within the structure. The data structure should be compatible, but not accesses to it. AFAIK, e820 is x86-only, so we don't need to worry about breaking anyone else. You can use qemu to emulate an x86 pc on anything... You're right, I wasn't thinking about non-x86 emulating x86. I'll send a v2 with your suggestion. Thanks, Alex -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
