Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
On 01/16/2017 06:46 PM, Andrea Bolognani wrote: On Wed, 2017-01-11 at 14:18 +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. I tested this both on x86/q35 (Debian guest) and on aarch64/virt (Fedora guest) very briefly, eg. started the guest, performed some network I/O and shut it down. It seems to be working fine :) Thanks for testing it! A tested-by tag would be appreciated for the next version :) Marcel -- Andrea Bolognani / Red Hat / Virtualization
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
On Wed, 2017-01-11 at 14:18 +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. I tested this both on x86/q35 (Debian guest) and on aarch64/virt (Fedora guest) very briefly, eg. started the guest, performed some network I/O and shut it down. It seems to be working fine :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
On 01/13/2017 10:35 AM, Gerd Hoffmann wrote: Hi, 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 :) I'd use msi-x right from start to avoid any backward compatibility issues. Hi Gerd, Michael I'll post a version with msi-x. Thanks, Marcel cheers, Gerd
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
Hi, > 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 :) I'd use msi-x right from start to avoid any backward compatibility issues. cheers, Gerd
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
On Thu, Jan 12, 2017 at 06:23:08PM +0200, Marcel Apfelbaum wrote: > 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? > > > > > > Thanks, > > > Marcel > > > > 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 :) > > Thanks, > Marcel If I'm right and msix will be less code, that would be a strage approach imho. But you decide. > > > > > 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 > > > > > > > > > > -- > > > > > 2.5.5
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
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? Thanks, Marcel 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 :) Thanks, Marcel 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 -- 2.5.5
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
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? > > Thanks, > Marcel 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. > > > 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 > > > > > > -- > > > 2.5.5
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
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? Thanks, Marcel 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 -- 2.5.5
Re: [Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
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. 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. 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 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. 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. > 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 > > -- > 2.5.5
[Qemu-devel] [PATCH V2 0/3] hw/pcie: Introduce Generic PCI Express Root Port
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. 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 -- 2.5.5
