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