Re: [Xen-devel] [v8][PATCH 06/16] hvmloader/pci: disable all pci devices conflicting with rdm

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 16, 2015 at 5:18 PM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Thu, Jul 16, 2015 at 4:39 PM, George Dunlap <george.dunlap@xxxxxxxxxx> 
> wrote:
>> On 07/16/2015 04:20 PM, Chen, Tiejun wrote:
>>>>> What about this?
>>>>
>>>> Looks reasonable (but don't forget that I continue to be unconvinced
>>>> that the patch as a whole makes sense).
>>>
>>> Yes, I always keep this in my mind as I mentioned in patch #00. Any risk
>>> you're still concerning? Is it that case if guest OS force enabling
>>> these devices again? IMO, at this point there are two cases:
>>>
>>> #1. Without passing through a RMRR device
>>>
>>> Those emulated devices don't create 1:1 mapping so its safe, right?
>>>
>>> #2. With passing through a RMRR device
>>>
>>> This just probably cause these associated devices not to work well, but
>>> still don't bring any impact to other Domains, right? I mean this isn't
>>> going to worsen the preexisting situation.
>>>
>>> If I'm wrong please correct me.
>>
>> But I think the issue is, without doing *something* about MMIO
>> collisions, the feature as a whole is sort of pointless.  You can
>> carefully specify rdm="strategy=host,reserved=strict", but you might
>> still get devices whose MMIO regions conflict with RMMRs, and there's
>> nothing you can really do about it.
>>
>> And although I personally think it might be possible / reasonable to
>> check in a newly-written, partial MMIO collision avoidance patch, not
>> everyone might agree.  Even if I were to rewrite and post a patch
>> myself, they may argue that doing such a complicated re-design after the
>> feature freeze shouldn't be allowed.
>
> What about something like this?
>
>  -George
>
> ---
>  [PATCH] hvmloader/pci: Try to avoid placing BARs in RMRRs
>
> Try to avoid placing PCI BARs over RMRRs:
>
> - If mmio_hole_size is not specified, and the existing MMIO range has
>   RMRRs in it, and there is space to expand the hole in lowmem without
>   moving more memory, then make the MMIO hole as large as possible.
>
> - When placing RMRRs, find the next RMRR higher than the current base
>   in the lowmem mmio hole.  If it overlaps, skip ahead of it and find
>   the next one.
>
> This certainly won't work in all cases, but it should work in a
> significant number of cases.  Additionally, users should be able to
> work around problems by setting mmio_hole_size larger in the guest
> config.
>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> ---
> THIS WILL NOT COMPILE, as it needs check_overlap_all() to be implemented.
>
> It's just a proof-of-concept for discussion.
> ---
>  tools/firmware/hvmloader/pci.c | 42 
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 5ff87a7..dcb8cd0 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,6 +38,25 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>  enum virtual_vga virtual_vga = VGA_none;
>  unsigned long igd_opregion_pgbase = 0;
>
> +/* Find the lowest RMRR higher than base */
> +int find_next_rmrr(uint32_t base)
> +{
> +    int next_rmrr=-1;
> +    uing64_t min_base = (1ull << 32);
> +
> +    for ( i = 0; i < memory_map.nr_map ; i++ )
> +    {
> +        if ( memory_map.map[i].type == E820_RESERVED
> +             && memory_map.map[i].addr > base
> +             && memory_map.map[i].addr < min_base)
> +        {
> +            next_rmrr = i;
> +            min_base = memory_map.map[i].addr;
> +        }
> +    }
> +    return next_rmrr;
> +}
> +
>  void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
> @@ -299,6 +318,15 @@ void pci_setup(void)
>                      || (((pci_mem_start << 1) >> PAGE_SHIFT)
>                          >= hvm_info->low_mem_pgend)) )
>              pci_mem_start <<= 1;
> +
> +        /*
> +         * Try to accomodate RMRRs in our MMIO region on a best-effort basis.
> +         * If we have RMRRs in the range, then make pci_mem_start just after
> +         * hvm_info->low_mem_pgend.
> +         */
> +        if ( pci_mem_start > (hvm_info->low_mem_pgend << PAGE_SHIFT) &&
> +             check_overlap_all(pci_mem_start, pci_mem_end-pci_mem_start) )
> +            pci_mem_start = (hvm_info->low_mem_pgend + 1 ) << PAGE_SHIFT);
>      }
>
>      if ( mmio_total > (pci_mem_end - pci_mem_start) )
> @@ -352,6 +380,8 @@ void pci_setup(void)
>      io_resource.base = 0xc000;
>      io_resource.max = 0x10000;
>
> +    next_rmrr = find_next_rmrr(pci_mem_start);
> +
>      /* Assign iomem and ioport resources in descending order of size. */
>      for ( i = 0; i < nr_bars; i++ )
>      {
> @@ -407,6 +437,18 @@ void pci_setup(void)
>          }
>
>          base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
> +
> +        /* If we're using mem_resource, check for RMRR conflicts */
> +        while ( resource == &mem_resource &&
> +                next_rmrr > 0 &&
> +                check_overlap(base, bar_sz,
> +                              memory_map.map[next_rmrr].addr,
> +                              memory_map.map[next_rmrr].size)) {
> +            base = memory_map.map[next_rmrr].addr +
> memory_map.map[next_rmrr].size;
> +            base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);

Sorry, this should obviously be

base = (base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);

I thought I'd changed it, but apparently I just skipped that step. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel