• Yann Droneaud's avatar
    IB/core: extended command: an improved infrastructure for uverbs commands · f21519b2
    Yann Droneaud authored
    Commit 400dbc96 ("IB/core: Infrastructure for extensible uverbs
    commands") added an infrastructure for extensible uverbs commands
    while later commit 436f2ad0 ("IB/core: Export ib_create/destroy_flow
    through uverbs") exported ib_create_flow()/ib_destroy_flow() functions
    using this new infrastructure.
    
    According to the commit 400dbc96, the purpose of this
    infrastructure is to support passing around provider (eg. hardware)
    specific buffers when userspace issue commands to the kernel, so that
    it would be possible to extend uverbs (eg. core) buffers independently
    from the provider buffers.
    
    But the new kernel command function prototypes were not modified to
    take advantage of this extension. This issue was exposed by Roland
    Dreier in a previous review[1].
    
    So the following patch is an attempt to a revised extensible command
    infrastructure.
    
    This improved extensible command infrastructure distinguish between
    core (eg. legacy)'s command/response buffers from provider
    (eg. hardware)'s command/response buffers: each extended command
    implementing function is given a struct ib_udata to hold core
    (eg. uverbs) input and output buffers, and another struct ib_udata to
    hold the hw (eg. provider) input and output buffers.
    
    Having those buffers identified separately make it easier to increase
    one buffer to support extension without having to add some code to
    guess the exact size of each command/response parts: This should make
    the extended functions more reliable.
    
    Additionally, instead of relying on command identifier being greater
    than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure rely on
    unused bits in command field: on the 32 bits provided by command
    field, only 6 bits are really needed to encode the identifier of
    commands currently supported by the kernel. (Even using only 6 bits
    leaves room for about 23 new commands).
    
    So this patch makes use of some high order bits in command field to
    store flags, leaving enough room for more command identifiers than one
    will ever need (eg. 256).
    
    The new flags are used to specify if the command should be processed
    as an extended one or a legacy one. While designing the new command
    format, care was taken to make usage of flags itself extensible.
    
    Using high order bits of the commands field ensure that newer
    libibverbs on older kernel will properly fail when trying to call
    extended commands. On the other hand, older libibverbs on newer kernel
    will never be able to issue calls to extended commands.
    
    The extended command header includes the optional response pointer so
    that output buffer length and output buffer pointer are located
    together in the command, allowing proper parameters checking. This
    should make implementing functions easier and safer.
    
    Additionally the extended header ensure 64bits alignment, while making
    all sizes multiple of 8 bytes, extending the maximum buffer size:
    
                                 legacy      extended
    
       Maximum command buffer:  256KBytes   1024KBytes (512KBytes + 512KBytes)
      Maximum response buffer:  256KBytes   1024KBytes (512KBytes + 512KBytes)
    
    For the purpose of doing proper buffer size accounting, the headers
    size are no more taken in account in "in_words".
    
    One of the odds of the current extensible infrastructure, reading
    twice the "legacy" command header, is fixed by removing the "legacy"
    command header from the extended command header: they are processed as
    two different parts of the command: memory is read once and
    information are not duplicated: it's making clear that's an extended
    command scheme and not a different command scheme.
    
    The proposed scheme will format input (command) and output (response)
    buffers this way:
    
    - command:
    
      legacy header +
      extended header +
      command data (core + hw):
    
        +----------------------------------------+
        | flags     |   00      00    |  command |
        |        in_words    |   out_words       |
        +----------------------------------------+
        |                 response               |
        |                 response               |
        | provider_in_words | provider_out_words |
        |                 padding                |
        +----------------------------------------+
        |                                        |
        .              <uverbs input>            .
        .              (in_words * 8)            .
        |                                        |
        +----------------------------------------+
        |                                        |
        .             <provider input>           .
        .          (provider_in_words * 8)       .
        |                                        |
        +----------------------------------------+
    
    - response, if present:
    
        +----------------------------------------+
        |                                        |
        .          <uverbs output space>         .
        .             (out_words * 8)            .
        |                                        |
        +----------------------------------------+
        |                                        |
        .         <provider output space>        .
        .         (provider_out_words * 8)       .
        |                                        |
        +----------------------------------------+
    
    The overall design is to ensure that the extensible infrastructure is
    itself extensible while begin more reliable with more input and bound
    checking.
    
    Note:
    
    The unused field in the extended header would be perfect candidate to
    hold the command "comp_mask" (eg. bit field used to handle
    compatibility).  This was suggested by Roland Dreier in a previous
    review[2].  But "comp_mask" field is likely to be present in the uverb
    input and/or provider input, likewise for the response, as noted by
    Matan Barak[3], so it doesn't make sense to put "comp_mask" in the
    header.
    
    [1]:
    http://marc.info/?i=CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA@mail.gmail.com
    
    [2]:
    http://marc.info/?i=CAL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA@mail.gmail.com
    
    [3]:
    http://marc.info/?i=525C1149.6000701@mellanox.comSigned-off-by: default avatarYann Droneaud <ydroneaud@opteya.com>
    Link: http://marc.info/?i=cover.1383773832.git.ydroneaud@opteya.com
    
    [ Convert "ret ? ret : 0" to the equivalent "ret".  - Roland ]
    Signed-off-by: default avatarRoland Dreier <roland@purestorage.com>
    f21519b2
main.c 52.8 KB