Re: [Xen-devel] [PATCH v7.1 07/24] x86/mm: Introduce modify_xen_mappings()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.16 at 16:04, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -5964,6 +5976,10 @@ void destroy_xen_mappings(unsigned long s, unsigned 
> long e)
>      unsigned int  i;
>      unsigned long v = s;
>  
> +    /* Set of valid PTE bits which may be altered. */
> +#define FLAGS_MASK (_PAGE_NX|_PAGE_RW|_PAGE_PRESENT)
> +    nf &= FLAGS_MASK;

While we don't need it right away, I think including _PAGE_USER
here would be quite fine.

> @@ -6055,13 +6085,23 @@ void destroy_xen_mappings(unsigned long s, unsigned 
> long e)
>          }
>          else
>          {
> +            l1_pgentry_t nl1e;
> +
>              /* Ordinary 4kB mapping. */
>              pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(v);
> -            l1e_write_atomic(pl1e, l1e_empty());
> +
> +            nl1e = !(nf & _PAGE_PRESENT) ? l1e_empty()
> +                : l1e_from_pfn(l1e_get_pfn(*pl1e),
> +                               (l1e_get_flags(*pl1e) & ~FLAGS_MASK) | nf);
> +
> +            l1e_write_atomic(pl1e, nl1e);

Up in the 2M and 1G super page modification logic you check we're
not creating a new mapping - why not here, too?

>              v += PAGE_SIZE;
>  
> -            /* If we are done with the L2E, check if it is now empty. */
> -            if ( (v != e) && (l1_table_offset(v) != 0) )
> +            /*
> +             * If we are destroying mappings and done with the L2E, check if
> +             * it is now empty.
> +             */
> +            if ( (nf & _PAGE_PRESENT) && (v != e) && (l1_table_offset(v) != 
> 0) )
>                  continue;

Doesn't this need to be

            if ( (nf & _PAGE_PRESENT) || ((v != e) && l1_table_offset(v)) )

?

Jan


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