Re: [Xen-devel] [PATCH v3] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

Jan,

On 12.06.19 10:27, Jan Beulich wrote:
> > Well, my point here is to left it as is, maybe add more documentation. If
> > one likes shooting his leg, we should only care about avoiding ricochet harms
> > hypervisor or other guests.
> > If you disagree, please suggest your interaction model, I'll be happy to
> > implement it.
> Well, if "mix as you like" is fine for guests to follow, then okay. But
> we need to be _really_ certain there's no issue with this.
I'm not aware about potential problems from the guest side. Do you have any 
ideas about it?

> Relaxing
> the interface is always possible, while tightening an interface is
> almost always at least a problem, if possible at all.
True.


> > > In a prior version you did the mechanical part of adjusting the VA-based
> > > code in a prereq patch, aiding review. Is there a particular reason you
> > > folded everything into one patch now?
> > I silently followed suggestion from George [1]. Any objections?
> Hmm, I can't read this into George's suggestion. Aiui he did suggest
> not to split the definition of the new interface from its implementation.
> But that doesn't necessarily mean to squash _everything_ in one
> patch.
OK.
It looks that what you said firstly is closer to V1 of this stuff. Will keep 
this in mind for the next version.


> Well, I'm afraid I don't understand what you're after. Of course
> compat mode guests need to continue to be supported, and the
> new interface would also better be available to them. And it is
> a fact that their runstate area layout differs from that of 64-bit
> guests.
OK.

> > > > --- a/xen/include/xen/sched.h
> > > > +++ b/xen/include/xen/sched.h
> > > > @@ -163,17 +163,31 @@ struct vcpu
> > > >        void            *sched_priv;    /* scheduler-specific data */
> > > >    
> > > >        struct vcpu_runstate_info runstate;
> > > > +
> > > > +    enum {
> > > > +        RUNSTATE_NONE = 0,
> > > > +        RUNSTATE_PADDR = 1,
> > > > +        RUNSTATE_VADDR = 2,
> > > > +    } runstate_guest_type;
> > > > +
> > > > +    unsigned long runstate_in_use;
> > > Why "unsigned long"? Isn't a bool all you need?
> > Bool should be enough. But it seems we will have a lock here.
> >
> > > Also these would now all want to be grouped in a sub-structure named
> > > "runstate", rather than having "runstate_" prefixes.
> > Member `runstate` has already a type of `struct vcpu_runstate_info` which is
> > an interface type.
> > `runstate_guest` is a union. I'd not like moving `runstate_guest` union into
> > another substructure. Because we would have long lines like
> > `v->struct.runstate_guest.virt.p->state_entry_time`.
> You didn't get my point then: What I'm after is
>
>      struct {
>          struct vcpu_runstate_info info;
>          enum {
>              RUNSTATE_NONE,
>              RUNSTATE_PADDR,
>              RUNSTATE_VADDR,
>          } guest_type;
>          bool in_use;
>      } runstate;
Did you miss runstate_guest as a member of that struct?

--
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel