On Thu, Jun 01, 2023 at 09:25:39PM +0300, Kirill A. Shutemov wrote:
> +void accept_memory(phys_addr_t start, phys_addr_t end)
> +{
> + struct efi_unaccepted_memory *unaccepted;
> + unsigned long range_start, range_end;
> + unsigned long flags;
> + u64 unit_size;
> +
> + if (efi.unaccepted == EFI_INVALID_TABLE_ADDR)
> + return;
efi_get_unaccepted_table() already does this test.
> + unaccepted = efi_get_unaccepted_table();
> + if (!unaccepted)
> + return;
So this looks weird: callers can call accept_memory() and that function
can fail. But they can't know whether it failed or not because it
returns void.
> + unit_size = unaccepted->unit_size;
> +
> + /*
> + * Only care for the part of the range that is represented
> + * in the bitmap.
> + */
> + if (start < unaccepted->phys_base)
> + start = unaccepted->phys_base;
So this silently trims start...
> + if (end < unaccepted->phys_base)
> + return;
But fails only when end is outside of range.
I'd warn here at least. And return an error so that the callers know.
> + /* Translate to offsets from the beginning of the bitmap */
> + start -= unaccepted->phys_base;
> + end -= unaccepted->phys_base;
> +
> + /* Make sure not to overrun the bitmap */
> + if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> + end = unaccepted->size * unit_size * BITS_PER_BYTE;
How is all that trimming not important to the caller?
It would assume that its memory got accepted but not really.
> + range_start = start / unit_size;
> +
> + spin_lock_irqsave(&unaccepted_memory_lock, flags);
> + for_each_set_bitrange_from(range_start, range_end, unaccepted->bitmap,
> + DIV_ROUND_UP(end, unit_size)) {
> + unsigned long phys_start, phys_end;
> + unsigned long len = range_end - range_start;
> +
> + phys_start = range_start * unit_size + unaccepted->phys_base;
> + phys_end = range_end * unit_size + unaccepted->phys_base;
> +
> + arch_accept_memory(phys_start, phys_end);
> + bitmap_clear(unaccepted->bitmap, range_start, len);
> + }
> + spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
> +}
> +
> +bool range_contains_unaccepted_memory(phys_addr_t start, phys_addr_t end)
> +{
> + struct efi_unaccepted_memory *unaccepted;
> + unsigned long flags;
> + bool ret = false;
> + u64 unit_size;
> +
> + unaccepted = efi_get_unaccepted_table();
> + if (!unaccepted)
> + return false;
> +
> + unit_size = unaccepted->unit_size;
> +
> + /*
> + * Only care for the part of the range that is represented
> + * in the bitmap.
> + */
> + if (start < unaccepted->phys_base)
> + start = unaccepted->phys_base;
Same comment as above. Trimming start is fine?
> + if (end < unaccepted->phys_base)
> + return false;
> +
> + /* Translate to offsets from the beginning of the bitmap */
> + start -= unaccepted->phys_base;
> + end -= unaccepted->phys_base;
Ditto as above.
> +
> + /* Make sure not to overrun the bitmap */
> + if (end > unaccepted->size * unit_size * BITS_PER_BYTE)
> + end = unaccepted->size * unit_size * BITS_PER_BYTE;
Ditto.
> + spin_lock_irqsave(&unaccepted_memory_lock, flags);
> + while (start < end) {
> + if (test_bit(start / unit_size, unaccepted->bitmap)) {
> + ret = true;
> + break;
I have a faint memory we've had this before but you need to check
*every* bit in the unaccepted bitmap before returning true. Doh.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
