| Message ID | 5944a3ea-cddf-7ddb-d167-a0a0aa9b4967@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: assorted (mostly) shadow mode adjustments | expand |
On 22/03/2023 9:35 am, Jan Beulich wrote: > Emulation related functions are involved in HVM handling only, and in > some cases they even invoke such checks after having already done things > which are valid for HVM domains only. OOS active also implies HVM. In > sh_remove_all_mappings() one of the two checks is redundant with an > earlier paging_mode_external() one (the other, however, needs to stay). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain > && (page->count_info & PGC_count_mask) <= 3 > && ((page->u.inuse.type_info & PGT_count_mask) > == (is_special_page(page) || > - (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) > + is_ioreq_server_page(d, page)))) ) > printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn > " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", > mfn_x(gmfn), gfn_x(gfn), Out of context here needs an equivalent adjustment. But in this case, I'm not sure the commit message covers the relevant details. ioreq servers have been made fully common since this code was written, and *that* is a better reason for dropping the predicates IMO than the redundancy with paging_mode_external(). That said... I'm not sure the logic here is correct any more. It used to be the case that ioreq pages were in the p2m, but they're outside of the p2m these days, so don't see how there can be any interaction with unexpected refcounts any more. I suspect that one way or another, this change wants to be in a separate patch. > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -3441,7 +3441,7 @@ int sh_rm_write_access_from_sl1p(struct > > #ifdef CONFIG_HVM > /* Remember if we've been told that this process is being torn down */ > - if ( curr->domain == d && is_hvm_domain(d) ) > + if ( curr->domain == d ) > curr->arch.paging.shadow.pagetable_dying > = mfn_to_page(gmfn)->pagetable_dying; > #endif This one is dangerous. After tracing, I can see that sh_rm_write_access_from_sl1p() is only called from OOS functions, but this function itself does its very best to look like it has mixed PV + HVM usage, and dropping this conditional means that pagetable_dying can, in principle at least, become non-NULL for a PV guest. I think this function needs to be made far more obviously HVM-only first. > --- a/xen/arch/x86/mm/shadow/oos.c > +++ b/xen/arch/x86/mm/shadow/oos.c > @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn > if ( pg->shadow_flags & > ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync) > || sh_page_has_multiple_shadows(pg) > - || !is_hvm_vcpu(v) > || !v->domain->arch.paging.shadow.oos_active ) This is reachable for PV guests as far as I can see. What am I missing ? The changes in hvm.c are all fine, and for those alone, consider it R-by if you end up splitting the patch. ~Andrew
On 23.03.2023 19:18, Andrew Cooper wrote: > On 22/03/2023 9:35 am, Jan Beulich wrote: >> Emulation related functions are involved in HVM handling only, and in >> some cases they even invoke such checks after having already done things >> which are valid for HVM domains only. OOS active also implies HVM. In >> sh_remove_all_mappings() one of the two checks is redundant with an >> earlier paging_mode_external() one (the other, however, needs to stay). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/mm/shadow/common.c >> +++ b/xen/arch/x86/mm/shadow/common.c >> @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain >> && (page->count_info & PGC_count_mask) <= 3 >> && ((page->u.inuse.type_info & PGT_count_mask) >> == (is_special_page(page) || >> - (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) >> + is_ioreq_server_page(d, page)))) ) >> printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn >> " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", >> mfn_x(gmfn), gfn_x(gfn), > > Out of context here needs an equivalent adjustment. I'm afraid I don't seen any further is_hvm_*() in this function. > But in this case, I'm not sure the commit message covers the relevant > details. ioreq servers have been made fully common since this code was > written, and *that* is a better reason for dropping the predicates IMO > than the redundancy with paging_mode_external(). How does "fully common" matter? It's still a HVM-only thing, hence the paging_mode_external() check just out of context. Also note that the ioreq-server-page check is only one side of || (and I realize that by correcting indentation here at the same time this might be better visible). > That said... I'm not sure the logic here is correct any more. It used > to be the case that ioreq pages were in the p2m, but they're outside of > the p2m these days, so don't see how there can be any interaction with > unexpected refcounts any more. > > I suspect that one way or another, this change wants to be in a separate > patch. I think that if there are further adjustments to make (like dropping is_ioreq_server_page() altogether, as you appear to suggest), that would want to be in a separate patch, but the change as done fully fits the given justification. (Of course in such a patch both _could_ also be dropped at the same time.) >> --- a/xen/arch/x86/mm/shadow/multi.c >> +++ b/xen/arch/x86/mm/shadow/multi.c >> @@ -3441,7 +3441,7 @@ int sh_rm_write_access_from_sl1p(struct >> >> #ifdef CONFIG_HVM >> /* Remember if we've been told that this process is being torn down */ >> - if ( curr->domain == d && is_hvm_domain(d) ) >> + if ( curr->domain == d ) >> curr->arch.paging.shadow.pagetable_dying >> = mfn_to_page(gmfn)->pagetable_dying; >> #endif > > This one is dangerous. > > After tracing, I can see that sh_rm_write_access_from_sl1p() is only > called from OOS functions, but this function itself does its very best > to look like it has mixed PV + HVM usage, and dropping this conditional > means that pagetable_dying can, in principle at least, become non-NULL > for a PV guest. > > I think this function needs to be made far more obviously HVM-only first. Oh, sure - the #ifdef inside the functions can be replaced collectively by one around it, now that OOS code is built separately and for HVM only. >> --- a/xen/arch/x86/mm/shadow/oos.c >> +++ b/xen/arch/x86/mm/shadow/oos.c >> @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn >> if ( pg->shadow_flags & >> ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync) >> || sh_page_has_multiple_shadows(pg) >> - || !is_hvm_vcpu(v) >> || !v->domain->arch.paging.shadow.oos_active ) > > This is reachable for PV guests as far as I can see. What am I missing ? Well, the footnote in patch 1 ("x86/shadow: fix and improve sh_page_has_multiple_shadows()") kind of explains this wrt the safety of the sh_page_has_multiple_shadows() use here: Since PV guests can't have OOS pages, there's no way SHF_out_of_sync could be set. > The changes in hvm.c are all fine, and for those alone, consider it R-by > if you end up splitting the patch. Thanks, but for now I'm not meaning to split the patch, as per above. There will be a new prereq patch as per your request. Jan
On 24/03/2023 7:38 am, Jan Beulich wrote: > On 23.03.2023 19:18, Andrew Cooper wrote: >> On 22/03/2023 9:35 am, Jan Beulich wrote: >>> Emulation related functions are involved in HVM handling only, and in >>> some cases they even invoke such checks after having already done things >>> which are valid for HVM domains only. OOS active also implies HVM. In >>> sh_remove_all_mappings() one of the two checks is redundant with an >>> earlier paging_mode_external() one (the other, however, needs to stay). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/mm/shadow/common.c >>> +++ b/xen/arch/x86/mm/shadow/common.c >>> @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain >>> && (page->count_info & PGC_count_mask) <= 3 >>> && ((page->u.inuse.type_info & PGT_count_mask) >>> == (is_special_page(page) || >>> - (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) >>> + is_ioreq_server_page(d, page)))) ) >>> printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn >>> " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", >>> mfn_x(gmfn), gfn_x(gfn), >> Out of context here needs an equivalent adjustment. > I'm afraid I don't seen any further is_hvm_*() in this function. Final parameter to the printk(), calculating the i=%d diagnostic. >> But in this case, I'm not sure the commit message covers the relevant >> details. ioreq servers have been made fully common since this code was >> written, and *that* is a better reason for dropping the predicates IMO >> than the redundancy with paging_mode_external(). > How does "fully common" matter? It's still a HVM-only thing, ioreq server info used to be in d->arch.hvm.ioreq. The is_hvm_domain() was guarding against speculative type confusion in the HVM union, and should have been removed by the ARM work that made it common. This isn't really related to the outer paging_mode_external(). >> That said... I'm not sure the logic here is correct any more. It used >> to be the case that ioreq pages were in the p2m, but they're outside of >> the p2m these days, so don't see how there can be any interaction with >> unexpected refcounts any more. >> >> I suspect that one way or another, this change wants to be in a separate >> patch. > I think that if there are further adjustments to make (like dropping > is_ioreq_server_page() altogether, as you appear to suggest), that would > want to be in a separate patch, but the change as done fully fits the > given justification. (Of course in such a patch both _could_ also be > dropped at the same time.) I'd still suggest doing it all separately. It's sufficiently unrelated to the justification for the other hunks of the patch. >>> --- a/xen/arch/x86/mm/shadow/oos.c >>> +++ b/xen/arch/x86/mm/shadow/oos.c >>> @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn >>> if ( pg->shadow_flags & >>> ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync) >>> || sh_page_has_multiple_shadows(pg) >>> - || !is_hvm_vcpu(v) >>> || !v->domain->arch.paging.shadow.oos_active ) >> This is reachable for PV guests as far as I can see. What am I missing ? > Well, the footnote in patch 1 ("x86/shadow: fix and improve > sh_page_has_multiple_shadows()") kind of explains this wrt the safety > of the sh_page_has_multiple_shadows() use here: Since PV guests can't > have OOS pages, there's no way SHF_out_of_sync could be set. Hmm, I suppose. We enter sh_unsync() directly from a demand write, but it is only meaningful when OOS is active to begin with. Although having looked through this, there ought to be an early exit for oos_active even ahead of the SHADOW_PRINTK(), and the single caller of this function doesn't check the return value. (This appears to be a common theme...) ~Andrew
On 28.03.2023 15:57, Andrew Cooper wrote: > On 24/03/2023 7:38 am, Jan Beulich wrote: >> On 23.03.2023 19:18, Andrew Cooper wrote: >>> On 22/03/2023 9:35 am, Jan Beulich wrote: >>>> Emulation related functions are involved in HVM handling only, and in >>>> some cases they even invoke such checks after having already done things >>>> which are valid for HVM domains only. OOS active also implies HVM. In >>>> sh_remove_all_mappings() one of the two checks is redundant with an >>>> earlier paging_mode_external() one (the other, however, needs to stay). >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> --- a/xen/arch/x86/mm/shadow/common.c >>>> +++ b/xen/arch/x86/mm/shadow/common.c >>>> @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain >>>> && (page->count_info & PGC_count_mask) <= 3 >>>> && ((page->u.inuse.type_info & PGT_count_mask) >>>> == (is_special_page(page) || >>>> - (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) >>>> + is_ioreq_server_page(d, page)))) ) >>>> printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn >>>> " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", >>>> mfn_x(gmfn), gfn_x(gfn), >>> Out of context here needs an equivalent adjustment. >> I'm afraid I don't seen any further is_hvm_*() in this function. > > Final parameter to the printk(), calculating the i=%d diagnostic. That one. This needs to stay, as calling is_ioreq_server_page() isn't safe for PV domains. ioreq_domain_init(), which initializes the involved spinlock, is called only for HVM domains. This is why I'm insisting ... >>> But in this case, I'm not sure the commit message covers the relevant >>> details. ioreq servers have been made fully common since this code was >>> written, and *that* is a better reason for dropping the predicates IMO >>> than the redundancy with paging_mode_external(). >> How does "fully common" matter? It's still a HVM-only thing, > > ioreq server info used to be in d->arch.hvm.ioreq. > > The is_hvm_domain() was guarding against speculative type confusion in > the HVM union, and should have been removed by the ARM work that made it > common. > > This isn't really related to the outer paging_mode_external(). ... that the dropping that I can safely do here is solely because of the earlier paging_mode_external(). This is irrespective to what the check may (also) have been used for before. >>>> --- a/xen/arch/x86/mm/shadow/oos.c >>>> +++ b/xen/arch/x86/mm/shadow/oos.c >>>> @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn >>>> if ( pg->shadow_flags & >>>> ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync) >>>> || sh_page_has_multiple_shadows(pg) >>>> - || !is_hvm_vcpu(v) >>>> || !v->domain->arch.paging.shadow.oos_active ) >>> This is reachable for PV guests as far as I can see. What am I missing ? >> Well, the footnote in patch 1 ("x86/shadow: fix and improve >> sh_page_has_multiple_shadows()") kind of explains this wrt the safety >> of the sh_page_has_multiple_shadows() use here: Since PV guests can't >> have OOS pages, there's no way SHF_out_of_sync could be set. > > Hmm, I suppose. We enter sh_unsync() directly from a demand write, but > it is only meaningful when OOS is active to begin with. > > Although having looked through this, there ought to be an early exit for > oos_active even ahead of the SHADOW_PRINTK(), and the single caller of > this function doesn't check the return value. (This appears to be a > common theme...) Well, of course there's much more cleanup left than done by this series. Jan
--- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain && (page->count_info & PGC_count_mask) <= 3 && ((page->u.inuse.type_info & PGT_count_mask) == (is_special_page(page) || - (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) ) + is_ioreq_server_page(d, page)))) ) printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n", mfn_x(gmfn), gfn_x(gfn), --- a/xen/arch/x86/mm/shadow/hvm.c +++ b/xen/arch/x86/mm/shadow/hvm.c @@ -204,10 +204,6 @@ hvm_emulate_write(enum x86_segment seg, if ( rc || !bytes ) return rc; - /* Unaligned writes are only acceptable on HVM */ - if ( (addr & (bytes - 1)) && !is_hvm_vcpu(v) ) - return X86EMUL_UNHANDLEABLE; - ptr = sh_emulate_map_dest(v, addr, bytes, sh_ctxt); if ( IS_ERR(ptr) ) return ~PTR_ERR(ptr); @@ -258,10 +254,6 @@ hvm_emulate_cmpxchg(enum x86_segment seg if ( rc ) return rc; - /* Unaligned writes are only acceptable on HVM */ - if ( (addr & (bytes - 1)) && !is_hvm_vcpu(v) ) - return X86EMUL_UNHANDLEABLE; - ptr = sh_emulate_map_dest(v, addr, bytes, sh_ctxt); if ( IS_ERR(ptr) ) return ~PTR_ERR(ptr); @@ -457,8 +449,7 @@ static void *sh_emulate_map_dest(struct #ifndef NDEBUG /* We don't emulate user-mode writes to page tables. */ - if ( is_hvm_domain(d) ? hvm_get_cpl(v) == 3 - : !guest_kernel_mode(v, guest_cpu_user_regs()) ) + if ( hvm_get_cpl(v) == 3 ) { gdprintk(XENLOG_DEBUG, "User-mode write to pagetable reached " "emulate_map_dest(). This should never happen!\n"); @@ -487,15 +478,6 @@ static void *sh_emulate_map_dest(struct sh_ctxt->mfn[1] = INVALID_MFN; map = map_domain_page(sh_ctxt->mfn[0]) + (vaddr & ~PAGE_MASK); } - else if ( !is_hvm_domain(d) ) - { - /* - * Cross-page emulated writes are only supported for HVM guests; - * PV guests ought to know better. - */ - put_page(mfn_to_page(sh_ctxt->mfn[0])); - return MAPPING_UNHANDLEABLE; - } else { /* This write crosses a page boundary. Translate the second page. */ --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -3441,7 +3441,7 @@ int sh_rm_write_access_from_sl1p(struct #ifdef CONFIG_HVM /* Remember if we've been told that this process is being torn down */ - if ( curr->domain == d && is_hvm_domain(d) ) + if ( curr->domain == d ) curr->arch.paging.shadow.pagetable_dying = mfn_to_page(gmfn)->pagetable_dying; #endif --- a/xen/arch/x86/mm/shadow/oos.c +++ b/xen/arch/x86/mm/shadow/oos.c @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn if ( pg->shadow_flags & ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync) || sh_page_has_multiple_shadows(pg) - || !is_hvm_vcpu(v) || !v->domain->arch.paging.shadow.oos_active ) return 0;
Emulation related functions are involved in HVM handling only, and in some cases they even invoke such checks after having already done things which are valid for HVM domains only. OOS active also implies HVM. In sh_remove_all_mappings() one of the two checks is redundant with an earlier paging_mode_external() one (the other, however, needs to stay). Signed-off-by: Jan Beulich <jbeulich@suse.com>