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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, May 11, 2020 at 03:46:38PM +0200, Jan Beulich wrote:
> On 08.05.2020 18:08, Roger Pau Monné wrote:
> > 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?
> 
> No, the goal is to not register unneeded handlers. But see below -
> I'll likely ditch the function anyway.
> 
> >>>> --- 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)
> 
> If it's truly unregistration - yes. But ...
> 
> >> 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've looked at our forward port, where this was first introduced.
> There we made the call in all cases, with the flag indicating what is
> wanted. Therefore I don't think we want to assign the flag being
> clear the meaning of "unregistration". I'll therefore switch to the
> simpler change of just expanding the if().

I'm not opposed to this. Leaving the vpci MMIO handlers for disabled
regions is fine, writes will be ignored and reads will return ~0.

This will prevent a PVH hardware domain from accessing those broken
MMCFG regions if it really wants to, but I think it's similar to how a
classic PV dom0 would behave (with the exception that in that case the
domain would be allowed to read from the MMCFG area).

Thanks, Roger.