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 05:22:08PM +0200, Jan Beulich wrote:
> 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?

AMD can specify no access at all for certain regions on the ACPI
tables from what I've read on the manual (IW = IR = 0 in IVMD Flags).
AFAICT amd_iommu_reserve_domain_unity_map can already call
iommu_identity_mapping with access p2m_access_n and that would get
propagated into set_identity_p2m_entry.

> And what's the (present,
> not future) reason for the x in p2m_mmio_direct_rwx?

Mapped ROM BARs, but I'm also unsure we shouldn't just map MMIO with
execute permissions by default unless stated otherwise.

Thanks, Roger.