Re: [Xen-devel] [PATCH v2 1/6] x86/EPT: don't walk entire page tables when globally changing types

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.04.14 at 17:41, <tim@xxxxxxx> wrote:
> At 13:28 +0100 on 22 Apr (1398169701), Jan Beulich wrote:
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -110,11 +110,18 @@ int hap_track_dirty_vram(struct domain *
>>          if ( begin_pfn != dirty_vram->begin_pfn ||
>>               begin_pfn + nr != dirty_vram->end_pfn )
>>          {
>> +            unsigned long ostart = dirty_vram->begin_pfn;
>> +            unsigned long oend = dirty_vram->end_pfn;
>> +
>>              dirty_vram->begin_pfn = begin_pfn;
>>              dirty_vram->end_pfn = begin_pfn + nr;
>>  
>>              paging_unlock(d);
>>  
>> +            if ( oend > ostart )
>> +                p2m_change_type_range(d, ostart, oend,
>> +                                      p2m_ram_logdirty, p2m_ram_rw);
>> +
> 
> Does this (and the simlar change below) not risk losing updates if
> we're in full log-dirty mode?  I think it should be fine to leave
> those entries as log-dirty, since at worst they'll provoke another
> fault-and-fixup.

I don't think any updates can be lost: When their types get re-
calculated, p2m_is_logdirty_range() (taking p2m->global_logdirty
into account) will still cause them to become p2m_ram_logdirty
again.

And these are needed mainly because ...

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -116,8 +116,14 @@ static int p2m_init_hostp2m(struct domai
>>  
>>      if ( p2m )
>>      {
>> -        d->arch.p2m = p2m;
>> -        return 0;
>> +        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> +                                            RANGESETF_prettyprint_hex);
> 
> Is there anything the guest can do to cause this rangeset to grow very
> large?  Like moving its video RAM around?

... we need to avoid this. If there was ever something added that
could cause multiple such regions to be created, we'd need to add
an upper limit on the number of permissible ranges. But there can
only be one VRAM region right now, so we're safe.

>> @@ -416,12 +470,11 @@ ept_set_entry(struct p2m_domain *p2m, un
>>      unsigned long gfn_remainder = gfn;
>>      int i, target = order / EPT_TABLE_ORDER;
>>      int rc = 0;
>> -    int ret = 0;
>>      bool_t direct_mmio = (p2mt == p2m_mmio_direct);
>>      uint8_t ipat = 0;
>>      int need_modify_vtd_table = 1;
>>      int vtd_pte_present = 0;
>> -    int needs_sync = 1;
>> +    int ret, needs_sync = -1;
> 
> Hmmm, another tristate.  I find these hard to follow, esp.  without
> comments to say which non-zero value is which.  I wonder if we could
> add some sort of framework for these to make them clearer.  Ideally
> we'd have an enum-like type and rely on the compiler to DTRT about
> optimizing the tests.
> 
> In this case, where needs_sync isn't being passed as a return value, I
> think we'd be fine with two booleans in place of this int.

I guess I'll make these tristates distinct enums then...

Jan


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