Re: [Xen-devel] [PATCH v4 3/9] x86/physdev: enable PHYSDEVOP_pci_mmcfg_reserved for PVH Dom0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.06.17 at 17:01, <roger.pau@xxxxxxxxxx> wrote:
> So that hotplug (or MMCFG regions not present in the MCFG ACPI table)
> can be added at run time by the hardware domain.

I think the emphasis should be the other way around. I'm rather certain
hotplug of bridges doesn't really work right now anyway; at least
IO-APIC hotplug code is completely missing.

> When a new MMCFG area is added to a PVH Dom0, Xen will scan it and add
> the devices to the hardware domain.

Adding the MMIO regions is certainly necessary, but what's the point of
also scanning the bus and adding the devices? We expect Dom0 to tell us
anyway, and not doing the scan in Xen avoids complications we presently
have in the segment 0 case when Dom0 decides to re-number busses (e.g.
in order to fit in SR-IOV VFs).

> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -89,6 +89,10 @@ static long hvm_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( !has_pirq(curr->domain) )
>              return -ENOSYS;
>          break;
> +    case PHYSDEVOP_pci_mmcfg_reserved:
> +        if ( !is_hardware_domain(curr->domain) )
> +            return -ENOSYS;
> +        break;

This physdevop (like most ones) is restricted to Dom0 use anyway
(properly expressed via XSM check), so I'd rather see you check
has_vpci() here, in line with e.g. the check visible in context.

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -559,6 +559,25 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          ret = pci_mmcfg_reserved(info.address, info.segment,
>                                   info.start_bus, info.end_bus, info.flags);
> +        if ( ret || !is_hvm_domain(currd) )
> +            break;
> +
> +        /*
> +         * For HVM (PVH) domains try to add the newly found MMCFG to the
> +         * domain.
> +         */
> +        ret = register_vpci_mmcfg_handler(currd, info.address, 
> info.start_bus,
> +                                          info.end_bus, info.segment);
> +        if ( ret == -EEXIST )
> +        {
> +            ret = 0;
> +            break;

I don't really understand this part: Why would handlers be registered
already? If you consider double registration, wouldn't that better
either be detected by pci_mmcfg_reserved() (and the call here avoided
altogether) or the fact indeed be reported back to the caller?

> @@ -1110,6 +1110,37 @@ void __hwdom_init setup_hwdom_pci_devices(
>      pcidevs_unlock();
>  }
>  
> +static int add_device(uint8_t devfn, struct pci_dev *pdev)
> +{
> +    return iommu_add_device(pdev);
> +}

You're discarding devfn here, just for iommu_add_device() to re-do the
phantom function handling. At the very least this is wasteful. Perhaps
you minimally want to call iommu_add_device() only when
devfn == pdev->devfn (if all of this code stays in the first place)?

> +int pci_scan_and_setup_segment(uint16_t segment)
> +{
> +    struct pci_seg *pseg = get_pseg(segment);
> +    struct setup_hwdom ctxt = {
> +        .d = current->domain,
> +        .handler = add_device,
> +    };
> +    int ret;
> +
> +    if ( !pseg )
> +        return -EINVAL;
> +
> +    pcidevs_lock();
> +    ret = _scan_pci_devices(pseg, NULL);
> +    if ( ret )
> +        goto out;
> +
> +    ret = _setup_hwdom_pci_devices(pseg, &ctxt);
> +    if ( ret )
> +        goto out;
> +
> + out:

Please let's avoid such unnecessary goto-s. Even the first one could be
easily avoided without making the code anywhere near unreadable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel