Re: [PATCH v2 1/2] x86: correct is_pv_domain() when !CONFIG_PV

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2021 12:53, Roger Pau Monné wrote:
> On Thu, Apr 15, 2021 at 11:34:55AM +0200, Jan Beulich wrote:
>> On x86, idle and other system domains are implicitly PV. While I
>> couldn't spot any cases where this is actively a problem, some cases
>> required quite close inspection to be certain there couldn't e.g. be
>> some ASSERT_UNREACHABLE() that would trigger in this case. Let's be on
>> the safe side and make sure these always have is_pv_domain() returning
>> true.
>>
>> For the build to still work, this requires a few adjustments elsewhere.
>> In particular is_pv_64bit_domain() now gains a CONFIG_PV dependency,
>> which means that is_pv_32bit_domain() || is_pv_64bit_domain() is no
>> longer guaranteed to be the same as is_pv_domain().
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Add comment.
> 
> Sorry for not replying earlier, I've been thinking about this because
> I don't really like this approach as I think it makes code harder to
> follow for two reasons, first is_pv_32bit_domain() ||
> is_pv_64bit_domain() != is_pv_domain(), which I could live with, and
> then also is_pv_64bit_domain() returning different values for system
> domains depending on whether CONFIG_PV is enabled.

Well, okay, I'll consider the patch rejected then, despite thinking
that it could save us from subtle issues down the road.

> Given that AFAICT this patch is not fixing any actively identified
> issue I would rather prefer to introduce is_system_domain and use it
> when appropriate?
> 
> I think that would be clearer long term, and avoid tying ourselves
> deeper into aliasing system domain with PV domains.

Of course, but it won't help until we've audited and (if needed)
amended all code using is_pv_*() or e.g. implying PV when !is_hvm_*().

Patch 2, while grouped with this one, is technically independent.
Therefore I'd still appreciate separate feedback there.

Jan