Re: [PATCH 4/7] x86/ioremap, resource: Introduce IORES_DESC_ENCRYPTED for encrypted PCI MMIO

[Alexey Kardashevskiy <aik@amd.com>]

On 7/10/25 19:23, Alexey Kardashevskiy wrote:
> 
> 
> On 27/8/25 13:52, Dan Williams wrote:
>> PCIe Trusted Execution Environment Device Interface Security Protocol
>> (TDISP) arranges for a PCI device to support encrypted MMIO. In support of
>> that capability, ioremap() needs a mechanism to detect when a PCI device
>> has been dynamically transitioned into this secure state and enforce
>> encrypted MMIO mappings.
>>
>> Teach ioremap() about a new IORES_DESC_ENCRYPTED type that supplements the
>> existing PCI Memory Space (MMIO) BAR resources. The proposal is that a
>> resource, "PCI MMIO Encrypted", with this description type is injected by
>> the PCI/TSM core for each PCI device BAR that is to be protected.
>>
>> Unlike the existing encryption determination which is "implied with a silent
>> fallback to an unencrypted mapping", this indication is "explicit with an
>> expectation that the request fails instead of fallback". IORES_MUST_ENCRYPT
>> is added to manage this expectation.
>>
>> Given that "PCI MMIO Encrypted" is an additional resource in the tree, the
>> IORESOURCE_BUSY flag will only be set on a descendant/child of that
>> resource. Adjust the resource tree walk to use walk_iomem_res_desc() and
>> check all intersecting resources for the IORES_MUST_ENCRYPT determination.
> 
> How is this expected to work exactly?
> 
> samples/devsec/tsm.c calls pci_tsm_alloc_encrypted_resources() which essentially does:
> 
> *__res[i] = DEFINE_RES_NAMED_DESC(pci_resource_start(pdev, i),
>                                    len, "PCI MMIO Encrypted",
>                                    flags, IORES_DESC_ENCRYPTED);
> if (insert_resource(&iomem_resource, __res[i]) != 0) {
> ...
> 
> By later on pci_iomap(pdev, N, PAGE_SIZE) on that BAR maps as unencrypted. The resource makes it to (hacked) iomem:
> 
> c000000000-c7ffffffff : PCI Bus 0000:00 fl=200201 desc=0
>    c000000000-c01fffffff : PCI Bus 0000:01 fl=102201 desc=0
>      c000000000-c003ffffff : 0000:01:00.0 fl=14220c desc=0
>        c000000000-c003ffffff : mydrv fl=80000200 desc=0
>      c004000000-c004000fff : PCI MMIO Encrypted fl=14220c desc=a
>        c004000000-c004000fff : 0000:01:00.0 fl=14220c desc=0
>      c004001000-c004001fff : 0000:01:00.0 fl=14220c desc=0
> 
> 
> and btw does not pci_resource_n(pdev, i) make more sense as a parent in insert_resource().
> 
> 
>>
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: x86@kernel.org
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>   arch/x86/mm/ioremap.c  | 32 +++++++++++++++++++++-----------
>>   include/linux/ioport.h |  2 ++
>>   2 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 12c8180ca1ba..78b677dadfdc 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -93,18 +93,24 @@ static unsigned int __ioremap_check_ram(struct resource *res)
>>    */
>>   static unsigned int __ioremap_check_encrypted(struct resource *res)
>>   {
>> +    u32 flags = 0;
>> +
>> +    if (res->desc == IORES_DESC_ENCRYPTED)
>> +        flags |= IORES_MUST_ENCRYPT;
>> +
>>       if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
>> -        return 0;
>> +        return flags;
>>       switch (res->desc) {
>>       case IORES_DESC_NONE:
>>       case IORES_DESC_RESERVED:
>>           break;
>> +    case IORES_DESC_ENCRYPTED:
>>       default:
>> -        return IORES_MAP_ENCRYPTED;
>> +        flags |= IORES_MAP_ENCRYPTED;
>>       }
>> -    return 0;
>> +    return flags;
>>   }
>>   /*
>> @@ -134,14 +140,10 @@ static int __ioremap_collect_map_flags(struct resource *res, void *arg)
>>   {
>>       struct ioremap_desc *desc = arg;
>> -    if (!(desc->flags & IORES_MAP_SYSTEM_RAM))
>> -        desc->flags |= __ioremap_check_ram(res);
>> -
>> -    if (!(desc->flags & IORES_MAP_ENCRYPTED))
>> -        desc->flags |= __ioremap_check_encrypted(res);
>> +    desc->flags |= __ioremap_check_ram(res);
>> +    desc->flags |= __ioremap_check_encrypted(res);
> 
> 
> Here the found "res" is actually "c000000000-c7ffffffff : PCI Bus 0000:00", not c004000000 (from the above example)...
> 
>> -    return ((desc->flags & (IORES_MAP_SYSTEM_RAM | IORES_MAP_ENCRYPTED)) ==
>> -                   (IORES_MAP_SYSTEM_RAM | IORES_MAP_ENCRYPTED));
>> +    return 0;
>>   }
>>   /*
>> @@ -161,7 +163,8 @@ static void __ioremap_check_mem(resource_size_t addr, unsigned long size,
>>       end = start + size - 1;
>>       memset(desc, 0, sizeof(struct ioremap_desc));

Adding this here:

+       walk_iomem_res_desc(IORES_DESC_ENCRYPTED, IORESOURCE_MEM, start, end, desc,
+                           __ioremap_collect_map_flags);


fixed the problem and the encryption bit is set. Thanks,


>> -    walk_mem_res(start, end, desc, __ioremap_collect_map_flags);
>> +    walk_iomem_res_desc(IORES_DESC_NONE, IORESOURCE_MEM, start, end, desc,
>> +                __ioremap_collect_map_flags);
> 
> 
> ... which seems to be the result of passing IORES_DESC_NONE. What do I miss? Thanks,
> 
> 
>>       __ioremap_check_other(addr, desc);
>>   }
>> @@ -209,6 +212,13 @@ __ioremap_caller(resource_size_t phys_addr, unsigned long size,
>>       __ioremap_check_mem(phys_addr, size, &io_desc);
>> +    if ((io_desc.flags & IORES_MUST_ENCRYPT) &&
>> +        !(io_desc.flags & IORES_MAP_ENCRYPTED)) {
>> +        pr_err("ioremap: encrypted mapping unavailable for %pa - %pa\n",
>> +               &phys_addr, &last_addr);
>> +        return NULL;
>> +    }
>> +
>>       /*
>>        * Don't allow anybody to remap normal RAM that we're using..
>>        */
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index e8b2d6aa4013..b46e42bcafe3 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -143,6 +143,7 @@ enum {
>>       IORES_DESC_RESERVED            = 7,
>>       IORES_DESC_SOFT_RESERVED        = 8,
>>       IORES_DESC_CXL                = 9,
>> +    IORES_DESC_ENCRYPTED            = 10,
>>   };
>>   /*
>> @@ -151,6 +152,7 @@ enum {
>>   enum {
>>       IORES_MAP_SYSTEM_RAM        = BIT(0),
>>       IORES_MAP_ENCRYPTED        = BIT(1),
>> +    IORES_MUST_ENCRYPT        = BIT(2), /* disable transparent fallback */
>>   };
>>   /* helpers to define resources */
> 

-- 
Alexey