Xen devel - Sep 2013 - wrong io/tpmif.h made it into upstream Linux

All,

in the course of reviewing the hypervisor side of this (i.e. the
canonical copy of the header) I had requested some renames,
and they had also been carried out there. Why did this not get
adjusted _before_ hitting Linus''es tree? It''s particularly
strange
because this can''t be because different people were doing one
side and the other...

Jan

>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
> in the course of reviewing the hypervisor side of this (i.e. the
> canonical copy of the header) I had requested some renames,
> and they had also been carried out there. Why did this not get
> adjusted _before_ hitting Linus''es tree? It''s
particularly strange
> because this can''t be because different people were doing one
> side and the other...
Additionally using xen:vtpm as module alias collides with the v1
implementation too afaict. Was avoiding conflicts with the old
interface also not being considered here at all? Afaict the
backend also would need to announce itself differently from
the v1 one to xenbus...

Jan

On Thu, Sep 26, 2013 at 03:17:42PM +0100, Jan Beulich
wrote:
> >>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
> > in the course of reviewing the hypervisor side of this (i.e. the
> > canonical copy of the header) I had requested some renames,
> > and they had also been carried out there. Why did this not get
> > adjusted _before_ hitting Linus''es tree? It''s
particularly strange
> > because this can''t be because different people were doing one
> > side and the other...
That is something Daniel will have to tell. But I am wondering if
the patches were posted, but just never picked up (ie, lost in
the Xen 4.3 release?).
> 
> Additionally using xen:vtpm as module alias collides with the v1
> implementation too afaict. Was avoiding conflicts with the old
OK, but there is no v1 upstream and never will be. Would it
be possible for the XenClassic kernels to mesh the classic
v1 and the v2 implementation in one?

> interface also not being considered here at all? Afaict the
> backend also would need to announce itself differently from
> the v1 one to xenbus...
I think that was covered. It announces itself as vtpm but looks
for "feature-protocol-v2". If that is not there it will not attach
itself to the backend.

I thought that was discussed in the review at length months
ago?
> 
> Jan
>

On 09/26/2013 10:17 AM, Jan Beulich wrote:
>>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>> in the course of reviewing the hypervisor side of this (i.e. the
>> canonical copy of the header) I had requested some renames,
>> and they had also been carried out there. Why did this not get
>> adjusted _before_ hitting Linus''es tree? It''s
particularly strange
>> because this can''t be because different people were doing one
>> side and the other...
This was a mistake on my part. When these changes were made, the header
for Linux had already been split off in order to remove unnecessary
typedefs and extra structure definitions in the Xen header. The v4 patch
for Linux was just based on the v3 Linux patch, and the patch for Xen
making these changes (which you wrote and I just Acked) didn''t mention
needing to make a parallel change the Linux patch, so I never made the
changes.
> Additionally using xen:vtpm as module alias collides with the v1
> implementation too afaict. Was avoiding conflicts with the old
> interface also not being considered here at all? Afaict the
> backend also would need to announce itself differently from
> the v1 one to xenbus...
>
> Jan
The feature-protcol-v2 node was created to allow distinguishing the new
interface from the old one. Naming the xenbus node "vtpm2" was
considered for a while, but I believe it was considered unnecessary with
the introduction of that node.

It should be possible for the the driver to choose which shared page
format to use based on the feature node, if a driver supporting both
protocols were needed.

-- 
Daniel De Graaf
National Security Agency

>>> On 26.09.13 at 16:45, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Thu, Sep 26, 2013 at 03:17:42PM +0100, Jan Beulich wrote:
>> >>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>> > in the course of reviewing the hypervisor side of this (i.e. the
>> > canonical copy of the header) I had requested some renames,
>> > and they had also been carried out there. Why did this not get
>> > adjusted _before_ hitting Linus''es tree? It''s
particularly strange
>> > because this can''t be because different people were doing
one
>> > side and the other...
> 
> That is something Daniel will have to tell. But I am wondering if
> the patches were posted, but just never picked up (ie, lost in
> the Xen 4.3 release?).
No, 4.3 has properly named things in io/tpmif.h.
>> Additionally using xen:vtpm as module alias collides with the v1
>> implementation too afaict. Was avoiding conflicts with the old
> 
> OK, but there is no v1 upstream and never will be. Would it
> be possible for the XenClassic kernels to mesh the classic
> v1 and the v2 implementation in one?
The interfaces are totally different, so having a new driver is the
right thing. But obviously the new frontend shouldn''t get loaded
against a device drive by an old backed and vice versa.

Furthermore I have no clue whether there is a way to distinguish
both interfaces without having a proper XenBus identifier.
>> interface also not being considered here at all? Afaict the
>> backend also would need to announce itself differently from
>> the v1 one to xenbus...
> 
> I think that was covered. It announces itself as vtpm but looks
> for "feature-protocol-v2". If that is not there it will not
attach
> itself to the backend.
> 
> I thought that was discussed in the review at length months ago?
Possible, but tying this to a secondary node is the wrong approach
afaict, as that way (if you so happen to have them) both frontend
drivers will get loaded upon a vtpm device appearing. And while
the v2 one will know to look for the extra node, the v1 one clearly
won''t bail if that node is there.

Jan

>>> On 26.09.13 at 16:53, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> On 09/26/2013 10:17 AM, Jan Beulich wrote:
>>>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>> in the course of reviewing the hypervisor side of this (i.e. the
>>> canonical copy of the header) I had requested some renames,
>>> and they had also been carried out there. Why did this not get
>>> adjusted _before_ hitting Linus''es tree? It''s
particularly strange
>>> because this can''t be because different people were doing
one
>>> side and the other...
> 
> This was a mistake on my part. When these changes were made, the header
> for Linux had already been split off in order to remove unnecessary
> typedefs and extra structure definitions in the Xen header. The v4 patch
> for Linux was just based on the v3 Linux patch, and the patch for Xen
> making these changes (which you wrote and I just Acked) didn''t
mention
> needing to make a parallel change the Linux patch, so I never made the
> changes.
To me it goes without saying that if the master copy changes,
clones should take care to propagate them properly.
>> Additionally using xen:vtpm as module alias collides with the v1
>> implementation too afaict. Was avoiding conflicts with the old
>> interface also not being considered here at all? Afaict the
>> backend also would need to announce itself differently from
>> the v1 one to xenbus...
> 
> The feature-protcol-v2 node was created to allow distinguishing the new
> interface from the old one. Naming the xenbus node "vtpm2" was
> considered for a while, but I believe it was considered unnecessary with
> the introduction of that node.
> 
> It should be possible for the the driver to choose which shared page
> format to use based on the feature node, if a driver supporting both
> protocols were needed.
But that leaves out the existing (non-upstream) v1 drivers that
won''t know to look for that new node. A protocol change should
never claim to be the same version protocol as its predecessor.

Jan

On 26/09/13 16:02, Jan Beulich wrote:
>>>> On 26.09.13 at 16:53, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>> On 09/26/2013 10:17 AM, Jan Beulich wrote:
>>>>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>>> in the course of reviewing the hypervisor side of this (i.e.
the
>>>> canonical copy of the header) I had requested some renames,
>>>> and they had also been carried out there. Why did this not get
>>>> adjusted _before_ hitting Linus''es tree? It''s
particularly strange
>>>> because this can''t be because different people were
doing one
>>>> side and the other...
>>
>> This was a mistake on my part. When these changes were made, the header
>> for Linux had already been split off in order to remove unnecessary
>> typedefs and extra structure definitions in the Xen header. The v4
patch
>> for Linux was just based on the v3 Linux patch, and the patch for Xen
>> making these changes (which you wrote and I just Acked) didn''t
mention
>> needing to make a parallel change the Linux patch, so I never made the
>> changes.
> 
> To me it goes without saying that if the master copy changes,
> clones should take care to propagate them properly.
So long as the ABI itself is consistent I don''t see any real problem
with there being differences in structure/field names.
>>> Additionally using xen:vtpm as module alias collides with the v1
>>> implementation too afaict. Was avoiding conflicts with the old
>>> interface also not being considered here at all? Afaict the
>>> backend also would need to announce itself differently from
>>> the v1 one to xenbus...
>>
>> The feature-protcol-v2 node was created to allow distinguishing the new
>> interface from the old one. Naming the xenbus node "vtpm2"
was
>> considered for a while, but I believe it was considered unnecessary
with
>> the introduction of that node.
>>
>> It should be possible for the the driver to choose which shared page
>> format to use based on the feature node, if a driver supporting both
>> protocols were needed.
> 
> But that leaves out the existing (non-upstream) v1 drivers that
> won''t know to look for that new node. A protocol change should
> never claim to be the same version protocol as its predecessor.
Surely there isn''t a problem here?  The v2 frontend won''t
connect to a
v1 backend because the v1 backend doesn''t report feature-protocol-v2,
right?

As for the module alias, we''re not going to add another tpm frontend
driver to the kernel so I don''t see a problem here either.

David

On 09/26/2013 11:02 AM, Jan Beulich wrote:
>>>> On 26.09.13 at 16:53, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>> On 09/26/2013 10:17 AM, Jan Beulich wrote:
>>>>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>>> in the course of reviewing the hypervisor side of this (i.e.
the
>>>> canonical copy of the header) I had requested some renames,
>>>> and they had also been carried out there. Why did this not get
>>>> adjusted _before_ hitting Linus''es tree? It''s
particularly strange
>>>> because this can''t be because different people were
doing one
>>>> side and the other...
>>
>> This was a mistake on my part. When these changes were made, the header
>> for Linux had already been split off in order to remove unnecessary
>> typedefs and extra structure definitions in the Xen header. The v4
patch
>> for Linux was just based on the v3 Linux patch, and the patch for Xen
>> making these changes (which you wrote and I just Acked) didn''t
mention
>> needing to make a parallel change the Linux patch, so I never made the
>> changes.
>
> To me it goes without saying that if the master copy changes,
> clones should take care to propagate them properly.
Right; this was an oversight, I was just explaining how it happened.
Since it was just a name change, it is also less important than if
it was an actual ABI change.
>>> Additionally using xen:vtpm as module alias collides with the v1
>>> implementation too afaict. Was avoiding conflicts with the old
>>> interface also not being considered here at all? Afaict the
>>> backend also would need to announce itself differently from
>>> the v1 one to xenbus...
>>
>> The feature-protcol-v2 node was created to allow distinguishing the new
>> interface from the old one. Naming the xenbus node "vtpm2"
was
>> considered for a while, but I believe it was considered unnecessary
with
>> the introduction of that node.
>>
>> It should be possible for the the driver to choose which shared page
>> format to use based on the feature node, if a driver supporting both
>> protocols were needed.
>
> But that leaves out the existing (non-upstream) v1 drivers that
> won''t know to look for that new node. A protocol change should
> never claim to be the same version protocol as its predecessor.
>
> Jan
No kernel currently has both drivers (since upstream never had v1),
so this isn''t a problem yet. The backend will bail if the frontend
doesn''t set its own feature-protocol-v2 node, so an old v1 frontend
won''t end up trying to talk to a v2 backend.

I agree that having a single kernel support both v1 and v2 will end
up being a bit cumbersome. I think it was considered to be unlikely
for a single kernel to want to support both, but I don''t recall the
details of that discussion.

-- 
Daniel De Graaf
National Security Agency

>>> On 26.09.13 at 17:59, David Vrabel <david.vrabel@citrix.com>
wrote:
> On 26/09/13 16:02, Jan Beulich wrote:
>>>>> On 26.09.13 at 16:53, Daniel De Graaf
<dgdegra@tycho.nsa.gov> wrote:
>>> On 09/26/2013 10:17 AM, Jan Beulich wrote:
>>>>>>> On 26.09.13 at 13:52, "Jan Beulich"
<JBeulich@suse.com> wrote:
>>>>> in the course of reviewing the hypervisor side of this
(i.e. the
>>>>> canonical copy of the header) I had requested some renames,
>>>>> and they had also been carried out there. Why did this not
get
>>>>> adjusted _before_ hitting Linus''es tree?
It''s particularly strange
>>>>> because this can''t be because different people
were doing one
>>>>> side and the other...
>>>
>>> This was a mistake on my part. When these changes were made, the
header
>>> for Linux had already been split off in order to remove unnecessary
>>> typedefs and extra structure definitions in the Xen header. The v4
patch
>>> for Linux was just based on the v3 Linux patch, and the patch for
Xen
>>> making these changes (which you wrote and I just Acked)
didn''t mention
>>> needing to make a parallel change the Linux patch, so I never made
the
>>> changes.
>> 
>> To me it goes without saying that if the master copy changes,
>> clones should take care to propagate them properly.
> 
> So long as the ABI itself is consistent I don''t see any real
problem
> with there being differences in structure/field names.
But it''s confusing at the very least.
>>>> Additionally using xen:vtpm as module alias collides with the
v1
>>>> implementation too afaict. Was avoiding conflicts with the old
>>>> interface also not being considered here at all? Afaict the
>>>> backend also would need to announce itself differently from
>>>> the v1 one to xenbus...
>>>
>>> The feature-protcol-v2 node was created to allow distinguishing the
new
>>> interface from the old one. Naming the xenbus node
"vtpm2" was
>>> considered for a while, but I believe it was considered unnecessary
with
>>> the introduction of that node.
>>>
>>> It should be possible for the the driver to choose which shared
page
>>> format to use based on the feature node, if a driver supporting
both
>>> protocols were needed.
>> 
>> But that leaves out the existing (non-upstream) v1 drivers that
>> won''t know to look for that new node. A protocol change should
>> never claim to be the same version protocol as its predecessor.
> 
> Surely there isn''t a problem here?  The v2 frontend won''t
connect to a
> v1 backend because the v1 backend doesn''t report
feature-protocol-v2, right?
But the other way round it would happen.

Jan

>>> On 26.09.13 at 18:25, Daniel De Graaf <dgdegra@tycho.nsa.gov>
wrote:
> No kernel currently has both drivers (since upstream never had v1),
> so this isn''t a problem yet. The backend will bail if the frontend
> doesn''t set its own feature-protocol-v2 node, so an old v1
frontend
> won''t end up trying to talk to a v2 backend.
Okay, that''s at least some level of protection. But would this also
allow both drivers getting loaded simultaneously, and the right one
getting connected?
> I agree that having a single kernel support both v1 and v2 will end
> up being a bit cumbersome. I think it was considered to be unlikely
> for a single kernel to want to support both, but I don''t recall
the
> details of that discussion.
That must have happened without consideration to our forward
ported kernels. However much I would like to stop having to use
that model, there''s still way too many things we have but
upstream doesn''t (the good news is - 3.12 allows two more items
to be crossed off from that list of things), and my limited time
doesn''t allow me to maintain more than the hypervisor and one
kind of kernel. Of course this is a resourcing problem internally
here, not something I''m trying to call others to be guilty of.

Jan