Re: [PATCH v2 1/4] x86/dom0: prevent access to MMCFG areas for PVH Dom0

[Roger Pau Monne <roger.pau@citrix.com>]

On Tue, Aug 22, 2017 at 06:26:23AM -0600, Jan Beulich wrote:
> >>> On 11.08.17 at 18:43, <roger.pau@citrix.com> wrote:
> > They are emulated by Xen, so they must not be mapped into Dom0 p2m.
> > Introduce a helper function to add the MMCFG areas to the list of
> > denied iomem regions for PVH Dom0.
> 
> "They are" or "They are going to be"?

This started as a series on top of vPCI, but I think it has a chance
of getting in before vPCI. I will change it.

> > --- a/xen/arch/x86/dom0_build.c
> > +++ b/xen/arch/x86/dom0_build.c
> > @@ -440,6 +440,10 @@ int __init dom0_setup_permissions(struct domain *d)
> >              rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
> >      }
> >  
> > +    /* For PVH prevent access to the MMCFG areas. */
> > +    if ( dom0_pvh )
> > +        rc |= pci_mmcfg_set_domain_permissions(d);
> 
> What about ones reported by Dom0 later on? Which then raises the
> question whether ...

This should be dealt with in the PHYSDEVOP_pci_mmcfg_reserved handler.
But since you propose to do white listing, I guess it doesn't matter
that much anymore.

> > @@ -175,6 +177,25 @@ void pci_mmcfg_arch_disable(unsigned int idx)
> >             cfg->pci_segment, cfg->start_bus_number, cfg->end_bus_number);
> >  }
> >  
> > +int pci_mmcfg_set_domain_permissions(struct domain *d)
> > +{
> > +    unsigned int idx;
> > +    int rc = 0;
> > +
> > +    for ( idx = 0; idx < pci_mmcfg_config_num; idx++ )
> > +    {
> > +        const struct acpi_mcfg_allocation *cfg = pci_mmcfg_virt[idx].cfg;
> > +        unsigned long start = PFN_DOWN(cfg->address) +
> > +                              PCI_BDF(cfg->start_bus_number, 0, 0);
> > +        unsigned long end = PFN_DOWN(cfg->address) +
> > +                            PCI_BDF(cfg->end_bus_number, ~0, ~0);
> > +
> > +        rc |= iomem_deny_access(d, start, end);
> 
> ... this shouldn't be unnecessary by, other than PV Dom0,
> starting out with no I/O memory being made accessible (i.e.
> white listing just like we decided we would do for other
> properties for PVH).

So would you like to switch to this white listing mode even for PV
Dom0, or just for PVH?

Should reserved regions and holes be added to it? Maybe only reserved
regions?

> Additionally while in the code that dom0_setup_permissions()
> was broken out from using |= was fine, there and here it's not
> really appropriate unless we want to continue to bake in the
> assumption that either iomem_deny_access() can only ever
> return a single error indicator or (b) the callers only care about
> the value being (non-)zero.

Right, I can fix that.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel