PV blk backend: Make 32bit PV guests work on 64bit PV Dom0

PATCH In review Christoph Egger 2 participants 8 messages Jul 28, 2008

lore

Share & tools

lore

Christoph Egger

author

2008-07-28 11:46 UTC #0 lore raw

PATCH 1/1 PV blk backend: Make 32bit PV guests work on 64bit PV Dom0

Hi,

Attached patch makes the PV block driver for 32bit PV guests working on 64bit PV Dom0.

In the io/blkif.h, the 'id' field of
struct blkif_request is 64bit, but on a LP32 system it's aligned on 32bits, while it'll be aligned to 64bits on a LP64 system. So on LP64 there'll be 4 bytes of padding between handle and id. The same is true for struct blkif_response: it's 11 bytes long, so will be aligned differently in the ring between a 32 and 64bit systems.

In order to fix this, attached patch introduces in the public headers, blkif_request/blkif_response, blkif_x86_32_request/blkif_x86_32_response and blkif_x86_64_request/blkif_x86_64_response.
NetBSD is doing so io/blkif.h in the public headers, while Linux does so in include/xen/blkif.h in the linux sources. But the result is the same ...

Successfully tested with the combinations:

64bit Dom0 32bit PAE PV guest
NetBSD Linux
Linux NetBSD

The combinations 64bit Dom0, 64bit PV DomU works with and without this patch.

Signed-off-by Christoph Egger <[email protected]>

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

xen/include/public/io/blkif.h +49 -0

diff -r 37fae02cc335 xen/include/public/io/blkif.h
@@ -91,6 +91,8 @@ struct blkif_request_segment {
     uint8_t     first_sect, last_sect;
 };
 
+/* native-type requests/responses (always used in frontends ) */
+
 struct blkif_request {
     uint8_t        operation;    /* BLKIF_OP_???                         */
     uint8_t        nr_segments;  /* number of segments                   */

@@ -108,6 +110,44 @@ struct blkif_response {
 };
 typedef struct blkif_response blkif_response_t;
 
+/* i386 requests/responses */
+struct blkif_x86_32_request {
+    uint8_t        operation;    /* BLKIF_OP_???                         */
+    uint8_t        nr_segments;  /* number of segments                   */
+    blkif_vdev_t   handle;       /* only for read/write requests         */
+    uint64_t       id;           /* private guest value, echoed in resp  */
+    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+} /* __packed */;
+typedef struct blkif_x86_32_request blkif_x86_32_request_t;
+
+struct blkif_x86_32_response {
+    uint64_t        id;              /* copied from request */
+    uint8_t         operation;       /* copied from request */
+    uint8_t         _pad; 
+    int16_t         status;          /* BLKIF_RSP_???       */
+} /* __packed */;
+typedef struct blkif_x86_32_response blkif_x86_32_response_t;
+
+/* amd64-type requests/responses (always used in frontends ) */
+
+struct blkif_x86_64_request {
+    uint8_t          operation;    /* BLKIF_OP_???                         */
+    uint8_t          nr_segments;  /* number of segments                   */
+    blkif_vdev_t     handle;       /* only for read/write requests         */
+    uint64_aligned_t id;/* private guest value, echoed in resp  */
+    blkif_sector_t   sector_number;/* start sector idx on disk (r/w only)  */
+    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+};
+typedef struct blkif_x86_64_request blkif_x86_64_request_t;
+
+struct blkif_x86_64_response {
+    uint64_aligned_t id; /* copied from request */
+    uint8_t          operation;       /* copied from request */
+    int16_t          status;          /* BLKIF_RSP_???       */
+};
+typedef struct blkif_x86_64_response blkif_x86_64_response_t;
+
 /*
  * STATUS RETURN CODES.
  */

@@ -123,6 +163,15 @@ typedef struct blkif_response blkif_resp
  */
 
 DEFINE_RING_TYPES(blkif, struct blkif_request, struct blkif_response);
+DEFINE_RING_TYPES(blkif_x86_32, struct blkif_x86_32_request, struct blkif_x86_32_response);
+DEFINE_RING_TYPES(blkif_x86_64, struct blkif_x86_64_request, struct blkif_x86_64_response);
+
+union blkif_back_ring_proto {
+	blkif_back_ring_t ring_n; /* native/common members */
+	blkif_x86_32_back_ring_t ring_32;
+	blkif_x86_64_back_ring_t ring_64;
+};
+typedef union blkif_back_ring_proto blkif_back_ring_proto_t;
 
 #define VDISK_CDROM        0x1
 #define VDISK_REMOVABLE    0x2

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-07-28 12:23 UTC #1 lore raw

Just do the same as we do in Linux and put these defs in your NetBSD private header space.

-- Keir

Quoted 30 lines · Christoph Egger

Hi,
Attached patch makes the PV block driver for 32bit PV guests working on 64bit PV Dom0.
In the io/blkif.h, the 'id' field of
struct blkif_request is 64bit, but on a LP32 system it's aligned on 32bits, while it'll be aligned to 64bits on a LP64 system. So on LP64 there'll be 4 bytes of padding between handle and id. The same is true for struct blkif_response: it's 11 bytes long, so will be aligned differently in the ring between a 32 and 64bit systems.
In order to fix this, attached patch introduces in the public headers, blkif_request/blkif_response, blkif_x86_32_request/blkif_x86_32_response and blkif_x86_64_request/blkif_x86_64_response.
NetBSD is doing so io/blkif.h in the public headers, while Linux does so in include/xen/blkif.h in the linux sources. But the result is the same ...
Successfully tested with the combinations:
64bit Dom0 32bit PAE PV guest
NetBSD Linux
Linux NetBSD
The combinations 64bit Dom0, 64bit PV DomU works with and without this patch.
Signed-off-by Christoph Egger <[email protected]>

Christoph Egger

author

2008-07-28 12:31 UTC #2 lore raw

Quoted 2 lines · On Monday 28 July 2008 14:23:00 Keir Fraser wrote:

Just do the same as we do in Linux and put these defs in your NetBSD private header space.

That sounds like "All OS's have to go the Linux way" (which also includes Xen's Minios (stubdom)) instead of "fix it once and for all".

Christoph

Quoted 33 lines

-- Keir
Quoted 29 lines · Christoph Egger
Hi,
Attached patch makes the PV block driver for 32bit PV guests working on 64bit PV Dom0.
In the io/blkif.h, the 'id' field of
struct blkif_request is 64bit, but on a LP32 system it's aligned on 32bits, while it'll be aligned to 64bits on a LP64 system. So on LP64 there'll be 4 bytes of padding between handle and id. The same is true for struct blkif_response: it's 11 bytes long, so will be aligned differently in the ring between a 32 and 64bit systems.
In order to fix this, attached patch introduces in the public headers, blkif_request/blkif_response, blkif_x86_32_request/blkif_x86_32_response and blkif_x86_64_request/blkif_x86_64_response.
NetBSD is doing so io/blkif.h in the public headers, while Linux does so in include/xen/blkif.h in the linux sources. But the result is the same ...
Successfully tested with the combinations:
64bit Dom0 32bit PAE PV guest
NetBSD Linux
Linux NetBSD
The combinations 64bit Dom0, 64bit PV DomU works with and without this patch.
Signed-off-by Christoph Egger <[email protected]>

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Keir Fraser

2008-07-28 12:36 UTC #3 lore raw

Quoted 6 lines · Christoph Egger

Quoted 2 lines · On Monday 28 July 2008 14:23:00 Keir Fraser wrote:
Just do the same as we do in Linux and put these defs in your NetBSD private header space.
That sounds like "All OS's have to go the Linux way" (which also includes Xen's Minios (stubdom)) instead of "fix it once and for all".

Quite the opposite. By doing it in your own header files you can specify the structure names and so on exactly how you would like them to be for the particular OS concerned.

-- Keir

Christoph Egger

author

2008-07-28 12:52 UTC #4 lore raw

Quoted 11 lines · On Monday 28 July 2008 14:36:49 Keir Fraser wrote:

Quoted 6 lines · Christoph Egger
Quoted 2 lines · On Monday 28 July 2008 14:23:00 Keir Fraser wrote:
Just do the same as we do in Linux and put these defs in your NetBSD private header space.
That sounds like "All OS's have to go the Linux way" (which also includes Xen's Minios (stubdom)) instead of "fix it once and for all".
Quite the opposite. By doing it in your own header files you can specify the structure names and so on exactly how you would like them to be for the particular OS concerned.

I'm concerned about having/keeping the API/ABI the same between all OS's and not about their implementation.

Christoph

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Keir Fraser

2008-07-28 13:00 UTC #5 lore raw

Quoted 6 lines · Christoph Egger

Quoted 3 lines
Quite the opposite. By doing it in your own header files you can specify the structure names and so on exactly how you would like them to be for the particular OS concerned.
I'm concerned about having/keeping the API/ABI the same between all OS's and not about their implementation.

The interfaces (both the API exposed by public/ headers; and the ABI they describe) won't change. At least not in a way that isn't backward compatible. Or we'd be breaking old OSes all the time!

-- Keir

Keir Fraser

2008-07-28 13:04 UTC #6 lore raw

Quoted 10 lines · Keir Fraser

Quoted 6 lines
Quoted 3 lines
Quite the opposite. By doing it in your own header files you can specify the structure names and so on exactly how you would like them to be for the particular OS concerned.
I'm concerned about having/keeping the API/ABI the same between all OS's and not about their implementation.
The interfaces (both the API exposed by public/ headers; and the ABI they describe) won't change. At least not in a way that isn't backward compatible. Or we'd be breaking old OSes all the time!

I might add: why would placing those extra struct definitions in the public headers make it any more or less likely that we'll break compatibility in future? That makes no sense to me.

-- Keir

Christoph Egger

author

2008-07-28 13:08 UTC #7 lore raw

In reply to Keir Fraser

Quoted 11 lines · On Monday 28 July 2008 15:00:35 Keir Fraser wrote:

Quoted 6 lines · Christoph Egger
Quoted 3 lines
Quite the opposite. By doing it in your own header files you can specify the structure names and so on exactly how you would like them to be for the particular OS concerned.
I'm concerned about having/keeping the API/ABI the same between all OS's and not about their implementation.
The interfaces (both the API exposed by public/ headers; and the ABI they describe) won't change. At least not in a way that isn't backward compatible. Or we'd be breaking old OSes all the time!

Haven't those got broken with the removal of non-PAE support? :)

Christoph

Signature · 12 lines

AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy