Re: [PATCH] x86/PVH: PHYSDEVOP_pci_mmcfg_reserved should not blindly register a region

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 08, 2020 at 05:11:35PM +0200, Jan Beulich wrote:
> On 08.05.2020 17:03, Roger Pau Monné wrote:
> > On Fri, May 08, 2020 at 02:43:38PM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/hvm/io.c
> >> +++ b/xen/arch/x86/hvm/io.c
> >> @@ -558,6 +558,47 @@ int register_vpci_mmcfg_handler(struct d
> >>      return 0;
> >>  }
> >>  
> >> +int unregister_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> >> +                                  unsigned int start_bus, unsigned int 
> >> end_bus,
> >> +                                  unsigned int seg)
> >> +{
> >> +    struct hvm_mmcfg *mmcfg;
> >> +    int rc = -ENOENT;
> >> +
> >> +    ASSERT(is_hardware_domain(d));
> >> +
> >> +    if ( start_bus > end_bus )
> >> +        return -EINVAL;
> >> +
> >> +    write_lock(&d->arch.hvm.mmcfg_lock);
> >> +
> >> +    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
> >> +        if ( mmcfg->addr == addr + (start_bus << 20) &&
> >> +             mmcfg->segment == seg &&
> >> +             mmcfg->start_bus == start_bus &&
> >> +             mmcfg->size == ((end_bus - start_bus + 1) << 20) )
> >> +        {
> >> +            list_del(&mmcfg->next);
> >> +            if ( !list_empty(&d->arch.hvm.mmcfg_regions) )
> >> +                xfree(mmcfg);
> >> +            else
> >> +            {
> >> +                /*
> >> +                 * Cannot unregister the MMIO handler - leave a fake entry
> >> +                 * on the list.
> >> +                 */
> >> +                memset(mmcfg, 0, sizeof(*mmcfg));
> >> +                list_add(&mmcfg->next, &d->arch.hvm.mmcfg_regions);
> > 
> > Instead of leaving this zombie entry around maybe we could add a
> > static bool in register_vpci_mmcfg_handler to signal whether the MMIO
> > intercept has been registered?
> 
> That was my initial plan indeed, but registration is per-domain.

Indeed, this would work now because it's only used by the hardware
domain, but it's not a good move long term.

What about splitting the registration into a
register_vpci_mmio_handler and call it from hvm_domain_initialise
like it's done for register_vpci_portio_handler?

That might be cleaner long term, sorry if it's more work.

> >> --- a/xen/arch/x86/physdev.c
> >> +++ b/xen/arch/x86/physdev.c
> >> @@ -559,12 +559,18 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
> >>          if ( !ret && has_vpci(currd) )
> >>          {
> >>              /*
> >> -             * For HVM (PVH) domains try to add the newly found MMCFG to 
> >> the
> >> -             * domain.
> >> +             * For HVM (PVH) domains try to add/remove the reported MMCFG
> >> +             * to/from the domain.
> >>               */
> >> -            ret = register_vpci_mmcfg_handler(currd, info.address,
> >> -                                              info.start_bus, 
> >> info.end_bus,
> >> -                                              info.segment);
> >> +            if ( info.flags & XEN_PCI_MMCFG_RESERVED )
> > 
> > Do you think you could also add a small note in physdev.h regarding
> > the fact that XEN_PCI_MMCFG_RESERVED is used to register a MMCFG
> > region, and not setting it would imply an unregister request?
> > 
> > It's not obvious to me from the name of the flag.
> 
> The main purpose of the flag is to identify whether a region can be
> used (because of having been found marked suitably reserved by
> firmware). The flag not set effectively means "region is not marked
> reserved".

Looking at pci_mmcfg_arch_disable, should the region then also be
removed from mmio_ro_ranges? (kind of tangential to this patch)

> You pointing this out makes me wonder whether instead I
> should simply expand the if() in context, without making it behave
> like unregistration. Then again we'd have no way to unregister a
> region, and hence (ab)using this function for this purpose seems to
> makes sense (and, afaict, not require any code changes elsewhere).

Right now the only user I know of PHYSDEVOP_pci_mmcfg_reserved is
Linux, and AFAICT it always sets the XEN_PCI_MMCFG_RESERVED flag (at
least upstream).

I don't mind that much what we end up doing, as long as it's
documented in physdev.h. There's no documentation of that physdevop
hypercall at all, so if we provide proper documentation I would be
fine with treating a call with no flags as an unregistration request
(which is kind of what we already do for a classic PV hardware
domain).

Thanks, Roger.