lkml.org 
[lkml]   [2023]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check
    Jonathan Cameron wrote:
    > On Thu, 2 Mar 2023 08:36:59 -0700
    > Dave Jiang <dave.jiang@intel.com> wrote:
    >
    > > On 3/1/23 11:23 PM, Fan Ni wrote:
    > > > On Wed, Mar 01, 2023 at 11:54:08AM -0700, Dave Jiang wrote:
    > > >>
    > > > Hi Dave,
    > > > Thanks for looking into this.
    > > >>
    > > >> On 2/28/23 3:40 PM, Fan Ni wrote:
    > > >>> Add COMMIT field check aside with existing COMMITTED field check during
    > > >>> hdm decoder initialization to avoid a system crash during module removal
    > > >>> after destroying a region which leaves the COMMIT field being reset while
    > > >>> the COMMITTED field still being set.
    > > >>
    > > >> Hi Fan. Are you seeing this issue on qemu emulation or hardware? The
    > > > I run into the issue with qemu emulation.
    > > >> situation does not make sense to me. If we clear the COMMIT bit, then the
    > > >> COMMITTED bit should be cleared by the hardware shortly after right?
    > > >
    > > > From the spec, I cannot find any statement saying clearing the COMMIT bit
    > > > will automatically clear the COMMITTED. If I have not missed the statement in
    > > > the spec, I assume we should not make the assumption that it will be
    > > > cleared automatically for real hardware. But you may be right, leaving the
    > > > COMMITTED bit set can potentially cause some issue? Need to check more.
    > >
    > > I have not been able to find direct verbiage that indicates this either.
    > > However, logically it would make sense. Otherwise, the COMMITTED field
    > > never clears and prevents reprogramming of the HDM decoders. The current
    > > QEMU implementation is creating a situation where the HDM decoder is
    > > always active after COMMIT bit is set the first time, regardless whether
    > > COMMIT field has been cleared later on during a teardown. It does sound
    > > like a bug with QEMU emulation currently.
    >
    > I agree that one sane interpretation is that unsetting commit should result in
    > the decoder being deactivated and hence the commit bit dropping. However
    > I'm not sure that's the only sane interpretation.
    >
    > There is no verbage that I'm aware of that says the committed bit being
    > set means that the current register values are in use. It simply says that
    > when the commit bit was set, the HDM decoder was successfully committed
    > (using registers as set at that time). There is a specific statement about
    > not changing the registers whilst checks are in progress, but those checks
    > are only required if lock on commit is set, so it doesn't cover this case.
    >
    > Wonderfully there isn't actually anything says what a commit transition to 0
    > means. Does that result in the decoder become uncommitted, or does that only
    > happen when the next 0 to 1 transition happens?
    >
    > The only stuff we have is what happens when lock on commit = 1, which isn't
    > the case here.
    >
    > So is there another valid implementation? I think yes.
    > In some implementations, there will be a complex state machine that is
    > triggered when commit is set. That will then write some entirely invisible
    > internal state for decode logic based on the contents of the registers.
    > As such, once it's set committed, it typically won't look at the registers
    > again until another commit 0->1 transition happens.
    > At that point the
    > committed bit drops and raised again once the commit state machine finishes
    > (given QEMU doesn't emulate that delay the upshot is if you set commit then
    > check committed it will be set ;)

    I'm only barely following along so I wanted to make sure I understand...

    Are you saying that at the instant commit 0->1 happens hardware will clear
    commited to 0 so that software can later check for commited vs error not
    commited?

    Ira

    >
    > In that implementation the commit 1->0 transition is an irrelevance and
    > it won't change the committed bit state.
    >
    > So whilst the QEMU code is doing the less obvious implementation, I think
    > the spec still allows it. I don't mind QEMU changing to the more obvious
    > one though if someone wants to send a patch.
    >
    > Jonathan
    >

    [...]

    \
     
     \ /
      Last update: 2023-03-27 00:43    [W:8.800 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site