On 20/02/2019 14:37, Jan Beulich wrote:
>>>> On 19.02.19 at 23:18, <andrew.coop...@citrix.com> wrote:
>> @@ -58,25 +57,67 @@ altp2m_vcpu_destroy(struct vcpu *v)
>>
>> int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn)
>> {
>> + struct domain *d = v->domain;
>> + struct altp2mvcpu *a = &vcpu_altp2m(v);
>> p2m_type_t p2mt;
>> + mfn_t mfn;
>> + struct page_info *pg;
>> + int rc;
>> +
>> + /* Early exit path if #VE is already configured. */
>> + if ( a->veinfo_pg )
>> + return -EEXIST;
>> +
>> + mfn = get_gfn(d, gfn_x(gfn), &p2mt);
>> +
>> + /*
>> + * Looking for a plain piece of guest writeable RAM. Take an extra page
>> + * reference to reflect our intent to point the VMCS at it.
>> + */
>> + if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) ||
>> + p2m_is_readonly(p2mt) || !get_page(pg = mfn_to_page(mfn), d) )
> p2m_is_ram() is pretty broad a class, and p2m_is_readonly() removes
> only usefully p2m_ram_ro and p2m_ram_shared from the set. In
> particular p2m_ioreq_server is thus permitted, as are all p2m_paging_*
> which don't produce INVALID_MFN. I don't think that's what you want.
> I also don't think you really want to exclude p2m_ram_logdirty - if
> anything you might need to convert that type to p2m_ram_rw in case
> you find the page in that state.
Flipping logdirty back to ram cannot reasonably be a caller requirement,
but you are right that logdirty pages ought to be eligible. Holding
extra references doesn't interact with logdirty operations.
> Can't you simply call check_get_page_from_gfn() here anyway?
>
> Furthermore I'm uncertain if get_page() is quite enough: Don't you
> also want a writable type reference? It may not strictly be needed at
> this point, but I think we're trying to make the distinction in new code,
> like was e.g. done in recent Viridian changes.
No - I don't want to take a writeable reference.
Not because it is necessarily the wrong thing to do longterm, but
because it is not the current behaviour, and is therefore substantially
more risky in terms of subtle unexpected side effects (especially for
4.12 at this point).
We have years and years of XSAs demonstrating that things tend to go
wrong. It is irresponsible to start introducing a new reference
counting scheme without doing the work to first justify the safety of
change, and have a reasonable demonstration that the result is safe,
e.g. with an IOMMU in the mix.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
