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.
