Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Markus Armbruster <armbru@redhat.com>, Andrew Jeffery <andrew@aj.id.au>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	berrange@redhat.com, "Eduardo Habkost" <ehabkost@redhat.com>,
	"Cameron Esfahani via" <qemu-devel@nongnu.org>,
	qemu-arm@nongnu.org, "Cédric Le Goater" <clg@kaod.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Joel Stanley" <joel@jms.id.au>
Subject: Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices
Date: Tue, 19 May 2020 13:42:07 +0200	[thread overview]
Message-ID: <9cfcba4e-1237-8eb9-7c83-490729f082fe@redhat.com> (raw)
In-Reply-To: <87v9ks5vg7.fsf@dusky.pond.sub.org>

On 5/19/20 7:45 AM, Markus Armbruster wrote:
> "Andrew Jeffery" <andrew@aj.id.au> writes:
> 
>> On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote:
>>> On 5/18/20 7:03 AM, Markus Armbruster wrote:
>>>> These devices are optional, and controlled by @nb_nics.
>>>> aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum
>>>> supported number.  aspeed_soc_ast2600_realize() and
>>>> aspeed_soc_realize() realize only the wanted number.  Works, although
>>>> it can leave unrealized devices hanging around in the QOM composition
>>>> tree.  Affects machines ast2500-evb, ast2600-evb, palmetto-bmc,
>>>> romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc.
>>>>
>>>> Make the init functions create only the wanted ones.  Visible in "info
>>>> qom-tree"; here's the change for ast2600-evb:
>>>>
>>>>       /machine (ast2600-evb-machine)
>>>>         [...]
>>>>         /soc (ast2600-a1)
>>>>           [...]
>>>>           /ftgmac100[0] (ftgmac100)
>>>>             /ftgmac100[0] (qemu:memory-region)
>>>>      -    /ftgmac100[1] (ftgmac100)
>>>>      -    /ftgmac100[2] (ftgmac100)
>>>>      -    /ftgmac100[3] (ftgmac100)
>>>>           /gpio (aspeed.gpio-ast2600)
>>>>           [...]
>>>>           /mii[0] (aspeed-mmi)
>>>>             /aspeed-mmi[0] (qemu:memory-region)
>>>>      -    /mii[1] (aspeed-mmi)
>>>>      -    /mii[2] (aspeed-mmi)
>>>>      -    /mii[3] (aspeed-mmi)
>>>>           /rtc (aspeed.rtc)
>>>>
>>>> I'm not sure creating @nb_nics devices makes sense.  How many does the
>>>> physical chip provide?
>>>
>>> The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine
>>> define the one it uses, generally MAC0 but the tacoma board uses MAC3.
>>>
>>> Shouldn't the model reflect the real address space independently from
>>> the NIC backends defined on the command line ?

If the SoC has N ftgmac100 peripherals, you need to mmio-map the N 
instances, else your guest will get MEMTX_DECODE_ERROR trying to access 
it, regardless command line NIC plugged.

>>
>> That's my feeling too, though I'm not sure what to make of the unrealised devices
>> in the QOM tree. Does it matter? It hasn't bothered me.
> 
> Depending on what the initialization code does, unrealized devices can
> be anything from a little wasted memory to open bear trap.  I don't
> really expect the latter extreme in the code, as I expect bear traps to
> quickly catch the developer that set them.
> 
> I guess the unrealized devices cleaned up in this patch did no actual
> harm.
> 
> Still, it's an unhealthy state, and that's why I clean it up.  "[PATCH
> 24/24] qdev: Assert onboard devices all get realized properly" should
> ensure we stay clean.
> 
> 