Re: [PATCH v3 9/9] x86/P2M: relax permissions of PVH Dom0's MMIO entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2021 17:15, Roger Pau Monné wrote:
> On Thu, Sep 23, 2021 at 02:15:25PM +0200, Jan Beulich wrote:
>> On 23.09.2021 13:54, Roger Pau Monné wrote:
>>> On Thu, Sep 23, 2021 at 01:32:52PM +0200, Jan Beulich wrote:
>>>> On 23.09.2021 13:10, Roger Pau Monné wrote:
>>>>> On Tue, Sep 21, 2021 at 09:21:11AM +0200, Jan Beulich wrote:
>>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>>> @@ -1319,6 +1319,18 @@ static int set_typed_p2m_entry(struct do
>>>>>>              return -EPERM;
>>>>>>          }
>>>>>>  
>>>>>> +        /*
>>>>>> +         * Gross bodge, to go away again rather sooner than later:
>>>>>> +         *
>>>>>> +         * For MMIO allow access permissions to accumulate, but only 
>>>>>> for Dom0.
>>>>>> +         * Since set_identity_p2m_entry() and set_mmio_p2m_entry() 
>>>>>> differ in
>>>>>> +         * the way they specify "access", this will allow the ultimate 
>>>>>> result
>>>>>> +         * to be independent of the sequence of operations.
>>>>>
>>>>> Wouldn't it be better to 'fix' those operations so that they can work
>>>>> together?
>>>>
>>>> Yes, but then we should do this properly by removing all abuse of
>>>> p2m_access_t.
>>>
>>> I'm not sure I follow what that abuse is.
>>
>> As was clarified, p2m_access_t should be solely used by e.g.
>> introspection agents, who are then the entity responsible for
>> resolving the resulting faults. Any other uses (to e.g. merely
>> restrict permissions for other reasons) are really abuses.
> 
> But some p2m types don't really have a fixed access tied to them, for
> example MMIO regions just inherit whatever is the default access for
> the domain, which makes all this look slightly weird. If the access
> should solely be used by introspection, then each type should have a
> fixed access tied to it?

I think so, yes. Hence e.g. p2m_ram_ro and p2m_grant_map_r{w,o}.

>> That
>> is, if e.g. a r/o direct-MMIO mapping is needed, this should not
>> be expressed as (p2m_mmio_direct, p2m_access_r) tuple, but would
>> require a distinct p2m_mmio_direct_ro type.
> 
> I guess we would then require a p2m_mmio_direct_ro,
> p2m_mmio_direct_rwx and a p2m_mmio_direct_n at least, and ideally we
> would need to differentiate the mandatory regions as present in ACPI
> tables using yet different types.

What would we need p2m_mmio_direct_n for? And what's the (present,
not future) reason for the x in p2m_mmio_direct_rwx?

Jan