[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH V3] hw/pci-host: Fix x86 Host Bridges 64bit PCI hole
Date: Thu, 9 Nov 2017 17:40:07 -0200
User-agent: Mutt/1.9.1 (2017-09-22)

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 <address@hidden>
> Thanks!
> Laszlo


reply via email to

[Prev in Thread] Current Thread [Next in Thread]