[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Julien Grall]

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(-)

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 */
-- 
1.7.10.4

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[One Thousand Gnomes]

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

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Julien Grall]

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.

-- 
Julien Grall

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[One Thousand Gnomes]

> > 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

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Ian Campbell]

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.

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[One Thousand Gnomes]

> 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

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Konrad Rzeszutek Wilk]

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

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[David Vrabel]

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

[PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Konrad Rzeszutek Wilk]

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

[Xen-devel] [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Ian Campbell]

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.


-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}
-------------- next part --------------
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
}

[Xen-devel] [PATCH v2] xen/block: Correctly define structures in public headers on ARM32 and ARM64

[Roger Pau Monné]

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>