Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/06/2019 11:48, George Dunlap wrote:
> On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote:
>>
>> On 26.09.2018 19:47, George Dunlap wrote:
>>> From: Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>
>>>
>>> This patch adds access control for NPT mode.
>>>
>>> There aren’t enough extra bits to store the access rights in the NPT p2m
>>> table, so we add a radix tree to store extra information.
>>>
>>> For efficiency:
>>>   - Only allocate this radix tree when we first store "non-default"
>>>     extra information
>>>
>>>   - Remove entires which match the default extra information rather
>>>     than continuing to store them
>>>
>>>   - For superpages, only store an entry for the first gfn in the
>>>     superpage.  Use the order of the p2m entry being read to determine
>>>     the proper place to look in the radix table.
>>>
>>> Modify p2m_type_to_flags() to accept and interpret an access value,
>>> parallel to the ept code.
>>>
>>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>>> of the p2m rather than setting it directly, to deal with different
>>> default permitted access values.
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>> ---
>>> NB, this is compile-tested only.
>>>
>>> cc'ing Paul because this is functionality he may want at some point in
>>> the future.
>>>
>>> I'm not sure why we only allow 'int' to be stored in the radix tree,
>>> but that throws away 30-some bits we could otherwise use.  We might
>>> consider revising this if we run out of bits here.
>>>
>>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>>> CC: Tim Deegan <tim@xxxxxxx>
>>> CC: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>>> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> Hi all,
>>
>> I know it's been some time from the start of this patch but can this 
>> move forward? Any thoughts or acks are appreciated.
> Right, well where we left this, the situation was that on AMD hardware
> with IOMMU / p2m sharing, there aren't enough bits.
>
> The two general fixes we have so far are:
> 1. Add a parallel tree for extra bits (this patch)
> 2. Rip out IOMMU / p2m sharing for AMD.
>
> #2 has the advantage that we don't need an entirely separate tree, as
> well as getting rid of code that has (apparently) been completely dead
> for 5 years. #1 has the advantage that we're set up for having a much
> larger number of IOREQ servers in the future.
>
> Nobody objected to #2.  Without looking deeply into it, it seems like it
> might be a good idea, but I can't be sure without seeing what it would
> actually look like.
>
> The easiest way to press the point then would be to post a patch series
> ripping it out.

IOMMU / p2m sharing on AMD doesn't work, and isn't available any more
(despite the code looking suspiciously like it is usable).

There was a bugfix to allow DMA mapping of granted pages, which is a
prerequisite for PVH support, which requires using a nonzero p2m type,
and is therefore incompatible with IOMMU/p2m sharing.

I don't see any feasible way to bring p2m-sharing back into a working
state on AMD.

As a result, we'd be much better ripping out the dead code and more
formally acknowledging that it is a dead feature, after which the
existing p2m type/access infrastructure should work compatibly with the
Intel implementation.

~Andrew

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