| Message ID | 1386085237-8334-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, 3 Dec 2013 15:40:37 +0000 Julien Grall <julien.grall@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
On 12/03/2013 04:00 PM, One Thousand Gnomes wrote: > On Tue, 3 Dec 2013 15:40:37 +0000 > Julien Grall <julien.grall@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.
> > 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
On Tue, 2013-12-03 at 16:32 +0000, 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.
> 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
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.vrabel@citrix.com> David
On Tue, Dec 03, 2013 at 05:10:50PM +0000, 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.vrabel@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
On Tue, Dec 03, 2013 at 05:03:10PM +0000, 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
On Tue, 2013-12-03 at 15:40 +0000, 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. Konrad asked for confirmation that this didn't change x86. Using http://stackoverflow.com/questions/9788679/how-to-get-the-relative-adress-of-a-field-in-a-structure-dump-c I created offsets.py as described there then for i386, amd64, arm, arm64 before and after I built with CONFIG_DEBUG_INFO but not CONFIG_DEBUG_INFO_REDUCED then: $ cat script python sys.path.insert(0, '') import offsets end offsets-of "struct blkif_request" offsets-of "struct blkif_request_rw" offsets-of "struct blkif_request_discard" offsets-of "struct blkif_request_other" offsets-of "struct blkif_request_indirect" $ gdb -x script --batch drivers/block/xen-blkfront.o struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 _pad1 => 3 id => 7 sector_number => 15 seg => 23 } struct blkif_request_discard { flag => 0 _pad1 => 1 _pad2 => 3 id => 7 sector_number => 15 nr_sectors => 23 _pad3 => 31 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 _pad3 => 3 id => 7 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 _pad1 => 3 id => 7 sector_number => 15 handle => 23 _pad2 => 25 indirect_grefs => 27 _pad3 => 59 } There is no difference to either x86 arch: $ diff -q x86_32.{before,after} && echo same same $ diff -q x86_64.{before,after} && echo same same $ And crucially: $ diff -q arm.after arm64.after && echo same same $ diff -q arm.after x86_64.after && echo same same $ Full results attached. Ian. struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 _pad1 => 3 id => 7 sector_number => 15 seg => 23 } struct blkif_request_discard { flag => 0 _pad1 => 1 _pad2 => 3 id => 7 sector_number => 15 nr_sectors => 23 _pad3 => 31 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 _pad3 => 3 id => 7 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 _pad1 => 3 id => 7 sector_number => 15 handle => 23 _pad2 => 25 indirect_grefs => 27 _pad3 => 59 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 id => 3 sector_number => 11 seg => 19 } struct blkif_request_discard { flag => 0 _pad1 => 1 id => 3 sector_number => 11 nr_sectors => 19 _pad3 => 27 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 id => 3 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 id => 3 sector_number => 11 handle => 19 _pad2 => 21 indirect_grefs => 23 _pad3 => 55 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 _pad1 => 3 id => 7 sector_number => 15 seg => 23 } struct blkif_request_discard { flag => 0 _pad1 => 1 _pad2 => 3 id => 7 sector_number => 15 nr_sectors => 23 _pad3 => 31 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 _pad3 => 3 id => 7 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 _pad1 => 3 id => 7 sector_number => 15 handle => 23 _pad2 => 25 indirect_grefs => 27 _pad3 => 59 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 id => 3 sector_number => 11 seg => 19 } struct blkif_request_discard { flag => 0 _pad1 => 1 id => 3 sector_number => 11 nr_sectors => 19 _pad3 => 27 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 id => 3 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 id => 3 sector_number => 11 handle => 19 _pad2 => 21 indirect_grefs => 23 _pad3 => 55 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 id => 3 sector_number => 11 seg => 19 } struct blkif_request_discard { flag => 0 _pad1 => 1 id => 3 sector_number => 11 nr_sectors => 19 _pad3 => 27 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 id => 3 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 id => 3 sector_number => 11 handle => 19 _pad2 => 21 indirect_grefs => 23 _pad3 => 55 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 id => 3 sector_number => 11 seg => 19 } struct blkif_request_discard { flag => 0 _pad1 => 1 id => 3 sector_number => 11 nr_sectors => 19 _pad3 => 27 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 id => 3 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 id => 3 sector_number => 11 handle => 19 _pad2 => 21 indirect_grefs => 23 _pad3 => 55 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 _pad1 => 3 id => 7 sector_number => 15 seg => 23 } struct blkif_request_discard { flag => 0 _pad1 => 1 _pad2 => 3 id => 7 sector_number => 15 nr_sectors => 23 _pad3 => 31 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 _pad3 => 3 id => 7 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 _pad1 => 3 id => 7 sector_number => 15 handle => 23 _pad2 => 25 indirect_grefs => 27 _pad3 => 59 } struct blkif_request { operation => 0 u => 1 } struct blkif_request_rw { nr_segments => 0 handle => 1 _pad1 => 3 id => 7 sector_number => 15 seg => 23 } struct blkif_request_discard { flag => 0 _pad1 => 1 _pad2 => 3 id => 7 sector_number => 15 nr_sectors => 23 _pad3 => 31 } struct blkif_request_other { _pad1 => 0 _pad2 => 1 _pad3 => 3 id => 7 } struct blkif_request_indirect { indirect_op => 0 nr_segments => 1 _pad1 => 3 id => 7 sector_number => 15 handle => 23 _pad2 => 25 indirect_grefs => 27 _pad3 => 59 }
On 12/12/13 15:19, Ian Campbell wrote: > On Tue, 2013-12-03 at 15:40 +0000, 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. > > Konrad asked for confirmation that this didn't change x86. I've also tested this using various combinations of kernels, and it seems to be perfectly fine, so: Acked-by: Roger Pau Monné <roger.pau@citrix.com>
diff --git a/include/xen/interface/io/blkif.h b/include/xen/interface/io/blkif.h index 65e1209..ae665ac 100644 --- a/include/xen/interface/io/blkif.h +++ b/include/xen/interface/io/blkif.h @@ -146,7 +146,7 @@ struct blkif_request_segment_aligned { struct blkif_request_rw { uint8_t nr_segments; /* number of segments */ blkif_vdev_t handle; /* only for read/write requests */ -#ifdef CONFIG_X86_64 +#ifndef CONFIG_X86_32 uint32_t _pad1; /* offsetof(blkif_request,u.rw.id) == 8 */ #endif uint64_t id; /* private guest value, echoed in resp */ @@ -163,7 +163,7 @@ struct blkif_request_discard { uint8_t flag; /* BLKIF_DISCARD_SECURE or zero. */ #define BLKIF_DISCARD_SECURE (1<<0) /* ignored if discard-secure=0 */ blkif_vdev_t _pad1; /* only for read/write requests */ -#ifdef CONFIG_X86_64 +#ifndef CONFIG_X86_32 uint32_t _pad2; /* offsetof(blkif_req..,u.discard.id)==8*/ #endif uint64_t id; /* private guest value, echoed in resp */ @@ -175,7 +175,7 @@ struct blkif_request_discard { struct blkif_request_other { uint8_t _pad1; blkif_vdev_t _pad2; /* only for read/write requests */ -#ifdef CONFIG_X86_64 +#ifndef CONFIG_X86_32 uint32_t _pad3; /* offsetof(blkif_req..,u.other.id)==8*/ #endif uint64_t id; /* private guest value, echoed in resp */ @@ -184,7 +184,7 @@ struct blkif_request_other { struct blkif_request_indirect { uint8_t indirect_op; uint16_t nr_segments; -#ifdef CONFIG_X86_64 +#ifndef CONFIG_X86_32 uint32_t _pad1; /* offsetof(blkif_...,u.indirect.id) == 8 */ #endif uint64_t id; @@ -192,7 +192,7 @@ struct blkif_request_indirect { blkif_vdev_t handle; uint16_t _pad2; grant_ref_t indirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST]; -#ifdef CONFIG_X86_64 +#ifndef CONFIG_X86_32 uint32_t _pad3; /* make it 64 byte aligned */ #else uint64_t _pad3; /* make it 64 byte aligned */
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. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Roger Pau Monne <roger.pau@citrix.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: David Vrabel <david.vrabel@citrix.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v2: - Reverse define Changes in v1: - This patch is the rework of "xen-block: correctly define structures in public headers" sent by Roger (see https://lkml.org/lkml/2013/12/3/155). --- include/xen/interface/io/blkif.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)