Re: [RFC PATCH v2 1/2] Basic PCIe DOE support

[Jonathan Cameron]

On Thu, 18 Feb 2021 19:46:54 -0500
Chris Browy <cbrowy@avery-design.com> wrote:

> > On Feb 18, 2021, at 2:11 PM, Jonathan Cameron <jonathan.cameron@huawei.com> 
> > wrote:
> > 
> > On Fri, 12 Feb 2021 16:58:21 -0500
> > Chris Browy <cbrowy@avery-design.com> wrote:
> >   
> >>> On Feb 12, 2021, at 11:24 AM, Jonathan Cameron 
> >>> <Jonathan.Cameron@Huawei.com> wrote:
> >>> 
> >>> On Tue, 9 Feb 2021 15:35:49 -0500
> >>> Chris Browy <cbrowy@avery-design.com> 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 (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 
> >>   
> > ...
> >   
>