Re: [PATCH 15/15] xen: tools: expose EPC in ACPI table

From: "Huang, Kai" <kai.huang@linux.intel.com>
To: Jan Beulich <JBeulich@suse.com>, Kai Huang <kaih.linux@gmail.com>
Cc: andrew.cooper3@citrix.com, wei.liu2@citrix.com,
	ian.jackson@eu.citrix.com, xen-devel@lists.xen.org
Subject: Re: [PATCH 15/15] xen: tools: expose EPC in ACPI table
Date: Mon, 17 Jul 2017 18:11:58 +1200	[thread overview]
Message-ID: <10dc0bd3-d87d-1706-8d0b-9300d07fb607@linux.intel.com> (raw)
In-Reply-To: <5968C7C1020000780016AAEE@prv-mh.provo.novell.com>

On 7/14/2017 11:31 PM, Jan Beulich wrote:
>>>> On 09.07.17 at 10:16, <kaih.linux@gmail.com> wrote:
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -330,6 +330,15 @@ cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
>>           : "0" (idx) );
>>   }
>>   
>> +void cpuid_count(uint32_t idx, uint32_t count, uint32_t *eax,
> 
> Please name the first two leaf and subleaf.

Sure will do.

> 
>> @@ -888,6 +897,18 @@ static uint8_t acpi_lapic_id(unsigned cpu)
>>       return LAPIC_ID(cpu);
>>   }
>>   
>> +static void get_epc_info(struct acpi_config *config)
>> +{
>> +    uint32_t eax, ebx, ecx, edx;
>> +
>> +    cpuid_count(0x12, 0x2, &eax, &ebx, &ecx, &edx);
>> +
>> +    config->epc_base = (((uint64_t)(ebx & 0xfffff)) << 32) |
>> +                       (uint64_t)(eax & 0xfffff000);
> 
> Pointless cast.
> 
>> +    config->epc_size = (((uint64_t)(edx & 0xfffff)) << 32) |
>> +                       (uint64_t)(ecx & 0xfffff000);
> 
> Again.

Will do.

> 
>> --- a/tools/libacpi/dsdt.asl
>> +++ b/tools/libacpi/dsdt.asl
>> @@ -441,6 +441,55 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
>>                   }
>>               }
>>           }
>> +
>> +        Device (EPC)
>> +        {
>> +            Name (_HID, EisaId ("INT0E0C"))
>> +            Name (_STR, Unicode ("Enclave Page Cache 1.5"))
>> +            Name (_MLS, Package (0x01)
>> +            {
>> +                Package (0x02)
>> +                {
>> +                    "en",
>> +                    Unicode ("Enclave Page Cache 1.5")
>> +                }
>> +            })
>> +            Name (RBUF, ResourceTemplate ()
>> +            {
>> +                QWordMemory (ResourceConsumer, PosDecode, MinFixed, MaxFixed,
>> +                    Cacheable, ReadWrite,
>> +                    0x0000000000000000, // Granularity
>> +                    0x0000000000000000, // Range Minimum
>> +                    0x0000000000000000, // Range Maximum
>> +                    0x0000000000000000, // Translation Offset
>> +                    0x0000000000000001, // Length
>> +                    ,, _Y03,
>> +                    AddressRangeMemory, TypeStatic)
>> +            })
>> +
>> +            Method(_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>> +            {
>> +                CreateQwordField (RBUF, \_SB.EPC._Y03._MIN, EMIN) // _MIN: Minimuum Base Address
>> +                CreateQwordField (RBUF, \_SB.EPC._Y03._MAX, EMAX) // _MIN: Maximum Base Address
>> +                CreateQwordField (RBUF, \_SB.EPC._Y03._LEN, ELEN) // _LEN: Length
> 
> Please see the comment in _SB.PCI0._CRS regarding operations
> on qword fields. Even if we may not formally support the named
> Windows versions anymore, we should continue to be careful
> here. You could have noticed this by seeing that ...
> 
>> @@ -21,6 +21,8 @@
>>              LMIN, 32,
>>              HMIN, 32,
>>              LLEN, 32,
>> -           HLEN, 32
>> +           HLEN, 32,
>> +           EMIN, 64,
>> +           ELEN, 64,
>>          }
> 
> ... there have been no 64-bit fields here so far.

Thank you for pointing this out. I'll take a look.

> 
>> @@ -156,6 +156,9 @@ static int init_acpi_config(libxl__gc *gc,
>>       config->lapic_id = acpi_lapic_id;
>>       config->acpi_revision = 5;
>>   
>> +    config->epc_base = b_info->u.hvm.sgx.epcbase;
>> +    config->epc_size = (b_info->u.hvm.sgx.epckb << 10);
> 
> Pointless parentheses. Plus I guess the field names could do with
> an underscore separator in the middle - it took me a moment to
> realize this is a kB value (explaining the shift by 10).

Sure. will change to epc_kb and epc_base :)

Thanks,
-Kai
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

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