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
signature.asc
Description: PGP signature
