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 02:43:38PM +0200, Jan Beulich wrote:
> The op has a register/unregister flag, and hence registration shouldn't
> happen unilaterally. Introduce unregister_vpci_mmcfg_handler() to handle
> this case.
> 
> Fixes: eb3dd90e4089 ("x86/physdev: enable PHYSDEVOP_pci_mmcfg_reserved for 
> PVH Dom0")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- 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?

> +            }
> +            rc = 0;
> +            break;
> +        }
> +
> +    write_unlock(&d->arch.hvm.mmcfg_lock);
> +
> +    return rc;
> +}
> +
>  void destroy_vpci_mmcfg(struct domain *d)
>  {
>      struct list_head *mmcfg_regions = &d->arch.hvm.mmcfg_regions;
> --- 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.

Thanks, Roger.