Re: [PATCH] pci: add romsize property

[Laszlo Ersek]

On 01/19/21 17:51, Philippe Mathieu-Daudé wrote:
> +pflash
> 
> On 12/18/20 7:27 PM, Paolo Bonzini wrote:
>> This property can be useful for distros to set up known-good ROM sizes for
>> migration purposes.  The VM will fail to start if the ROM is too large,
>> and migration compatibility will not be broken if the ROM is too small.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/pci/pci.c             | 19 +++++++++++++++++--
>>  hw/xen/xen_pt_load_rom.c | 14 ++++++++++++--
>>  include/hw/pci/pci.h     |  1 +
>>  3 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index d4349ea577..fd25253c2a 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -67,6 +67,7 @@ static void pcibus_reset(BusState *qbus);
>>  static Property pci_props[] = {
>>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>> +    DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, -1),
>>      DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>> @@ -2106,6 +2107,11 @@ static void pci_qdev_realize(DeviceState *qdev, Error 
>> **errp)
>>      bool is_default_rom;
>>      uint16_t class_id;
>>  
>> +    if (pci_dev->romsize != -1 && !is_power_of_2(pci_dev->romsize)) {
>> +        error_setg(errp, "ROM size %d is not a power of two", 
>> pci_dev->romsize);
>> +        return;
>> +    }
> 
> Some cloud providers already complained the pow2 check in the pflash
> device (wasting host storage). Personally I find using pow2 easier
> and safer.
> 
> The pow2 check looks like a separate change however, maybe add in a
> separate patch? Or maybe not :)
> 
> Regards,
> 
> Phil.
> 

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)

- Phil: you CC'd me on a context from which the larger part of the patch
had been removed. That's not really useful (I have no idea what the new
property actually does.) Hmmm let me see the patch in qemu-devel...

- OK, so get_image_size() returns an int64_t, which pci_add_option_rom()
assigns to an "int" without any range checking. 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.) NB
"size" is also passed to pci_patch_ids(), which takes it as an "int" too
(shudder).

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.)

Thanks
Laszlo