On Mon, 2020-12-14 at 15:20 +0000, Joao Martins wrote:
> On 12/14/20 2:58 PM, David Woodhouse wrote:
> > On Mon, 2020-12-14 at 13:29 +0000, Joao Martins wrote:
> > > We might be missing the case where only shared_info is registered. Something like:
> > > 
> > >          if (vcpu->xen.shinfo_set && !vcpu->xen.vcpu_info_set) {
> > >                  offset = offsetof(struct compat_vcpu_info, time);
> > >                  offset += offsetof(struct shared_info, vcpu_info);
> > >                  offset += (v - kvm_get_vcpu_by_id(0)) * sizeof(struct compat_vcpu_info);
> > >                  
> > >                  kvm_setup_pvclock_page(v, &vcpu->xen.shinfo_cache, offset);
> > >          }
> > > 
> > > Part of the reason I had a kvm_xen_setup_pvclock_page() was to handle this all these
> > > combinations i.e. 1) shared_info && !vcpu_info 2) vcpu_info and unilaterially updating
> > > secondary time info.
> > > 
> > > But maybe introducing this xen_vcpu_info() helper to accommodate all this.
> > 
> > There was complexity.
> > 
> > I don't like complexity.
> > 
> > I made it go away.
> > 
> 
> Considering what you said earlier, yes, it would be unnecessary complexity.

To be fair I don't really make it go away completely; I push it up into
userspace. Which now has to keep track of whether a given vCPU has an
explicitly-set vcpu_info page, or whether it has just used the default
one in the shared_info.

And if the shared_info *changes*, as it might on a kexec or just guest
weirdness, the VMM should ideally change only those vCPUs which were at
the default location.

When it was just v->vcpu_info?:shinfo->vcpu_info[N] in the kernel that
part was slightly simpler. It was just slightly hampered by the fact
that the kernel has no way of knowing what N should be :)