Re: [Xen-devel] [PATCH 1/3] x86: drop is_pv_32on64_vcpu()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.06.15 at 03:08, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 06/24/2015 07:49 PM, Andrew Cooper wrote:
>> On 24/06/2015 22:35, Boris Ostrovsky wrote:
>>> On 06/23/2015 11:18 AM, Jan Beulich wrote:
>>>> ... as being identical to is_pv_32bit_vcpu() after the x86-32 removal.
>>>>
>>>> In a few cases this includes an additional is_pv_32bit_vcpu() ->
>>>> is_pv_32bit_domain() conversion.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> We have
>>> struct arch_domain
>>> {
>>>      ...
>>>      /* Is a 32-bit PV (non-HVM) guest? */
>>>      bool_t is_32bit_pv;
>>>      /* Is shared-info page in 32-bit format? */
>>>      bool_t has_32bit_shinfo;
>>>     ...
>>> }
>>>
>>> and currently both of these fields are set/unset together (except for
>>> one HVM case --- hvm_latch_shinfo_size()). Why not have a single 'bool
>>> is_32bit' and then replace macros at the top of
>>> include/asm-x86/domain.h with is_32bit_vcpu/domain()?
>>>
>>> I think in majority of places when we test for
>>> is_pv_32bit_vcpu/domain() we already know that we are PV so it
>>> shouldn't add any additional tests.
>> For the PV case, the two are equivalent.  For HVM, they are not.
>>
>> HVM domains have shared info, but may be latched as either 32 or 64bit,
>> depending on the mode they were running in when they most recently wrote
>> a hypercall page.  Sadly, the shared info layout is width-dependent
>> which is why such hacks need to exist.
> 
> Why can't we latch the mode into is_32bit field? I am essentially 
> suggesting to drop is_32bit_pv and rename has_32bit_shinfo to is_32bit. 
> Then is_pv_32bit_vcpu() becomes '(is_pv_vcpu() && domain->is_32bit)' (or 
> simply domain->is_32bit, depending on context) and has_32bit_shinfo()  
> becomes domain->is_32bit.
> 
> The reason I am asking is because for the 32b PVH I will need to switch 
> a few places from using is_pv_32bit_vcpu() to has_32bit_shinfo() and 
> that would look strange: asking whether the guest is 32-bit looks more 
> natural than asking whether its shared info is 32-bit. At least it's 
> more natural to my eye.

But it's incorrect: Guest mode may change, and claiming a guest is
32-bit is valid only for PV (at least as soon as PVH is permitted to
switch between 32- and 64-bit mode). HVM guests (and by analogy
PVH ones) should have no general bitness associated with them
(which is also why the macro name has "pv" in it) - the mode they
currently execute in is variable, and we explicitly don't tie shared
info layout to their current execution mode.

Introducing arbitrary checks like you suggest (for other than shared
info accesses) would need to be temporary, i.e. would need to be
tagged with a fixme comment (of which - as said recently - I'd prefer
not to see new instances introduced in connection with PVH).

Jan


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