Re: [Xen-devel] [PATCH 2/2] x86/paging: Rename paging_mark_pfn_dirty() and use pfn_t

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/16 15:22, Jan Beulich wrote:
>>>> On 14.12.16 at 15:26, <andrew.cooper3@xxxxxxxxxx> wrote:
>> paging_mark_gfn_dirty() actually takes a pfn, even by paramter name.  Rename
>> the function and alter the type to pfn_t to match.
>>
>> Push pfn_t into the LOGDIRTY_IDX() macros, and clean up a couple of local
>> variable types in paging_mark_pfn_dirty().
>>
>> Leave an explicit comment in vmx_vcpu_flush_pml_buffer() when we intentally
>> perform a straight conversion from gfn to pfn.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with two remarks:
>
>> @@ -331,8 +331,8 @@ void paging_mark_gfn_dirty(struct domain *d, unsigned 
>> long pfn)
>>      if ( changed )
>>      {
>>          PAGING_DEBUG(LOGDIRTY,
>> -                     "marked mfn %" PRI_mfn " (pfn=%lx), dom %d\n",
>> -                     mfn_x(mfn), pfn, d->domain_id);
>> +                     "marked mfn %" PRI_mfn " (pfn %" PRI_pfn "), dom %d\n",
>> +                     mfn_x(mfn), pfn_x(pfn), d->domain_id);
> Mind making the domain part canonical (i.e. d%d), and perhaps
> even moving it to the front ("d%d: ...\n")?

Ok.

>
>> @@ -345,23 +345,23 @@ void paging_mark_gfn_dirty(struct domain *d, unsigned 
>> long pfn)
>>  /* Mark a page as dirty */
>>  void paging_mark_dirty(struct domain *d, mfn_t gmfn)
>>  {
>> -    unsigned long pfn;
>> +    pfn_t pfn;
>>  
>>      if ( !paging_mode_log_dirty(d) || !mfn_valid(gmfn) ||
>>           page_get_owner(mfn_to_page(gmfn)) != d )
>>          return;
>>  
>>      /* We /really/ mean PFN here, even for non-translated guests. */
>> -    pfn = get_gpfn_from_mfn(mfn_x(gmfn));
>> +    pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
>>  
>> -    paging_mark_gfn_dirty(d, pfn);
>> +    paging_mark_pfn_dirty(d, pfn);
>>  }
> Looking at all of this, could patch 1 perhaps rename gmfn to mfn
> in this function?

This actually follows the shadow naming convention, of an mfn belonging
to a guest, and as checked in the first if clause.

~Andrew

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