From a39474a245c545ac0fb858a6daa5773d1d13a186 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Tue, 9 Apr 2019 16:24:12 +0200 Subject: [PATCH] block-request stream: split Operation from Request This patch splits the 'Request' definition into smaller types that are suitable for the client-side API too. The new 'Operation' type comprises the block operation's type (opcode) and the operation's arguments (block number, block count). The former 'Request::operation_defined' is now 'Operation::valid'. The 'Request' aggregates an 'Operation', which changes its object layout. Note that this commit relaxes the bit-precise definition of 'Request' to facilitate the use of 'unsigned long' where appropriate, in particular for the request tag (which should correspond to an 'Id_space::Id'). The originally bit-precise definition was pursued to allow the sharing of the 'Request' type between SPARK and C++ code. However, it turns out that defining a native type in each language and a (set of) converting constructors is a more natural approach. Issue #3283 --- repos/os/include/block/request.h | 55 +++++++++++----- repos/os/include/block/request_stream.h | 65 ++++++++++--------- .../os/src/test/block_request_stream/main.cc | 4 +- 3 files changed, 75 insertions(+), 49 deletions(-) diff --git a/repos/os/include/block/request.h b/repos/os/include/block/request.h index 0b7f7e5ec0..167ab15dc9 100644 --- a/repos/os/include/block/request.h +++ b/repos/os/include/block/request.h @@ -17,28 +17,51 @@ /* Genode includes */ #include -namespace Block { struct Request; } +namespace Block { + + typedef Genode::uint64_t block_number_t; + typedef Genode::size_t block_count_t; + typedef Genode::off_t off_t; + + struct Operation; + struct Request; +} + + +struct Block::Operation +{ + enum class Type { INVALID = 0, READ = 1, WRITE = 2, SYNC = 3 }; + + Type type; + block_number_t block_number; + block_count_t count; + + bool valid() const + { + return type == Type::READ || type == Type::WRITE || type == Type::SYNC; + } +}; struct Block::Request { - enum class Operation : Genode::uint32_t { INVALID = 0, READ = 1, WRITE = 2, SYNC = 3 }; - enum class Success : Genode::uint32_t { FALSE = 0, TRUE = 1 }; + struct Tag { unsigned long value; }; - Operation operation; - Success success; - Genode::uint64_t block_number; - Genode::uint64_t offset; - Genode::uint32_t count; - Genode::uint32_t tag; + Operation operation; - bool operation_defined() const - { - return operation == Operation::READ - || operation == Operation::WRITE - || operation == Operation::SYNC; - } + bool success; -} __attribute__((packed)); + /** + * Location of payload within the packet stream + */ + off_t offset; + + /** + * Client-defined identifier to associate acknowledgements with requests + * + * The underlying type corresponds to 'Id_space::Id'. + */ + Tag tag; +}; #endif /* _INCLUDE__BLOCK__REQUEST_H_ */ diff --git a/repos/os/include/block/request_stream.h b/repos/os/include/block/request_stream.h index 22878d3040..6beb7011be 100644 --- a/repos/os/include/block/request_stream.h +++ b/repos/os/include/block/request_stream.h @@ -26,8 +26,8 @@ class Block::Request_stream : Genode::Noncopyable { public: - struct Block_size { Genode::uint32_t value; }; - struct Align_log2 { Genode::size_t value; }; + struct Block_size { Genode::size_t value; }; + struct Align_log2 { Genode::size_t value; }; /** * Interface for accessing the content of a 'Request' @@ -59,7 +59,7 @@ class Block::Request_stream : Genode::Noncopyable */ Genode::size_t _request_size(Block::Request const &request) const { - return request.count * _info.block_size; + return request.operation.count * _info.block_size; } bool _valid_range_and_alignment(Block::Request const &request) const @@ -190,26 +190,27 @@ class Block::Request_stream : Genode::Noncopyable Packet_descriptor const packet = tx_sink.peek_packet(); - auto operation = [] (Packet_descriptor::Opcode op) + auto type = [] (Packet_descriptor::Opcode op) { switch (op) { - case Packet_descriptor::READ: return Request::Operation::READ; - case Packet_descriptor::WRITE: return Request::Operation::WRITE; - case Packet_descriptor::END: return Request::Operation::INVALID; + case Packet_descriptor::READ: return Operation::Type::READ; + case Packet_descriptor::WRITE: return Operation::Type::WRITE; + case Packet_descriptor::END: return Operation::Type::INVALID; }; - return Request::Operation::INVALID; + return Operation::Type::INVALID; }; - bool const packet_valid = (tx_sink.packet_valid(packet) - && (packet.offset() >= 0) - && (packet.size() <= (size_t)((uint32_t)~0UL))); + bool const packet_valid = tx_sink.packet_valid(packet) + && (packet.offset() >= 0); - Request request { .operation = operation(packet.operation()), - .success = Request::Success::FALSE, - .block_number = (uint64_t)packet.block_number(), - .offset = (uint64_t)packet.offset(), - .count = (uint32_t)packet.block_count(), - .tag = 0}; + Operation operation { .type = type(packet.operation()), + .block_number = packet.block_number(), + .count = packet.block_count() }; + + Request request { .operation = operation, + .success = false, + .offset = packet.offset(), + .tag = { 0 } }; Response const response = packet_valid ? fn(request) @@ -260,9 +261,9 @@ class Block::Request_stream : Genode::Noncopyable bool _submitted = false; - Genode::uint32_t const _block_size; + Genode::size_t const _block_size; - Ack(Tx_sink &tx_sink, Genode::uint32_t block_size) + Ack(Tx_sink &tx_sink, Genode::size_t block_size) : _tx_sink(tx_sink), _block_size(block_size) { } public: @@ -275,24 +276,27 @@ class Block::Request_stream : Genode::Noncopyable } typedef Block::Packet_descriptor Packet_descriptor; - Packet_descriptor packet { (Genode::off_t)request.offset, - request.count * _block_size }; + Packet_descriptor + packet { (Genode::off_t)request.offset, + request.operation.count * _block_size }; - auto opcode = [] (Request::Operation operation) + auto opcode = [] (Operation::Type type) { - switch (operation) { - case Request::Operation::READ: return Packet_descriptor::READ; - case Request::Operation::WRITE: return Packet_descriptor::WRITE; - case Request::Operation::SYNC: return Packet_descriptor::END; - case Request::Operation::INVALID: return Packet_descriptor::END; + switch (type) { + case Operation::Type::READ: return Packet_descriptor::READ; + case Operation::Type::WRITE: return Packet_descriptor::WRITE; + case Operation::Type::SYNC: return Packet_descriptor::END; + case Operation::Type::INVALID: return Packet_descriptor::END; }; return Packet_descriptor::END; }; - packet = Packet_descriptor(packet, opcode(request.operation), - request.block_number, request.count); + packet = Packet_descriptor(packet, + opcode(request.operation.type), + request.operation.block_number, + request.operation.count); - packet.succeeded(request.success == Request::Success::TRUE); + packet.succeeded(request.success); _tx_sink.try_ack_packet(packet); _submitted = true; @@ -326,5 +330,4 @@ class Block::Request_stream : Genode::Noncopyable void wakeup_client_if_needed() { _tx.sink()->wakeup(); } }; - #endif /* _INCLUDE__BLOCK__REQUEST_STREAM_H_ */ diff --git a/repos/os/src/test/block_request_stream/main.cc b/repos/os/src/test/block_request_stream/main.cc index 502bd35b34..d60aca07ce 100644 --- a/repos/os/src/test/block_request_stream/main.cc +++ b/repos/os/src/test/block_request_stream/main.cc @@ -106,7 +106,7 @@ struct Test::Jobs : Noncopyable for (unsigned i = 0; i < N; i++) { if (_entries[i].state == Entry::IN_PROGRESS) { _entries[i].state = Entry::COMPLETE; - _entries[i].request.success = Block::Request::Success::TRUE; + _entries[i].request.success = true; progress = true; } } @@ -137,7 +137,7 @@ struct Test::Jobs : Noncopyable completed_job(request); - if (request.operation_defined()) + if (request.operation.valid()) fn(request); } };