Re: [Qemu-devel] [RFC PATCH] spapr_pci: make index property mandatory

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2310 bytes --]

On Mon, Sep 11, 2017 at 01:35:03PM +0200, Greg Kurz wrote:
> Creating several PHBs without index property confuses the DRC code
> and causes issues:
> - only the first index-less PHB is functional, the other ones will
>   silently ignore hotplugging of PCI devices
> - QEMU will even terminate if these PHBs have cold-plugged devices
> 
> qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
>  is still awaiting release
> 
> This happens because DR connectors for child PCI devices are created
> with a DRC index that is derived from the PHB's index property. If the
> PHBs are created without index, then the same value of -1 is used to
> compute the DRC indexes for both PHBs, hence causing the collision.
> 
> Also, the index property is used to compute the placement of the PHB's
> memory regions. It is limited to 31 or 255, depending on the machine
> type version. This fits well with the requirements of DRC indexes, which
> need the PHB index to be a 16-bit value.
> 
> This patch hence makes the index property mandatory. As a consequence,
> the PHB's memory regions and BUID are now always configured according
> to the index, and it is no longer possible to set them from the command
> line. This is a reasonable trade-off, as it is very unlikely that people
> create PHBs without index (at least libvirt doesn't do it FWIW).
> 
> We have to introduce a PHB instance init function to initialize the
> 64-bit window address to -1 because pseries-2.7 and older machines
> don't set it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> Hi,
> 
> This is a proposal to address the issue uncovered during the review of the
> PHB hotplug patches:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2017-07/msg00557.html
> 
> I'd like to address this properly before resuming work on PHB hotplug
> itself. Please comment.

Yes, I think this looks good.  Commit message should explicitly
mention that this does break backwards compat - but we don't think the
non-index PHB feature was used in practice and the simplification is
worth it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]