[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 1/4] x86/mm: Shadow and p2m changes for PV mem_access
>> +void p2m_access_to_flags(u32 *flags, u32 gflags, p2m_access_t access) >> +{ >> + >> + /* Restrict with access permissions */ >> + switch (access) >> + { >> + case p2m_access_r: >> + *flags &= ~(_PAGE_RW); >> + *flags |= (_PAGE_NX_BIT|_PAGE_PRESENT); >> + break; >> + case p2m_access_rx: >> + case p2m_access_rx2rw: >> + *flags &= ~(_PAGE_NX_BIT|_PAGE_RW); >> + *flags |= _PAGE_PRESENT; >> + break; >> + case p2m_access_rw: >> + *flags |= (_PAGE_NX_BIT|_PAGE_RW|_PAGE_PRESENT); >> + break; >> + case p2m_access_rwx: >> + default: >> + *flags &= ~(_PAGE_NX_BIT); >> + *flags |= (_PAGE_RW|_PAGE_PRESENT); >> + break; >> + } > >Tim will have the final say here, but I'd suggest dropping all these >needless parentheses (I see only one case where they're really >needed). OK, I will drop all the needless ones. Could you please point out the case they are needed? I couldn't find anything in the CODING_STYLE that dictates what should be done. >> + >> + // Allow more restrictive guest flags to be propagated instead of access >> + // permissions > >Coding style (there are more of these, which I'm not going to >individually comment on). Sorry, I let the C++ style comments slip in. I will fix all of them to C style comments. >> +static int >> +p2m_mem_access_set_entry(struct p2m_domain *p2m, unsigned long >gfn, mfn_t mfn, >> + unsigned int page_order, p2m_type_t p2mt, >> + p2m_access_t p2ma) >> +{ >> + struct domain *d = p2m->domain; >> + mfn_t *access_lookup_table = p2m->access_lookup_table; >> + uint table_idx; >> + uint page_idx; >> + uint8_t *access_table_page; >> + >> + ASSERT(shadow_mode_mem_access(d) && access_lookup_table != >NULL); > >Better split into two ASSERT()s, so that if it triggers one would know >which of the two conditions wasn't met? You are right. I should have done that in the first place. >> + >> + /* For PV domains we only support rw, rx, rx2rw, rwx access permissions >*/ >> + if ( unlikely(p2ma != p2m_access_r && >> + p2ma != p2m_access_rw && >> + p2ma != p2m_access_rx && >> + p2ma != p2m_access_rwx && >> + p2ma != p2m_access_rx2rw) ) >> + return -EINVAL; > >Perhaps better done as a switch() statement. Sure. That will definitely be cleaner. >> + >> + if ( page_get_owner(mfn_to_page(mfn)) != d ) >> + return -ENOENT; >> + >> + gfn = get_gpfn_from_mfn(mfn_x(mfn)); > >You get "gfn" passed in and blindly overwrite it here? _If_ you need to >do this lookup, shouldn't you instead check it matches the passed in one? The "gfn" and "mfn" passed in to the function should be the same as it is a PV guest. That is why I am overwriting "gfn" to get the "gpfn" from the machine_to_phys_mapping. >> + >> + /* >> + * Values with the MSB set denote MFNs that aren't really part of the >> + * domain's pseudo-physical memory map (e.g., the shared info frame). >> + * Nothing to do here. >> + */ >> + if ( unlikely(!VALID_M2P(gfn)) ) >> + return 0; >> + >> + if ( gfn > (d->tot_pages - 1) ) >> + return -EINVAL; > >Hardly - a PV domain can easily make its address space sparse, and >iirc pv-ops Linux even does so by default to avoid PFN/MFN collisions >on MMIO space. (And as a side note, this would better be "gfn >= >d->tot_pages".) I was using this as a guard against going out of bounds in the access lookup table which is created based on the domains tot_pages. Please let me know what I should be using instead of tot_pages when doing this. >> + paging_lock(d); >> + >> + table_idx = MEM_ACCESS_TABLE_IDX(gfn); >> + page_idx = MEM_ACCESS_PAGE_IDX(gfn); >> + access_table_page = >map_domain_page(mfn_x(access_lookup_table[table_idx])); >> + access_table_page[page_idx] = p2ma; >> + unmap_domain_page(access_table_page); >> + >> + if ( sh_remove_all_mappings(d->vcpu[0], mfn) ) > >Is there anything guaranteeing d->vcpu and d->vcpu[0] being non- >NULL? I am not sure. I see shadow_track_dirty_vram() calling this without a check. Maybe I should check and call it only if d->vcpu is not null. >> +static mfn_t >> +p2m_mem_access_get_entry(struct p2m_domain *p2m, unsigned long >gfn, >> + p2m_type_t *t, p2m_access_t *a, p2m_query_t q, >> + unsigned int *page_order) >> +{ >> + struct domain *d = p2m->domain; >> + mfn_t *access_lookup_table = p2m->access_lookup_table; >> + uint table_idx; >> + uint page_idx; >> + uint8_t *access_table_page; >> + mfn_t mfn = _mfn(gfn); // For PV guests mfn == gfn >> + >> + ASSERT(shadow_mode_mem_access(d) && access_lookup_table != >NULL); >> + >> + /* Not necessarily true, but for non-translated guests, we claim >> + * it's the most generic kind of memory */ > >I think you copied this comment verbatim from elsewhere, but I don't >think it's correct as is. Yes, indeed. I copied this from __get_gfn_type_access(). I will get rid of the comment. >> + *t = p2m_ram_rw; >> + >> + if ( page_get_owner(mfn_to_page(mfn)) != d ) >> + return _mfn(INVALID_MFN); >> + >> + gfn = get_gpfn_from_mfn(mfn_x(mfn)); > >Same comment as earlier. > >> + if ( gfn > (d->tot_pages - 1) ) > >Dito. I have the same reasoning here as I did above. >> + table_idx = MEM_ACCESS_TABLE_IDX(gfn); >> + page_idx = MEM_ACCESS_PAGE_IDX(gfn); >> + >> + access_table_page = >map_domain_page(mfn_x(access_lookup_table[table_idx])); >> + >> + /* This is a hint to take the default permissions */ >> + if ( access_table_page[page_idx] == p2m_access_n ) >> + access_table_page[page_idx] = p2m->default_access; > >We're in "get" here - why does that modify any global state? I do not initialize the access lookup table to the default value. But the table is zeroed as part of initialization when each page is allocated using shadow_alloc_p2m_page(). I then use the "p2m_access_n" / 0 value as hint that the default value should be returned as p2m_access_n is not valid for PV domains. If you prefer that I initialize the table to the default value, I will gladly do so. >> +void p2m_mem_access_teardown(struct p2m_domain *p2m) >> +{ >> + struct domain *d = p2m->domain; >> + mfn_t *access_lookup_table = p2m->access_lookup_table; >> + uint32_t nr_access_table_pages; >> + uint32_t ctr; >> + >> + /* Reset the set_entry and get_entry function pointers */ >> + p2m_pt_init(p2m); >> + >> + if ( !access_lookup_table ) >> + return; >> + >> + nr_access_table_pages = get_domain_nr_access_table_pages(d); >> + >> + for ( ctr = 0; ctr < nr_access_table_pages; ctr++ ) > >No new effectively unbounded loops please. OK, I will add a check for preemption and add hypercall continuation logic here. >> +int p2m_mem_access_init(struct p2m_domain *p2m) >> +{ >> + struct domain *d = p2m->domain; >> + mfn_t *access_lookup_table; >> + uint32_t nr_access_table_pages; >> + uint32_t ctr; >> + >> + nr_access_table_pages = get_domain_nr_access_table_pages(d); >> + access_lookup_table = xzalloc_array(mfn_t, nr_access_table_pages); > >This surely is going to be an order > 0 allocation, and you even risk >it being an order > MAX_ORDER one. The former is disallowed at >runtime by convention/agreement, the latter is an outright bug. Sorry, I was not aware of the convention. What should I be doing here instead? >(And again just as a side note - you don't appear to need the zero >initialization here.) > >> + if ( !access_lookup_table ) >> + return -ENOMEM; >> + >> + p2m->access_lookup_table = access_lookup_table; >> + >> + for ( ctr = 0; ctr < nr_access_table_pages; ctr++ ) > >Same comment regarding the effective unboundedness of the loop. OK, I will add a check for preemption and add hypercall continuation logic here too. >> @@ -1414,6 +1419,8 @@ long p2m_set_mem_access(struct domain *d, >unsigned long pfn, uint32_t nr, >> if ( pfn == ~0ul ) >> { >> p2m->default_access = a; >> + if ( is_pv_domain(d) ) >> + return p2m_mem_access_set_default(p2m); > >Is it really correct to set p2m->default_access _before_ calling that >function? Perhaps it wouldn't be correct doing it the other way >around either - I suppose you need to hold the necessary lock across >both operations. I do not see the problem here. p2m_mem_access_set_default() just blows the shadows away after taking the paging_lock. There is no actual setting of default permission for all pages in the PV case. >> @@ -585,9 +585,17 @@ int paging_domctl(struct domain *d, >xen_domctl_shadow_op_t *sc, >> { >> >> case XEN_DOMCTL_SHADOW_OP_ENABLE: >> + /* >> + * Shadow mem_access mode should only be enabled when >mem_access is >> + * enabled in XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE. >> + */ >> + if ( sc->mode & XEN_DOMCTL_SHADOW_ENABLE_MEM_ACCESS ) >> + return -EINVAL; >> + >> if ( !(sc->mode & XEN_DOMCTL_SHADOW_ENABLE_LOG_DIRTY) ) >> break; >> /* Else fall through... */ >> + >> case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY: > >Stray blank line - the fall through here makes it preferable to keep the >two case blocks un-separated. OK, I will keep them un-seperated. >> @@ -2443,7 +2443,8 @@ int sh_remove_all_mappings(struct vcpu *v, >mfn_t gmfn) >> if ( !(shadow_mode_external(v->domain) >> && (page->count_info & PGC_count_mask) <= 3 >> && ((page->u.inuse.type_info & PGT_count_mask) >> - == !!is_xen_heap_page(page))) ) >> + == !!is_xen_heap_page(page))) >> + && !(shadow_mode_mem_access(v->domain)) ) > >You're breaking indentation, there are pointless parentheses again, >but most importantly - why? Sorry, I meant to ask a question about this in my patch message and mark this as a workaround. I am seeing the message on all sh_remove_all_mappings() and I was not able to figure out why this was happening. I just added this as a work around. I was hoping you or Tim would shed more light on this. >> @@ -625,6 +627,20 @@ _sh_propagate(struct vcpu *v, >> } >> } >> >> + /* Propagate access permissions */ >> + if ( unlikely((level == 1) && >> + mem_event_check_ring(&d->mem_event->access) && >> + !sh_mfn_is_a_page_table(target_mfn)) ) > >Just a general remark here - unlikely() used like this is pointless, it >ought to be used on the individual constituents of && or ||. OK, I will leave this for mem_event_check_ring() case. >> + { >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + p2m_access_t a; >> + p2m_type_t t; >> + mfn_t mfn; >> + mfn = p2m->get_entry(p2m, mfn_x(target_mfn), &t, &a, 0, NULL); > >Missing blank line between declarations and statements. Also this >statement could instead be an initializer. I will make it an initializer. >> @@ -2822,6 +2838,8 @@ static int sh_page_fault(struct vcpu *v, >> int r; >> fetch_type_t ft = 0; >> p2m_type_t p2mt; >> + p2m_access_t p2ma; > >This variable ... > >> @@ -3009,7 +3027,80 @@ static int sh_page_fault(struct vcpu *v, >> >> /* What mfn is the guest trying to access? */ >> gfn = guest_l1e_get_gfn(gw.l1e); >> - gmfn = get_gfn(d, gfn, &p2mt); >> + if ( likely(!mem_event_check_ring(&d->mem_event->access)) ) >> + gmfn = get_gfn(d, gfn, &p2mt); >> + /* >> + * A mem_access listener is present, so we will first check if a >> violation >> + * has occurred. >> + */ >> + else >> + { >> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > >... seems to be used only in this scope, and with you already adding >scope restricted variables it should go here. OK, I will do that. >> + >> + gmfn = get_gfn_type_access(p2m, gfn_x(gfn), &p2mt, &p2ma, 0, >NULL); >> + if ( mfn_valid(gmfn) && !sh_mfn_is_a_page_table(gmfn) >> + && (regs->error_code & PFEC_page_present) >> + && !(regs->error_code & PFEC_reserved_bit) ) >> + { >> + int violation = 0; >> + bool_t access_w = !!(regs->error_code & PFEC_write_access); >> + bool_t access_x = !!(regs->error_code & PFEC_insn_fetch); >> + bool_t access_r = access_x ? 0 : !(access_w); > >Stray parentheses again. I'm not going to repeat this again - please >just check your code before submitting. I am really sorry. I will fix this is in the next patch series. >> + /* >> + * Do not police writes to guest memory emanating from the Xen >> + * kernel. Trying to do so will cause the same pagefault to >> occur >> + * over and over again with an event being sent to the access >> + * listener for each fault. If the access listener's vcpu is not >> + * scheduled during this time, the violation is never resolved >> and >> + * will eventually end with the host crashing. >> + */ >> + if ( (violation && access_w) && >> + (regs->eip >= XEN_VIRT_START && regs->eip <= XEN_VIRT_END) >> ) >> + { >> + violation = 0; >> + rc = p2m->set_entry(p2m, gfn_x(gfn), gmfn, PAGE_ORDER_4K, >> + p2m_ram_rw, p2m_access_rw); >> + } > >This looks more like a hack than a proper solution - shouldn't the >listener be allowed to know of hypervisor side accesses? Ideally the listener should know of the hypervisor side accesses but I ran in to cases where the listener would not get to run and the pagefault would be tried over and over, eventually causing the host to crash. I guess more detail about the problem is needed. I will send out a separate email regarding this, CCing you and Tim. >> +/* Number of access table pages for a PV domain */ >> +#define get_domain_nr_access_table_pages(d) \ >> + DIV_ROUND_UP(P2M_ACCESS_SIZE * (d->tot_pages - 1), PAGE_SIZE) > >And once again. I wonder whether this code was tested on a suitably >big pv-ops PV guest. The largest PV domain I tried this with was 28GB. Thanks, Aravindh _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |