Re: [RFC PATCH 02/15] cxl/core/hdm: Bail on endpoint init fail

[Luis Chamberlain <mcgrof@kernel.org>]

On Thu, May 12, 2022 at 08:50:14AM -0700, Ben Widawsky wrote:
> On 22-04-18 16:37:12, Adam Manzanares wrote:
> > On Wed, Apr 13, 2022 at 02:31:42PM -0700, Dan Williams wrote:
> > > On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> > > > Endpoint decoder enumeration is the only way in which we can determine
> > > > Device Physical Address (DPA) -> Host Physical Address (HPA) mappings.
> > > > Information is obtained only when the register state can be read
> > > > sequentially. If when enumerating the decoders a failure occurs, all
> > > > other decoders must also fail since the decoders can no longer be
> > > > accurately managed (unless it's the last decoder in which case it can
> > > > still work).
> > > 
> > > I think this should be expanded to fail if any decoder fails to
> > > allocate anywhere in the topology otherwise it leaves a mess for
> > > future address translation code to work through cases where decoder
> > > information is missing.
> > > 
> > > The current approach is based around the current expectation that
> > > nothing is enumerating pre-existing regions, and nothing is performing
> > > address translation.
> > 
> > Does the qemu support currently allow testing of this patch? If so, it would 
> > be good to reference qemu configurations. Any other alternatives would be 
> > welcome as well. 
> > 
> > +Luis on cc.
> > 
> 
> No. This type of error injection would be cool to have, but I'm not sure of a
> good way to support that in a scalable way. Maybe Jonathan has some ideas?

In case it helps on the Linux front the least intrusive way is to use
ALLOW_ERROR_INJECTION(). It's what I hope we'll slowly strive for on
the block layer and filesystems slowly. That incurs one macro call per error
routine you want to allow error injection on.

Then you use debugfs to dynamically enable / disable the error
injection / rate etc.

So I think this begs the question, what error injection mechanisms
exist for qemu and would new functionality be welcomed?

Linux builds off a brilliantly simple simple interface borrowed from
failmalloc [0]. The initial implementation on Linux then was also really
simple [1] [2] [3] however it required adding stubs on each call with a
respective build option to enable failure injection. Configuration was done
through debugfs.

Later Josef enabled us to use BPF to allow overriding kprobed functions
to return arbitrary values[4], and further generalized away from kprobes
by Masami [5].

If no failure injection is present in qemu something as simple as the initial
approach could be considered [1] [2] [3], but a dynamic interface
would certainly be wonderful long term.

[0] http://www.nongnu.org/failmalloc/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=de1ba09b214056365d9082982905b255caafb7a2
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6ff1cb355e628f8fc55fa2d01e269e5e1bbc2fe9
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a8b6502fb669c3a0638a08955442814cedc86b1
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=92ace9991da08827e809c2d120108a96a281e7fc
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=540adea3809f61115d2a1ea4ed6e627613452ba1

  Luis

> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > ---
> > > >  drivers/cxl/core/hdm.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index bfc8ee876278..c3c021b54079 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -255,6 +255,8 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
> > > >                                       cxlhdm->regs.hdm_decoder, i);
> > > >                 if (rc) {
> > > >                         put_device(&cxld->dev);
> > > > +                       if (is_endpoint_decoder(&cxld->dev))
> > > > +                               return rc;
> > > >                         failed++;
> > > >                         continue;
> > > >                 }
> > > > --
> > > > 2.35.1
> > > >
> > >