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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/04/16 18:18, Jan Beulich wrote:
>>>> On 11.04.16 at 16:04, <andrew.cooper3@citrix.com> 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.

I considered that.  Until we have a valid use for doing so, I chose not
to give people the opportunity to shoot themselves in the foot.

>
>> @@ -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?

Good point.  I will add one.  (I did have one orignally, and it got lost
through a couple of refactors.)

>
>>              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)) )
>
> ?

It should.  On further consideration, I am also going to invert the
sense of the comment, to match the code.

~Andrew

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