RE: [PATCH 2/3] PCI: Enable host bridge emulation for PCI_DOMAINS_GENERIC platforms

[dan.j.williams]

Michael Kelley wrote:
[..]
> > Here is the replacement fixup that I will fold in if it looks good to
> > you:
> > 
> > -- 8< --
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index cfe9806bdbe4..f1079a438bff 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -3642,9 +3642,9 @@ static int hv_pci_probe(struct hv_device *hdev,
> >  {
> >  	struct pci_host_bridge *bridge;
> >  	struct hv_pcibus_device *hbus;
> > -	u16 dom_req, dom;
> > +	int ret, dom;
> > +	u16 dom_req;
> >  	char *name;
> > -	int ret;
> > 
> >  	bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> >  	if (!bridge)
> > @@ -3673,8 +3673,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> >  	 * collisions) in the same VM.
> >  	 */
> >  	dom_req = hdev->dev_instance.b[5] << 8 | hdev->dev_instance.b[4];
> > -	dom = pci_bus_find_emul_domain_nr(dom_req);
> > -
> 
> As an additional paragraph the larger comment block above, let's include a
> massaged version of the comment associated with HVPCI_DOM_INVALID.
> Perhaps:
> 
>  *
>  * Because Gen1 VMs use domain 0, don't allow picking domain 0 here, even
>  * if bytes 4 and 5 of the instance GUID are both zero.
>  */

That looks good to me.

> 
> > +	dom = pci_bus_find_emul_domain_nr(dom_req, 1, U16_MAX);
> >  	if (dom < 0) {
> >  		dev_err(&hdev->device,
> >  			"Unable to use dom# 0x%x or other numbers", dom_req);
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index f60244ff9ef8..30935fe85af9 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -881,7 +881,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >  	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
> > 
> >  	sd->vmd_dev = vmd->dev;
> > -	sd->domain = pci_bus_find_emul_domain_nr(PCI_DOMAIN_NR_NOT_SET);
> > +
> > +	/*
> > +	 * Emulated domains start at 0x10000 to not clash with ACPI _SEG
> > +	 * domains.  Per ACPI r6.0, sec 6.5.6,  _SEG returns an integer, of
> > +	 * which the lower 16 bits are the PCI Segment Group (domain) number.
> > +	 * Other bits are currently reserved.
> > +	 */
> > +	sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
> >  	if (sd->domain < 0)
> >  		return sd->domain;
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 833ebf2d5213..de42e53f07d0 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6695,34 +6695,15 @@ static void pci_no_domains(void)
> >  #ifdef CONFIG_PCI_DOMAINS
> >  static DEFINE_IDA(pci_domain_nr_dynamic_ida);
> > 
> > -/*
> > - * Find a free domain_nr either allocated by pci_domain_nr_dynamic_ida or
> > - * fallback to the first free domain number above the last ACPI segment number.
> > - * Caller may have a specific domain number in mind, in which case try to
> > - * reserve it.
> > - *
> > - * Note that this allocation is freed by pci_release_host_bridge_dev().
> > +/**
> > + * pci_bus_find_emul_domain_nr() - allocate a PCI domain number per constraints
> > + * @hint: desired domain, 0 if any id in the range of @min to @max is acceptable
> > + * @min: minimum allowable domain
> > + * @max: maximum allowable domain, no ids higher than INT_MAX will be returned
> >   */
> > -int pci_bus_find_emul_domain_nr(int hint)
> > +u32 pci_bus_find_emul_domain_nr(u32 hint, u32 min, u32 max)
> 
> Shouldn't the return type here still be "int"?  ida_alloc_range() can return a negative
> errno if it fails. And the call sites in hv_pci_probe() and vmd_enable_domain()
> store the return value into an "int".

Yup, good catch.

> Other than that, and my suggested added comment, this looks good.

Thanks!