[prev in list] [next in list] [prev in thread] [next in thread] 

List:       flashrom
Subject:    [flashrom] Unify flashrom range types
From:       Evan Benn via flashrom <flashrom () flashrom ! org>
Date:       2022-08-25 7:14:45
Message-ID: CAKz_xw2u0nE5=dPqPAAq=hk6W1XO_OiDvXcjnXDcYF3TQ2LwnQ () mail ! gmail ! com
[Download RAW message or body]

# Introduction

The goal is to unify the integer type used for ranges. libflashrom has
2 ranges in the API: write protect (wp) ranges and layout ranges. The
types for these are not consistent between wp and layout, between
getters and setters, and between the API and the implementation.
Additionally the types are different sized depending on the platform
(ie size_t on 32 vs 64 bit).

Demo: https://review.coreboot.org/c/flashrom/+/67004

# Background / Motivation

Assertion: Flashrom only cares about fairly normal platforms (32 and
64 bit pointers).

size_t: size_t is the C type returned by sizeof. It is an unsigned
integer. Throughout the C library it is used as the type for counting
the length of and indexing into arrays. It is at least 16 bits. It is
often the same size as uintptr_t and void* but this is not required.

uintptr_t: A C type large enough to hold a pointer.

unsigned int: At least 16 bits, probably 32 bits on all the platforms
we care about.

uint32_t: Exactly 32 bit type, not required to exist (ie, flashrom
won't compile on platforms where this doesn't exist).

usize: A rust type corresponding to uintptr_t.

uintptr: A go type corresponding to uintptr_t. Go also has C.size_t.

# Flashrom Today

getsize -> size_t
read/write/verify -> size_t
implementation: unsigned int
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/cc9524d9ebf5d2d3ceb1c62c94880f8e8da50b64:src/third_party/flashrom/include/flash.h;l=224


Based on the implementation using `unsigned int`, Flashrom probably
only works with up to 32 bit sized flashes.

layout_add_region -> size_t, (start, end)
layout_get_region -> unsigned int (start, len)
impl: uint32_t (start, end)
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/cc9524d9ebf5d2d3ceb1c62c94880f8e8da50b64:src/third_party/flashrom/include/layout.h;l=29


These types are probably fine on 32 bit platforms where they are all
the same. On 64 bit you could add a region that will be truncated /
wrapped without warning. The get and add using different ranges;
(start, end) and (start, len) is very confusing - I have already
gotten this wrong several times while refactoring.

wp_set_range -> size_t (start, len)
wp_get_range -> size_t (start, len)
impl: size_t (start, len)

All consistent here.

wp_ranges_count -> size_t
wp_ranges_get_range -> unsigned int
impl: size_t

Changing the get_range to take a size_t seems like a simple fix here.
This is unlikely to ever cause a bug but is a strange style.

# Rust

Bindgen can be told to treat size_t as usize. Without this flag a rust
type for size_t is chosen for the platform (ie u32 on 32 bit, u64 on
64 bit). Only usize can be used to index slices in rust.

# Proposal

## Goal

Unify all the length types (internal and external)
Unify all the range bounds ((start, len) vs (start, end))
Make bindings straightforward

## Non goal
16 bit platforms
> 32 bit sized flashes

Change the libflashrom API and the flashrom internals to use uint32_t
for all flash sizes and ranges. Keep size_t for host related sizes, eg
the buffer size in flashrom_layout_read_fmap_from_buffer or
flashrom_image_verify.

Add a layout_add (start, len) method and try to deprecate the (start,
end) method. This will make all of the methods use the same concept of
range. The wp implementation uses (start, len) while the layout
implementation uses (start, end). I do not propose to change this.

In the rust fat bindings, use usize to represent all ranges and
offsets. After libflashrom changes from the current implementation to
uint32_t, the rust bindings must convert between u32 and usize. We
will only support 32 and 64 bit usize, so this will be
straightforward. The rust bindings will reject attempts to provide
values > 32bit at runtime.

# Alternatives

Use uint64_t. This has the pro of supporting large SPI flash on 64 bit
platforms. However this wont make large flash supported on 32 bit
platforms, as single buffers are passed around for data access;
flashrom_image_write (struct flashrom_flashctx *flashctx, void
*buffer, size_t buffer_len, const void *refbuffer)

Use size_t everywhere. Size_t makes some sense to use, as the
implementation and API deals with full flash sized buffers, and they
can only be indexed by size_t sized ranges anyway. For example
flashrom would support 16 bit of flash on a 16 bit platform, and 64
bit of flash on a 64 bit platform. However, tying the usable flash
size to the platform size doesn't really make sense and is likely to
cause confusion, and we probably only care about 32 bit flash.

Use a typedef, eg chipsize_t. I think this is reasonable internally
but don't prefer it. It would theoretically allow a simple change to
swap from 32 to 64 bit flashes. However C won't warn about code using
the wrong type while they are 'compatible', see for example the
functions using unsigned int and uint32_t interchangeably in the
codebase already. And this won't really work for libflashrom, it's too
messy to make the API function differently depending on library
compile time choices, this really creates incompatible APIs with
identical names.

For supporting larger-than-size_t size data objects, see posix lseek64
and friends.

Use u32 for the rust API. This makes sense as it pushes out-of-range
errors into the user code. However it complicates a lot of usage as
u32 constantly need to be converted into usize to do array indexing.

https://review.coreboot.org/c/flashrom/+/67004
_______________________________________________
flashrom mailing list -- flashrom@flashrom.org
To unsubscribe send an email to flashrom-leave@flashrom.org


[prev in list] [next in list] [prev in thread] [next in thread]