Hi,
At 10:35 +0800 on 27 Mar (1427452549), Kai Huang wrote:
> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
> +{
> + uint64_t *pml_buf;
> + unsigned long pml_idx;
> +
> + ASSERT(vmx_vcpu_pml_enabled(v));
> +
> + vmx_vmcs_enter(v);
> +
> + __vmread(GUEST_PML_INDEX, &pml_idx);
> +
> + /* Do nothing if PML buffer is empty */
> + if ( pml_idx == (PML_ENTITY_NUM - 1) )
> + goto out;
> +
> + pml_buf = map_domain_page(page_to_mfn(v->arch.hvm_vmx.pml_pg));
> +
> + /*
> + * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is
> not
> + * full), and in latter case PML index always points to next available
> + * entity.
> + */
> + if (pml_idx >= PML_ENTITY_NUM)
> + pml_idx = 0;
> + else
> + pml_idx++;
> +
> + for ( ; pml_idx < PML_ENTITY_NUM; pml_idx++ )
> + {
> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> + unsigned long gfn;
> + mfn_t mfn;
> + p2m_type_t t;
> + p2m_access_t a;
> +
> + gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
> + mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
Please don't call p2m->get_entry() directly -- that interface should
only be used inside the p2m code. As it happens, I don't think this
lookup is correct anyway: the logging only sees races (which are not
interesting) or buggy hardware (which is not worth the extra lookup to
detect).
So you only need this to get 'mfn' to pass to paging_mark_dirty().
That's also buggy, because there's no locking here to make sure
gfn->mfn->gfn ends up in the right place. :(
I think the right thing to do is:
- split paging_park_dirty() into paging_mark_gfn_dirty() (the bulk of
the current function) and a paging_mark_dirty() wrapper that does
get_gpfn_from_mfn(mfn_x(gmfn)) and calls paging_mark_gfn_dirty().
- call paging_mark_gfn_dirty() from vmx_vcpu_flush_pml_buffer().
That will avoid _two_ p2m lookups in this function. :)
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
