Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, Dec 03, 2013 at 05:03:10PM +, One Thousand Gnomes wrote: > > itself. The protocol is defined by an entity which is external to Linux. > > If this had been a bug in the IP protocol handling we would fix it and > > move on. This case is no different IMHO. > > Actually that is quite untrue. We are *very* careful that we can talk to > other internet nodes that speak broken versions of TCP. There have even > been changes made to protocol definitions during initial research to > avoid tripping bugs in existing implementations. > > > Once we set the ABI in stone then this is the sort of thing we will care > > very much about (as we have done for many years on x86). Until then it > > is not. > > Maybe Xen doesn't, but perhaps Linux doesn't wish to be tarred with the > same brush. What Xen decides is the official protocol is Xen's decision. > What a Linux guest does to keep compatibility ought to follow what Linux > does as policy. I believe that this patch does not alter the x86 protocols. Only the ARM based ones which are still in Technical Preview. > > Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, Dec 03, 2013 at 05:10:50PM +, David Vrabel wrote: > On 03/12/13 15:40, Julien Grall wrote: > > On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will > > result on different structure from Xen and Linux repositories. > > > > As Linux is using __packed__ attribute, it must have a 4-bytes padding > > before > > each "id" field. > > > > This change breaks guest block support with older kernel. IMHO, it's > > acceptable > > because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet > > freezed. > > > > Only one architecture (x86_32) doesn't have 64-bit ABI for the block > > interface. > > Don't add padding if Linux is compiled for this architecture. > > I'm now satisfied that this is the right thing to do. > > Acked-by: David Vrabel Before we go any further, I need testing confirmation that with this patch can still run on x86 hardware the following combinations: 32-bit guest on 64-bit dom0 64-bit guest on 32-bit dom0 and with an 3.12 dom0 B/c I am not seeing that mentioned anywhere and I think that is creating confusion with folks thinking it would break the x86-world (which it shouldn't). > > David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On 03/12/13 15:40, Julien Grall wrote: > On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will > result on different structure from Xen and Linux repositories. > > As Linux is using __packed__ attribute, it must have a 4-bytes padding before > each "id" field. > > This change breaks guest block support with older kernel. IMHO, it's > acceptable > because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet > freezed. > > Only one architecture (x86_32) doesn't have 64-bit ABI for the block > interface. > Don't add padding if Linux is compiled for this architecture. I'm now satisfied that this is the right thing to do. Acked-by: David Vrabel David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
> itself. The protocol is defined by an entity which is external to Linux. > If this had been a bug in the IP protocol handling we would fix it and > move on. This case is no different IMHO. Actually that is quite untrue. We are *very* careful that we can talk to other internet nodes that speak broken versions of TCP. There have even been changes made to protocol definitions during initial research to avoid tripping bugs in existing implementations. > Once we set the ABI in stone then this is the sort of thing we will care > very much about (as we have done for many years on x86). Until then it > is not. Maybe Xen doesn't, but perhaps Linux doesn't wish to be tarred with the same brush. What Xen decides is the official protocol is Xen's decision. What a Linux guest does to keep compatibility ought to follow what Linux does as policy. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, 2013-12-03 at 16:32 +, One Thousand Gnomes wrote: > > > How does the patch ensure new kernels on existing hypervisor versions > > > don't break ? > > > > As Ian said on the thread "xen-block: correctly define structures in > > public headers" (see thread https://lkml.org/lkml/2013/12/3/155), the > > ABI is not yet fixed for ARM. > > And if you are one of the existing users that helps how ? The existing users are using something which was explicitly marked as a tech preview and for which it was stated clearly that the ABI was not set in stone. They know to expect this sort of thing and have experienced it more than once already as this stuff was developed. This is actually something of a red-herring though, this is a protocol between two peers and it has now transpired that Linux was not implementing the specified protocol, even though it was able to talk to itself. The protocol is defined by an entity which is external to Linux. If this had been a bug in the IP protocol handling we would fix it and move on. This case is no different IMHO. > > > It seems to me you should be defining > > > > > > struct blkif_request_rw_v2 > > > > > > and using the correct version according to which API the hypervisor > > > requires, not just breaking it. > > > > This API doesn't involve the hypervisor. It's only a way to talk between > > DOM0 and a guest. Without this change you will break compatibility with > > other OSes. > > With this change you break compatibility between the existing OS's,new > guests and old DOM0 and vice versa. > > If a request in old format is guaranteed to error in new format (or you > can construct one that will) then you can trivially support both APIs on > the guest side at least for a while. That will avoid regressions when > people mix versions and also mean you've got a much better ability to > find a bug if stuff breaks as you won't have to switch guest and dom0 > together when debugging. Once we set the ABI in stone then this is the sort of thing we will care very much about (as we have done for many years on x86). Until then it is not. Ian. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
> > How does the patch ensure new kernels on existing hypervisor versions > > don't break ? > > As Ian said on the thread "xen-block: correctly define structures in > public headers" (see thread https://lkml.org/lkml/2013/12/3/155), the > ABI is not yet fixed for ARM. And if you are one of the existing users that helps how ? > > > > What is the failure case given the alignment change seems potentially to > > produce valid but incorrect I/O requests - can it cause corruption ? > > The request ID will likely be wrong, so the guest won't accept the > request. It should not corrupt the block device. "Would likely" That seems joyously confident. So at the very least your guest should deliberately issue a request which will error if the ABI version mismatches, and at that point you know which ABI to use so the guest can keep compatibility trivially. > > It seems to me you should be defining > > > > struct blkif_request_rw_v2 > > > > and using the correct version according to which API the hypervisor > > requires, not just breaking it. > > This API doesn't involve the hypervisor. It's only a way to talk between > DOM0 and a guest. Without this change you will break compatibility with > other OSes. With this change you break compatibility between the existing OS's,new guests and old DOM0 and vice versa. If a request in old format is guaranteed to error in new format (or you can construct one that will) then you can trivially support both APIs on the guest side at least for a while. That will avoid regressions when people mix versions and also mean you've got a much better ability to find a bug if stuff breaks as you won't have to switch guest and dom0 together when debugging. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On 12/03/2013 04:00 PM, One Thousand Gnomes wrote: > On Tue, 3 Dec 2013 15:40:37 + > Julien Grall wrote: > >> On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will >> result on different structure from Xen and Linux repositories. >> >> As Linux is using __packed__ attribute, it must have a 4-bytes padding before >> each "id" field. >> >> This change breaks guest block support with older kernel. IMHO, it's >> acceptable >> because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet >> freezed. > > How does a guest ascertain which API to use ? > > How does the patch ensure new kernels on existing hypervisor versions > don't break ? As Ian said on the thread "xen-block: correctly define structures in public headers" (see thread https://lkml.org/lkml/2013/12/3/155), the ABI is not yet fixed for ARM. > > What is the failure case given the alignment change seems potentially to > produce valid but incorrect I/O requests - can it cause corruption ? The request ID will likely be wrong, so the guest won't accept the request. It should not corrupt the block device. > It seems to me you should be defining > > struct blkif_request_rw_v2 > > and using the correct version according to which API the hypervisor > requires, not just breaking it. This API doesn't involve the hypervisor. It's only a way to talk between DOM0 and a guest. Without this change you will break compatibility with other OSes. -- Julien Grall -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, 3 Dec 2013 15:40:37 + Julien Grall wrote: > On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will > result on different structure from Xen and Linux repositories. > > As Linux is using __packed__ attribute, it must have a 4-bytes padding before > each "id" field. > > This change breaks guest block support with older kernel. IMHO, it's > acceptable > because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet > freezed. How does a guest ascertain which API to use ? How does the patch ensure new kernels on existing hypervisor versions don't break ? What is the failure case given the alignment change seems potentially to produce valid but incorrect I/O requests - can it cause corruption ? It seems to me you should be defining struct blkif_request_rw_v2 and using the correct version according to which API the hypervisor requires, not just breaking it. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, 3 Dec 2013 15:40:37 + Julien Grall julien.gr...@linaro.org wrote: On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will result on different structure from Xen and Linux repositories. As Linux is using __packed__ attribute, it must have a 4-bytes padding before each id field. This change breaks guest block support with older kernel. IMHO, it's acceptable because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet freezed. How does a guest ascertain which API to use ? How does the patch ensure new kernels on existing hypervisor versions don't break ? What is the failure case given the alignment change seems potentially to produce valid but incorrect I/O requests - can it cause corruption ? It seems to me you should be defining struct blkif_request_rw_v2 and using the correct version according to which API the hypervisor requires, not just breaking it. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On 12/03/2013 04:00 PM, One Thousand Gnomes wrote: On Tue, 3 Dec 2013 15:40:37 + Julien Grall julien.gr...@linaro.org wrote: On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will result on different structure from Xen and Linux repositories. As Linux is using __packed__ attribute, it must have a 4-bytes padding before each id field. This change breaks guest block support with older kernel. IMHO, it's acceptable because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet freezed. How does a guest ascertain which API to use ? How does the patch ensure new kernels on existing hypervisor versions don't break ? As Ian said on the thread xen-block: correctly define structures in public headers (see thread https://lkml.org/lkml/2013/12/3/155), the ABI is not yet fixed for ARM. What is the failure case given the alignment change seems potentially to produce valid but incorrect I/O requests - can it cause corruption ? The request ID will likely be wrong, so the guest won't accept the request. It should not corrupt the block device. It seems to me you should be defining struct blkif_request_rw_v2 and using the correct version according to which API the hypervisor requires, not just breaking it. This API doesn't involve the hypervisor. It's only a way to talk between DOM0 and a guest. Without this change you will break compatibility with other OSes. -- Julien Grall -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
How does the patch ensure new kernels on existing hypervisor versions don't break ? As Ian said on the thread xen-block: correctly define structures in public headers (see thread https://lkml.org/lkml/2013/12/3/155), the ABI is not yet fixed for ARM. And if you are one of the existing users that helps how ? What is the failure case given the alignment change seems potentially to produce valid but incorrect I/O requests - can it cause corruption ? The request ID will likely be wrong, so the guest won't accept the request. It should not corrupt the block device. Would likely That seems joyously confident. So at the very least your guest should deliberately issue a request which will error if the ABI version mismatches, and at that point you know which ABI to use so the guest can keep compatibility trivially. It seems to me you should be defining struct blkif_request_rw_v2 and using the correct version according to which API the hypervisor requires, not just breaking it. This API doesn't involve the hypervisor. It's only a way to talk between DOM0 and a guest. Without this change you will break compatibility with other OSes. With this change you break compatibility between the existing OS's,new guests and old DOM0 and vice versa. If a request in old format is guaranteed to error in new format (or you can construct one that will) then you can trivially support both APIs on the guest side at least for a while. That will avoid regressions when people mix versions and also mean you've got a much better ability to find a bug if stuff breaks as you won't have to switch guest and dom0 together when debugging. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, 2013-12-03 at 16:32 +, One Thousand Gnomes wrote: How does the patch ensure new kernels on existing hypervisor versions don't break ? As Ian said on the thread xen-block: correctly define structures in public headers (see thread https://lkml.org/lkml/2013/12/3/155), the ABI is not yet fixed for ARM. And if you are one of the existing users that helps how ? The existing users are using something which was explicitly marked as a tech preview and for which it was stated clearly that the ABI was not set in stone. They know to expect this sort of thing and have experienced it more than once already as this stuff was developed. This is actually something of a red-herring though, this is a protocol between two peers and it has now transpired that Linux was not implementing the specified protocol, even though it was able to talk to itself. The protocol is defined by an entity which is external to Linux. If this had been a bug in the IP protocol handling we would fix it and move on. This case is no different IMHO. It seems to me you should be defining struct blkif_request_rw_v2 and using the correct version according to which API the hypervisor requires, not just breaking it. This API doesn't involve the hypervisor. It's only a way to talk between DOM0 and a guest. Without this change you will break compatibility with other OSes. With this change you break compatibility between the existing OS's,new guests and old DOM0 and vice versa. If a request in old format is guaranteed to error in new format (or you can construct one that will) then you can trivially support both APIs on the guest side at least for a while. That will avoid regressions when people mix versions and also mean you've got a much better ability to find a bug if stuff breaks as you won't have to switch guest and dom0 together when debugging. Once we set the ABI in stone then this is the sort of thing we will care very much about (as we have done for many years on x86). Until then it is not. Ian. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
itself. The protocol is defined by an entity which is external to Linux. If this had been a bug in the IP protocol handling we would fix it and move on. This case is no different IMHO. Actually that is quite untrue. We are *very* careful that we can talk to other internet nodes that speak broken versions of TCP. There have even been changes made to protocol definitions during initial research to avoid tripping bugs in existing implementations. Once we set the ABI in stone then this is the sort of thing we will care very much about (as we have done for many years on x86). Until then it is not. Maybe Xen doesn't, but perhaps Linux doesn't wish to be tarred with the same brush. What Xen decides is the official protocol is Xen's decision. What a Linux guest does to keep compatibility ought to follow what Linux does as policy. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On 03/12/13 15:40, Julien Grall wrote: On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will result on different structure from Xen and Linux repositories. As Linux is using __packed__ attribute, it must have a 4-bytes padding before each id field. This change breaks guest block support with older kernel. IMHO, it's acceptable because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet freezed. Only one architecture (x86_32) doesn't have 64-bit ABI for the block interface. Don't add padding if Linux is compiled for this architecture. I'm now satisfied that this is the right thing to do. Acked-by: David Vrabel david.vra...@citrix.com David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, Dec 03, 2013 at 05:10:50PM +, David Vrabel wrote: On 03/12/13 15:40, Julien Grall wrote: On ARM (32 bits and 64 bits), the double-word is 8-bytes aligned. This will result on different structure from Xen and Linux repositories. As Linux is using __packed__ attribute, it must have a 4-bytes padding before each id field. This change breaks guest block support with older kernel. IMHO, it's acceptable because Xen on ARM is still on Tech Preview and the hypercall ABI is not yet freezed. Only one architecture (x86_32) doesn't have 64-bit ABI for the block interface. Don't add padding if Linux is compiled for this architecture. I'm now satisfied that this is the right thing to do. Acked-by: David Vrabel david.vra...@citrix.com Before we go any further, I need testing confirmation that with this patch can still run on x86 hardware the following combinations: 32-bit guest on 64-bit dom0 64-bit guest on 32-bit dom0 and with an 3.12 dom0 B/c I am not seeing that mentioned anywhere and I think that is creating confusion with folks thinking it would break the x86-world (which it shouldn't). David -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64
On Tue, Dec 03, 2013 at 05:03:10PM +, One Thousand Gnomes wrote: itself. The protocol is defined by an entity which is external to Linux. If this had been a bug in the IP protocol handling we would fix it and move on. This case is no different IMHO. Actually that is quite untrue. We are *very* careful that we can talk to other internet nodes that speak broken versions of TCP. There have even been changes made to protocol definitions during initial research to avoid tripping bugs in existing implementations. Once we set the ABI in stone then this is the sort of thing we will care very much about (as we have done for many years on x86). Until then it is not. Maybe Xen doesn't, but perhaps Linux doesn't wish to be tarred with the same brush. What Xen decides is the official protocol is Xen's decision. What a Linux guest does to keep compatibility ought to follow what Linux does as policy. I believe that this patch does not alter the x86 protocols. Only the ARM based ones which are still in Technical Preview. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
