Hi,
This patch doesn't address most of my concerns with the last version.
My comments about callers of gva_to_gfn still stand -- basically,
gva_to_gfn should return an N1 GFN, and most callers (including hvm_copy)
should not need to know about N2 GFNs or multiple p2ms.
Also, you're still copying large parts of the existing HAP walker
(e.g. the next_level function). Can you avoid the duplication by
using a write_p2m_entry() callback, since that seems to be the part
that's different?
Skipping a flush when p2m->cr3 == 0 is not safe. I suggested using -1
as the magic value last time.
My comments on why p2m_flush_locked() isn't enough to reclaim an in-use
p2m for recycling still stand.
I have one more comment on the new code:
At 18:45 +0000 on 12 Nov (1289587557), Christoph Egger wrote:
> hap_write_p2m_entry(struct vcpu *v, unsigned long gfn, l1_pgentry_t *p,
> mfn_t table_mfn, l1_pgentry_t new, unsigned int level)
> {
> + struct domain *d = v->domain;
> uint32_t old_flags;
>
> - hap_lock(v->domain);
> + old_flags = l1e_get_flags(*p);
>
> - old_flags = l1e_get_flags(*p);
> + /* We know always use the host p2m here, regardless if the vcpu
> + * is in host or guest mode. The vcpu can be in guest mode by
> + * a hypercall which passes a domain and chooses mostly the first
> + * vcpu.
> + * XXX This is the reason why this function can not be used re-used
> + * for updating the nestedp2m. Otherwise, hypercalls would randomly
> + * operate on host p2m and nested p2m.
> + */
> + if ( nestedhvm_enabled(d) ) {
> + mfn_t omfn = _mfn(l1e_get_pfn(*p));
> + p2m_type_t op2mt = p2m_flags_to_type(old_flags);
> +
> + if ( p2m_is_valid(op2mt) && mfn_valid(omfn) ) {
I think you need to do this flush even if !mfn_valid(omfn) - for example
if you're removing a mapping of an MMIO area.
> + mfn_t nmfn = _mfn(l1e_get_pfn(new));
> + p2m_type_t np2mt = p2m_flags_to_type(l1e_get_flags(new));
> +
> + if ( p2m_is_valid(np2mt)
> + && mfn_valid(nmfn)
> + && !(l1e_get_flags(new) & _PAGE_PRESENT) )
I don't understand this test at all - you need to flush if you're
removing an old present entry regardless of what replaces it. The only
case where you can skip the flush is if old == new.
Cheers,
Tim.
> + {
> + /* This GFN -> MFN is going to get removed. */
> + /* XXX There is a more efficient way to do that
> + * but it works for now.
> + * Note, p2m_flush_nestedp2m calls hap_lock() internally.
> + */
> + p2m_flush_nestedp2m(d);
> + }
> + }
> + }
> +
> + hap_lock(d);
> +
--
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd. (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home