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 <ler...@redhat.com>
>
> Thanks!
> Laszlo
--
Eduardo
