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

[Tim Deegan <tim@xxxxxxx>]

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>

> 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.

Cheers,

Tim.