Re: [Qemu-devel] [PATCH v2 1/2] ppc/pnv: Add model for Power8 PHB3 PCIe Host bridge

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

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

On Tue, Jul 24, 2018 at 02:18:30PM +0200, Cédric Le Goater wrote:
> Hello,
> 
> >From the discussion on the XICS MSI object, I gather that exporting
> icp_irq is fine. 
> 
> Some more comments below, I have tried to answer the parts that were 
> not addressed yet.

[snip]
> > Could you use memory_region_set_enabled() rather than having to add
> > and delete the subregion and keep track of it in this separate
> > variable?
> 
> pnv_phb3_check_m32/m64 remap the regions.

Ah, ok.

[snip]
> > That said... shouldn't you filter our invalid or read-only regs before
> > updating the cache?
> 
> hmm, the current model relies on the fact that some registers have
> their value implicitly updated. But I guess, we can dump which are
> accessed and implement a default behavior for these. I will take a
> look.

I don't really follow what you're saying here, but I guess I'll see
what it looks like in te next spin.

> 
> [ ... ]
> 
> >> +static const MemoryRegionOps pnv_phb3_msi_ops = {
> >> +    /* There is no .read as the read result is undefined by PCI spec */
> > 
> > What will qemu do if it hits a NULL read here?  The behaviour may be
> > undefind, but we'd prefer it didn't crash qemu..
> 
> I will add one with a standard error message. It is better to have
> some output anyway.
> 
> [ ... ]
> 
> >> +static void pnv_phb3_pci_create(PnvPHB3 *phb, Error **errp)
> >> +{
> >> +    PCIHostState *pcih = PCI_HOST_BRIDGE(phb);
> >> +    PCIDevice *brdev;
> >> +    PCIDevice *pdev;
> >> +    PCIBus *parent;
> >> +    uint8_t chassis = phb->chip_id * 4 + phb->phb_id;
> >> +    uint8_t chassis_nr = 128;
> >> +
> >> +    /* Add root complex */
> >> +    pdev = pci_create(pcih->bus, 0, TYPE_PNV_PHB3_RC);
> >> +    object_property_add_child(OBJECT(phb), "phb3-rc", OBJECT(pdev), NULL);
> >> +    qdev_prop_set_uint8(DEVICE(pdev), "chassis", chassis);
> >> +    qdev_prop_set_uint16(DEVICE(pdev), "slot", 1);
> >> +    qdev_init_nofail(DEVICE(pdev));
> >> +
> >> +    /* Setup bus for that chip */
> >> +    parent = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> >> +
> >> +    brdev = pci_create(parent, 0, "pci-bridge");
> > 
> > What is this pci bridge representing?  I know PCI-e PHBs typically
> > have a pseudo P2P bridge right under them, but isn't that represnted
> > by the root complex above?
> 
> This is the legacy pci bridge under the pcie bus.

Ah, ok.  Didn't realise there was a vanilla PCI bridge built in.

> Here is is the qdev hierarchy :
> 
>   dev: pnv-phb3, id ""
>     index = 0 (0x0)
>     chip-id = 0 (0x0)
>     bus: phb3-root-bus
>       type pnv-phb3-root-bus
>       dev: pnv-phb3-rc, id ""
>         power_controller_present = true
>         chassis = 0 (0x0)
>         slot = 1 (0x1)
>         port = 0 (0x0)
>         aer_log_max = 8 (0x8)
>         addr = 00.0
>         romfile = ""
>         rombar = 1 (0x1)
>         multifunction = false
>         command_serr_enable = true
>         x-pcie-lnksta-dllla = true
>         x-pcie-extcap-init = true
>         class PCI bridge, addr 00:00.0, pci id 1014:03dc (sub 0000:0000)
>         bus: pcie.0
>           type PCIE
>           dev: pci-bridge, id ""
>             chassis_nr = 128 (0x80)
>             msi = "off"
>             shpc = false
>             addr = 00.0
>             romfile = ""
>             rombar = 1 (0x1)
>             multifunction = false
>             command_serr_enable = true
>             x-pcie-lnksta-dllla = true
>             x-pcie-extcap-init = true
>             class PCI bridge, addr 00:00.0, pci id 1b36:0001 (sub 0000:0000)
>             bus: pci.0
>               type PCI
> 
> [ ... ]
> 
> >> +static const PropertyInfo pnv_phb3_phb_id_propinfo = {
> >> +    .name = "irq",
> >> +    .get = pnv_phb3_get_phb_id,
> >> +    .set = pnv_phb3_set_phb_id,
> >> +};
> > 
> > Can't you use a static DeviceProps style property for this, which is a
> > bit simpler?
> 
> OK. We will address user creatable PHBs in some other way. Most
> certainly in the realize routine like you suggested.
> 
> Thanks,
> 
> C. 
> 

-- 
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 --]