Re: [Xen-devel] [PATCH v3 6/7] vpci/msix: carve p2m hole for MSIX MMIO regions

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

On Mon, Nov 05, 2018 at 10:07:24AM -0700, Jan Beulich wrote:
> >>> On 30.10.18 at 16:41, <roger.pau@xxxxxxxxxx> wrote:
> > Make sure the MSIX MMIO regions don't have p2m entries setup, so that
> > accesses to them trap into the hypervisor and can be handled by vpci.
> > 
> > This is a side-effect of commit 042678762 for PVH Dom0, which added
> > mappings for all the reserved regions into the Dom0 p2m.
> 
> I'm afraid the description is ambiguous or misleading, as I don't suppose
> you want to state that what the patch here does is a side effect of the
> mentioned commit. Instead I assume you mean that p2m entries we
> don't want get set up without the change here.

Yes, arch_iommu_hwdom_init will setup such entries. What's done here
is just to make sure there are no mappings established for the MSIX
MMIO regions, or else no trapping would happen. I will reword the
commit message to make it clearer.

> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -88,6 +88,14 @@ static void modify_decoding(const struct pci_dev *pdev, 
> > bool map, bool rom_only)
> >      uint16_t cmd;
> >      unsigned int i;
> >  
> > +    /*
> > +     * Make sure there are no mappings in the MSIX MMIO areas, so that 
> > accesses
> > +     * can be trapped (and emulated) by Xen when the memory decoding bit is
> > +     * enabled.
> > +     */
> > +    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> > +        return;
> 
> If I'm not mistaken, you punch holes after having set up p2m entries.
> This may be fine for Dom0, but looks racy for (future) DomU use of
> this code. If so, please add a respective fixme annotation.

Ack. All the BAR handling/mapping must be much more strict for DomU.

> > +int vpci_make_msix_hole(const struct pci_dev *pdev)
> > +{
> > +    struct domain *d = pdev->domain;
> > +    unsigned int i;
> > +
> > +    if ( !pdev->vpci->msix )
> > +        return 0;
> > +
> > +    /* Make sure there's a hole for the MSIX table/PBA in the p2m. */
> > +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> > +    {
> > +        unsigned long start = PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> > +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
> > +                                     vmsix_table_size(pdev->vpci, i) - 1);
> > +
> > +        for ( ; start <= end; start++ )
> > +        {
> > +            p2m_type_t t;
> > +            mfn_t mfn = get_gfn_query(d, start, &t);
> > +
> > +            if ( t == p2m_mmio_direct && mfn_x(mfn) == start )
> > +                    clear_identity_p2m_entry(d, start);
> 
> Indentation.
> 
> > +            else if ( t != p2m_mmio_dm )
> 
> Can you please also permit p2m_invalid right away, as the long term
> plan is to default to that type instead of p2m_mmio_dm for unpopulated
> p2m entries? And perhaps using switch() then produces easier to read
> code.

Sure.

Thanks, Roger.

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