Re: [Xen-devel] [PATCH v1 1/2] x86/mm: Add mem access rights to NPT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.05.18 at 13:11, <aisaila@xxxxxxxxxxxxxxx> wrote:
> This patch adds access rights for the NPT pages. The access rights are
> saved in bits 59:56 of pte that are manipulated through p2m_set_access()
> and p2m_get_access() functions.

You don't give any rationale for the choice of bits. Right now p2m-pt.c still
assumes that CPU and IOMMU page tables might be shared, despite
amd_iommu_init() unconditionally turning this functionality off. As long as the
option for that mode hasn't been removed from p2m-pt.c, I think bits used
by the IOMMU (here: bit 59) should not be used for software purposes. The
alternative therefore is for you to supply a prereq patch purging the sharing
functionality from p2m-pt.c and preferably also from the AMD IOMMU code.
That's of course only an option if we don't foresee any means by which this
mode may become usable again.

> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -159,6 +159,15 @@ static inline intpte_t put_pte_flags(unsigned int x)
>   */
>  #define _PAGE_GUEST_KERNEL (1U<<12)
>  
> +/*
> + * Bits 19:16 of a 24-bit flag mask are used to store p2m_access_t.
> + * This corresponds to bits 59:56 of a pte
> + */
> +#define PAGE_ACCESS_START          59
> +#define PAGE_ACCESS_LEN            4
> +#define PAGE_ACCESS_MASK           ((1UL << PAGE_ACCESS_LEN) - 1)
> +#define PAGE_ACCESS_BITFIELD_MASK  (PAGE_ACCESS_MASK << PAGE_ACCESS_START)

Afaics you don't need these outside of p2m-pt.c, in which case I would think
it's better to confine their visibility to that file. That'll avoid the possibly
misleading comment about bits 19:16 here (you never convert the flags to
that form).

Additionally I'd prefer if you defined just a single constant instead of 4 of
them, and if you then used MASK_EXTR() / MASK_INSR() as appropriate.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel