[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 5/6] x86/ioreq server: Asynchronously reset outstanding p2m_ioreq_server entries.
>>> On 07.04.17 at 12:50, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote: > On 4/7/2017 6:28 PM, George Dunlap wrote: >> On 07/04/17 11:14, Yu Zhang wrote: >>> >>> On 4/7/2017 5:40 PM, Jan Beulich wrote: >>>>>>> On 06.04.17 at 17:53, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote: >>>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>>> @@ -544,6 +544,12 @@ static int resolve_misconfig(struct p2m_domain >>>>> *p2m, unsigned long gfn) >>>>> e.ipat = ipat; >>>>> if ( e.recalc && p2m_is_changeable(e.sa_p2mt) ) >>>>> { >>>>> + if ( e.sa_p2mt == p2m_ioreq_server ) >>>>> + { >>>>> + ASSERT(p2m->ioreq.entry_count > 0); >>>>> + p2m->ioreq.entry_count--; >>>>> + } >>>>> + >>>>> e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn >>>>> + i, gfn + i) >>>>> ? p2m_ram_logdirty : p2m_ram_rw; >>>> I don't think this can be right: Why would it be valid to change the >>>> type from p2m_ioreq_server to p2m_ram_rw (or p2m_ram_logdirty) >>>> here, without taking into account further information? This code >>>> can run at any time, not just when you want to reset things. So at >>>> the very least there is a check missing whether a suitable ioreq >>>> server still exists (and only if it doesn't you want to do the type >>>> reset). >>> Also I do not think we need to check if a suitable ioreq server still >>> exists. We have guaranteed >>> in our patch that no new ioreq server will be mapped as long as the p2m >>> table is not clean. :) >> Jan is saying that you should only change ioreq_server -> ram if there >> is *not* an ioreq server; and that if this is called with an ioreq >> server still active, then it must be some other change you're looking at. >> >> The problem, though, is that misconfiguration happens in many >> circumstances. Grep for "memory_type_changed()" -- each of those >> results in a recalculation of the entire p2m, which will (in the current >> code) wipe out any ioreq_server entries. > > Well, I'm not aware that other actions besides the logdirty will cause the > reset. > But if that would happen, will below change solve this? > > @@ -546,12 +546,16 @@ static int resolve_misconfig(struct p2m_domain *p2m, > unsigned long gfn) > { > if ( e.sa_p2mt == p2m_ioreq_server ) > { > - ASSERT(p2m->ioreq.entry_count > 0); > - p2m->ioreq.entry_count--; > + if ( p2m->ioreq.server == NULL ) > + { > + ASSERT(p2m->ioreq.entry_count > 0); > + p2m->ioreq.entry_count--; > + e.sa_p2mt = p2m_ram_rw; > + } > } > - > - e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + i, gfn > + i) > - ? p2m_ram_logdirty : p2m_ram_rw; > + else > + e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + i, > gfn + i) > + ? p2m_ram_logdirty : p2m_ram_rw; Now you _never_ change away from ioreq-server, you only adjust the counter. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |