Re: [PATCH v2 1/3] xen/pci: do not register devices with segments >= 0x10000

[Bjorn Helgaas <helgaas@kernel.org>]

It looks like the convention for this file is to capitalize the
subject, e.g.,

  xen/pci: Do not register ...

On Tue, Jan 14, 2025 at 11:33:11AM +0100, Roger Pau Monne wrote:
> The current hypercall interface for doing PCI device operations always uses
> a segment field that has a 16 bit width.  However on Linux there are buses
> like VMD that hook up devices into the PCI hierarchy at segment >= 0x10000,
> after the maximum possible segment enumerated in ACPI.
> 
> Attempting to register or manage those devices with Xen would result in
> errors at best, or overlaps with existing devices living on the truncated
> equivalent segment values.  Note also that the VMD segment numbers are
> arbitrarily assigned by the OS, and hence there would need to be some
> negotiation between Xen and the OS to agree on how to enumerate VMD
> segments and devices behind them.
> 
> Skip notifying Xen about those devices.  Given how VMD bridges can
> multiplex interrupts on behalf of devices behind them there's no need for
> Xen to be aware of such devices for them to be usable by Linux.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Adjust commit message width to 75 columns.
>  - Expand commit message.
> ---
>  drivers/xen/pci.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> index 416f231809cb..08e82fd1263e 100644
> --- a/drivers/xen/pci.c
> +++ b/drivers/xen/pci.c
> @@ -43,6 +43,13 @@ static int xen_add_device(struct device *dev)
>  		pci_mcfg_reserved = true;
>  	}
>  #endif
> +

I think a brief comment here would be helpful so people don't have to
dig out the commit log to understand why this is invalid for Xen.

> +	if (pci_domain_nr(pci_dev->bus) >> 16) {
> +		dev_info(dev,
> +			 "not registering with Xen: invalid PCI segment\n");
> +		return 0;
> +	}
> +
>  	if (pci_seg_supported) {
>  		DEFINE_RAW_FLEX(struct physdev_pci_device_add, add, optarr, 1);
>  
> @@ -149,6 +156,12 @@ static int xen_remove_device(struct device *dev)
>  	int r;
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  
> +	if (pci_domain_nr(pci_dev->bus) >> 16) {
> +		dev_info(dev,
> +			 "not unregistering with Xen: invalid PCI segment\n");
> +		return 0;
> +	}
> +
>  	if (pci_seg_supported) {
>  		struct physdev_pci_device device = {
>  			.seg = pci_domain_nr(pci_dev->bus),
> @@ -182,6 +195,12 @@ int xen_reset_device(const struct pci_dev *dev)
>  		.flags = PCI_DEVICE_RESET_FLR,
>  	};
>  
> +	if (pci_domain_nr(dev->bus) >> 16) {
> +		dev_info(&dev->dev,
> +			 "unable to notify Xen of device reset: invalid PCI segment\n");
> +		return 0;
> +	}
> +
>  	return HYPERVISOR_physdev_op(PHYSDEVOP_pci_device_reset, &device);
>  }
>  EXPORT_SYMBOL_GPL(xen_reset_device);
> -- 
> 2.46.0
>