Re: [Xen-devel] [PATCH 10/17] PVH xen: introduce vmx_pvh.c and pvh.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.13 at 02:40, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 02 May 2013 07:53:18 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 02.05.13 at 03:17, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > Ok, I redid it. Created a new function read_descriptor_sel() and
>> > rewrote vmx_pvh_read_descriptor(). Please lmk if looks ok to you.
>> > thanks a lot :
>> > 
>> > 
>> > static int read_descriptor_sel(unsigned int sel,
>> >                                enum sel_type which_sel,
>> >                                const struct vcpu *v,
>> >                                const struct cpu_user_regs *regs,
>> >                                unsigned long *base,
>> >                                unsigned long *limit,
>> >                                unsigned int *ar,
>> >                                unsigned int vm86attr)
>> > {
>> >     if ( is_pvh_vcpu(v) )
>> >         return hvm_read_descriptor(which_sel, v, regs, base, limit,
>> > ar);
>> 
>> Why not insert this into read_descriptor(), rather than creating a
>> new wrapper?
> 
> There are other callers of read_descriptor() which would need to be 
> unnecessaraly changed, we need PVH support for only one caller. So this
> seemed the least intrusive.

Ah, okay - that's fine then.

>> While secondary, I'm also a bit puzzled about the non-natural and
>> non-logical ordering (CS, DS, SS, GS, FS, ES)...
> 
> Not sure what the natural ordering is, so I sorted according to
> the enum x86_segment:

Yes, that's one of the three reasonable orderings now. The others
would be by register number or alphabetically.

> int vmx_pvh_read_descriptor(enum x86_segment selector, const struct vcpu *v,
>                             const struct cpu_user_regs *regs,
>                             unsigned long *base, unsigned long *limit,
>                             unsigned int *ar)
> {
>     unsigned int tmp_ar = 0;
>     ASSERT(v == current);
>     ASSERT(is_pvh_vcpu(v));
> 
>     switch ( selector )
>     {
>     case x86_seg_cs:
>         *base = __vmread(GUEST_CS_BASE);
>         *limit = __vmread(GUEST_CS_LIMIT);
>         tmp_ar = __vmread(GUEST_CS_AR_BYTES);
>         break;
> 
>     case x86_seg_ss:
>         *base = __vmread(GUEST_SS_BASE);
>         *limit = __vmread(GUEST_SS_LIMIT);
>         tmp_ar = __vmread(GUEST_SS_AR_BYTES);
>         break;
> 
>     case x86_seg_ds:
>         *base = __vmread(GUEST_DS_BASE);
>         *limit = __vmread(GUEST_DS_LIMIT);
>         tmp_ar = __vmread(GUEST_DS_AR_BYTES);
>         break;
> 
>     case x86_seg_es:
>         *base = __vmread(GUEST_ES_BASE);
>         *limit = __vmread(GUEST_ES_LIMIT);
>         tmp_ar = __vmread(GUEST_ES_AR_BYTES);
>         break;
> 
>     case x86_seg_fs:
>         *base = __vmread(GUEST_FS_BASE);
>         *limit = __vmread(GUEST_FS_LIMIT);
>         tmp_ar = __vmread(GUEST_FS_AR_BYTES);
>         break;
> 
>     case x86_seg_gs:
>         *base = __vmread(GUEST_GS_BASE);
>         *limit = __vmread(GUEST_GS_LIMIT);
>         tmp_ar = __vmread(GUEST_GS_AR_BYTES);
>         break;
> 
>     default:
>         gdprintk(XENLOG_WARNING, "Unmatched segment selector:%d\n", selector);

This message is now stale and hence confusing.

And with the way the function is now I don't see why at least the
whole switch can't be dropped, and the function instead call
vmx_get_segment_register(); perhaps that could even be done
in vendor independent code, calling hvm_get_segment_register().

>         return 0;
>     }
> 
>     if ( (tmp_ar & X86_SEG_AR_CS_LM_ACTIVE) && selector < x86_seg_fs  )

This is still wrong. As said before you need to look as the _CS_
access rights, not the ones of the selector register you read.

But as also hinted at - do you really need the override at all?

Jan

>     {
>         *base = 0UL;
>         *limit = ~0UL;
>     }
> 
>     /* Fix ar so that it looks the same as in native mode */
>     *ar = (tmp_ar << 8);
> 
>     return 1;
> }



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