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

[Cédric Le Goater]

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. 

[ ... ]

>> +/*
>> + * The CONFIG_DATA register expects little endian accesses, but as the
>> + * region is big endian, we have to swap the value.
>> + */
>> +static void pnv_phb3_config_write(PnvPHB3 *phb, unsigned off,
>> +                                  unsigned size, uint64_t val)
>> +{
>> +    uint32_t cfg_addr, limit;
>> +    PCIDevice *pdev;
>> +
>> +    pdev = pnv_phb3_find_cfg_dev(phb);
>> +    if (!pdev) {
>> +        return;
>> +    }
>> +    cfg_addr = (phb->regs[PHB_CONFIG_ADDRESS >> 3] >> 32) & 0xfff;
>> +    cfg_addr |= off;
> 
> This looks weird - there are callers of this that appear to have low
> bits in 'off', then you're ORing it with overlapping low bits.

Here is an extract of the specs of the PHB_CONFIG_ADDRESS register :

  20:29 Register Number(00:09) RW 0

  Register Number to use for the configuration cycle access. 

  30:31 Register Number(10:11) RO 0

  Don’t Cares. The value written will be dropped, and zeros are
  always returned for a read.  

So, yes, we shoud be using 0xffc, just like in read.

>> +    limit = pci_config_size(pdev);
>> +    if (limit <= cfg_addr) {
>> +        /* conventional pci device can be behind pcie-to-pci bridge.
>> +           256 <= addr < 4K has no effects. */
>> +        return;
>> +    }
>> +    switch (size) {
>> +    case 1:
>> +        break;
>> +    case 2:
>> +        val = bswap16(val);
>> +        break;
>> +    case 4:
>> +        val = bswap32(val);
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +    pci_host_config_write_common(pdev, cfg_addr, limit, val, size);
>> +}
>> +
>> +static uint64_t pnv_phb3_config_read(PnvPHB3 *phb, unsigned off,
>> +                                     unsigned size)
>> +{
>> +    uint32_t cfg_addr, limit;
>> +    PCIDevice *pdev;
>> +    uint64_t val;
>> +
>> +    pdev = pnv_phb3_find_cfg_dev(phb);
>> +    if (!pdev) {
>> +        return ~0ull;
>> +    }
>> +    cfg_addr = (phb->regs[PHB_CONFIG_ADDRESS >> 3] >> 32) & 0xffc;
>> +    cfg_addr |= off;
> 
> This looks better, should it be 0xffc in the write path as well?

yes.

>> +    limit = pci_config_size(pdev);
>> +    if (limit <= cfg_addr) {
>> +        /* conventional pci device can be behind pcie-to-pci bridge.
>> +           256 <= addr < 4K has no effects. */
>> +        return ~0ull;
>> +    }
>> +    val = pci_host_config_read_common(pdev, cfg_addr, limit, size);
>> +    switch (size) {
>> +    case 1:
>> +        return val;
>> +    case 2:
>> +        return bswap16(val);
>> +    case 4:
>> +        return bswap32(val);
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +}
>> +
>> +static void pnv_phb3_check_m32(PnvPHB3 *phb)
>> +{
>> +    uint64_t base, start, size;
>> +    MemoryRegion *parent;
>> +    PnvPBCQState *pbcq = &phb->pbcq;
>> +
>> +    if (phb->m32_mapped) {
>> +        memory_region_del_subregion(phb->mr_m32.container, &phb->mr_m32);
>> +        phb->m32_mapped = false;
> 
> 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. 

[ ... ]

>> +    case IODA2_TBL_PEEV:
>> +        tptr = phb->ioda_PEEV;
>> +        mask = 3;
>> +        break;
>> +    default:
>> +        return NULL;
>> +    }
>> +    index &= mask;
> 
> Do we want to silently mask, rather than logging an error if there's
> an access out of bounds for a particular table?

I will add an error.

[ ... ]

>> +    /* Handle masking */
>> +    switch (off) {
>> +    case PHB_M64_UPPER_BITS:
> 
> Couldn't you handle this in the switch below - it should be ok to
> momentarily have the invalid bits set in your reg case, as long as you
> mask them before applying the side-effects, yes?

yes.

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

[ ... ]

>> +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. 

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.