Re: [Xen-devel] [PATCH v2 5/9] x86/pagewalk: Helpers for reserved bit handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/03/2017 05:45, Juergen Gross wrote:
> On 23/03/17 18:35, Andrew Cooper wrote:
>> On 23/03/17 17:12, Tim Deegan wrote:
>>> At 17:02 +0000 on 23 Mar (1490288548), Andrew Cooper wrote:
>>>> On 23/03/17 16:55, Tim Deegan wrote:
>>>>> At 16:31 +0000 on 16 Mar (1489681899), Andrew Cooper wrote:
>>>>>> Some bits are unconditionally reserved in pagetable entries, or reserved
>>>>>> because of alignment restrictions.  Other bits are reserved because of 
>>>>>> control
>>>>>> register configuration.
>>>>>>
>>>>>> Introduce helpers which take an individual vcpu and guest pagetable 
>>>>>> entry, and
>>>>>> calculates whether any reserved bits are set.
>>>>>>
>>>>>> While here, add a couple of newlines to aid readability.
>>>>>>
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> Reviewed-by: Tim Deegan <tim@xxxxxxx>, although:
>>>>>
>>>>>> +/* Mask covering the reserved bits from superpage alignment. */
>>>>>> +#define SUPERPAGE_RSVD(bit)                                             
>>>>>> \
>>>>>> +    (((1ULL << (bit)) - 1) & ~(_PAGE_PSE_PAT | (_PAGE_PSE_PAT - 1)))
>>>>> I think this will be wrong if we ever get l4 superpages, as the mask
>>>>> is only 32 bits wide.
>>>> What is 32 bits wide?  1ULL should cause everything else to be suitably
>>>> promoted, no?
>>> ~(_PAGE_PSE_PAT | (_PAGE_PSE_PAT - 1)) == 0xffffe000.  Promotion comes
>>> too late.
>> Oh - good point.  Best fix this right now.
>>
>> Would you prefer ~((uint64_t)_PAGE_PSE_PAT | (_PAGE_PSE_PAT - 1)) or
>> ~(_PAGE_PSE_PAT | (_PAGE_PSE_PAT - 1) | 0ULL)
> Wouldn't it be better to just define the _PAGE_PSE bits accordingly?
>
> Letting them be 32 bit only seems to be very error prone.

As you see from the other replies, that's not the currently intended use
of these flags.

However, the development of this pagewalk series has caused me to
question the sense in keeping pte flags as a 32bit quantity.  There is a
non-trivial amount of packing and unpacking of them, which is surely
more expensive  than just using a 64bit quantity, now that we are 64bit
only.

~Andrew

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