[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH RFC] xen/pci: detect when BARs overlap RAM


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 26 Jan 2022 17:09:56 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0X2Fs5FIItK1brESSdlpvADRiP4VXsgO3Ryq0xoLQ7s=; b=HIU9bdHrvZqWvIbHvMm6+wt7ViPbzgK9PTUw3ortIW3azcAVzpHJ6ZssnBAnrXY8RFVjxYoiUP9KV+wv72To3EPJz0IxWQf37QGbsXMljzwlAlgemeLroHKroXokf1/TL7Zs0gCUGquFCd/XhgPcHzJnONqe0EvECVERAsO52MP/yHwQSnZMTOd3O4N+o5k8OKpL0gi1NChCyrGkKTaaIZnqn4Ed/9lNK7IxUlkqQuvYu9m+a3sKMnOQCfpzxGA2NLroV8KqbVawCwtb0hJdEc939yV89zTFPaCczXugWs2Wc3ZHFSLKeRnH3DY5Acsn2PjwlY1OmxFXdgtqwAtsTA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XifWNigV2Z9FyzNOU6PxTUSEBEMlro11PJRcaqK6Zksz/L4n6mrOi1nxGPP2//q0kuqTE1amzVnGUE/Xw3iLmcHNlDmDsj24DhEo+gHOyZD+sXGyZEIuTmbkHV6u+A8i9DLAim6JN9fTgX+mnb6hr5u+g10923SozJWpnjWJPHTnezW789NxuavNGYlxsyt+ZjBlnSvmhD/l1pMFQcBX2BinSahuPYF2FfKNmrATfx0kjjIj8+RFK/xktJVU3L4GIpWPuGJILVwxiw4qmbXwfAafx6l1ZzM+gPOOtZdHW/HeNN1RMjXbwCvoRJzIv6826mjFCaKUZ884a4IQvbbcMA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 26 Jan 2022 16:10:11 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.01.2022 16:45, Roger Pau Monné wrote:
> On Wed, Jan 26, 2022 at 03:08:28PM +0100, Jan Beulich wrote:
>> On 26.01.2022 13:26, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -479,6 +479,26 @@ unsigned int page_get_ram_type(mfn_t mfn)
>>>      return type ?: RAM_TYPE_UNKNOWN;
>>>  }
>>>  
>>> +bool is_iomem_range(uint64_t start, uint64_t size)
>>
>> Might be nice to have this sit next it is_iomem_page(). And wouldn't
>> at least "start" better be paddr_t?
> 
> (paddr_t, size_t) would be better, but AFAICT size_t can be an
> unsigned long and on Arm32 that won't be suitable for holding a 64bit
> BAR size.

Indeed. We'd need a resource_size_t or alike.

>>> +{
>>> +    unsigned int i;
>>> +
>>> +    for ( i = 0; i < e820.nr_map; i++ )
>>> +    {
>>> +        struct e820entry *entry = &e820.map[i];
>>
>> const?
>>
>>> +        if ( entry->type != E820_RAM && entry->type != E820_ACPI &&
>>> +             entry->type != E820_NVS )
>>> +            continue;
>>
>> I think UNUSABLE also needs checking for here. Even further, it might
>> be best to reject any range overlapping an E820 entry, since colliding
>> with a RESERVED one could mean an overlap with e.g. MMCFG space.
> 
> Hm, I've though about adding UNUSABLE and RESERVED (and should have
> added a note about this), but that might be too restrictive.

Why (other than because of firmware bugs)?

>>> +        if ( size && !is_iomem_range(addr, size) )
>>> +        {
>>> +            /*
>>> +             * Return without enabling memory decoding if BAR overlaps with
>>> +             * RAM region.
>>> +             */
>>> +            printk(XENLOG_WARNING
>>> +                   "%pp: disabled: BAR%u [%" PRIx64 ", %" PRIx64
>>> +                   ") overlaps with RAM\n",
>>
>> Mentioning "RAM" in comment and log message is potentially misleading
>> when considering what other types get also checked (irrespective of my
>> earlier comment). (Ftaod I don't mind the title using "RAM".)
> 
> Would the message below be better?
> 
> "%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlap detected\n"

This or maybe

"%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlaps with memory map\n"

in particular if, on x86, we'd be checking for any E820 entry type.

>>> @@ -399,8 +465,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
>>> u8 bus, u8 devfn)
>>>              break;
>>>      }
>>>  
>>> -    check_pdev(pdev);
>>>      apply_quirks(pdev);
>>> +    check_pdev(pdev);
>>
>> I can't find the description say anything about this re-ordering. What's
>> the deal here?
> 
> Should have mentioned: this is required so that ignore_bars is set
> prior to calling check_pdev.

Ah, I see. Makes sense, but indeed wants mentioning.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.