In a previous project, I have demonstrated how to develop and use a DMA IP core for the Zynq-7000 SoC in Genode. Given that the Genode architecture aims for a strict sandboxing of software components such as device drivers, the use of DMA must be carefully debated as it is able to break the isolation assumptions.
In order to explain this, let's first have a look at how hardware access is mediated by the Genode OS Framework. As illustrated in the below figure, the Core / Init component possesses ultimate authority over the hardware and provides unmediated access to devices via the IRQ and MMIO services. The Platform Driver component thereby gains authority over all device resources. It further provides the Platform service via which particular components are able to gain authority over individual devices. More precisely, the platform driver hands out the memory mapped I/O dataspaces and IRQ sessions that correspond to the requested (and granted) device. For DMA-capable devices, a client must further possess memory for which it knows the physical address. Yet, since components unlike the platform driver have no means to get the physical address of a chunk of memory, the Platform service also provides an interface to allocate DMA buffers with a known physical address on the client's behalf.
Without any protection mechanism such as an IOMMU in place, only trustworthy software components should be provided authority over DMA-capable devices. The reason for this is the fact that the DMA-capable device has unrestricted access to the entire memory. Being able to program the device thereby provides the software component ultimate access to the main memory, which will become a security issue if the component cannot be trusted.
In addition to the security aspect, DMA can become an issue for the stability and robustness of the system. Typically, a device driver in Genode can be started/killed at any point in time. This is motivated by the complexity of device drivers, which occasionally implies instability, and the ability to sculpt the Genode system at runtime. Particularly when taking (re)programmable logic into account, the latter becomes essential. The platform driver will know about the disappearance of the device driver and free all resources (including DMA buffers) that were allocated on the device driver's behalf. However, by design, it does not know about the programming of the driven device and thus cannot tell whether there are any pending DMA transfers. It may therefore occur that the memory that was previously used as a DMA buffer gets re-allocated while there are still pending DMA transfers operating on these addresses. This will ultimately lead to memory corruption.
The issue of unrestricted DMA is not Genode-specific, though. Most modern general-purpose computing platforms therefore comprise an IOMMU that restricts memory accesses from peripherals just as the MMU restricts memory accesses from the CPU. By configuring the IOMMU, the platform driver is able to assert to which DMA buffer a client and a peripheral device gain access and revoke it when the buffer is freed.
Unfortunately, the Zynq-7000 SoC does not possess an IOMMU. For peripherals implemented in the FPGA, however, one can implement a similar functionality. In this project, I am going to tell the story of implementing a new IP core for this purpose and how it is used in Genode.
GoalThe goal of this project was to implement an IP core, called DMA Guard, that intercepts AXI signals to the memory. The below screenshot shows the relevant section of the resulting block design for a loopback AXI DMA core. The DMA Guard IP core is configured via MMIO registers for which it implements an AXI Lite slave interface (s_axi_control
). Moreover, it implements a single AXI slave interface and a single AXI master interface. The IP core resides in between the AXI DMA and the AXI Memory Interconnect. In order to connect both AXI master interfaces from the AXI DMA core to the single slave interface of the DMA Guard, I added an AXI SmartConnect core.
The DMA Guard only forwards a read/write transfer issued by the connected AXI master to the connected AXI slave if the memory address has been explicitly granted. In Genode, this is configured by the platform driver which uses the IP core's register interface to grant the address ranges of the allocated DMA buffers.
The register interface that I came up with comprises one control register and ten segment registers. The control register has a global read-deny bit (bit 0) and a global write-deny bit (bit 1). Using these bits, one can deny all read/write accesses irrespective of how the segment registers are configured. The segment registers have a valid bit (bit 0), a writeable bit (bit 1) as well as a size field (bits 4-11) and an address field (bits 12-31). The valid bit must be asserted in order to let the IP core evaluate the register. If the writeable bit is deasserted, only read accesses for the specified address range will be granted. The address field specifies the 20 most significant bits of the start address, which may be subject to alignment depending on the specified size. The smallest alignment is 4KB. The size field takes values from 0 to 31, which specify the size of the address range as binary logarithm minus 2. Given x is the value of the size field, the address range's size is thus calculated by 2^(x+2).
Control register (0x00):
+--------------------------------------+
| | write-deny | read-deny |
+--------------------------------------+
|31 2| 1 | 0 |
Segment register 1 (0x04):
+--------------------------------------+
| address | size | | W | V |
+--------------------------------------+
31 12|11 4|3 2| 1 | 0 |
Segment register 2 (0x08):
...
Implementing the IP coreFor implementing the DMA Guard IP core, I started off with the "Create and Package New IP" wizard by selecting Tools → Create and Package New IP
from my DMA loopback design and choosing to create a new AXI4 peripheral in the dialogue as shown below.
In the wizard, I renamed the default AXI lite interface to s_axi_control
and changed the number of registers to 11. Moreover, I added one full AXI master interface (M00_AXI
) and one full AXI slave interface (S00_AXI
). The result is depicted below. In the next dialogue, I made sure to select "Edit IP" as a next step.
Since I want to pass through most of the AXI signals, I removed the dma_guard_v1_0_S00_AXI.v and the dma_guard_v1_0_M00_AXI.v files from the design. I also removed the corresponding instantiations from the top level file dma_guard_v1_0.v and removed the m00_axi_error
, m00_axi_txn_done
and m00_axi_init_axi_txn
signals. Moreover, I replaced the s00_
/m00_axi_aclk
and s00_
/m00_axi_aresetn
signals by a single pair and removed the C_M00_AXI_BURST_LEN
and C_M00_AXI_TARGET_SLAVE_BASE_ADDR
parameters.
The only remaining instantiation is the s_axi_control
module that implements the register interface. I extended this module by four input signals as well as two output signals: the read address, the read length, the write address and the write length as well as a read-allowed and write-allowed signal.
// signals passed to s_axi_control module
wire ar_allowed;
wire aw_allowed;
wire [14 : 0] rd_len;
wire [14 : 0] wr_len;
// calculate length in bytes
assign rd_len = s00_axi_arlen << s00_axi_arsize;
assign wr_len = s00_axi_awlen << s00_axi_awsize;
// Instantiation of Axi Bus Interface s_axi_control
dma_guard_v1_0_s_axi_control # (
.C_S_AXI_DATA_WIDTH(C_s_axi_control_DATA_WIDTH),
.C_S_AXI_ADDR_WIDTH(C_s_axi_control_ADDR_WIDTH)
) dma_guard_v1_0_s_axi_control_inst (
.rd_allowed(ar_allowed),
.rd_addr(s00_axi_araddr),
.rd_len(rd_len),
.wr_allowed(aw_allowed),
.wr_addr(s00_axi_awaddr),
.wr_len(wr_len),
.S_AXI_ACLK(s_axi_control_aclk),
// ...
);
Next, I started adding user logic for the emulation of AXI transfers. For the read channel, the DMA Guard must only forward the address if the read transfer is granted. It therefore drives the arvalid
and arready
signals. Moreover, it generates a read response to let the master know about the read error in case the transfer was denied. Since the read response is sent with the last beat, the DMA Guard needs to drive the rvalid
, rresp
and rlast
signals. All the remaining signals of the read channels are passed through.
// Add user logic here
// signals for read-transfer emulation/interception
reg axi_arvalid;
reg axi_arready;
reg axi_rvalid;
reg axi_rresp;
reg axi_rlast;
For the write channel, the DMA Guard also drives the awvalid
and awready
signals in order to selectively forward the write address. Moreover, it may inject its own write response, for which it drives the bresp
and bvalid
signal. In contrast to the read channel, however, the IP core is going to accept (and discard) the transferred payload in case the write access was denied. It therefore needs to drive the wvalid
and wready
signal.
// signals for write-transfer emulation/interception
reg axi_awvalid;
reg axi_awready;
reg axi_wvalid;
reg axi_wready;
reg axi_bresp;
reg axi_bvalid;
These signals are driven by a synchronous state machine (one for each channel) for which I added the following state signals:
// read/write state signals
reg [1:0] read_state;
reg [1:0] write_state;
// read/write state values
localparam [1:0]
FORWARD = 2'b00,
READY = 2'b01,
WAIT = 2'b10,
RESP = 2'b11;
For better readability, I also defined a local parameter for the response values:
// read/write response values
localparam [1:0]
OKAY = 2'b00,
EXOKAY = 2'b01,
SLVERR = 2'b10,
DECERR = 2'b11;
I assigned the read-channel signals as follows. Note that the arvalid
and arready
signals are only passed through in FORWARD state and if ar_allowed
is asserted. Otherwise, the DMA Guard drives these signals. Similarly, the rresp
, rlast
and rvalid
signals are passed through in all states but the RESP state.
assign m00_axi_arid = s00_axi_arid;
assign m00_axi_araddr = s00_axi_araddr;
assign m00_axi_arlen = s00_axi_arlen;
assign m00_axi_arsize = s00_axi_arsize;
assign m00_axi_arburst = s00_axi_arburst;
assign m00_axi_arlock = s00_axi_arlock;
assign m00_axi_arcache = s00_axi_arcache;
assign m00_axi_arprot = s00_axi_arprot;
assign m00_axi_arqos = s00_axi_arqos;
assign m00_axi_aruser = s00_axi_aruser;
assign m00_axi_arvalid = read_state == FORWARD && ar_allowed == 1
? s00_axi_arvalid : axi_arvalid;
assign s00_axi_arready = read_state == FORWARD && ar_allowed == 1
? m00_axi_arready : axi_arready;
assign s00_axi_rid = m00_axi_rid;
assign s00_axi_rdata = m00_axi_rdata;
assign s00_axi_rresp = read_state == RESP ? axi_rresp : m00_axi_rresp;
assign s00_axi_rlast = read_state == RESP ? axi_rlast : m00_axi_rlast;
assign s00_axi_ruser = m00_axi_ruser;
assign s00_axi_rvalid = read_state == RESP ? axi_rvalid : m00_axi_rvalid;
assign m00_axi_rready = s00_axi_rready;
I then implemented the synchronous state machine for the read channel with the following always
block. In the FORWARD state, I check for an invalid transfer and set the arvalid
and arready
signals. In the next clock cycle, the state machine continues with the READY state where it deasserts the arready
signal and sets read state to WAIT. In the WAIT state, we wait for any activity of the slave on the read channel to finish (e.g. the previous transfer) before continuing with the RESP state. In this state, I inject a beat with rlast
asserted and rresp
set to DECERR before resetting the state to FORWARD.
// implement clock-synchronous read signal updates
always @(posedge axi_aclk)
begin
if (axi_aresetn == 1'b0)
begin
read_state <= FORWARD;
axi_arvalid <= 1'b0;
axi_arready <= 1'b0;
axi_rvalid <= 1'b0;
axi_rresp <= 1'b0;
axi_rlast <= 1'b0;
end
else
begin
case (read_state)
FORWARD: begin
// master tries issueing an invalid transfer
if (~ar_allowed && s00_axi_arvalid)
begin
// don't forward valid signal because we are
// blocking the transfer
axi_arvalid <= 1'b0;
// yet, accept address
axi_arready <= 1'b1;
read_state <= READY;
end
else
begin
axi_arvalid <= 1'b0;
axi_arready <= 1'b0;
axi_rvalid <= 1'b0;
axi_rresp <= 1'b0;
axi_rlast <= 1'b0;
end
end
READY: begin
if (axi_arready && s00_axi_arvalid)
begin
// deassert ready signal to finish read request
axi_arready <= 1'b0;
read_state <= WAIT;
end
end
WAIT: begin
// wait for m00 read channel to become silent
if (~m00_axi_rvalid)
begin
read_state <= RESP;
axi_rvalid <= 1'b0;
end
end
RESP: begin
// insert our own response
if (~axi_rvalid)
begin
axi_rresp <= DECERR;
axi_rvalid <= 1'b1;
axi_rlast <= 1'b1;
end
else if (s00_axi_rready)
begin
axi_rvalid <= 1'b0;
axi_rlast <= 1'b0;
read_state <= FORWARD;
end
end
default: begin
read_state <= FORWARD;
end
endcase
end
end
For the write channel, the assign statements and the always block look pretty similar. The latter differs in the WAIT state where it waits for the last beat of the master.
assign m00_axi_awid = s00_axi_awid;
assign m00_axi_awaddr = s00_axi_awaddr;
assign m00_axi_awlen = s00_axi_awlen;
assign m00_axi_awsize = s00_axi_awsize;
assign m00_axi_awburst = s00_axi_awburst;
assign m00_axi_awlock = s00_axi_awlock;
assign m00_axi_awcache = s00_axi_awcache;
assign m00_axi_awprot = s00_axi_awprot;
assign m00_axi_awqos = s00_axi_awqos;
assign m00_axi_awuser = s00_axi_awuser;
assign m00_axi_awvalid = write_state == FORWARD && aw_allowed
? s00_axi_awvalid : axi_awvalid;
assign s00_axi_awready = write_state == FORWARD && aw_allowed
? m00_axi_awready : axi_awready;
assign m00_axi_wdata = s00_axi_wdata;
assign m00_axi_wstrb = s00_axi_wstrb;
assign m00_axi_wlast = s00_axi_wlast;
assign m00_axi_wuser = s00_axi_wuser;
assign m00_axi_wvalid = write_state == FORWARD && aw_allowed
? s00_axi_wvalid : axi_wvalid;
assign s00_axi_wready = write_state == FORWARD && aw_allowed
? m00_axi_wready : axi_wready;
assign s00_axi_bid = m00_axi_bid;
assign s00_axi_bresp = write_state == RESP ? axi_bresp : m00_axi_bresp;
assign s00_axi_buser = m00_axi_buser;
assign s00_axi_bvalid = write_state == RESP ? axi_bvalid : m00_axi_bvalid;
assign m00_axi_bready = s00_axi_bready;
// implement clock-synchronous write signal updates
always @(posedge axi_aclk)
begin
if (axi_aresetn == 1'b0)
begin
write_state <= FORWARD;
axi_awvalid <= 1'b0;
axi_awready <= 1'b0;
axi_wvalid <= 1'b0;
axi_wready <= 1'b0;
axi_bresp <= 1'b0;
axi_bvalid <= 1'b0;
end
else
begin
case (write_state)
FORWARD: begin
// master tries issuing an invalid transfer
if (~aw_allowed && s00_axi_awvalid)
begin
// don't forward valid signal because we are
// blocking the transfer
axi_awvalid <= 1'b0;
// yet, accept address
axi_awready <= 1'b1;
write_state <= READY;
end
else
begin
axi_awvalid <= 1'b0;
axi_awready <= 1'b0;
axi_wvalid <= 1'b0;
axi_wready <= 1'b0;
axi_bresp <= 1'b0;
axi_bvalid <= 1'b0;
end
end
READY: begin
if (axi_awready && s00_axi_awvalid)
begin
// deassert ready signal to stop accepting write requests
axi_awready <= 1'b0;
// assert the wready signal to accept data
axi_wready <= 1'b1;
write_state <= WAIT;
end
end
WAIT: begin
// wait for last wdata
if (s00_axi_wvalid && s00_axi_wlast)
begin
write_state <= RESP;
axi_wready <= 1'b0;
axi_bvalid <= 1'b0;
end
end
RESP: begin
// insert our own response
if (~axi_bvalid)
begin
axi_bresp <= DECERR;
axi_bvalid <= 1'b1;
end
else if (s00_axi_bready)
begin
axi_bvalid <= 1'b0;
axi_bresp <= 1'b0;
write_state <= FORWARD;
end
end
default: begin
write_state <= FORWARD;
end
endcase
end
end
Next, I implemented the address-checking logic in the s_axi_control
module. In dma_guard_v1_0_s_axi_control.v, I started by adding two parameters for the width of the read/write address.
// Users to add parameters here
parameter integer RD_ADDR_WIDTH = 32,
parameter integer WR_ADDR_WIDTH = 32,
// User parameters ends
Then, I added the additional ports that I've already used in the instantiation.
// Users to add ports here
output wire rd_allowed,
input wire [RD_ADDR_WIDTH-1:0] rd_addr,
input wire [7:0] rd_len,
output wire wr_allowed,
input wire [WR_ADDR_WIDTH-1:0] wr_addr,
input wire [7:0] wr_len,
// User ports ends
For the user logic, I implemented a couple of helper functions. The first five functions perform simple calculations on a single segment register. The last function implements a range check that returns whether a given transfer is within a certain start address and end address.
// Add user logic here
function readable (input [31 : 0] cfg);
readable = cfg[0];
endfunction
function writeable (input [31 : 0] cfg);
writeable = cfg[0] & cfg[1];
endfunction
function [31 : 0] size (input [31 : 0] cfg);
/* 2^(size+2) - 1 */
size = cfg[8:4] >= 30 ? 32'hffffffff : (1 << cfg[8:4]+2)-1;
endfunction
function [31 : 0] start_addr (input [31 : 0] cfg);
start_addr = (cfg[31:12] << 12) & ~size(cfg);
endfunction
function [31 : 0] end_addr (input [31 : 0] cfg);
end_addr = start_addr(cfg) | size(cfg);
endfunction
function within_range;
input [31 : 0] addr;
input integer len;
input [31 : 0] start_addr;
input [31 : 0] end_addr;
begin
within_range = addr >= start_addr && addr+len-1 <= end_addr;
end
endfunction
In order to use a generate
statement to generate logic for all ten segment registers, I defined the following signal vectors/arrays.
wire [9:0] rd_valid;
wire [9:0] wr_valid;
wire [31 : 0] addr_regs [9:0];
assign addr_regs[0] = slv_reg1;
assign addr_regs[1] = slv_reg2;
assign addr_regs[2] = slv_reg3;
assign addr_regs[3] = slv_reg4;
assign addr_regs[4] = slv_reg5;
assign addr_regs[5] = slv_reg6;
assign addr_regs[6] = slv_reg7;
assign addr_regs[7] = slv_reg8;
assign addr_regs[8] = slv_reg9;
assign addr_regs[9] = slv_reg10;
Note that the entries of addr_regs
are simply assigned to the wizard-generated registers. I assigned the rd_valid
and wr_valid
vectors using a generate statement so that i-th bit in these vectors indicates whether the i-th segment register would grant the current read/write access.
genvar i;
generate
for (i=0; i < 10; i=i+1) begin
assign rd_valid[i] = readable(addr_regs[i])
&& within_range(rd_addr,
rd_len,
start_addr(addr_regs[i]),
end_addr(addr_regs[i]));
assign wr_valid[i] = writeable(addr_regs[i])
&& within_range(wr_addr,
wr_len,
start_addr(addr_regs[i]),
end_addr(addr_regs[i]));
end
endgenerate
With these prerequisites, I was able to assign the output registers rd_allowed
and wr_allowed
so that rd_allowed
(wr_allowed
) is asserted if read (write) accesses are not globally disabled and at least one bit in rd_valid
(wr_valid
) is asserted.
assign rd_allowed = ~slv_reg0[0] && |rd_valid;
assign wr_allowed = ~slv_reg0[1] && |wr_valid;
// User logic ends
After completing the implementation, I conducted the following customizations in Package IP view (click on component.xml in Design Sources to open the view).
On the Customization Parameters page, I first ran the merge wizard and, second, removed unneeded parameters (C_S00_AXI_HIGHADDR
, C_S00_AXI_BASEADDR
, C_M00_AXI_BURST_LEN
, C_M00_AXI_TARGET_SLAVE_BASE_ADDR
). Furthermore, I modified the C_M00_AXI_DATA_WIDTH
and C_S00_AXI_DATA_WIDTH
parameters: in the Edit IP Parameter dialogue, I set Editable to Yes and added a second option to the list of values so that one can choose between 32 and 64 (see screenshot below). Note that there should be a way to add a constraint that both _DATA_WIDTH parameters must have the same value, yet I was not able to find the corresponding documentation. If you have any knowledge about how to achieve this, please let me know.
Next, on the Ports and Interfaces page, I removed the m00_axi_error
, m00_axi_txn_done
and m00_axi_init_axi_txn
ports since I have also removed the corresponding signals from the source code.
Last, on the Addressing and Memory page, I noticed that the slave interface only provided an address range of 4KB. Ideally, I would have liked to pass the address ranges from the master interface (which will be connected to the AXI Memory Interconnect) to the slave interface, yet I have not found any documentation about how to achieve this. I therefore opted for having a fully configurable address range for the slave interface, which I accomplished by copying the values of Range, Range dependency and Width dependency from the master interface (see screenshot above) to the slave interface (see screenshot below). Note, that I replaced the M00
by S00
in the dependency strings after copying. I also set the Range resolve type and Width resolve type to "dependent" and double-checked that Usage was set to "memory".
After these modifications, I packaged the IP, closed the project window and got back to my DMA loopback design. Here, I added a DMA Guard IP core for each axi_dma block since they are connected to different memory interfaces at the Zynq processing system. After adding the IP core to the block design, I removed the existing connections from axi_dma to axi_mem_intercon that I wanted to replace. As replacement, I connected the AXI master interface of the DMA Guard block to the axi_mem_intercon and the AXI slave interface to a new AXI SmartConnect block. Next, I connected the AXI master interfaces of the axi_dma block to the SmartConnect block. The resulting block design is shown in the screenshot below. Before validating the design, I customized the DMA Guard blocks to set the _DATA_WIDTH
parameters to 64 and double-checked that the same value is set for the axi_mem_intercon blocks.
By hitting F6, I triggered the design-validation wizard, which asked whether to auto-assign the unassigned address segments. Shortly after answering with Yes, I was presented with a bunch of errors for incomplete addressing paths. Looking at the address editor, I noticed a couple of unresolved paths (see screenshot below). I believe the reason for the auto-assignment to fail is that I was not able to let Vivado know that I implemented a pass-through AXI core. In consequence, Vivado assigns the DMA Guard's slave and master interface to different networks.
I managed to solve the unresolved-paths issue by unassigning each path via its context menu and by changing the Master base address and Range of the S00_AXI_mem segments to 0x0 resp. 1G (see screenshot below). I also needed to exclude all segments but the ACP_DDR_LOWOCM segment from Network 2.
With these changes I was able to validate the design and generate the bitstream. Despite Vivado generated the bitstream successfully, I later recognised that the design did not work properly. After looking at other causes for a while, I noticed that the clock rate was too high. In the end, I had to reduce FCLK_CLK0
from the 250MHz I started with to 167MHz.
Finally, as described in my previous project, I packaged everything as a Goa project in order to publish a ready-to-use depot archive for Genode. Please find the steps on how to download this archive and how to run the resulting scenario in the last section.
Testing the IP in simulationBefore I actually tested the bitstream on hardware, I verfied my implementation in simulation. Fortunately, there exists a special IP core for this purpose that I learned from this guide.
Following this guide, I added an AXI Verification IP to my project, right-clicked on the newly added block and chose Open IP Example Design from its context menu. This generated and opened a simulation project in a new Vivado window with this block design:
It uses the AXI Verification IP in three different configurations: master, passthrough and slave. I removed the passthrough instance in the middle and replaced it with a DMA Guard block. Note that you may have to add the IP repo to the project settings first (via Settings → IP → Repositories
) if you haven't opened the example project from a project that already made use of this repo. I connected the signals as shown in the screenshot below and triggered the validation wizard (F6). Again, I was asked whether to auto-assign the unassigned address segments, chose Yes and was, once again, presented with an error. I thus followed my approach of unassigning the problematic address segments using the Address Editor and setting base address and range of the S00_AXI_mem appropriately.
After fixing the address segments, I successfully validated the design and ran the simulation as described in the aforementioned guide. In short, I chose a simulation scenario from the Sources view, and used its context menu to make it active and run a simulation. Note that I have not connected the s_axi_control
signal so that I needed to set the configuration manually. I did this by looking up the slv_regX
signals via the Scope and Objects view and forcing them to particular values. Moreover, I also had to force the global reset signal as it was not driven by the simulation. Before starting the simulation, I made sure to add the signals I was interested in by dragging them from the Objects view to the waveform. With this approach, I was able to develop the DMA Guard's implementation as presented in the previous section.
In the genode-zynq repository, you find a test scenario based on the DMA loopback test from my previous project. With the following steps, you are able to reproduce the scenario and execute it on a Zybo Z7-20 board.
Note I'm assuming you have already cloned the main genode repository and the genode-zynq repository (see Genode 101: Getting started with the Zybo Z7). This also implies that you have created a build directory for arm_v7a.
First, make sure that you have checked out current master of both, the genode and the genode-zynq repository:
genode #> git checkout master
genode #> git pull origin master
genode #> cd repos/zynq
genode/repos/zynq #> git checkout master
genode/repos/zynq #> git pull origin master
Next, download the depot archive that comprises the bitstream:
genode #> ./tool/depot/download jschlatow/bin/arm_v7a/zybo_z720_dma_guard_demo-bitstream/2023-05-24
Now, you are able to run the test scenario with this command:
genode #> make -C build/arm_v7a run/dma_guard_test BOARD=zynq_zybo_z7
Comments