Re: [PATCH 2/2][4.15?] x86/shadow: encode full GFN in magic MMIO entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2021 10:39, Tim Deegan wrote:
> At 16:37 +0100 on 05 Mar (1614962265), Jan Beulich wrote:
>> Since we don't need to encode all of the PTE flags, we have enough bits
>> in the shadow entry to store the full GFN. Don't use literal numbers -
>> instead derive the involved values. Or, where derivation would become
>> too ugly, sanity-check the result (invoking #error to identify failure).
>>
>> This then allows dropping from sh_l1e_mmio() again the guarding against
>> too large GFNs.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Tim Deegan <tim@xxxxxxx>

Thanks.

>> I wonder if the respective check in sh_audit_l1_table() is actually
>> useful to retain with these changes.
> 
> Yes, I think so.  We care about these PTEs being bogus for any reason,
> not just the ones that this could address.
> 
>> -#define SH_L1E_MAGIC 0xffffffff00000001ULL
>> +#define SH_L1E_MAGIC_NR_META_BITS 4
>> +#define SH_L1E_MAGIC_MASK ((~0ULL << (PADDR_BITS - PAGE_SHIFT + \
>> +                                      SH_L1E_MAGIC_NR_META_BITS)) | \
>> +                           _PAGE_PRESENT)
> 
> I don't think this makes the code any more readable, TBH, but if you
> prefer it that's OK.  I'd be happier with it if you added a
> BUILD_BUG_ON that checks that 1ULL << (PADDR_BITS - 1) is set in the
> mask, since that's the main thing we care about.
> 
>> -#define SH_L1E_MMIO_MAGIC       0xffffffff00000001ULL
>> -#define SH_L1E_MMIO_MAGIC_MASK  0xffffffff00000009ULL
>> -#define SH_L1E_MMIO_GFN_MASK    0x00000000fffffff0ULL
>> +#define SH_L1E_MMIO_MAGIC       SH_L1E_MAGIC_MASK
>> +#define SH_L1E_MMIO_MAGIC_BIT   ((_PAGE_PRESENT | _PAGE_RW | _PAGE_USER) + 
>> 1)
> 
> IMO this would be more readable as a straight 0x8 (or even _PAGE_PWT).
> The ack stands either way.

For both of your remarks, I agree that readability suffers by doing
it this way, so I'll undo some of it since you'd prefer it to be
more readable. My goal wasn't so much readability though but ease
of changes down the road (no need to change multiple related
definitions) and documentation of why these values actually are the
way they are.

But let's first see anyway what further changes are needed in
response to Andrew's remarks.

Jan