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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan



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