Re: [PATCH v3 07/32] xen/x86: fix arch_set_info_guest for HVM guests

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-07-23 at 05:29 -0600, Jan Beulich wrote:
> > 
> > > > On 23.07.15 at 12:25, <roger.pau@citrix.com> wrote:
> > El 13/07/15 a les 16.01, Jan Beulich ha escrit:
> > > > > > On 03.07.15 at 13:34, <roger.pau@citrix.com> wrote:
> > > > --- a/xen/arch/x86/domain.c
> > > > +++ b/xen/arch/x86/domain.c
> > > > @@ -795,6 +795,15 @@ int arch_set_info_guest(
> > > >                c.nat->fs_base || c.nat->gs_base_user)) )
> > > >              return -EINVAL;
> > > >      }
> > > > +    else if ( is_hvm_domain(d) )
> > > > +    {
> > > > +        if ( c(ctrlreg[0]) || c(ctrlreg[1]) || c(ctrlreg[2]) 
> > > > ||
> > > > +             c(ctrlreg[3]) || c(ctrlreg[4]) || c(ctrlreg[5]) 
> > > > ||
> > > > +             c(ctrlreg[6]) || c(ctrlreg[7]) || c(ldt_base) ||
> > > > +             c(ldt_ents) || c(kernel_ss) || c(kernel_sp) ||
> > > > +             c(gdt_ents) )
> > > > +            return -EINVAL;
> > > > +    }
> > > 
> > > In addition to what Andrew said - is the use of c() here really 
> > > correct
> > > considering
> > > 
> > >     compat = is_pv_32bit_domain(d);
> > > 
> > > #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
> > > 
> > > near the beginning of the function?
> > 
> > I've been thinking about this. Since HVM vCPUs are always started 
> > in
> > 32bit mode, I would ideally like to use the 
> > vcpu_guest_context_x86_32_t
> > struct to set the vCPU context. This means that for HVM guests 
> > "compat"
> > should always be true.
> > 
> > The problem is that inside of the guest there's no notion of
> > vcpu_guest_context_x86_32_t or vcpu_guest_context_x86_64_t, there's 
> > only
> > vcpu_guest_context, which defaults to the native bitness of the 
> > guest.
> > So if your guest is running in 64bit mode vcpu_guest_context is 
> > aliased
> > to vcpu_guest_context_x86_64_t by default.
> > 
> > TBH I'm not sure about the best way to solve this. Should
> > vcpu_guest_context_x86_32_t and vcpu_guest_context_x86_64_t be 
> > exposed
> > to the guest like it's done for the tools?
> 
> Why? We're in arch_set_info_guest(), which is unreachable for a
> HVM guest on itself. Or is this in preparation of allowing HVM
> guests to use VCPUOP_initialise? In that case, exposing might be
> an option, but remember that the compat mode layout even today
> is used only for PV guests. So I'd rather avoid exposing both
> layouts to guests, and do translation instead inside the hypervisor.

On ARM we deliberately arranged that the vcpu_guest_context was the
same for both arm32 and arm64. Moving to PVH seems like an ideal
opportunity to do something similar for x86, if not by standardising on
the x86_64 layout then by declaring a new struct which can encompass
both and declaring the old ones PV legacy.

Ian.