netfront pv driver building

netfront pv driver building

[Jan Beulich]

netfront requires the symbol __supported_pte_mask when PAE is enabled in the
kernel being built for. That symbol, however, isn't being exported, and it doesn't
seem likely that mainline would want to see this get exported (after all, the
general assumption is that the page table handling inline functions and macros
are supposed to be used only by the memory management code).

Additionally, both uses are inside a conditional depending upon
!XENFEAT_auto_translated_physmap, which hence can't ever be true if this
code is being built as pv driver. Wouldn't it hence make sense to #ifdef these
code blocks, or to enhance xen/features.h so that checks for those features
that are always on in hvm and return a constant 1 rather than using the
xen_features array.

Thanks, Jan

Re: netfront pv driver building

[Ian Campbell]

On Wed, 2007-01-24 at 13:46 +0000, Jan Beulich wrote:
> netfront requires the symbol __supported_pte_mask when PAE is enabled in the
> kernel being built for. That symbol, however, isn't being exported, and it doesn't
> seem likely that mainline would want to see this get exported (after all, the
> general assumption is that the page table handling inline functions and macros
> are supposed to be used only by the memory management code).

I committed a patch the other day which defines __supported_pte_mask to
~0ULL when building the unmodified drivers. It is 13555:687b1120765e in
xen-unstable. You are using 3.0.4-testing?

(it occurs to me now that I might have wanted ~PAGE_NX for greatest
compatibility, need to think about that a bit more.)

I wonder how this affects the PV-on-PV version when built as a module.
Presumably we export the symbol in the Xen tree but as you say that is
unlikely to go upstream.

> Additionally, both uses are inside a conditional depending upon
> !XENFEAT_auto_translated_physmap, which hence can't ever be true if this
> code is being built as pv driver. Wouldn't it hence make sense to #ifdef these
> code blocks, or to enhance xen/features.h so that checks for those features
> that are always on in hvm and return a constant 1 rather than using the
> xen_features array.

Is it necessary with the fix I mentioned above?

Ian.

Re: netfront pv driver building

[Jan Beulich]

>>> Ian Campbell <Ian.Campbell@XenSource.com> 24.01.07 15:05 >>>
>On Wed, 2007-01-24 at 13:46 +0000, Jan Beulich wrote:
>> netfront requires the symbol __supported_pte_mask when PAE is enabled in the
>> kernel being built for. That symbol, however, isn't being exported, and it doesn't
>> seem likely that mainline would want to see this get exported (after all, the
>> general assumption is that the page table handling inline functions and macros
>> are supposed to be used only by the memory management code).
>
>I committed a patch the other day which defines __supported_pte_mask to
>~0ULL when building the unmodified drivers. It is 13555:687b1120765e in
>xen-unstable. You are using 3.0.4-testing?

I did look at -unstable, but I only checked the sparse tree... But yes, I found
the problem in 3.0.4-testing.

>(it occurs to me now that I might have wanted ~PAGE_NX for greatest
>compatibility, need to think about that a bit more.)

Yes, I think this is dangerous, and I would rather want to see it reverted,
or as a minimum, as you say, converted to ~PAGE_NX (although I dislike
the lost bit of security here).
Especially because, as I said, at present the symbol is needed only for
dead code (so you don't really test whether the change would actually
work the way you want it to), and if at some point it makes it into active
code, we may end up with ugly issues.

>I wonder how this affects the PV-on-PV version when built as a module.
>Presumably we export the symbol in the Xen tree but as you say that is
>unlikely to go upstream.
>
>> Additionally, both uses are inside a conditional depending upon
>> !XENFEAT_auto_translated_physmap, which hence can't ever be true if this
>> code is being built as pv driver. Wouldn't it hence make sense to #ifdef these
>> code blocks, or to enhance xen/features.h so that checks for those features
>> that are always on in hvm and return a constant 1 rather than using the
>> xen_features array.
>
>Is it necessary with the fix I mentioned above?

With above fix it wouldn't be necessary, but as above, I don't think we should
override the OS here.

Jan

Re: netfront pv driver building

[Keir Fraser]

On 24/1/07 14:23, "Jan Beulich" <jbeulich@novell.com> wrote:

> Yes, I think this is dangerous, and I would rather want to see it reverted,
> or as a minimum, as you say, converted to ~PAGE_NX (although I dislike
> the lost bit of security here).
> Especially because, as I said, at present the symbol is needed only for
> dead code (so you don't really test whether the change would actually
> work the way you want it to), and if at some point it makes it into active
> code, we may end up with ugly issues.

Setting it to zero would make it very obvious if any usage became live, then
we could think harder about this at that time.

 -- Keir