[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 15:56, <george.dunlap@xxxxxxxxxx> wrote: Most important thing first: The logic looks correct to me now. > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -533,6 +533,7 @@ static int resolve_misconfig(struct p2m_domain *p2m, > unsigned long gfn) > { > for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES; ++i ) > { > + p2m_type_t nt; > e = atomic_read_ept_entry(&epte[i]); Blank line between these two please. > @@ -542,10 +543,15 @@ static int resolve_misconfig(struct p2m_domain *p2m, > unsigned long gfn) > _mfn(e.mfn), 0, &ipat, > e.sa_p2mt == p2m_mmio_direct); > e.ipat = ipat; > - if ( e.recalc && p2m_is_changeable(e.sa_p2mt) ) > - { > - e.sa_p2mt = p2m_is_logdirty_range(p2m, gfn + i, gfn > + i) > - ? p2m_ram_logdirty : p2m_ram_rw; > + nt = p2m_recalc_type(e.recalc, e.sa_p2mt, p2m, gfn + i); > + if ( nt != e.sa_p2mt ) { Brace on its own line please. > + if ( e.sa_p2mt == p2m_ioreq_server ) > + { > + ASSERT(p2m->ioreq.entry_count > 0); > + p2m->ioreq.entry_count--; > + } > + > + e.sa_p2mt = nt; > ept_p2m_type_to_flags(p2m, &e, e.sa_p2mt, e.access); > } This changes why ept_p2m_type_to_flags() is bypassed. I think this is correct, but that wasn't immediately clear. > @@ -562,23 +568,24 @@ static int resolve_misconfig(struct p2m_domain *p2m, > unsigned long gfn) > > if ( recalc && p2m_is_changeable(e.sa_p2mt) ) > { > - unsigned long mask = ~0UL << (level * EPT_TABLE_ORDER); > - > - switch ( p2m_is_logdirty_range(p2m, gfn & mask, > - gfn | ~mask) ) > - { > - case 0: > - e.sa_p2mt = p2m_ram_rw; > - e.recalc = 0; > - break; > - case 1: > - e.sa_p2mt = p2m_ram_logdirty; > - e.recalc = 0; > - break; > - default: /* Force split. */ > - emt = -1; > - break; > - } > + unsigned long mask = ~0UL << (level * EPT_TABLE_ORDER); > + > + ASSERT(e.sa_p2mt != p2m_ioreq_server); > + switch ( p2m_is_logdirty_range(p2m, gfn & mask, > + gfn | ~mask) ) > + { > + case 0: > + e.sa_p2mt = p2m_ram_rw; > + e.recalc = 0; > + break; > + case 1: > + e.sa_p2mt = p2m_ram_logdirty; > + e.recalc = 0; > + break; > + default: /* Force split. */ > + emt = -1; > + break; > + } > } So if I'm getting it right the change here really is just the addition of an ASSERT() and re-indentation? I think you will want to also adjust indentation in the previous hunk then. (I'm afraid it was me who broke it back when introducing that code...) > @@ -606,6 +616,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long > gfn, mfn_t mfn, > > if ( page_order == PAGE_ORDER_4K ) > { > + p2m_type_t p2mt_old; > + > rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn, > L2_PAGETABLE_SHIFT - PAGE_SHIFT, > L2_PAGETABLE_ENTRIES, PGT_l1_page_table, 1); > @@ -629,6 +641,21 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long > gfn, mfn_t mfn, > if ( entry_content.l1 != 0 ) > p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags); > > + p2mt_old = p2m_flags_to_type(l1e_get_flags(*p2m_entry)); > + > + /* > + * p2m_ioreq_server is only used for 4K pages, so > + * the count shall only be performed for level 1 entries. > + */ > + if ( p2mt == p2m_ioreq_server ) > + p2m->ioreq.entry_count++; > + > + if ( p2mt_old == p2m_ioreq_server ) > + { > + ASSERT(p2m->ioreq.entry_count > 0); > + p2m->ioreq.entry_count--; > + } > + > /* level 1 entry */ > p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1); > /* NB: paging_write_p2m_entry() handles tlb flushes properly */ > @@ -726,15 +753,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long > gfn, mfn_t mfn, > return rc; > } I would have hoped for the two ASSERT()s to be added for the 2M and 1G cases, which we did talk about. > --- a/xen/include/asm-x86/p2m.h > +++ b/xen/include/asm-x86/p2m.h > @@ -120,7 +120,10 @@ typedef unsigned int p2m_query_t; > > /* Types that can be subject to bulk transitions. */ > #define P2M_CHANGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \ > - | p2m_to_mask(p2m_ram_logdirty) ) > + | p2m_to_mask(p2m_ram_logdirty) \ > + | p2m_to_mask(p2m_ioreq_server) ) > + > +#define P2M_IOREQ_TYPES (p2m_to_mask(p2m_ioreq_server)) > > #define P2M_POD_TYPES (p2m_to_mask(p2m_populate_on_demand)) > > @@ -157,6 +160,7 @@ typedef unsigned int p2m_query_t; > #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES) > #define p2m_is_discard_write(_t) (p2m_to_mask(_t) & P2M_DISCARD_WRITE_TYPES) > #define p2m_is_changeable(_t) (p2m_to_mask(_t) & P2M_CHANGEABLE_TYPES) > +#define p2m_is_ioreq(_t) (p2m_to_mask(_t) & P2M_IOREQ_TYPES) I don't think this and P2M_IOREQ_TYPES are needed anymore. > @@ -744,6 +749,25 @@ static inline p2m_type_t p2m_flags_to_type(unsigned long > flags) > return (flags >> 12) & 0x7f; > } > > +static inline p2m_type_t p2m_recalc_type_range(bool_t recalc, p2m_type_t t, Just "bool" please (also below). > + struct p2m_domain *p2m, > + unsigned long gfn_start, > + unsigned long gfn_end) > +{ > + if ( !recalc || !p2m_is_changeable(t) ) > + return t; > + if ( t == p2m_ioreq_server && p2m->ioreq.server != NULL ) > + return t; > + return p2m_is_logdirty_range(p2m, gfn_start, gfn_end) ? p2m_ram_logdirty > + : p2m_ram_rw; Commonly we'd align the : with either the ? or the start of the conditional expression. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |