Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Jan Beulich]

>>> On 21.09.16 at 19:09,  wrote:
> On Wed, Sep 21, 2016 at 2:56 PM, Jan Beulich  wrote:
>> Again this looks like much clutter with little benefit to me, i.e. I'd
>> then rather go with the unmodified original proposal. That's largely
>> because nothing really enforces anyone to use the (disconnected)
>> xen_dmop_foo_entry type. I.e. it continues to be a matter of the
>> programmer's and the reviewers' attention/discipline.
> 
> But is "Must use hypercall dmop, subop foo with struct dmop_foo" any
> different than "Must use hypercall bar with struct bar"?
> 
> In theory there could be a mismatch between the struct libxc expected
> to use for a whole hypercall with the struct Xen expected to find for
> an entire hypercall. But in practice that never happens because we set
> up the call with the appropriate struct once and then never change it.

Yes.

> (That is, we may change the struct elements, but not the name.)  This
> seems to me like a fairly similar case.

I don't think so - what I'm talking about here is the equivalent of
"we may change the struct elements". Such changes would go
unnoticed by the compiler the respective pseudo-struct-element
is a handle.

But anyway - I think we've beaten this horse to death, and I'm the
only one worried about type issues here. Therefore I think we
should just stop this discussion and go with the proposed model.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[George Dunlap]

On Wed, Sep 21, 2016 at 2:56 PM, Jan Beulich  wrote:
> Again this looks like much clutter with little benefit to me, i.e. I'd
> then rather go with the unmodified original proposal. That's largely
> because nothing really enforces anyone to use the (disconnected)
> xen_dmop_foo_entry type. I.e. it continues to be a matter of the
> programmer's and the reviewers' attention/discipline.

But is "Must use hypercall dmop, subop foo with struct dmop_foo" any
different than "Must use hypercall bar with struct bar"?

In theory there could be a mismatch between the struct libxc expected
to use for a whole hypercall with the struct Xen expected to find for
an entire hypercall. But in practice that never happens because we set
up the call with the appropriate struct once and then never change it.
(That is, we may change the struct elements, but not the name.)  This
seems to me like a fairly similar case.

Note that I'm not arguing for the extra "clutter" per se, I just think
that it will be pretty effective in mitigating the type safety issue.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, re 
qemu depriv) [and 1 more messages]"):
> On 21.09.16 at 15:24, <ian.jack...@eu.citrix.com> wrote:
> > Would this be enough of an improvement, for you, to be worth the
> > additional type name clutter etc. ?
> 
> Again this looks like much clutter with little benefit to me, i.e. I'd
> then rather go with the unmodified original proposal. That's largely
> because nothing really enforces anyone to use the (disconnected)
> xen_dmop_foo_entry type. I.e. it continues to be a matter of the
> programmer's and the reviewers' attention/discipline.

Fair enough.

Thanks for your opinions.

Regards,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Jan Beulich]

>>> On 21.09.16 at 15:24, <ian.jack...@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, 
> re qemu depriv) [and 1 more messages]"):
>> On 21.09.16 at 14:23, <ian.jack...@eu.citrix.com> wrote:
>> > But changes in the contents of the specific struct /will/ be spotted.
>> 
>> As long as it is a structure, yes. What about someone changing
>> uint64_t to xen_pfn_t?
> 
> You mean if someone changes one of the buffers from "array of
> uint64_t" to "array of xen_pfn_t", so that the ABI changes on some but
> not all architectures ?
> 
> We could declare a rule that a dmop buffer may contain only a struct
> or type name specific to that dmop (or an array of such), and must
> always be accessed through a copy to or from such a type.
> 
> The rule would mean that if we had a dmop which wanted to call, using
> one of the buffers, a function like this:
> 
>  int some_function(uint64_t *pfns, size_t n_pfns);
> 
> You'd have to write the dmop definition like this:
> 
>  typedef uint64_t xen_dmop_foo_entry;
> 
>  xen_dmop_foo_entry *pfns;
>  size_t n_pfns;
> 
>  ...allocate table somehow...
>  ret = copy_dm_buffer_from_guest(pfns,
>  sizeof(pfns[0])*n_pfns,
>  buffers, nr_buffers, 1);
>  ...
>  ret = some_function(pfns, n_pfns);
> 
> And similar code, again using xen_dmop_foo_entry. on the calling side.
> Then if the buffer entry type is changed to xen_pfn_t you change it
> to:
> 
>  typedef xen_pfn_t xen_dmop_foo_entry;
> 
> Now, unless the rest of the code is changed too, the compiler will
> warn that a xen_pfn_t* cannot be converted to a uint64_t* in the call
> to some_function.  (And likewise at the hypercall site in libxc.)
> 
> Would this be enough of an improvement, for you, to be worth the
> additional type name clutter etc. ?

Again this looks like much clutter with little benefit to me, i.e. I'd
then rather go with the unmodified original proposal. That's largely
because nothing really enforces anyone to use the (disconnected)
xen_dmop_foo_entry type. I.e. it continues to be a matter of the
programmer's and the reviewers' attention/discipline.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, re 
qemu depriv) [and 1 more messages]"):
> On 21.09.16 at 14:23, <ian.jack...@eu.citrix.com> wrote:
> > But changes in the contents of the specific struct /will/ be spotted.
> 
> As long as it is a structure, yes. What about someone changing
> uint64_t to xen_pfn_t?

You mean if someone changes one of the buffers from "array of
uint64_t" to "array of xen_pfn_t", so that the ABI changes on some but
not all architectures ?

We could declare a rule that a dmop buffer may contain only a struct
or type name specific to that dmop (or an array of such), and must
always be accessed through a copy to or from such a type.

The rule would mean that if we had a dmop which wanted to call, using
one of the buffers, a function like this:

 int some_function(uint64_t *pfns, size_t n_pfns);

You'd have to write the dmop definition like this:

 typedef uint64_t xen_dmop_foo_entry;

 xen_dmop_foo_entry *pfns;
 size_t n_pfns;

 ...allocate table somehow...
 ret = copy_dm_buffer_from_guest(pfns,
 sizeof(pfns[0])*n_pfns,
 buffers, nr_buffers, 1);
 ...
 ret = some_function(pfns, n_pfns);

And similar code, again using xen_dmop_foo_entry. on the calling side.
Then if the buffer entry type is changed to xen_pfn_t you change it
to:

 typedef xen_pfn_t xen_dmop_foo_entry;

Now, unless the rest of the code is changed too, the compiler will
warn that a xen_pfn_t* cannot be converted to a uint64_t* in the call
to some_function.  (And likewise at the hypercall site in libxc.)

Would this be enough of an improvement, for you, to be worth the
additional type name clutter etc. ?


> > But I don't think it is really fair to criticise *the discussion
> > document* (which is what Jennifer's email was) on the grounds that it
> > ommitted to discuss a potential downside which its authors felt was
> > minor[1].
> 
> Hmm, then I'm sorry if this came over the wrong way. I didn't
> mean to criticize the document as a whole, or how it was written.

Thanks.

> >  The purpose of a discussion document is to put forward a
> > proposal and/or summarise previous discussion.  It is not required to
> > be either neutral or, indeed, comprehensive - and even so, I found
> > this one quite comprehensive.
> 
> Nevertheless I think such a document should be as honest as
> possible wrt downsides of the (by the author(s)) preferred of
> potentially multiple approaches.

I agree with that, although the word "honest" is rather tendentious.

>  If some aspect is deemed minor,
> I don't think it should be omitted (as then readers won't know
> whether the aspect was considered at all), but mentioned and
> stated to be believed to be minor.

I think there is a balance to be struck between mentioning every
possible consideration, however minor and remote, and making the
document concise and readable.

It will inevitably sometimes occur (as it did here) that some issue
that one person thought not worth mentioning, seems quite important to
another person.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Jan Beulich]

>>> On 21.09.16 at 14:23,  wrote:
> Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, 
> re qemu depriv)"):
>> Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, 
>> > So the actual hypercall call sites are all in-tree, in libxc.  If the
>> > format of the struct used for one of these guest handle slots changes,
>> > the same struct definition from the Xen headers is used both in the
>> > hypervisor and in libxc, and any incompatibility will be detected.
>> 
>> Wait, no. The guest handle slots in Jenny's proposal are basically
>> typeless:
> ...
>> Each sub-op implies a certain type for each handle slot it actually
>> uses.
> 
> Yes.  But in practice each such slot will in practice be accessed by
> using copy_dm_buffer_from_guest (or whatever) to copy it to a struct.
> It is intended that that same struct type will be used by libxc to
> populate the buffer.
> 
> Now, it is true that the compiler cannot check that the _same type_ is
> used in both places.  So, as I say:
> 
>It's true that changes to the semantics of these slots (for example, a
>change of a slot from "array of struct X" to "one struct Y") will not
>be detected by the compiler.
> 
> But changes in the contents of the specific struct /will/ be spotted.

As long as it is a structure, yes. What about someone changing
uint64_t to xen_pfn_t?

>>  but I do think that it failed to point out this downside,
>> which imo moves the balance between the two proposals.
> 
> I'm afraid that I want to complain about this part of your approach to
> the thread, which I think is unduly harsh.
> 
> It is of course fair to point out a potential downside of the proposal,
> which wasn't clearly discussed in the discussion document.  And it is
> sensible for us all to consider that potential downside along with the
> others - as indeed I do above.
> 
> But I don't think it is really fair to criticise *the discussion
> document* (which is what Jennifer's email was) on the grounds that it
> ommitted to discuss a potential downside which its authors felt was
> minor[1].

Hmm, then I'm sorry if this came over the wrong way. I didn't
mean to criticize the document as a whole, or how it was written.

>  The purpose of a discussion document is to put forward a
> proposal and/or summarise previous discussion.  It is not required to
> be either neutral or, indeed, comprehensive - and even so, I found
> this one quite comprehensive.

Nevertheless I think such a document should be as honest as
possible wrt downsides of the (by the author(s)) preferred of
potentially multiple approaches. If some aspect is deemed minor,
I don't think it should be omitted (as then readers won't know
whether the aspect was considered at all), but mentioned and
stated to be believed to be minor.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Device model operation hypercall (DMOP, re qemu depriv) [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, re 
qemu depriv)"):
> Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, 
> > So the actual hypercall call sites are all in-tree, in libxc.  If the
> > format of the struct used for one of these guest handle slots changes,
> > the same struct definition from the Xen headers is used both in the
> > hypervisor and in libxc, and any incompatibility will be detected.
> 
> Wait, no. The guest handle slots in Jenny's proposal are basically
> typeless:
...
> Each sub-op implies a certain type for each handle slot it actually
> uses.

Yes.  But in practice each such slot will in practice be accessed by
using copy_dm_buffer_from_guest (or whatever) to copy it to a struct.
It is intended that that same struct type will be used by libxc to
populate the buffer.

Now, it is true that the compiler cannot check that the _same type_ is
used in both places.  So, as I say:

   It's true that changes to the semantics of these slots (for example, a
   change of a slot from "array of struct X" to "one struct Y") will not
   be detected by the compiler.

But changes in the contents of the specific struct /will/ be spotted.

For example, consider the struct device_model_op, which in Jennifer's
document is in slot 0.  If someone edits struct device_model_op to
change its contents, both libxc and Xen see the same modified struct.

The problem can only occur if someone accesses a slot with entirely
the wrong struct type, or by the wrong slot number.  In such a
situation at least in new code, the thing probably wouldn't work at
all.

And even if we had some kind of typesafe system, the typesafety still
wouldn't spot uncontrolled ABI changes which would silently break old
callers.  So the need for careful review of DMOP changes is still
present.

So overall I think this typesafety problem is fairly limited.

> And then I disagree with the general view taken here: The issue
> I see is not with caller of the libxc interfaces (it was always clear
> that it would be possible to have type safety at that layer), but
> between libxc as the consumer and Xen as the producer of the
> interface.

I think you have misunderstood me.  I was discussing precisely
typesafety of that interface.


> > But *all* ABI changes to the DMOP interface need to be accompanied by
> > changes to libxc to add compatibility code.  I think it will be fairly
> > straightforward to check, during review, that each DMOP change comes
> > with a corresponding libxc change.
> 
> Well, yes, we can only hope for that. The main risk is that someone
> doesn't update the other side at all, when a change to one side gets
> done which compiles fine at both ends, and perhaps doesn't even
> alter the public header.

As I say I think if people are making uncontrolled changes to this
area, we have ABI stability problems not covered by the guest handle
typesafety system.  If we wanted to sort that would probably need to
make a proper DSL for the ABI.


> > But if you do not agree, then how about hiding the slots with a macro
> > setup ?  Something like:
> >[...]
> 
> Well, that's exactly the kind of workaround I'd like to avoid.

Well, I think it's a lot of effort for not much gain.  But it's
certainly doable and if you think it worthwhile I'm prepared to try
it.


Jan Beulich writes ("Re: [Xen-devel] Device model operation hypercall (DMOP, re 
qemu depriv)"):
> On 21.09.16 at 13:28,  wrote:
> > If someone feels strongly enough to Nack the other version, please say
> > so now; otherwise, I think Ian (since it seems like he'll be the one
> > implementing it) should make his best judgement based on the arguments
> > available and begin implementation.
> 
> No, I don't think I'd nack David's / Jenny's version (after all the type
> issue can be taken care off by sufficient discipline of both submitters
> and reviewers),

Thanks.

Personally I think David/Jennifer's version is better than the
previous proposal based on implicit or separate address limits.


>  but I do think that it failed to point out this downside,
> which imo moves the balance between the two proposals.

I'm afraid that I want to complain about this part of your approach to
the thread, which I think is unduly harsh.

It is of course fair to point out a potential downside of the proposal,
which wasn't clearly discussed in the discussion document.  And it is
sensible for us all to consider that potential downside along with the
others - as indeed I do above.

But I don't think it is really fair to criticise *the discussion
document* (which is what Jennifer's email was) on the grounds that it
ommitted to discuss a potential downside which its authors felt was
minor[1].  The purpose of a discussion document is to put forward a
proposal and/or summarise previous discussion.  It is not required to
be either neutral or, indeed, comprehensive - and even so, I found
this one quite comprehensive.