Re: [Xen-devel] [V11 PATCH 20/21] PVH xen: introduce vmexit handler for PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.08.13 at 02:35, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Fri, 23 Aug 2013 10:12:16 +0100 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 23.08.13 at 03:19, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
>> > +static int vmxit_msr_read(struct cpu_user_regs *regs)
>> > +{
>> > +    u64 msr_content = 0;
>> > +
>> > +    switch ( regs->ecx )
>> 
>> Did you mean regs->_ecx?
> 
> Hmm.. don't understand why? HVM uses ecx:
> 
>   hvm_msr_read_intercept(regs->ecx, &msr_content) == X86EMUL_OKAY )

And the declaration of that function is

int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content);

i.e. the 64-bit regs->ecx correctly gets truncated to 32 bits while
passing arguments to the function.

We've had quite a few similar bugs in the code in the past, so I'd
really appreciate if you could avoid introducing similar ones again.

>> > +    default:
>> > +        /* PVH fixme: see hvm_msr_read_intercept(). */
>> > +        rdmsrl(regs->ecx, msr_content);
>> 
>> So what does this comment refer to? There's no change to the
>> referred to function here. And it seems rather questionable that
>> reading the physical MSR values for everything but
>> MSR_IA32_MISC_ENABLE is correct/secure. I appreciate the
>> "fixme" annotation, but I'm afraid this is not sufficient here.
> 
> Yes, it needs to be revisited, best with AMD port so that a good
> solution can be contrived for PVH.

Nice that you say "yes" here, but the request was to make the
comment understandable to others than just you.

>> > +{
>> > +    int vector = (__vmread(VM_EXIT_INTR_INFO)) &
>> > INTR_INFO_VECTOR_MASK;
>> > +    int rc = -ENOSYS;
>> > +
>> > +    dbgp1(" EXCPT: vec:%#x cs:%#lx rip:%#lx\n", vector,
>> > +          __vmread(GUEST_CS_SELECTOR), regs->eip);
>> 
>> Do you continue to have these funny dbgp constructs in here. Are
>> they supposed to go away before this gets committed? If not,
>> please use a model similar to HVM_DBG_LOG().
> 
> Like the commit log says, it helps debug, but can be removed anytime.
> I left it there thinking it might be useful for first couple months
> while it gets thoroughly tested.

And as said - I don't mind it left there if done properly.

Jan


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