Re: [Xen-devel] [PATCH 02/18] PVH xen: add XENMEM_add_to_physmap_range

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.13 at 00:19, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 06 Jun 2013 07:43:09 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 05.06.13 at 22:41, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> wrote:
>> > On Wed, 05 Jun 2013 08:32:52 +0100
>> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> > 
> ..........
>> +        if ( rc == -EAGAIN )
>> +            rc = hypercall_create_continuation(
>> +                __HYPERVISOR_memory_op, "ih", op, arg);
>> +
>> +        return rc;
>> +    }
>> +
>>      case XENMEM_set_memory_map:
>>      {
>>          struct xen_foreign_memory_map fmap;
>> 
>> If the hypercall were handled before, adding a new case statement
>> to arch_memory_op() would cause a compilation error. All that was
> 
> No, not really. The hcall was handled by xen by returning -ENOSYS. 

That's a completely bogus argument imo.

>> there before this patch was the definition of the hypercall (for ARM),
>> but I'm quite strongly opposed to adding x86 support for this
>> hypercall only for one half of the possible set of PV (and HVM?)
>> guests; the fact that PVH is 64-bit only for the moment has nothing
>> to do with this. The only alternative would be to constrain the
>> specific sub-hypercall to PVH, but that would be rather contrived,
>> so I'm already trying to make clear that I wouldn't accept such a
>> solution to the original comment.
> 
> Let me add my perpective: 
> 
> 32bit HVM and PV guest:
>    Before my patch, if a call is made with XENMEM_add_to_physmap_range,
>    which it can since its an existing hcall, it will come into 
>    compat_arch_memory_op() which will return -ENOSYS.
> 
>    After my patch, it will do the same, return -ENOSYS. 
> 
> So how does this patch cause a regression for existing 32bit 
> guests?

I never said it's a regression. It's incomplete functionality you add.
Someone adding a use of this to PV or PV drivers, testing it on
64-bit, may validly expect that this would work the same on 32-bit.

> Moreover, in the past, you've asked to remove even the simplest
> benign line space change because it was unrelated to the patch. So changing
> something as part of PVH patchset for PV and HVM guests, wouldn't that 
> be unrelated and contradictory to the message you've sent? 

In no way - I'm simply asking for consistency here. If you handled
the hypercall _only_ for PVH guests, and left the implementation
for 32-bit PVH out entirely because such guests can't work anyway
with the patch set in place, that would be consistent. But as said
before: This would artificially limit a hypercall, and that's a
separate reason not to accept such a partial implementation.

> You've found some very genunie issues in the patchset, and I really 
> appreciate that. But I struggle with this request.

Then leave it out, and I'll waste my time on getting it implemented
once the patch set is in. But please add a clear note of this state
to the patch description.

Jan


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