Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
> On Mar 4, 2021, at 2:21 PM, Jonathan Cameron
> wrote:
>
> On Tue, 9 Feb 2021 15:35:49 -0500
> Chris Browy wrote:
>
> Hi Chris,
>
> One more thing hit whilst debugging linux side of this.
>
>> +static void pcie_doe_irq_assert(DOECap *doe_cap)
>> +{
>> +PCIDevice *dev = doe_cap->doe->pdev;
>> +
>> +if (doe_cap->cap.intr && doe_cap->ctrl.intr) {
>
>
> need something like
>
> doe_cap->status.intr = 1;
>
> I think or anyone checking the status register is going to think
> this interrupt is spurious.
You’re absolutely right, good catch!
>
> Otherwise all seems to work. I need to do a bit of tidying up on
> kernel code but should be able to send out early next week.
>
We’re putting out the v3 by end of this week. We’re spent a bit longer
tidying up on our end but sounds like coming together real soon in 5.12
release!
>> +/* Interrupt notify */
>> +if (msix_enabled(dev)) {
>> +msix_notify(dev, doe_cap->cap.vec);
>> +} else if (msi_enabled(dev)) {
>> +msi_notify(dev, doe_cap->cap.vec);
>> +}
>> +/* Not support legacy IRQ */
>> +}
>> +}
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
On Tue, 9 Feb 2021 15:35:49 -0500
Chris Browy wrote:
Hi Chris,
One more thing hit whilst debugging linux side of this.
> +static void pcie_doe_irq_assert(DOECap *doe_cap)
> +{
> +PCIDevice *dev = doe_cap->doe->pdev;
> +
> +if (doe_cap->cap.intr && doe_cap->ctrl.intr) {
need something like
doe_cap->status.intr = 1;
I think or anyone checking the status register is going to think
this interrupt is spurious.
Otherwise all seems to work. I need to do a bit of tidying up on
kernel code but should be able to send out early next week.
> +/* Interrupt notify */
> +if (msix_enabled(dev)) {
> +msix_notify(dev, doe_cap->cap.vec);
> +} else if (msi_enabled(dev)) {
> +msi_notify(dev, doe_cap->cap.vec);
> +}
> +/* Not support legacy IRQ */
> +}
> +}
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
On Thu, 18 Feb 2021 19:46:54 -0500 Chris Browy wrote: > > On Feb 18, 2021, at 2:11 PM, Jonathan Cameron > > wrote: > > > > On Fri, 12 Feb 2021 16:58:21 -0500 > > Chris Browy wrote: > > > >>> On Feb 12, 2021, at 11:24 AM, Jonathan Cameron > >>> wrote: > >>> > >>> On Tue, 9 Feb 2021 15:35:49 -0500 > >>> Chris Browy wrote: > >>> > >>> Run ./scripts/checkpatch.pl over the patches and fix all the warnings > >>> before > >>> posting. It will save time by clearing out most of the minor formatting > >>> issues > >>> and similar that inevitably sneak in during development. > >>> > >> Excellent suggestion. We’re still newbies! > >> > >>> The biggest issue I'm seeing in here is that the abstraction of > >>> multiple DOE capabiltiies accessing same protocols doesn't make sense. > >>> > >>> Each DOE ecap region and hence mailbox can have it's own set of > >>> (possibly overlapping) protocols. > >>> > >>> From the ECN: > >>> "It is permitted for a protocol using data object exchanges to require > >>> that a Function implement a unique instance of DOE for that specific > >>> protocol, and/or to allow sharing of a DOE instance to only a specific > >>> set of protocols using data object exchange, and/or to allow a Function > >>> to implement multiple instances of DOE supporting the specific protocol." > >>> > >>> Tightly couple the ECAP and DOE. If we are in the multiple instances > >>> of DOE supporting a specific protocol case, then register it separately > >>> for each one. The individual device emulation then needs to deal with > >>> any possible clashes etc. > >> > >> Not sure how configurable we want to make the device. It is a simple type > >> 3 > >> device after all. > > > > Agreed, but what I (or someone else) really doesn't want to have to do > > in the future is reimplement DOE because we made design decisions that make > > this version hard to reuse. Unless it is particularly nasty to do we should > > try to design something that is generally useful rather than targeted to > > closely at the specific case we are dealing with. > > > > I'd argue the ECAP and the DOE mailbox are always tightly coupled 1-to-1. > > Whether the device wants to implement multiple protocols on each DOE mailbox > > or indeed run individual protocols on multiple DOE mailboxes is a design > > decision, but the actual mechanics of DOE match up with the config > > space structures anything else is impdef on the device. > > Yes I agree that there is 1-to-1 between DOE extended cap (ECAP) and DOE > Mailbox. If we want to provide complete flexibility we should let the user > pass > device property arrays to QEMU command for how many DOE ECAP’s to build > out and how to assign protocol(s) to each of them. Array index is the DOE > instance #. > > Also we can provide a property for cdat binary (blob) filename to initialize > the CDAT structure[entries]. This just reads in whatever mix of CDAT > structure > types are in the blob. > > -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,size=256M \ > doe-ecap-instances=2 \ > doe-ecap[0]=5 // bitwise OR for protocols shared > doe-ecap[1]=2 //bitwise OR for protocols shared > doe-ecap-cdat[1]=mycdat.bin > > where let’s say protocols bitvector > bit [0]=CMA > bit [1]=CDAT > bit [2}=Compliance > > Let me know if you some better alternatives and we’ll implement it. > Gut feeling for DOE is that the particular combination of protocol and ECAP/DOE is device dependent. As such... I'm not sure we actually want to expose it as command line controllable at all. If we do, I'd suggest a small number of sane choices that exercise cases we want to check. From a testing point of view, 2 DOE, one of which supports multiple protocols and we will have enough to test likely failure modes in the code. The one protocol running on multiple mailboxes is already covered by the discovery protocol which they all support. That might not exercise all the potential problems on the emulator side (as no need to do locking etc) but it will proabbly exercise those in the OS and firmware. Almost all users of DOE functionality in QEMU in the long run are likely to be emulating a particular device so will hard code the DOE instances present on that device in their emulation of whatever PCIe device they are emulating. This is no different to picking a particular layout for config space. We could make it fully flexible, but it's rarely useful to do so. If anyone wants to check something unusual, they can hack it into QEMU. As a side note, a protocol bit vector is going to unmaintainable as there will be lots of protocols and last thing we want is that vector to mean different things on different emulated PCI devices. Jonathan > > > > >> > >> The DOE spec does leave it pretty arbitrary regarding N DOE instances (DOE > >> Extended Cap entry points) for M protocols, including where N>1 and M=1. > >> Currently we implement N=2 DOE caps
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
> On Feb 18, 2021, at 2:11 PM, Jonathan Cameron > wrote: > > On Fri, 12 Feb 2021 16:58:21 -0500 > Chris Browy wrote: > >>> On Feb 12, 2021, at 11:24 AM, Jonathan Cameron >>> wrote: >>> >>> On Tue, 9 Feb 2021 15:35:49 -0500 >>> Chris Browy wrote: >>> >>> Run ./scripts/checkpatch.pl over the patches and fix all the warnings before >>> posting. It will save time by clearing out most of the minor formatting >>> issues >>> and similar that inevitably sneak in during development. >>> >> Excellent suggestion. We’re still newbies! >> >>> The biggest issue I'm seeing in here is that the abstraction of >>> multiple DOE capabiltiies accessing same protocols doesn't make sense. >>> >>> Each DOE ecap region and hence mailbox can have it's own set of >>> (possibly overlapping) protocols. >>> >>> From the ECN: >>> "It is permitted for a protocol using data object exchanges to require >>> that a Function implement a unique instance of DOE for that specific >>> protocol, and/or to allow sharing of a DOE instance to only a specific >>> set of protocols using data object exchange, and/or to allow a Function >>> to implement multiple instances of DOE supporting the specific protocol." >>> >>> Tightly couple the ECAP and DOE. If we are in the multiple instances >>> of DOE supporting a specific protocol case, then register it separately >>> for each one. The individual device emulation then needs to deal with >>> any possible clashes etc. >> >> Not sure how configurable we want to make the device. It is a simple type 3 >> device after all. > > Agreed, but what I (or someone else) really doesn't want to have to do > in the future is reimplement DOE because we made design decisions that make > this version hard to reuse. Unless it is particularly nasty to do we should > try to design something that is generally useful rather than targeted to > closely at the specific case we are dealing with. > > I'd argue the ECAP and the DOE mailbox are always tightly coupled 1-to-1. > Whether the device wants to implement multiple protocols on each DOE mailbox > or indeed run individual protocols on multiple DOE mailboxes is a design > decision, but the actual mechanics of DOE match up with the config > space structures anything else is impdef on the device. Yes I agree that there is 1-to-1 between DOE extended cap (ECAP) and DOE Mailbox. If we want to provide complete flexibility we should let the user pass device property arrays to QEMU command for how many DOE ECAP’s to build out and how to assign protocol(s) to each of them. Array index is the DOE instance #. Also we can provide a property for cdat binary (blob) filename to initialize the CDAT structure[entries]. This just reads in whatever mix of CDAT structure types are in the blob. -device cxl-type3,bus=rp0,memdev=cxl-mem1,id=cxl-pmem0,size=256M \ doe-ecap-instances=2 \ doe-ecap[0]=5 // bitwise OR for protocols shared doe-ecap[1]=2 //bitwise OR for protocols shared doe-ecap-cdat[1]=mycdat.bin where let’s say protocols bitvector bit [0]=CMA bit [1]=CDAT bit [2}=Compliance Let me know if you some better alternatives and we’ll implement it. > >> >> The DOE spec does leave it pretty arbitrary regarding N DOE instances (DOE >> Extended Cap entry points) for M protocols, including where N>1 and M=1. >> Currently we implement N=2 DOE caps (instances), one for CDAT, one for >> Compliance Mode.[ >> >> Maybe a more complex MLD device might have one or more DOE instances >> for the CDAT protocol alone to define each HDM but currently we only have >> one pmem (SLD) so we can’t really do much more than what’s supported. >> >> Open to further suggestion though. Based on answer to above we’ll follow >> the suggestion lower in the code review regarding >> > ... >
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
On Fri, 12 Feb 2021 16:58:21 -0500 Chris Browy wrote: > > On Feb 12, 2021, at 11:24 AM, Jonathan Cameron > > wrote: > > > > On Tue, 9 Feb 2021 15:35:49 -0500 > > Chris Browy wrote: > > > > Run ./scripts/checkpatch.pl over the patches and fix all the warnings before > > posting. It will save time by clearing out most of the minor formatting > > issues > > and similar that inevitably sneak in during development. > > > Excellent suggestion. We’re still newbies! > > > The biggest issue I'm seeing in here is that the abstraction of > > multiple DOE capabiltiies accessing same protocols doesn't make sense. > > > > Each DOE ecap region and hence mailbox can have it's own set of > > (possibly overlapping) protocols. > > > > From the ECN: > > "It is permitted for a protocol using data object exchanges to require > > that a Function implement a unique instance of DOE for that specific > > protocol, and/or to allow sharing of a DOE instance to only a specific > > set of protocols using data object exchange, and/or to allow a Function > > to implement multiple instances of DOE supporting the specific protocol." > > > > Tightly couple the ECAP and DOE. If we are in the multiple instances > > of DOE supporting a specific protocol case, then register it separately > > for each one. The individual device emulation then needs to deal with > > any possible clashes etc. > > Not sure how configurable we want to make the device. It is a simple type 3 > device after all. Agreed, but what I (or someone else) really doesn't want to have to do in the future is reimplement DOE because we made design decisions that make this version hard to reuse. Unless it is particularly nasty to do we should try to design something that is generally useful rather than targeted to closely at the specific case we are dealing with. I'd argue the ECAP and the DOE mailbox are always tightly coupled 1-to-1. Whether the device wants to implement multiple protocols on each DOE mailbox or indeed run individual protocols on multiple DOE mailboxes is a design decision, but the actual mechanics of DOE match up with the config space structures anything else is impdef on the device. > > The DOE spec does leave it pretty arbitrary regarding N DOE instances (DOE > Extended Cap entry points) for M protocols, including where N>1 and M=1. > Currently we implement N=2 DOE caps (instances), one for CDAT, one for > Compliance Mode.[ > > Maybe a more complex MLD device might have one or more DOE instances > for the CDAT protocol alone to define each HDM but currently we only have > one pmem (SLD) so we can’t really do much more than what’s supported. > > Open to further suggestion though. Based on answer to above we’ll follow > the suggestion lower in the code review regarding > ...
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
> On Feb 12, 2021, at 11:24 AM, Jonathan Cameron
> wrote:
>
> On Tue, 9 Feb 2021 15:35:49 -0500
> Chris Browy wrote:
>
> Run ./scripts/checkpatch.pl over the patches and fix all the warnings before
> posting. It will save time by clearing out most of the minor formatting
> issues
> and similar that inevitably sneak in during development.
>
Excellent suggestion. We’re still newbies!
> The biggest issue I'm seeing in here is that the abstraction of
> multiple DOE capabiltiies accessing same protocols doesn't make sense.
>
> Each DOE ecap region and hence mailbox can have it's own set of
> (possibly overlapping) protocols.
>
> From the ECN:
> "It is permitted for a protocol using data object exchanges to require
> that a Function implement a unique instance of DOE for that specific
> protocol, and/or to allow sharing of a DOE instance to only a specific
> set of protocols using data object exchange, and/or to allow a Function
> to implement multiple instances of DOE supporting the specific protocol."
>
> Tightly couple the ECAP and DOE. If we are in the multiple instances
> of DOE supporting a specific protocol case, then register it separately
> for each one. The individual device emulation then needs to deal with
> any possible clashes etc.
Not sure how configurable we want to make the device. It is a simple type 3
device after all.
The DOE spec does leave it pretty arbitrary regarding N DOE instances (DOE
Extended Cap entry points) for M protocols, including where N>1 and M=1.
Currently we implement N=2 DOE caps (instances), one for CDAT, one for
Compliance Mode.
Maybe a more complex MLD device might have one or more DOE instances
for the CDAT protocol alone to define each HDM but currently we only have
one pmem (SLD) so we can’t really do much more than what’s supported.
Open to further suggestion though. Based on answer to above we’ll follow
the suggestion lower in the code review regarding
>
> Various comments inline, but mostly small stuff.
>
> Jonathan
>
>
>> ---
>> MAINTAINERS | 7 +
>> hw/pci/meson.build| 1 +
>> hw/pci/pcie.c | 2 +-
>> hw/pci/pcie_doe.c | 414
>> ++
>> include/hw/pci/pci_ids.h | 2 +
>> include/hw/pci/pcie.h | 1 +
>> include/hw/pci/pcie_doe.h | 166
>> include/hw/pci/pcie_regs.h| 4 +
>> include/standard-headers/linux/pci_regs.h | 3 +-
>> 9 files changed, 598 insertions(+), 2 deletions(-)
>> create mode 100644 hw/pci/pcie_doe.c
>> create mode 100644 include/hw/pci/pcie_doe.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 981dc92..4fb865e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1655,6 +1655,13 @@ F: docs/pci*
>> F: docs/specs/*pci*
>> F: default-configs/pci.mak
>>
>> +PCIE DOE
>> +M: Huai-Cheng Kuo
>> +M: Chris Browy
>> +S: Supported
>> +F: include/hw/pci/pcie_doe.h
>> +F: hw/pci/pcie_doe.c
>> +
>> ACPI/SMBIOS
>> M: Michael S. Tsirkin
>> M: Igor Mammedov
>> diff --git a/hw/pci/meson.build b/hw/pci/meson.build
>> index 5c4bbac..115e502 100644
>> --- a/hw/pci/meson.build
>> +++ b/hw/pci/meson.build
>> @@ -12,6 +12,7 @@ pci_ss.add(files(
>> # allow plugging PCIe devices into PCI buses, include them even if
>> # CONFIG_PCI_EXPRESS=n.
>> pci_ss.add(files('pcie.c', 'pcie_aer.c'))
>> +pci_ss.add(files('pcie_doe.c'))
>> softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: files('pcie_port.c',
>> 'pcie_host.c'))
>> softmmu_ss.add_all(when: 'CONFIG_PCI', if_true: pci_ss)
>>
>
> ...
>
>> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
>> new file mode 100644
>> index 000..df8e92e
>> --- /dev/null
>> +++ b/hw/pci/pcie_doe.c
>> @@ -0,0 +1,414 @@
>
> Add a copyright header / license etc before v3.
>
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi/error.h"
>> +#include "qemu/range.h"
>> +#include "hw/pci/pci.h"
>> +#include "hw/pci/pcie.h"
>> +#include "hw/pci/pcie_doe.h"
>> +#include "hw/pci/msi.h"
>> +#include "hw/pci/msix.h"
>> +
>> +/*
>> + * DOE Default Protocols (Discovery, CMA)
>> + */
>> +/* Discovery Request Object */
>> +struct doe_discovery {
>> +DOEHeader header;
>> +uint8_t index;
>> +uint8_t reserved[3];
>> +} QEMU_PACKED;
>> +
>> +/* Discovery Response Object */
>> +struct doe_discovery_rsp {
>> +DOEHeader header;
>> +uint16_t vendor_id;
>> +uint8_t doe_type;
>> +uint8_t next_index;
>> +} QEMU_PACKED;
>> +
>> +/* Callback for Discovery */
>> +static bool pcie_doe_discovery_rsp(DOECap *doe_cap)
>> +{
>> +PCIEDOE *doe = doe_cap->doe;
>> +struct doe_discovery *req = pcie_doe_get_req(doe_cap);
>> +uint8_t index = req->index;
>> +DOEProtocol *prot = NULL;
>> +
>> +/* Request length mismatch, discard */
>> +if (req->header.length < dwsizeof(struct doe_discovery)) {
>> +
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
On Tue, 9 Feb 2021 15:35:49 -0500
Chris Browy wrote:
Run ./scripts/checkpatch.pl over the patches and fix all the warnings before
posting. It will save time by clearing out most of the minor formatting issues
and similar that inevitably sneak in during development.
The biggest issue I'm seeing in here is that the abstraction of
multiple DOE capabiltiies accessing same protocols doesn't make sense.
Each DOE ecap region and hence mailbox can have it's own set of
(possibly overlapping) protocols.
From the ECN:
"It is permitted for a protocol using data object exchanges to require
that a Function implement a unique instance of DOE for that specific
protocol, and/or to allow sharing of a DOE instance to only a specific
set of protocols using data object exchange, and/or to allow a Function
to implement multiple instances of DOE supporting the specific protocol."
Tightly couple the ECAP and DOE. If we are in the multiple instances
of DOE supporting a specific protocol case, then register it separately
for each one. The individual device emulation then needs to deal with
any possible clashes etc.
Various comments inline, but mostly small stuff.
Jonathan
> ---
> MAINTAINERS | 7 +
> hw/pci/meson.build| 1 +
> hw/pci/pcie.c | 2 +-
> hw/pci/pcie_doe.c | 414
> ++
> include/hw/pci/pci_ids.h | 2 +
> include/hw/pci/pcie.h | 1 +
> include/hw/pci/pcie_doe.h | 166
> include/hw/pci/pcie_regs.h| 4 +
> include/standard-headers/linux/pci_regs.h | 3 +-
> 9 files changed, 598 insertions(+), 2 deletions(-)
> create mode 100644 hw/pci/pcie_doe.c
> create mode 100644 include/hw/pci/pcie_doe.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 981dc92..4fb865e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1655,6 +1655,13 @@ F: docs/pci*
> F: docs/specs/*pci*
> F: default-configs/pci.mak
>
> +PCIE DOE
> +M: Huai-Cheng Kuo
> +M: Chris Browy
> +S: Supported
> +F: include/hw/pci/pcie_doe.h
> +F: hw/pci/pcie_doe.c
> +
> ACPI/SMBIOS
> M: Michael S. Tsirkin
> M: Igor Mammedov
> diff --git a/hw/pci/meson.build b/hw/pci/meson.build
> index 5c4bbac..115e502 100644
> --- a/hw/pci/meson.build
> +++ b/hw/pci/meson.build
> @@ -12,6 +12,7 @@ pci_ss.add(files(
> # allow plugging PCIe devices into PCI buses, include them even if
> # CONFIG_PCI_EXPRESS=n.
> pci_ss.add(files('pcie.c', 'pcie_aer.c'))
> +pci_ss.add(files('pcie_doe.c'))
> softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: files('pcie_port.c',
> 'pcie_host.c'))
> softmmu_ss.add_all(when: 'CONFIG_PCI', if_true: pci_ss)
>
...
> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
> new file mode 100644
> index 000..df8e92e
> --- /dev/null
> +++ b/hw/pci/pcie_doe.c
> @@ -0,0 +1,414 @@
Add a copyright header / license etc before v3.
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "qemu/range.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pcie.h"
> +#include "hw/pci/pcie_doe.h"
> +#include "hw/pci/msi.h"
> +#include "hw/pci/msix.h"
> +
> +/*
> + * DOE Default Protocols (Discovery, CMA)
> + */
> +/* Discovery Request Object */
> +struct doe_discovery {
> +DOEHeader header;
> +uint8_t index;
> +uint8_t reserved[3];
> +} QEMU_PACKED;
> +
> +/* Discovery Response Object */
> +struct doe_discovery_rsp {
> +DOEHeader header;
> +uint16_t vendor_id;
> +uint8_t doe_type;
> +uint8_t next_index;
> +} QEMU_PACKED;
> +
> +/* Callback for Discovery */
> +static bool pcie_doe_discovery_rsp(DOECap *doe_cap)
> +{
> +PCIEDOE *doe = doe_cap->doe;
> +struct doe_discovery *req = pcie_doe_get_req(doe_cap);
> +uint8_t index = req->index;
> +DOEProtocol *prot = NULL;
> +
> +/* Request length mismatch, discard */
> +if (req->header.length < dwsizeof(struct doe_discovery)) {
> +return DOE_DISCARD;
return false; Or better yet a meaningful error code to make debugging
easier.
> +}
> +
> +/* Point to the requested protocol */
> +if (index < doe->protocol_num) {
> +prot = >protocols[index];
> +}
> +
> +struct doe_discovery_rsp rsp = {
> +.header = {
> +.vendor_id = PCI_VENDOR_ID_PCI_SIG,
> +.doe_type = PCI_SIG_DOE_DISCOVERY,
> +.reserved = 0x0,
Nice thing about c99 based structure assignment is that unspecified
elements are set to 0 automatically. So you can drop this particular
element safely and end up with slightly cleaner code.
> +.length = dwsizeof(struct doe_discovery_rsp),
> +},
> +.vendor_id = (prot) ? prot->vendor_id : 0x,
> +.doe_type = (prot) ? prot->doe_type : 0xFF,
> +.next_index = (index + 1) < doe->protocol_num ?
> + (index + 1) :
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
No consensus yet but I’d suggest that we’ll do the QEMU work and Jonathan
focuses
on the linux kernel and UEFI/edk2 and CXL SSWG efforts. Seems like
a way to maximize resources and everyone’s contribution and expertise. QEMU
part
requires the least expertise which is why we’re best suited for it compared to
other
areas ;)
Review comments will be folded into next patch.
> On Feb 9, 2021, at 4:42 PM, Ben Widawsky wrote:
>
> Have you/Jonathan come to consensus about which implementation is going
> forward?
> I'd rather not have to review two :D
>
> On 21-02-09 15:35:49, Chris Browy wrote:
>> ---
>> MAINTAINERS | 7 +
>> hw/pci/meson.build| 1 +
>> hw/pci/pcie.c | 2 +-
>> hw/pci/pcie_doe.c | 414
>> ++
>> include/hw/pci/pci_ids.h | 2 +
>> include/hw/pci/pcie.h | 1 +
>> include/hw/pci/pcie_doe.h | 166
>> include/hw/pci/pcie_regs.h| 4 +
>> include/standard-headers/linux/pci_regs.h | 3 +-
>> 9 files changed, 598 insertions(+), 2 deletions(-)
>> create mode 100644 hw/pci/pcie_doe.c
>> create mode 100644 include/hw/pci/pcie_doe.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 981dc92..4fb865e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1655,6 +1655,13 @@ F: docs/pci*
>> F: docs/specs/*pci*
>> F: default-configs/pci.mak
>>
>> +PCIE DOE
>> +M: Huai-Cheng Kuo
>> +M: Chris Browy
>> +S: Supported
>> +F: include/hw/pci/pcie_doe.h
>> +F: hw/pci/pcie_doe.c
>> +
>> ACPI/SMBIOS
>> M: Michael S. Tsirkin
>> M: Igor Mammedov
>> diff --git a/hw/pci/meson.build b/hw/pci/meson.build
>> index 5c4bbac..115e502 100644
>> --- a/hw/pci/meson.build
>> +++ b/hw/pci/meson.build
>> @@ -12,6 +12,7 @@ pci_ss.add(files(
>> # allow plugging PCIe devices into PCI buses, include them even if
>> # CONFIG_PCI_EXPRESS=n.
>> pci_ss.add(files('pcie.c', 'pcie_aer.c'))
>> +pci_ss.add(files('pcie_doe.c'))
>
> It looks like this should be like the below line:
> softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: pci_doe.c))
>
>> softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: files('pcie_port.c',
>> 'pcie_host.c'))
>> softmmu_ss.add_all(when: 'CONFIG_PCI', if_true: pci_ss)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 1ecf6f6..f7516c4 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -735,7 +735,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>>
>> hotplug_event_notify(dev);
>>
>> -/*
>> +/*
>
> Please drop this.
>
>> * 6.7.3.2 Command Completed Events
>> *
>> * Software issues a command to a hot-plug capable Downstream Port by
>> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
>> new file mode 100644
>> index 000..df8e92e
>> --- /dev/null
>> +++ b/hw/pci/pcie_doe.c
>> @@ -0,0 +1,414 @@
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/error-report.h"
>> +#include "qapi/error.h"
>> +#include "qemu/range.h"
>> +#include "hw/pci/pci.h"
>> +#include "hw/pci/pcie.h"
>> +#include "hw/pci/pcie_doe.h"
>> +#include "hw/pci/msi.h"
>> +#include "hw/pci/msix.h"
>> +
>> +/*
>> + * DOE Default Protocols (Discovery, CMA)
>> + */
>> +/* Discovery Request Object */
>> +struct doe_discovery {
>> +DOEHeader header;
>> +uint8_t index;
>> +uint8_t reserved[3];
>> +} QEMU_PACKED;
>> +
>> +/* Discovery Response Object */
>> +struct doe_discovery_rsp {
>> +DOEHeader header;
>> +uint16_t vendor_id;
>> +uint8_t doe_type;
>> +uint8_t next_index;
>> +} QEMU_PACKED;
>> +
>> +/* Callback for Discovery */
>> +static bool pcie_doe_discovery_rsp(DOECap *doe_cap)
>> +{
>> +PCIEDOE *doe = doe_cap->doe;
>> +struct doe_discovery *req = pcie_doe_get_req(doe_cap);
>> +uint8_t index = req->index;
>> +DOEProtocol *prot = NULL;
>> +
>> +/* Request length mismatch, discard */
>> +if (req->header.length < dwsizeof(struct doe_discovery)) {
>
> Use DIV_ROUND_UP instead of rolling your own thing.
>
>> +return DOE_DISCARD;
>> +}
>> +
>> +/* Point to the requested protocol */
>> +if (index < doe->protocol_num) {
>> +prot = >protocols[index];
>> +}
>
> What happens on else, should that still return DOE_SUCCESS?
>
>> +
>> +struct doe_discovery_rsp rsp = {
>> +.header = {
>> +.vendor_id = PCI_VENDOR_ID_PCI_SIG,
>> +.doe_type = PCI_SIG_DOE_DISCOVERY,
>> +.reserved = 0x0,
>> +.length = dwsizeof(struct doe_discovery_rsp),
>> +},
>
> mixed declarations are not allowed.
> DIV_ROUND_UP
>
>> +.vendor_id = (prot) ? prot->vendor_id : 0x,
>> +.doe_type = (prot) ? prot->doe_type : 0xFF,
>> +.next_index = (index + 1) < doe->protocol_num ?
>> + (index + 1) : 0,
>> +};
>
> I prefer:
> next_index = (index + 1) % doe->protocol_num
>
>> +
>> +
Re: [RFC PATCH v2 1/2] Basic PCIe DOE support
Have you/Jonathan come to consensus about which implementation is going forward?
I'd rather not have to review two :D
On 21-02-09 15:35:49, Chris Browy wrote:
> ---
> MAINTAINERS | 7 +
> hw/pci/meson.build| 1 +
> hw/pci/pcie.c | 2 +-
> hw/pci/pcie_doe.c | 414
> ++
> include/hw/pci/pci_ids.h | 2 +
> include/hw/pci/pcie.h | 1 +
> include/hw/pci/pcie_doe.h | 166
> include/hw/pci/pcie_regs.h| 4 +
> include/standard-headers/linux/pci_regs.h | 3 +-
> 9 files changed, 598 insertions(+), 2 deletions(-)
> create mode 100644 hw/pci/pcie_doe.c
> create mode 100644 include/hw/pci/pcie_doe.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 981dc92..4fb865e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1655,6 +1655,13 @@ F: docs/pci*
> F: docs/specs/*pci*
> F: default-configs/pci.mak
>
> +PCIE DOE
> +M: Huai-Cheng Kuo
> +M: Chris Browy
> +S: Supported
> +F: include/hw/pci/pcie_doe.h
> +F: hw/pci/pcie_doe.c
> +
> ACPI/SMBIOS
> M: Michael S. Tsirkin
> M: Igor Mammedov
> diff --git a/hw/pci/meson.build b/hw/pci/meson.build
> index 5c4bbac..115e502 100644
> --- a/hw/pci/meson.build
> +++ b/hw/pci/meson.build
> @@ -12,6 +12,7 @@ pci_ss.add(files(
> # allow plugging PCIe devices into PCI buses, include them even if
> # CONFIG_PCI_EXPRESS=n.
> pci_ss.add(files('pcie.c', 'pcie_aer.c'))
> +pci_ss.add(files('pcie_doe.c'))
It looks like this should be like the below line:
softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: pci_doe.c))
> softmmu_ss.add(when: 'CONFIG_PCI_EXPRESS', if_true: files('pcie_port.c',
> 'pcie_host.c'))
> softmmu_ss.add_all(when: 'CONFIG_PCI', if_true: pci_ss)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 1ecf6f6..f7516c4 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -735,7 +735,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>
> hotplug_event_notify(dev);
>
> -/*
> +/*
Please drop this.
> * 6.7.3.2 Command Completed Events
> *
> * Software issues a command to a hot-plug capable Downstream Port by
> diff --git a/hw/pci/pcie_doe.c b/hw/pci/pcie_doe.c
> new file mode 100644
> index 000..df8e92e
> --- /dev/null
> +++ b/hw/pci/pcie_doe.c
> @@ -0,0 +1,414 @@
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "qemu/range.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pcie.h"
> +#include "hw/pci/pcie_doe.h"
> +#include "hw/pci/msi.h"
> +#include "hw/pci/msix.h"
> +
> +/*
> + * DOE Default Protocols (Discovery, CMA)
> + */
> +/* Discovery Request Object */
> +struct doe_discovery {
> +DOEHeader header;
> +uint8_t index;
> +uint8_t reserved[3];
> +} QEMU_PACKED;
> +
> +/* Discovery Response Object */
> +struct doe_discovery_rsp {
> +DOEHeader header;
> +uint16_t vendor_id;
> +uint8_t doe_type;
> +uint8_t next_index;
> +} QEMU_PACKED;
> +
> +/* Callback for Discovery */
> +static bool pcie_doe_discovery_rsp(DOECap *doe_cap)
> +{
> +PCIEDOE *doe = doe_cap->doe;
> +struct doe_discovery *req = pcie_doe_get_req(doe_cap);
> +uint8_t index = req->index;
> +DOEProtocol *prot = NULL;
> +
> +/* Request length mismatch, discard */
> +if (req->header.length < dwsizeof(struct doe_discovery)) {
Use DIV_ROUND_UP instead of rolling your own thing.
> +return DOE_DISCARD;
> +}
> +
> +/* Point to the requested protocol */
> +if (index < doe->protocol_num) {
> +prot = >protocols[index];
> +}
What happens on else, should that still return DOE_SUCCESS?
> +
> +struct doe_discovery_rsp rsp = {
> +.header = {
> +.vendor_id = PCI_VENDOR_ID_PCI_SIG,
> +.doe_type = PCI_SIG_DOE_DISCOVERY,
> +.reserved = 0x0,
> +.length = dwsizeof(struct doe_discovery_rsp),
> +},
mixed declarations are not allowed.
DIV_ROUND_UP
> +.vendor_id = (prot) ? prot->vendor_id : 0x,
> +.doe_type = (prot) ? prot->doe_type : 0xFF,
> +.next_index = (index + 1) < doe->protocol_num ?
> + (index + 1) : 0,
> +};
I prefer:
next_index = (index + 1) % doe->protocol_num
> +
> +pcie_doe_set_rsp(doe_cap, );
> +
> +return DOE_SUCCESS;
> +}
> +
> +/* Callback for CMA */
> +static bool pcie_doe_cma_rsp(DOECap *doe_cap)
> +{
> +doe_cap->status.error = 1;
> +
> +memset(doe_cap->read_mbox, 0,
> + PCI_DOE_MAX_DW_SIZE * sizeof(uint32_t));
> +
> +doe_cap->write_mbox_len = 0;
> +
> +return DOE_DISCARD;
> +}
> +
> +/*
> + * DOE Utilities
> + */
> +static void pcie_doe_reset_mbox(DOECap *st)
> +{
> +st->read_mbox_idx = 0;
> +
> +st->read_mbox_len = 0;
> +st->write_mbox_len = 0;
> +
> +memset(st->read_mbox, 0,
