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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> --- 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". 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).

Jan