Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Sat, Mar 3, 2018 at 7:55 PM, Michael S. Tsirkin  wrote:
> On Tue, Feb 13, 2018 at 02:47:24PM -0800, Andrey Smirnov wrote:
>> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
>> > On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
>> >> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
>> >> 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.
>> >
>> > IMHO if Linux driver code does cpu_to_le then it seems best to be
>> > consistent with that.
>> >
>>
>> Well, all of the DW code does so implicitly by using readl()/writel()
>> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
>> seems to me that it could be either because the access does have to be
>> LE always or simply because readl()/writel() are goto memory helpers
>> on ARM/LE-platforms.
>
> PCI things generally tend to be LE since that's how standard
> PCI registers are defined, and being consistent avoids confusion.
>
>> FWIW: Somewhat similar precedent of MIPS/Boston mac

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Michael S. Tsirkin]

On Tue, Feb 13, 2018 at 02:47:24PM -0800, Andrey Smirnov wrote:
> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
> > On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
> >> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
> >> 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.
> >
> > IMHO if Linux driver code does cpu_to_le then it seems best to be
> > consistent with that.
> >
> 
> Well, all of the DW code does so implicitly by using readl()/writel()
> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
> seems to me that it could be either because the access does have to be
> LE always or simply because readl()/writel() are goto memory helpers
> on ARM/LE-platforms.

PCI things generally tend to be LE since that's how standard
PCI registers are defined, and being consistent avoids confusion.

> FWIW: Somewhat similar precedent of MIPS/Boston machine can serve as
> counter-example to my assumption, since Xilinx PCIE IP there seem to
> be wired to be LE despite being attached to BE CPU.
> 
> Thanks,
> Andrey Smirnov

OK so

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Tue, Feb 13, 2018 at 2:47 PM, Andrey Smirnov
 wrote:
> On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
>> On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
>>> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  
>>> 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.
>>
>> IMHO if Linux driver code does cpu_to_le then it seems best to be
>> consistent with that.
>>
>
> Well, all of the DW code does so implicitly by using readl()/writel()
> helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
> seems to me that it could be either because the access does have to be
> LE always or simply because readl()/writel() are goto memory helpers
> on ARM/LE-platforms.
>
> FWIW: Somewhat similar precedent of MIPS/Boston machine can serve as
> counter-example to my assumption, since Xilinx PCIE IP there seem to
> be wired to be LE despite being attached to BE CPU.
>

Michael, Peter:

Just in case we are in an accidental deadlock waiting on each other,
my assumption is that this patch in particular and the rest in the
series are good as is to be applied. Please let me know if any changes
need to be made and I'll submit v7.

Tha

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Tue, Feb 13, 2018 at 2:15 PM, Michael S. Tsirkin  wrote:
> On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
>> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  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.
>
> IMHO if Linux driver code does cpu_to_le then it seems best to be
> consistent with that.
>

Well, all of the DW code does so implicitly by using readl()/writel()
helpers which will perform cpu_to_le/le_to_cpu under the hood. But is
seems to me that it could be either because the access does have to be
LE always or simply because readl()/writel() are goto memory helpers
on ARM/LE-platforms.

FWIW: Somewhat similar precedent of MIPS/Boston machine can serve as
counter-example to my assumption, since Xilinx PCIE IP there seem to
be wired to be LE despite being attached to BE CPU.

Thanks,
Andrey Smirnov

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Michael S. Tsirkin]

On Tue, Feb 13, 2018 at 12:24:40PM -0800, Andrey Smirnov wrote:
> On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  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.

IMHO if Linux driver code does cpu_to_le then it seems best to be
consistent with that.

> 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

I agree it's probably not critical for a target-specific device.

-- 
MST

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

On Tue, Feb 13, 2018 at 10:13 AM, Michael S. Tsirkin  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

Re: [Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Michael S. Tsirkin]

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.

-- 
MST

[Qemu-devel] [PATCH v6 1/3] pci: Add support for Designware IP block

[Andrey Smirnov]

Add code needed to get a functional PCI subsytem when using in
conjunction with upstream Linux guest (4.13+). Tested to work against
"e1000e" (network adapter, using MSI interrupts) as well as
"usb-ehci" (USB controller, using legacy PCI interrupts).

Based on "i.MX6 Applications Processor Reference Manual" (Document
Number: IMX6DQRM Rev. 4) as well as corresponding dirver in Linux
kernel (circa 4.13 - 4.16 found in drivers/pci/dwc/*)

Cc: Peter Maydell 
Cc: Jason Wang 
Cc: Philippe Mathieu-Daudé 
Cc: Marcel Apfelbaum 
Cc: Michael S. Tsirkin 
Cc: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org
Cc: yurov...@gmail.com
Signed-off-by: Andrey Smirnov 
---
 default-configs/arm-softmmu.mak  |   2 +
 include/hw/pci-host/designware.h | 102 ++
 include/hw/pci/pci_ids.h |   2 +
 hw/pci-host/designware.c | 755 +++
 hw/pci-host/Makefile.objs|   2 +
 5 files changed, 863 insertions(+)
 create mode 100644 include/hw/pci-host/designware.h
 create mode 100644 hw/pci-host/designware.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ca34cf4462..877bb7dd36 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -133,3 +133,5 @@ CONFIG_GPIO_KEY=y
 CONFIG_MSF2=y
 CONFIG_FW_CFG_DMA=y
 CONFIG_XILINX_AXI=y
+CONFIG_PCI_DESIGNWARE=y
+
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
new file mode 100644
index 00..a4f2c0695b
--- /dev/null
+++ b/include/hw/pci-host/designware.h
@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * Designware PCIe IP block emulation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see
+ * .
+ */
+
+#ifndef DESIGNWARE_H
+#define DESIGNWARE_H
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/pci/pcie_host.h"
+#include "hw/pci/pci_bridge.h"
+
+#define TYPE_DESIGNWARE_PCIE_HOST "designware-pcie-host"
+#define DESIGNWARE_PCIE_HOST(obj) \
+ OBJECT_CHECK(DesignwarePCIEHost, (obj), TYPE_DESIGNWARE_PCIE_HOST)
+
+#define TYPE_DESIGNWARE_PCIE_ROOT "designware-pcie-root"
+#define DESIGNWARE_PCIE_ROOT(obj) \
+ OBJECT_CHECK(DesignwarePCIERoot, (obj), TYPE_DESIGNWARE_PCIE_ROOT)
+
+struct DesignwarePCIERoot;
+typedef struct DesignwarePCIERoot DesignwarePCIERoot;
+
+typedef struct DesignwarePCIEViewport {
+DesignwarePCIERoot *root;
+
+MemoryRegion cfg;
+MemoryRegion mem;
+
+uint64_t base;
+uint64_t target;
+uint32_t limit;
+uint32_t cr[2];
+
+bool inbound;
+} DesignwarePCIEViewport;
+
+typedef struct DesignwarePCIEMSIBank {
+uint32_t enable;
+uint32_t mask;
+uint32_t status;
+} DesignwarePCIEMSIBank;
+
+typedef struct DesignwarePCIEMSI {
+uint64_t base;
+MemoryRegion iomem;
+
+#define DESIGNWARE_PCIE_NUM_MSI_BANKS1
+
+DesignwarePCIEMSIBank intr[DESIGNWARE_PCIE_NUM_MSI_BANKS];
+} DesignwarePCIEMSI;
+
+struct DesignwarePCIERoot {
+PCIBridge parent_obj;
+
+uint32_t atu_viewport;
+
+#define DESIGNWARE_PCIE_VIEWPORT_OUTBOUND0
+#define DESIGNWARE_PCIE_VIEWPORT_INBOUND 1
+#define DESIGNWARE_PCIE_NUM_VIEWPORTS4
+
+DesignwarePCIEViewport viewports[2][DESIGNWARE_PCIE_NUM_VIEWPORTS];
+DesignwarePCIEMSI msi;
+};
+
+typedef struct DesignwarePCIEHost {
+PCIHostState parent_obj;
+
+DesignwarePCIERoot root;
+
+struct {
+AddressSpace address_space;
+MemoryRegion address_space_root;
+
+MemoryRegion memory;
+MemoryRegion io;
+
+qemu_irq irqs[4];
+} pci;
+
+MemoryRegion mmio;
+} DesignwarePCIEHost;
+
+#endif  /* DESIGNWARE_H */
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 35df1874a9..23fefe1bc6 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -266,4 +266,6 @@
 #define PCI_VENDOR_ID_TEWS   0x1498
 #define PCI_DEVICE_ID_TEWS_TPCI200   0x30C8
 
+#define PCI_VENDOR_ID_SYNOPSYS   0x16C3
+
 #endif
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
new file mode 100644
index 00..18935c11f6
--- /dev/null
+++ b/hw/pci-host/designware.c
@@ -0,0 +1,755 @@
+/*
+ * Copyright (c) 2018, Impinj, Inc.
+ *
+ * Designware PCIe IP block emulation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the