On 6/7/22 16:35, Warner Losh wrote:
>
>
>> On Jun 7, 2022, at 3:23 PM, Richard Henderson <richard.henderson@linaro.org> wrote:
>>
>> On 6/7/22 14:51, Warner Losh wrote:
>>> void unlock_iovec(IOVecMap *map, bool copy_out)
>>> {
>>> for (int i = 0, count = map->count; i < count; ++i) {
>>> if (map->host[i].iov_base) {
>>> abi_ulong target_base = tswapal(map->target[i].iov_base);
>>> unlock_user(map->host[i].iov_base, target_base,
>>> copy_out ? map->host[i].iov_len : 0);
>>> }
>>> And wouldn't we want to filter out the iov_base that == 0 since
>>> we may terminate the loop before we get to the count. When the
>>> I/O is done, we'll call it not with the number we mapped, but with
>>> the original number... Or am I not understanding something here...
>>
>> I'm not following -- when and why are you adjusting count?
>
> When we hit a memory range we can’t map after the first one,
> we effectively stop mapping in (in the current linux code we
> do map after, but then destroy the length). So that means
> we’ll have entries in the iovec that are zero, and this code
> doesn’t account for that. We’re not changing the count, per
> se, but have a scenario where they might wind up NULL.
... and so skip them with the if.
I mean, I suppose you could set map->count on error, as you say, so that we don't iterate
so far, but... duh, error case. So long as you don't actively fail, there's no point in
optimizing for it.
Setting the count would be hard because we'd have to allocate and free
state that we're not currently doing. Better to just skip it with an if. We allocate
a vector that's used in a number of places, and we'd have to change that
code if we did things differently. While I'm open to suggestions here, I think
that just accounting for the possible error with an if is our best bet for now.
I have a lot of code to get in, and am hoping to not rewrite things unless there's
some clear benefit over the existing structure (like fixing bugs, matching linux-user,
or increasing performance).
Warner