Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 2 May 2022 07:57:44 +0000
From:   Oliver Upton <oupton@google.com>
To:     Gavin Shan <gshan@redhat.com>
Cc:     kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
        eauger@redhat.com, Jonathan.Cameron@huawei.com,
        vkuznets@redhat.com, will@kernel.org, shannon.zhaosl@gmail.com,
        james.morse@arm.com, mark.rutland@arm.com, maz@kernel.org,
        pbonzini@redhat.com, shan.gavin@gmail.com
Subject: Re: [PATCH v6 03/18] KVM: arm64: Add SDEI virtualization
 infrastructure
Message-ID: <Ym+O+JLU5e9NUs39@google.com>
References: <YmMiyt/TDjJt0mdG@google.com>
 <36899ea9-e8bd-27b2-8dfb-75b76eab50d7@redhat.com>
 <YmRI7Bh7fWCYLUGT@google.com>
 <0e26da1a-00bb-3d63-a8bf-6cd3271b0a38@redhat.com>
 <Ymr45B+8xTlhi7vk@google.com>
 <96711526-c4f3-3b50-c015-beba8cc9fcc9@redhat.com>
 <Ym1EztjkJIHrg4Qz@google.com>
 <62f06a03-d6fc-3803-a2d2-7a85cf733459@redhat.com>
 <Ym9So9YariC0M7Zu@google.com>
 <2d631426-17fd-e7e3-5c62-eda547732bb7@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2d631426-17fd-e7e3-5c62-eda547732bb7@redhat.com>
Precedence: bulk

On Mon, May 02, 2022 at 03:25:40PM +0800, Gavin Shan wrote:
> Oliver, how about to adjust struct kvm_sdei_vcpu like below. With the
> changes, struct kvm_sdei_vcpu::unregistering is dropped, to match with
> the specification strictly.
> 
>    struct kvm_sdei_vcpu {
>        unsigned long registered;
>        unsigned long enabled;
>        unsigned long running;        // renamed from 'active' to match the specification strictly
>        unsigned long pending;        // event pending for delivery
>           :
>    };
> 
>    state                          @registered  @enabled  @running  @pending
>    --------------------------------------------------------------------------------
>    unregistered                   0            0         0/1       0
>    registered-disabled            1            0         0         0/1
>    registered-enabled             1            1         0/1       0/1
>    handler-running                0/1          0/1       1         0/1
> 
> We can use the specific encoding to represent the unregistration-pending.
> 
>    state                          @registered  @enabled  @running  @pending
>    -------------------------------------------------------------------------
>    handler-running                0            0          1        0

Right, this is what I had in mind. This encodes the
'handler-unregister-pending' state.

> Thanks for your valuable comments, Oliver. I'm not starting to work on
> v7 yet. I also would like to make everything clear before that. In that
> case, it will be easier for you to review next revision :)
> 
> > > > >           unsigned long pending;       /* the event is pending for delivery and handling */
> > > > >           unsigned long active;        /* the event is currently being handled           */
> > > > > 
> > > > >           :
> > > > >           <this part is just like what you suggested>
> > > > >       };
> > > > > 
> > > > > I rename @pending to @unregister. Besides, there are two states added:
> > > > > 
> > > > >      @pending: Indicate there has one event has been injected. The next step
> > > > >                for the event is to deliver it for handling. For one particular
> > > > >                event, we allow one pending event in the maximum.
> > > > 
> > > > Right, if an event retriggers when it is pending we still dispatch a
> > > > single event to the guest. And since we're only doing normal priority
> > > > events, it is entirely implementation defined which gets dispatched
> > > > first.
> > > > 
> > > 
> > > Yep, we will simply rely on find_first_bit() for the priority. It means
> > > the software signaled event, whose number is zero, will have the highest
> > > priority.
> > > 
> > > > >      @active:  Indicate the event is currently being handled. The information
> > > > >                stored in 'struct kvm_sdei_event_context' instance can be
> > > > >                correlated with the event.
> > > > 
> > > > Does this need to be a bitmap though? We can't ever have more than one
> > > > SDEI event active at a time since this is private to a vCPU.
> > > > 
> > > 
> > > Yes, one event is active at most on one particular vCPU. So tt don't
> > > have to be a bitmap necessarily. The reason I proposed to use bitmap
> > > for this state is to having all (event) states represented by bitmaps.
> > > In this way, all states are managed in a unified fashion. The alternative
> > > way is to have "unsigned long active_event", which traces the active
> > > event number. It also consumes 8-bytes when live migration is concerned.
> > > So I prefer a bitmap :)
> > > 
> > 
> > The small benefit of using the event number is that we can address all
> > events in 8 bytes, whereas we'd need to extend the bitmap for >64
> > events. I suppose we'll run into that issue either way, since the
> > pending, registered, and enabled portions are also bitmaps.
> > 
> > When live migration is in scope we should probably bark at userspace if
> > it attempts to set more than a single bit in the register.
> > 
> 
> Even it's unlikely to support the shared event, bitmap will help in that
> case. I'm not sure about other VMM, the pseudo firmware registers are
> almost transparent to user space in QEMU. They're accessed and no one
> cares the values reading from and writing to these registers in QEMU ;-)

Regardless of whether userspace actually manipulates the registers we
should still reject unsupported values. For example:

Let's say the VM is started on a kernel that introduced yet another SDEI
widget outside of your series. The VM was migrated back to an older
kernel w/o the SDEI widget, and as such the VMM attempts to set the
widget bit. Since the old kernel doesn't know what to do with the value
it should return EINVAL to userspace.

--
Thanks,
Oliver