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.
