Xen devel - Dec 2006 - [patch/rfc] multiprotocol blkback drivers (32-on-64)

Hi,

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added
to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Frontend changes:
 * Create a new node "protocol", add the protocol number it speaks
   there.

Backend changes:
 * Look at the "protocol" number of the frontend and switch ring
   handling accordingly.  If the protocol node isn''t present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn''t need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Tools changes:
 * Add one more option to the disk configuration, so one can specify the
   protocol the frontend speaks in the config file.  This is needed for
   old frontends which don''t advertise the protocol they are speaking
   themself.
   I''m not that happy with this approach, but it works for now and
I''m
   kida lost in the stack of python classes doing domain and device
   handling ...

Consider the code experimental, not all frontend/backend combinations
are tested.

Comments?  Questions?  Suggesions?

cheers,
  Gerd

PS: Anyone working on blkback/blktap code sharing?  While walking
    through the code I''ve noticed quite alot of it is cut&paste ...

-- 
Gerd Hoffmann <kraxel@suse.de>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I understand you favor this over the bi-modal approach I took? Any specific
advantages? Jan
>>> Gerd Hoffmann <kraxel@suse.de> 18.12.06 17:39 >>>
Hi,

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added
to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Frontend changes:
 * Create a new node "protocol", add the protocol number it speaks
   there.

Backend changes:
 * Look at the "protocol" number of the frontend and switch ring
   handling accordingly.  If the protocol node isn''t present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn''t need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Tools changes:
 * Add one more option to the disk configuration, so one can specify the
   protocol the frontend speaks in the config file.  This is needed for
   old frontends which don''t advertise the protocol they are speaking
   themself.
   I''m not that happy with this approach, but it works for now and
I''m
   kida lost in the stack of python classes doing domain and device
   handling ...

Consider the code experimental, not all frontend/backend combinations
are tested.

Comments?  Questions?  Suggesions?

cheers,
  Gerd

PS: Anyone working on blkback/blktap code sharing?  While walking
    through the code I''ve noticed quite alot of it is cut&paste ...

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gerd''s description is along the lines of what I would implement myself.
How
does your bi-modal approach work?

 -- Keir

On 18/12/06 17:09, "Jan Beulich" <jbeulich@novell.com> wrote:
> I understand you favor this over the bi-modal approach I took? Any specific
> advantages? Jan
> 
>>>> Gerd Hoffmann <kraxel@suse.de> 18.12.06 17:39
>>>
> Hi,
> 
> This is a patch for the block interface, frontend drivers, backend
> drivers and tools to support multiple ring protocols.  Right there are
> now just two: the 32bit and the 64bit one.  If needed it can be extended.
> 
> Interface changes (io/blkif.h)
>  * Have both request structs there, with "v1" and "v2"
added to the
>    name.  The old name is aliased to the native protocol of the
>    architecture.
>  * Add helper functions to convert v1/v2 requests to native.
> 
> Frontend changes:
>  * Create a new node "protocol", add the protocol number it
speaks
>    there.
> 
> Backend changes:
>  * Look at the "protocol" number of the frontend and switch ring
>    handling accordingly.  If the protocol node isn''t present it
assumes
>    native protocol.
>  * As the request struct is copied anyway before being processed (for
>    security reasons) it is converted to native at that point so most
>    backend code doesn''t need to know what the frontend speaks.
>  * In case of blktap this is completely transparent to userspace, the
>    kernel/userspace ring is always native no matter what the frontend
>    speaks.
> 
> Tools changes:
>  * Add one more option to the disk configuration, so one can specify the
>    protocol the frontend speaks in the config file.  This is needed for
>    old frontends which don''t advertise the protocol they are
speaking
>    themself.
>    I''m not that happy with this approach, but it works for now and
I''m
>    kida lost in the stack of python classes doing domain and device
>    handling ...
> 
> Consider the code experimental, not all frontend/backend combinations
> are tested.
> 
> Comments?  Questions?  Suggesions?
> 
> cheers,
>   Gerd
> 
> PS: Anyone working on blkback/blktap code sharing?  While walking
>     through the code I''ve noticed quite alot of it is
cut&paste ...

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

By adding abstraction to the ring macros and the affected headers, and then
replacing
direct structure member accesses with appropriate macros. Reference patches
attached (not checked whether they would apply cleanly on -unstable). Jan
>>> Keir Fraser <keir@xensource.com> 18.12.06 18:58 >>>
Gerd''s description is along the lines of what I would implement myself.
How
does your bi-modal approach work?

 -- Keir

On 18/12/06 17:09, "Jan Beulich" <jbeulich@novell.com> wrote:
> I understand you favor this over the bi-modal approach I took? Any specific
> advantages? Jan
> 
>>>> Gerd Hoffmann <kraxel@suse.de> 18.12.06 17:39
>>>
> Hi,
> 
> This is a patch for the block interface, frontend drivers, backend
> drivers and tools to support multiple ring protocols.  Right there are
> now just two: the 32bit and the 64bit one.  If needed it can be extended.
> 
> Interface changes (io/blkif.h)
>  * Have both request structs there, with "v1" and "v2"
added to the
>    name.  The old name is aliased to the native protocol of the
>    architecture.
>  * Add helper functions to convert v1/v2 requests to native.
> 
> Frontend changes:
>  * Create a new node "protocol", add the protocol number it
speaks
>    there.
> 
> Backend changes:
>  * Look at the "protocol" number of the frontend and switch ring
>    handling accordingly.  If the protocol node isn''t present it
assumes
>    native protocol.
>  * As the request struct is copied anyway before being processed (for
>    security reasons) it is converted to native at that point so most
>    backend code doesn''t need to know what the frontend speaks.
>  * In case of blktap this is completely transparent to userspace, the
>    kernel/userspace ring is always native no matter what the frontend
>    speaks.
> 
> Tools changes:
>  * Add one more option to the disk configuration, so one can specify the
>    protocol the frontend speaks in the config file.  This is needed for
>    old frontends which don''t advertise the protocol they are
speaking
>    themself.
>    I''m not that happy with this approach, but it works for now and
I''m
>    kida lost in the stack of python classes doing domain and device
>    handling ...
> 
> Consider the code experimental, not all frontend/backend combinations
> are tested.
> 
> Comments?  Questions?  Suggesions?
> 
> cheers,
>   Gerd
> 
> PS: Anyone working on blkback/blktap code sharing?  While walking
>     through the code I''ve noticed quite alot of it is
cut&paste ...

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com 
http://lists.xensource.com/xen-devel





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

There are a couple of things I''d like to see changed if this is what we
want to
go with:

- "if (protocol == 1) {} else {}" should be switches, failing (or even
BUGing) for
  all protocol versions other than 1 and 2
- assuming the abstraction is meant to scale to future protocol versions, adding
  many such explicit version handling code paths seems undesirable, as seems
  adding extra version specific variables or (non-union) structure members
- using all error possible values returned from xenbus_gather to indicate an old
  frontend seems odd at least - one specific error value should be
  recognized here
- unconditionally using #pragma pack(), __attribute__(()), and __i386__ or
  __x86_64__ in public Xen headers is, in my opinion, a no-go (these header
  should all be suitable for building e.g. Windows drivers, too - I know this
isn''t
  generally the case at present, but I don''t think anything else can be
the goal,
  and hence the situation shouldn''t be made worse)

Jan
>>> Gerd Hoffmann <kraxel@suse.de> 18.12.06 17:39 >>>
Hi,

This is a patch for the block interface, frontend drivers, backend
drivers and tools to support multiple ring protocols.  Right there are
now just two: the 32bit and the 64bit one.  If needed it can be extended.

Interface changes (io/blkif.h)
 * Have both request structs there, with "v1" and "v2" added
to the
   name.  The old name is aliased to the native protocol of the
   architecture.
 * Add helper functions to convert v1/v2 requests to native.

Frontend changes:
 * Create a new node "protocol", add the protocol number it speaks
   there.

Backend changes:
 * Look at the "protocol" number of the frontend and switch ring
   handling accordingly.  If the protocol node isn''t present it assumes
   native protocol.
 * As the request struct is copied anyway before being processed (for
   security reasons) it is converted to native at that point so most
   backend code doesn''t need to know what the frontend speaks.
 * In case of blktap this is completely transparent to userspace, the
   kernel/userspace ring is always native no matter what the frontend
   speaks.

Tools changes:
 * Add one more option to the disk configuration, so one can specify the
   protocol the frontend speaks in the config file.  This is needed for
   old frontends which don''t advertise the protocol they are speaking
   themself.
   I''m not that happy with this approach, but it works for now and
I''m
   kida lost in the stack of python classes doing domain and device
   handling ...

Consider the code experimental, not all frontend/backend combinations
are tested.

Comments?  Questions?  Suggesions?

cheers,
  Gerd

PS: Anyone working on blkback/blktap code sharing?  While walking
    through the code I''ve noticed quite alot of it is cut&paste ...

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
> I understand you favor this over the bi-modal approach I took? Any specific
> advantages? Jan
IMHO the code is more readable and I''d rate the chance to be accepted
by
lkml review higher.  I don''t like the approach to hide alot of the
logic
in preprocessor magic.

It leaves the door open to add more protocols.  Not that I see a need
right now.  But maybe the lkml folks ask us to consolidate to some
struct layout which doesn''t look different on different architectures
(i.e. sort struct members by size), then we maybe have to support a
third protocol ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich wrote:
> There are a couple of things I''d like to see changed if this is
what we want to
> go with:
yep, it''s a first at least partly working rfc patch, certainly not
final
yet ;)
> - "if (protocol == 1) {} else {}" should be switches, failing (or
even BUGing) for
>   all protocol versions other than 1 and 2
BUG() should be ok, in theory the code should never ever reach one of
the switches with an uninitialized protocol.
> - assuming the abstraction is meant to scale to future protocol versions,
adding
>   many such explicit version handling code paths seems undesirable, as
seems
>   adding extra version specific variables or (non-union) structure members
Using unions is one of the things I plan to change.
> - using all error possible values returned from xenbus_gather to indicate
an old
>   frontend seems odd at least - one specific error value should be
>   recognized here
Yep, would be a bit cleaner, although I don''t see any other possible
reason than a nonexisting node why it should fail at that point ...
> - unconditionally using #pragma pack(), __attribute__(()), and __i386__ or
>   __x86_64__ in public Xen headers is, in my opinion, a no-go (these header
>   should all be suitable for building e.g. Windows drivers, too - I know
this isn''t
>   generally the case at present, but I don''t think anything else
can be the goal,
>   and hence the situation shouldn''t be made worse)
Yep, we need some solution for that.  The sun folks will veto at least
the attribute stuff too.  Not sure pragma pack is a portability issue.
I think we need some compiler.h which provides that kind of stuff, i.e.

#ifdef __GNUC__
#define __align8__ __attribute__((__aligned__(8)))
#endif

#ifdef __suncc__
#define __align8__ something_else
#endif

Also some of the bits I''ve placed into blkif.h for now should go to a
linux-kernel header instead I think.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Next version attached.  WARNING: compiles, but untested otherwise.
> - "if (protocol == 1) {} else {}" should be switches, failing (or
even BUGing) for
>   all protocol versions other than 1 and 2
fixed.
> - assuming the abstraction is meant to scale to future protocol versions,
adding
>   many such explicit version handling code paths seems undesirable, as
seems
>   adding extra version specific variables or (non-union) structure members
switch to unions done, also killed some "switch (protocols)"
statements
along the way, leaving only two places with different code paths: ring
initialization and copying requests out of the ring.
> - using all error possible values returned from xenbus_gather to indicate
an old
>   frontend seems odd at least - one specific error value should be
>   recognized here
Hmm, looked at netfront/back to figure the correct error code.  They act
the same way though, i.e. consider errors on reading "feature-*" nodes
as "doesn''t exist" ...
> - unconditionally using #pragma pack(), __attribute__(()), and __i386__ or
>   __x86_64__ in public Xen headers is, in my opinion, a no-go (these header
>   should all be suitable for building e.g. Windows drivers, too - I know
this isn''t
>   generally the case at present, but I don''t think anything else
can be the goal,
>   and hence the situation shouldn''t be made worse)
Ideas for that one?  Ok to create xen/include/public/compiler.h with
that kind of stuff in?

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 19/12/06 13:32, "Gerd Hoffmann" <kraxel@suse.de> wrote:
>> - unconditionally using #pragma pack(), __attribute__(()), and __i386__
or
>>   __x86_64__ in public Xen headers is, in my opinion, a no-go (these
header
>>   should all be suitable for building e.g. Windows drivers, too - I
know this
>> isn''t
>>   generally the case at present, but I don''t think anything
else can be the
>> goal,
>>   and hence the situation shouldn''t be made worse)
> 
> Ideas for that one?  Ok to create xen/include/public/compiler.h with
> that kind of stuff in?
I think it would be reasonable to put this stuff in a (new) Linux-specific
header file that wraps the Xen-public blkif.h. We could put just enough
support in blkif.h itself to allow it to be multiply-compiled. Then
different OSes can wrap or rewrite blkif.h as they see fit to get the
required layout for 32-bit and 64-bit ABIs. This conveniently sidesteps some
of these issues and allows you to concentrate on Linux and GCC, while not
constraining the implementation choices for anyone else.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi,
> I think it would be reasonable to put this stuff in a (new) Linux-specific
> header file that wraps the Xen-public blkif.h. We could put just enough
> support in blkif.h itself to allow it to be multiply-compiled. Then
> different OSes can wrap or rewrite blkif.h as they see fit to get the
> required layout for 32-bit and 64-bit ABIs. This conveniently sidesteps
some
> of these issues and allows you to concentrate on Linux and GCC, while not
> constraining the implementation choices for anyone else.
Next interation with exactly that implemented, this time even tested
with blkback in all four combinations out of 32/64 dom0, 32/64 guest.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Looks a lot nicer now, except that I dislike the replication of the
request/response
structures in now three places. Besides possibly being a maintenance issue, this
now seems worse in terms of scaling up to eventual future protocol versions. I
had
understood Keir in a much different way - adding a compiler abstraction header
(which maybe could even make use of Linux'' native ones) to include/xen,
and
making use of its abstraction directly in xen/include/public/io/blkif.h.

Jan
>>> Gerd Hoffmann <kraxel@suse.de> 20.12.06 16:12 >>>
Hi,
> I think it would be reasonable to put this stuff in a (new) Linux-specific
> header file that wraps the Xen-public blkif.h. We could put just enough
> support in blkif.h itself to allow it to be multiply-compiled. Then
> different OSes can wrap or rewrite blkif.h as they see fit to get the
> required layout for 32-bit and 64-bit ABIs. This conveniently sidesteps
some
> of these issues and allows you to concentrate on Linux and GCC, while not
> constraining the implementation choices for anyone else.
Next interation with exactly that implemented, this time even tested
with blkback in all four combinations out of 32/64 dom0, 32/64 guest.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 20/12/06 15:47, "Jan Beulich" <jbeulich@novell.com> wrote:
> Looks a lot nicer now, except that I dislike the replication of the
> request/response
> structures in now three places. Besides possibly being a maintenance issue,
> this
> now seems worse in terms of scaling up to eventual future protocol
versions. I
> had
> understood Keir in a much different way - adding a compiler abstraction
header
> (which maybe could even make use of Linux'' native ones) to
include/xen, and
> making use of its abstraction directly in xen/include/public/io/blkif.h.
I quite like the explicitness of this approach. The duplication is small and
the structures aren''t going to change (without changing protocol
version
too) so maintenance is not that much of an issue. It would be nicer to put
the structure definition inside a macro, instantiated multiple times, or
inside another header file, included multiple times, though.

My other suggested approach is slightly hindered by the fact that all the
Xen public headers are include-only-once, although we could circumvent that
for this case I suppose. It''s also questionable whether this can be
done
really cleanly in a way that most compilers can adapt to -- do most have the
kind of flexible packing pragmas that GCC has?

Xen/include/public/io/blkif.h changes infrequently enough that we could
define linux/include/xen/blkif.h to be Linux''s version and not include
the
Xen-provided one directly at all. The Xen headers need Linux formatting
anyway, so it''s not like the Linux blkif.h will ever be a direct copy
of the
original Xen-provided blkif.h.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel