On Thu, Oct 21, 2021 at 05:39:34PM +0200, Paolo Bonzini wrote:
> On 06/10/21 22:37, Michael Roth wrote:
> > +struct sev_sync_data {
> > + uint32_t token;
> > + bool pending;
> > + bool done;
> > + bool aborted;
> > + uint64_t info;
> > +};
> > +
>
> Please add a comment explaining roughly the design and what the fields are
> for. Maybe the bools can be replaced by an enum { DONE, ABORT, SYNC,
> RUNNING } (running is for pending==false)?
That makes sense. And SYNC is basically pending==true.
Though since it would be the test code calling sev_check_guest_sync() that
sets pending to true after each sync, "RUNNABLE" might be a better name
since whether it's actually running depends on a separate call to
vcpu_run().
>
> Also, for the part that you can feel free to ignore: this seems to be
> similar to the ucall mechanism. Is it possible to implement the ucall
> interface in terms of this one (or vice versa)?
>
> One idea could be to:
>
> - move ucall to the main lib/ directory
>
> - make it use a struct of function pointers, whose default implementation
> would be in the existing lib/ARCH/ucall.c files
>
> - add a function to register the struct for the desired implementation
>
> - make sev.c register its own implementation
That seems doable. We'd also need to register an opaque pointer that, in the
case of SEV, would be for the shared page used for syncing. The guest would
also need the pointers for the ops/opaque, but ucall_init() could
additionally set some globals to provide the GVAs for them in the guest
environment.
Will look at that for the next spin.
>
> Thanks,
>
> Paolo
>
