Re: [Xen-devel] [PATCH v2] pci: a workaround for nonstandard PCI devices whose PBA shares

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.04.18 at 15:16, <chao.gao@xxxxxxxxx> wrote:
> Given that parsing parameters starts at very early stage in which xmalloc is
> unusable, I choose to continue using an array other than a list to store SBDFs
> of such kind devices, like the way how we manage phantom_devs.

Yes, I was about to say that on v1 when I saw v2 come in.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1423,6 +1423,16 @@ Defaults to booting secondary processors.
>  
>  > Default: `on`
>  
> +### pba\_shared\_quirk
> +> `= List of [<seg>:]<bus>:<device>.<function>`
> +
> +Specify a list of SBDF of devices. When assigning devices in this list
> +to guest, reading or writing the page where MSI-X PBA resides are
> +allowed. This option provides a workaround for nonstandard PCI devices
> +whose MSI-X PBA shares the same 4K-byte page with registers irrelevant
> +to MSI-X. Note that adding an untrusted device to this option would
> +undermine security of the entire system.

No underscores in new command line options please - use dashes.

I'm not convinced though that a global option is well suited here:
Exposing the device in this way may be okay for one guest, but
not for another. With your model one would need to reboot the
entire host for such a usage change.

Furthermore boot time specification of SBDF is a problem with the
Dom0 kernel possibly re-organizing the topology, and is not going
to work well with hot-plugged devices.

IOW - I think this setting needs to be specified at device
assignment time.

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -992,7 +992,22 @@ static int msix_capability_init(struct pci_dev *dev,
>          if ( rangeset_add_range(mmio_ro_ranges, msix->table.first,
>                                  msix->table.last) )
>              WARN();
> -        if ( rangeset_add_range(mmio_ro_ranges, msix->pba.first,
> +
> +        /*
> +         * If pages where MSI-X PBA resides overlap with other read-only mmio
> +         * range, pba_shared_quirk won't meet our desire. Hence disable it.
> +         */
> +        if ( msix->pba_shared_quirk_enabled &&
> +             rangeset_overlaps_range(mmio_ro_ranges, msix->pba.first,
> +                                     msix->pba.last) )
> +        {
> +            printk("PBA_shared_quirk is disabled for %04x:%02x:%02x.%u",
> +                   seg, bus, slot, func);
> +            msix->pba_shared_quirk_enabled = false;
> +        }
> +
> +        if ( !msix->pba_shared_quirk_enabled &&
> +             rangeset_add_range(mmio_ro_ranges, msix->pba.first,
>                                  msix->pba.last) )
>              WARN();
>  
> @@ -1139,7 +1154,8 @@ static void _pci_cleanup_msix(struct arch_msix *msix)
>          if ( rangeset_remove_range(mmio_ro_ranges, msix->table.first,
>                                     msix->table.last) )
>              WARN();
> -        if ( rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
> +        if ( !msix->pba_shared_quirk_enabled &&
> +             rangeset_remove_range(mmio_ro_ranges, msix->pba.first,
>                                     msix->pba.last) )

I don't think you need to alter the condition here.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -184,6 +184,39 @@ static int __init parse_phantom_dev(const char *str)
>  }
>  custom_param("pci-phantom", parse_phantom_dev);
>  
> +static struct pba_shared_quirk_dev {
> +    pci_sbdf_t sbdf;
> +} pba_shared_quirk_devs[8];

Any reason this can't be of type pci_sbdf_t[8]?

> +static unsigned int nr_pba_shared_quirk_devs;

Both __read_mostly please.

> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -239,6 +239,7 @@ struct arch_msix {
>      int table_idx[MAX_MSIX_TABLE_PAGES];
>      spinlock_t table_lock;
>      bool host_maskall, guest_maskall;
> +    bool pba_shared_quirk_enabled;

I don't think you need the "_enabled" suffix - it's a boolean after all.

Jan


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