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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Jul 14, 2017 at 04:32:19AM -0600, Jan Beulich wrote:
> >>> 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.

IO-APICs can also be hot-plugged? Didn't even know about that...

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

It's not strictly necessary, the same can be accomplished by Dom0
calling PHYSDEVOP_manage_pci_add on each device.

Just thought it wouldn't hurt to do it here, but given your comment
below I'm not sure. I will wait for your reply before deciding what to
do.

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

Is this renumbering performed by changing the
Primary/Secondary/Subordinate bus number registers in the bridge?

If so we could detect such accesses (by adding traps to type 01h
headers) and react accordingly.

What if Dom0 re-numbers the bus after having already registered the
devices with Xen?

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

Ack.

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

Yes, this can be done in pci_mmcfg_reserved, it's just that so far
pci_mmcfg_reserved doesn't return -EEXIST for duplicated bridges.

> > @@ -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)?

Doesn't the IOMMU also need to know about the phantom functions in
order to add translations for them too?

I assume phantom_stride already takes care of this, so yes, if this
has to stay here a pdev->dev == devfn check should be added.

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

Right, that's not a problem.

Thanks, Roger.

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