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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

> 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.

Thanks, Roger.