[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express R

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
Date: Thu, 12 Jan 2017 18:23:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 01/12/2017 06:20 PM, Michael S. Tsirkin wrote:
On Thu, Jan 12, 2017 at 06:05:20PM +0200, Marcel Apfelbaum wrote:
On 01/12/2017 05:56 PM, Michael S. Tsirkin wrote:
On Wed, Jan 11, 2017 at 02:18:53PM +0200, Marcel Apfelbaum wrote:
v1 -> v2:
 - Rebased on master.

The Generic Root Port behaves the same as the
Intel's IOH device with id 3420, without having
Intel specific attributes.

The device has two purposes:
 (1) Can be used on both X86 and ARM machines.
 (2) It will allow us to tweak the behaviour
    (e.g add vendor-specific PCI capabilities)
     - something that obviously cannot be done
       on a known device.

Patch 1/3: Introduce a base class for Root Ports - most of the code
           is migrated from IOH3420 implementation.
Patch 2/3: Derives the IOH3420 from the new base class
Patch 3/3: Introduces the generic Root Port.

Tested with Linux and Windows guests only on x86 hosts.

Hi Michael,

So I understand how it's helpful to share code
with an existing bridge, but I worries me that
you picked up some arbitrary values from the intel chip
and promote them as a standard pcie thing.

I suppose you are referring to the macros, I can duplicate
them, of course.

You want to have e.g. base-pcie-root-port and
inherit that. Only put really common stuff in there.
Similar to how we do it in hw/pci/pci_bridge.c

So you prefer a different code file for the generic device.
I currently put them both in the same file, I'll split.

Now we do not want to over-engineer, so I understand
how you might want to say hey I use this option
and intel does the same so we do not need a flag
for that now. And that's fine but please add a comment
to that end.

Indeed, I wanted to keep things simple. I'll add a comment
on the functions that are not generic but are the same on
both implementations 'by chance'.

And when you do you will discover there's some stuff you
might be able to simplify, e.g. I don't yet see why
you would want AER support there.

I am not sure *why not*.
I am 'afraid' people will not use the new port
because of missing features and they can see the AER as one of them.
Do you see a special issue in imitating Intel's behavior on AER?


Not really, fair enough. Just keep an eye out for caps
you don't need. E.g. it's worth considering whether
msi-x would be better since then you can just use
a fixed vector number without playing tricks like
you have to with msi.

Sure, thanks for the pointers!
I suppose I can change later to msi-x in a follow up patch
to avoid introducing new bugs from the beginning :)


Marcel Apfelbaum (3):
  hw/pcie: Introduce a base class for PCI Express Root Ports
  hw/ioh3420: derive from PCI Express Root Port base class
  hw/pcie: Introduce Generic PCI Express Root Port

 default-configs/arm-softmmu.mak    |   1 +
 default-configs/i386-softmmu.mak   |   1 +
 default-configs/x86_64-softmmu.mak |   1 +
 hw/pci-bridge/Makefile.objs        |   1 +
 hw/pci-bridge/ioh3420.c            | 152 ++-----------------------
 hw/pci-bridge/pcie_root_port.c     | 228 +++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h               |   1 +
 include/hw/pci/pcie_port.h         |  18 +++
 8 files changed, 258 insertions(+), 145 deletions(-)
 create mode 100644 hw/pci-bridge/pcie_root_port.c


reply via email to

[Prev in Thread] Current Thread [Next in Thread]