Hi Oliver,
On 5/2/22 3:57 PM, Oliver Upton wrote:
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.
Cool, Thanks for your confirm. I think we're on same page for the
data structures now. With this, I'm able to start working on next
revision. Oliver, I'm sorry for taking you too much time to reach
to the point :)
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.
Yep, agreed. Thanks for the examples and details. Lets have more
discussion when the series to support migration is posted.
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
