Re: [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel <tamas@tklengyel.com>]

On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap <george.dunlap@citrix.com> wrote:
> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>> I could go on in the analysis, but the point is that there's a morass
>>> of interactions here all of which need to be correct, which this patch
>>> does not address.  You have a long way to go before sharing and altp2m
>>> can be safely used on the same gfn ranges.
>>>
>>
>> Hi George,
>> certainly there are cornercases where if the user does not setup things in
>> the right order things can still go out of whack. My goal with this patch is
>> not to address all of them. The goal of this patch is to not crash the
>> hypervisor when the user at least tries to experiment with it, which is the
>> current state. So this patch improves the status quo. Also, both mem_sharing
>> and altp2m is considered experimental/tech_preview, so the fact that their
>> combination is also experimental should be assumed as well. As I explained,
>> with this patch in place there is at least one way they can be safely used
>> together if the user tracks unsharing requirements through mem_access and
>> does unsharing and fixup of the altp2m views manually. There are other ways
>> where it would not be safe as after unsharing we don't know how the user
>> would want things to look in altp2ms. I don't think we want to start
>> guessing about that either so I will not be looking to implement that. So I
>> don't agree with this reasoning being grounds for rejecting this patch that
>> does incrementally improve the current state.
>
> So you keep saying "user"; I assume you mean whatever program is
> sitting in domain 0, which is going to be doing memsharing, altp2m,
> and memaccess stuff all at once?
>
> The altp2m code was not written for that purpose.  It was written for
> *guest* administrators to use within the guest.

That's simply not true. It was written specifically to allow both
usecases - both internal *and* external. Mixing the use-cases was not
envisioned.

  There's no problem
> with finding additional uses for it, as long as the *original* purpose
> doesn't get broken; and this patch most certainly does break things
> for that purpose.

This patch most certainly *does not break* the in-guest usecase by
itself. If the in-guest usecase is used without any mem_sharing going
on, this patch does not have any effect on that usecase.

  Guests using altp2m should not have to know that
> sharing is happening behind their backs; nor should they be required
> to use mem_access to manually fix up what the hypervisor has allowed
> to be broken.

And they are not required. This requirement only applies if the user
mixes in in-guest and external usecases.

>
> If at the moment altp2m + memsharing just plain crashes, then that
> should be fixed; and if the lock-ordering parts of the patch fix that,
> then they should be applied.

Yes, that would be a start at least.

  But the code which make sure that the
> same gfn range cannot both be shared and subject to altp2m must remain
> until they interact properly, with all the corner cases carefully
> thought out.

Well, I don't see that what you suggest is going to happen if
incremental improvements are not allowed in. Anyhow, at this point I'm
going to start carrying out-of-tree patches for Xen in my project and
just resign from my mem_sharing maintainership as I feel like it's
pretty pointless.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel