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

[Jan Beulich <jbeulich@xxxxxxxx>]

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