On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Tue, Feb 13, 2018 at 09:07:10AM -0800, Andrey Smirnov wrote: >> +static void designware_pcie_root_class_init(ObjectClass *klass, void *data) >> +{ >> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + >> + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >> + >> + k->vendor_id = PCI_VENDOR_ID_SYNOPSYS; >> + k->device_id = 0xABCD; >> + k->revision = 0; >> + k->class_id = PCI_CLASS_BRIDGE_PCI; >> + k->is_express = true; >> + k->is_bridge = true; >> + k->exit = pci_bridge_exitfn; >> + k->realize = designware_pcie_root_realize; >> + k->config_read = designware_pcie_root_config_read; >> + k->config_write = designware_pcie_root_config_write; >> + >> + dc->reset = pci_bridge_reset; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->user_creatable = false; >> + dc->vmsd = &vmstate_designware_pcie_root; >> +} >> + >> +static uint64_t designware_pcie_host_mmio_read(void *opaque, hwaddr addr, >> + unsigned int size) >> +{ >> + PCIHostState *pci = PCI_HOST_BRIDGE(opaque); >> + PCIDevice *device = pci_find_device(pci->bus, 0, 0); >> + >> + return pci_host_config_read_common(device, >> + addr, >> + pci_config_size(device), >> + size); >> +} >> + >> +static void designware_pcie_host_mmio_write(void *opaque, hwaddr addr, >> + uint64_t val, unsigned int size) >> +{ >> + PCIHostState *pci = PCI_HOST_BRIDGE(opaque); >> + PCIDevice *device = pci_find_device(pci->bus, 0, 0); >> + >> + return pci_host_config_write_common(device, >> + addr, >> + pci_config_size(device), >> + val, size); >> +} >> + >> +static const MemoryRegionOps designware_pci_mmio_ops = { >> + .read = designware_pcie_host_mmio_read, >> + .write = designware_pcie_host_mmio_write, >> + .endianness = DEVICE_NATIVE_ENDIAN, >> + .impl = { >> + /* >> + * Our device would not work correctly if the guest was doing >> + * unaligned access. This might not be a limitation on the real >> + * device but in practice there is no reason for a guest to access >> + * this device unaligned. >> + */ >> + .min_access_size = 4, >> + .max_access_size = 4, >> + .unaligned = false, >> + }, >> +}; > > Could you pls add some comments explaining why is DEVICE_NATIVE_ENDIAN > appropriate here? Most of these cases are plain "we never bothered > about cross-endian setups". Some are "there's a mix of different > endian-ness values, need to handle in a special way". > > I suspect you really need DEVICE_LITTLE_ENDIAN. >
That MemoryRegion corresponds to a register file permanently mapped into CPU's address space, so my assumption is that SoC designers will wire it according to CPUs endianness be it big or little. I am not aware of any big-endian CPU based SoC on the market using Designware's IP block, so I don't think there are any precedent confirming or denying correctness of my assumption. IMHO, this is also the reason why all of Linux driver code for that IP assumes little endianness. I can't say that I testing this code against a big-endian guest/CPU, but that is primarily due to the fact that there's no real use case and any test set up I can put toghere would be a contrived example pointlessly proving my point. Anyway, I am more than happy to switch it to use DEVICE_LITTLE_ENDIAN, I just don't know if doing so is any more justified than keeping it DEVICE_NATIVE_ENDIAN. Thanks, Andrey Smirnov