Re: [PATCHv6 05/30] x86/tdx: Exclude shared bit from __PHYSICAL_MASK

Thomas Gleixner
2022-03-17T14:39:14Z

On Thu, Mar 17 2022 at 16:58, Kirill A. Shutemov wrote:

> On Thu, Mar 17, 2022 at 01:16:00AM +0100, Thomas Gleixner wrote:
>> On Wed, Mar 16 2022 at 05:08, Kirill A. Shutemov wrote:
>> > @@ -82,6 +82,14 @@ void __init tdx_early_init(void)
>> >
>> > cc_set_vendor(CC_VENDOR_INTEL);
>> >
>> > + /*
>> > + * All bits above GPA width are reserved and kernel treats shared bit
>> > + * as flag, not as part of physical address.
>> > + *
>> > + * Adjust physical mask to only cover valid GPA bits.
>> > + */
>> > + physical_mask &= GENMASK_ULL(gpa_width - 2, 0);
>> > +
>>
>> Hrm. I forgot about the second use case for gpa_width, but my comment
>> about ordering still stands. OTOH:
>>
>> GENMASK_ULL(gpa_width - 2, 0) == BIT_UL(gpa_width - 1) - 1
>>
>> right? So you really can consolidate on the fact that cc_mask is a
>> single bit which is above the guests physical address space boundary.
>>
>> I.e. make the code tell the story instead of adding lengthy comments
>> explaining the obfuscation.
>
> So it will looks something like this:
>
>
> cc_set_vendor(CC_VENDOR_INTEL);
> cc_mask = get_cc_mask();
> cc_set_mask(cc_mask);
>
> /*
> * All bits above GPA width are reserved and kernel treats shared bit
> * as flag, not as part of physical address.
> *
> * Adjust physical mask to only cover valid GPA bits.
> */
> physical_mask &= cc_mask - 1;
>
> I still think these comments are useful. I hided comment for cc_mask
> calclulation inside get_cc_mask().
>
> Does it look fine to you?

Yes.