On Wed, Sep 21, 2022 at 06:47:21PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@xxxxxxxxxx> wrote: > > It means that using FP_STATE_TASK as a value for the fp_type > > member of the task struck recording what type of state is > > currently stored for the task is not valid, one of the other two > > values representing what was actually saved must be chosen. > Then this definitely represents something else, and shouldn't be a > state or a type, whatever you decide to call it in the end. There is > the state of the FP/SVE unit, and what some piece of SW wants to > save. They match in some cases, and differ in other (the TASK > value). I'd rather you encode them as them as different types to lift > the ambiguity. I did try the other way as well and found it was creating it's own problems - you end up with two almost identical enums which need to be separately named in some meaningful yet not excessively verbose fashion and have to remember which one to use in which context. > > > OK, how many discrete arguments are we going to pass to this function, > > > which most of them are part the vcpu structure? It really feels like > > > what you want is a getter for the per-cpu structure, and let the KVM > > > code do the actual business. If this function was supposed to provide > > > some level of abstraction, well, it's a fail. > > I agree that this is not an ideal interface, I am merely > > following the previously chosen idiom since I haven't been able > > to figure out why we were doing it in the first place and with a > > lot of these things it turns out that there's some actual reason. > Huh. If we're changing anything around this code, we'd better > understand what we are doing... I'm confident I understand well enough what it's doing, I'm just really unclear on why it's written the way it is. > > It's not even like fpsimd_bind_task_to_cpu() has ever been > > written in terms of this function, there's two parallel > > implementations. My best guess was that it was some combination > > of not peering at KVM internals and keeping struct > > fpsimd_last_state_struct internal to fpsimd.c (since we're > > effectively just passing one of those in in a more verbose form) > > but never anything solid enough to be sure. > Up to you, but adding extra parameters to this function really feels > like the wrong thing to do. Indeed, I'm not a big fan myself. I also don't want to get this and some other work I'm doing tied up on stylistic improvements to what's already there which seem likely introduce additional iterations - I'd rather loop back on this as a separate thing later.
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
