Re: [Xen-devel] [PATCH 1/3] Revert "x86/vvmx: fix virtual interrupt injection when Ack on exit control is used"

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 24, 2020 at 08:49:27AM +0000, Tian, Kevin wrote:
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: Tuesday, March 24, 2020 4:10 PM
> > 
> > On 24.03.2020 06:41, Tian, Kevin wrote:
> > >> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > >> Sent: Monday, March 23, 2020 10:49 PM
> > >>
> > >> On Mon, Mar 23, 2020 at 09:09:59AM +0100, Jan Beulich wrote:
> > >>> On 20.03.2020 20:07, Roger Pau Monne wrote:
> > >>>> This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458.
> > >>>>
> > >>>> The commit is wrong, as the whole point of nvmx_update_apicv is to
> > >>>> update the guest interrupt status field when the Ack on exit VMEXIT
> > >>>> control feature is enabled.
> > >>>>
> > >>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > >>>
> > >>> Before anyone gets to look at the other two patches, should this
> > >>> be thrown in right away?
> > >>
> > >> I would like if possible get a confirmation from Kevin (or anyone
> > >> else) that my understanding is correct. I find the nested code very
> > >> confusing, and I've already made a mistake while trying to fix it.
> > >> That being said, this was spotted by osstest as introducing a
> > >> regression, so I guess it's safe to just toss it in now.
> > >>
> > >> FWIW patch 2/3 attempts to provide a description of my understanding
> > >> of how nvmx_update_apicv works.
> > >>
> > >
> > > I feel it is not good to take this patch alone, as it was introduced to 
> > > fix
> > > another problem. W/o understanding whether the whole series can
> > > fix both old and new problems, we may risk putting nested interrupt
> > > logic in an even worse state...
> > 
> > Well, okay, I'll wait then, but it would seem to me that reverting
> > wouldn't put us in a worse state than we were in before that change
> > was put in.
> 
> Roger needs to make the call, i.e. which problem is more severe, old or
> new one.

AFAICT those problems affect different kind of hardware, so it doesn't
make much difference. IMO f96e1469ad06b6 should be reverted because
it's plain wrong, and albeit it seemed to fix the issue in one of my
boxes it was just luck.

I don't thinks it's going to make matters worse, as osstest is already
blocked, but reverting it alone without the rest of the series is not
going to unblock osstest either. If you agree it's wrong I think it
could go in now, even if it's just to avoid having to repost it with
newer versions of the series.

Thanks, Roger.