Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Nov 09, 2017 at 12:02:10PM +0100, Laszlo Ersek wrote:
[...]
> (3) An idea for the property defaults: you remove
> DEFAULT_PCI_HOLE64_SIZE, which is cool. How about introducing (in the
> proper header files)
> 
> #define DEFAULT_I440FX_PCI_HOLE64_SIZE (1ULL << 31)
> #define DEFAULT_Q35_PCI_HOLE64_SIZE    (1ULL << 35)
> 
> The main reason for my suggestion is that (1ULL << 35) is used twice in
> the code, for an obscure qdev/qom reason. The comments definitely help,
> so keep them, but a greppable macro would help even more, IMO.
> 
> And, once we add DEFAULT_Q35_PCI_HOLE64_SIZE, we should add
> DEFAULT_I440FX_PCI_HOLE64_SIZE too, for consistency.

Agreed, especially considering how the code that initializes the
defaults is non-obvious.

> 
> Anyway, I'll leave this up to you as well.

If we do that, I recommend adding a bigger warning to
q35_host_props.  The one in this patch is very easy to miss if
people try to touch the defaults for other mch.* properties in
the future.  I suggest something like:

    /*
     * NOTE: setting defaults for the mch.* fields in this table
     * don't work, because mch is a separate QOM object that is
     * zeroed by the object_initialize(&s->mch, ...) call inside
     * q35_host_initfn().  The default values for those
     * properties need to be initialized manually by
     * q35_host_initfn() after the object_initialize() call.
     */


> 
> 
> (4) I also have a suggestion for the commit message: please move the
> paragraph that starts with
> 
>   "Even if the new defaults..."
> 
> from the v2->v3 changelog to the commit message proper. IMO it is
> important information.
> 
> 
> With (4) addressed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo

-- 
Eduardo