Re: [PATCH v2 1/1] pcie: Add hotplug detect state register to cmask

[Leonardo Bras Soares Passos <leobras@redhat.com>]

On Thu, Jul 6, 2023 at 5:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jul 06, 2023 at 03:02:07PM -0400, Peter Xu wrote:
> > On Thu, Jul 06, 2023 at 02:50:20PM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jul 06, 2023 at 02:14:37PM -0400, Peter Xu wrote:
> > > > On Thu, Jul 06, 2023 at 03:07:40PM -0300, Leonardo Bras Soares Passos wrote:
> > > > > > I asked the same question, and I still keep confused: whether there's a
> > > > > > first bad commit?  Starting from when it fails?
> > > > > >
> > > > > > For example, is this broken on 6.0 binaries too with pc-q35-6.0?
> > > > >
> > > > > I tested for qemu 6.0, and it still reproduces, but have not pursued
> > > > > this any further.
> > > >
> > > > I see, thanks!
> > > >
> > > > But then do you know why it's never hit before?  I assume it means this bug
> > > > has been there for a long time.
> > >
> > > It's a race - you have to migrate after the bit has been set before
> > > the bit got cleared.
> > > cmask is exactly for bits that qemu modifies itself.
> >
> > Michael, do you mean that Leo's patch is wrong?
>
>
> I mean his patch is exactly right. cmask was designed with this
> kind of use case in mind.
> Will queue.

Thanks Michael!

Any chance this will get in on time for v8.1 ?

>
> > I just got understood why it got cleared - I think Leo didn't mention that
> > the device was actually offlined before migration, IIUC that's why the PDS
> > bit got cleared, if PDS was trying to describe that of the slot.
> >
> > According to:
> >
> >     /* Used to enable checks on load. Note that writable bits are
> >      * never checked even if set in cmask. */
> >     uint8_t *cmask;
> >
> > It does sound reasonable to me to have PDS cleared when device offlined.
> > Since hypervisor doesn't really know what the condition the slot presence
> > bit would be when migrating, it seems we should just clear the bit in
> > cmask.
> >
> > So with the last reply from Leo, the patch looks all right to me.  It's
> > just that as Leo mentioned, we should mention the offline process if that's
> > the case, because that's definitely an important step to reproduce the issue.
> >
> > Thanks,
>
> If you want to suggest more text to the commit log, for the benefit
> of backporters, that is fine by me.
>
> > --
> > Peter Xu
>