Re: [Xen-devel] [V10 PATCH 12/23] PVH xen: Support privileged op emulation for PVH

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Wed, 7 Aug 2013 14:49:50 +0100
George Dunlap <dunlapg@xxxxxxxxx> wrote:

> On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
> <mukesh.rathor@xxxxxxxxxx> wrote:
....
> >
> >  const struct hvm_function_table * __init start_vmx(void)
> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index a3ca70b..fe8b94c 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -480,6 +480,10 @@ static unsigned int
> > check_guest_io_breakpoint(struct vcpu *v, unsigned int width, i,
> > match = 0; unsigned long start;
> >
> > +    /* PVH fixme: support io breakpoint. */
> > +    if ( is_pvh_vcpu(v) )
> > +        return 0;
> 
> Does this one, and the check to IO below, have anything to do with
> privileged op emulation?

Yes, it's called from emulate_privileged_op(). 

...

> > +static int read_descriptor_sel(unsigned int sel,
> > +                               enum x86_segment which_sel,
> > +                               struct vcpu *v,
> > +                               const struct cpu_user_regs *regs,
> > +                               unsigned long *base,
> > +                               unsigned long *limit,
> > +                               unsigned int *ar,
> > +                               unsigned int vm86attr)
> > +{
> > +    struct segment_register seg;
> > +    bool_t long_mode;
> > +
> > +    if ( !is_pvh_vcpu(v) )
> > +        return read_descriptor(sel, v, regs, base, limit, ar,
> > vm86attr);
> 
> Again, wouldn't it be better to rename read_desrciptor to
> pv_read_descriptor(), name this one pvh_read_desrciptor(), give them a
> similar function signature (e.g., have both take a which_sel and have
> it look up the selector itself), rather than have this
> one-function-calls-another-function thing?

If you go back to where we discussed this in previous reviews, it 
is being done this way because of other callers of read_descriptor
that don't need to be changed to pass enum x86_segment.

> >      int user_mode = !(v->arch.flags & TF_kernel_mode);
> >  #define TOGGLE_MODE() if ( user_mode ) toggle_guest_mode(v)
> >
> > +    /*
> > +     * For PVH we check this in vmexit for
> > EXIT_REASON_IO_INSTRUCTION
> > +     * and so don't need to check again here.
> > +     */
> > +    if ( is_pvh_vcpu(v) )
> > +        return 1;
> 
> Same question re IO emulation.

Same answer.

> > + * We need vcpu because during context switch, going from PV to
> > PVH,
> > + * in save_segments() current has been updated to next, and no
> > longer pointing
> > + * to the PV, but the intention is to get selector for the PV.
> > Checking
> > + * is_pvh_vcpu(current) will yield incorrect results in such a
> > case.
> > + */
> > +#define read_segment_register(vcpu, regs, name)                   \
> > +({  u16 __sel;                                                    \
> > +    struct cpu_user_regs *_regs = (regs);                         \
> > +                                                                  \
> > +    if ( is_pvh_vcpu(vcpu) && guest_mode(_regs) )                 \
> > +        __sel = pvh_get_selector(vcpu, x86_seg_##name);           \
> > +    else                                                          \
> > +        asm volatile ( "movw %%" #name ",%0" : "=r" (__sel) );    \
> 
> Is there a reason you discarded the STR() macro here?

Suggested by Jan to change it, not sure the reason. Jan do you recall?

-Mukesh


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel