Re: [Xen-devel] Ping: [PATCH v3 0/4] x86/HVM: implement memory read caching

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Oct 2, 2018, at 11:51 AM, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 02/10/18 11:36, Jan Beulich wrote:
>>>>> On 25.09.18 at 16:14, <JBeulich@xxxxxxxx> wrote:
>>> Emulation requiring device model assistance uses a form of instruction
>>> re-execution, assuming that the second (and any further) pass takes
>>> exactly the same path. This is a valid assumption as far as use of CPU
>>> registers goes (as those can't change without any other instruction
>>> executing in between), but is wrong for memory accesses. In particular
>>> it has been observed that Windows might page out buffers underneath
>>> an instruction currently under emulation (hitting between two passes).
>>> If the first pass translated a linear address successfully, any subsequent
>>> pass needs to do so too, yielding the exact same translation.
>>> 
>>> Introduce a cache (used just by guest page table accesses for now, i.e.
>>> a form of "paging structure cache") to make sure above described
>>> assumption holds. This is a very simplistic implementation for now: Only
>>> exact matches are satisfied (no overlaps or partial reads or anything).
>>> 
>>> There's also some seemingly unrelated cleanup here which was found
>>> desirable on the way.
>>> 
>>> 1: x86/mm: add optional cache to GLA->GFN translation
>>> 2: x86/mm: use optional cache in guest_walk_tables()
>>> 3: x86/HVM: implement memory read caching
>>> 4: x86/HVM: prefill cache with PDPTEs when possible
>>> 
>>> As for v2, I'm omitting "VMX: correct PDPTE load checks" from v3, as I
>>> can't currently find enough time to carry out the requested further
>>> rework.
>> Andrew, George?
> 
> You've not fixed anything from my concerns with v1.
> 
> This doesn't behave like real hardware, and definitely doesn't behave as
> named - "struct hvmemul_cache" is simply false.  If it were named
> hvmemul_psc (or some other variation on Paging Structure Cache) then it
> wouldn't be so bad, as the individual levels do make more sense in that
> context (not that it would make the behaviour any closer to how hardware
> actually works).
> 
> I'm also not overly happy with the conditional nature of the caching, or
> that it isn't a transparent read-through cache.  This leads to a large
> amount of boilerplate code for every user.

What I’m hearing from you are three basic objections:

1. Although it’s closer to real hardware in some ways, it’s still pretty far 
away

2. The name “cache” is confusing

3. Having it non-transparent adds a lot of boilerplate code to the places that 
do need it.

#2 is easily dealt with.  The other two are reasons to look for better options, 
but not reasons to reject Jan’s series if other improvements are a lot of extra 
work (or it’s not clear they’re better).

Since this is a bug fix, unless you can show that Jan’s series introduces worse 
bugs, I think Jan’s request that you either 1) fix it yourself by 4.12, or 2) 
acquiesce to this series (or something close to it) being accepted is 
reasonable.

If you want to say, “I won’t Ack it but I won’t object if someone else does”, 
then I’ll get to it when I get a chance (hopefully before November).

 -George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel