Re: [PATCH] pci: add romsize property

[Paolo Bonzini <pbonzini@redhat.com>]

On 19/01/21 18:20, Laszlo Ersek wrote:
> I only have superficial comments:
> 
> - if we're talking uint32_t, I'd kind of prefer UINT32_MAX to (-1),
> style-wise -- but feel free to ignore
> 
> - we should print a uint32_t with ("%" PRIu32), not "%d" (again, only
> pedantry, but PRIu32 is widely used in qemu, AFAICS)

I would use %u, but yeah you're correct.

> - OK, so get_image_size() returns an int64_t, which pci_add_option_rom()
> assigns to an "int" without any range checking.

This is pre-existing, but it should be fixed indeed.

> Then we compare that int
> against the new uint32_t property... or else, on the other branch, we
> assign pow2ceil() -- a uint64_t -- to the new (uint32_t) property.
> 
> - In pci_assign_dev_load_option_rom(), "st.st_size" (which is an off_t)
> is assigned to the new property...
> 
> 
> I find it hard to reason about whether this is safe. I'd suggest first
> cleaning up "int size" in pci_add_option_rom() -- use an int64_t, and
> maybe check it explicitly against some 32-bit limit? --, then introduce
> the new property as uint64_t. (Print it with PRIu64 then, I guess.)

ROM BARs cannot be 64-bit in size.  There's just no room in 
configuration space for that.  Anyway yes pci_add_option_rom() is iffy 
and I'll send v2.

Paolo

> BTW there's another aspect of is_power_of_2(): it catches the zero
> value. If the power-of-two check is dropped, I wonder if a zero property
> value could cause a mess, so it might be prudent to catch that
> explicitly. (Precedent: see the (size == 0) check in pci_add_option_rom().)
> 
> Anyway, feel free to ignore all of my points; I just find it hard to
> reason about the "logic" when the code is not obviously overflow-free in
> the first place. (I'm not implying there are overflows; the code may be
> free of overflows -- it's just not*obviously*  so, to me anyway.)