Re: [Xen-devel] [PATCH 14/18] PVH xen: Checks, asserts, and limitations for PVH

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.13 at 04:43, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 25 Jun 2013 10:54:15 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > --- a/xen/arch/x86/hvm/mtrr.c
>> > +++ b/xen/arch/x86/hvm/mtrr.c
>> > @@ -578,6 +578,9 @@ int32_t hvm_set_mem_pinned_cacheattr(
>> >  {
>> >      struct hvm_mem_pinned_cacheattr_range *range;
>> >  
>> > +    /* A PVH guest writes to MSR_IA32_CR_PAT natively. */
>> > +    ASSERT(!is_pvh_domain(d));
>> 
>> This can't be an assert, or did I overlook you preventing the
>> function to be called for PVH guests.
>> 
>> The comment would then be wrong too, as there is a path
>> leading here from a domctl (i.e. unaffected by how the guest
>> itself would access the MSR).
> 
> Well, there are no callers right now, and I wanted to catch any during
> my test runs. But, now I think the ASSERT should be replaced with
> returning  -ENOSYS. Let me know if you disagree.

An error return seems correct, but -ENOSYS doesn't seem the
best possible error code to correctly identify the kind of error.
-EOPNOTSUPP perhaps?

>> > --- a/xen/arch/x86/x86_64/traps.c
>> > +++ b/xen/arch/x86/x86_64/traps.c
>> > @@ -440,6 +440,8 @@ static long register_guest_callback(struct
>> > callback_register *reg) long ret = 0;
>> >      struct vcpu *v = current;
>> >  
>> > +    ASSERT(!is_pvh_vcpu(v));
>> > +
>> 
>> For one, I don't think there has been anything so far making
>> clear that this is unreachable for PVH.
> 
> hvm_do_hypercall() returns -ENOSYS for both callers of 
> register_guest_callback
> so this is unreachable for PVH. I can even remove the ASSERT if you'd
> like.
> 
>> And then it is inconsistent to do this here, but not also in
>> unregister_guest_callback().
> 
> I can add one here too, or remove the one from register.

Removing the one above would be my preference, but the
only requirement I have is that both cases should be consistent
with one another.

Jan


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