| Message ID | 1a4f90045a0c61518c65fe583c96080f6f461f70.1499586046.git.kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/07/17 10:16, Kai Huang wrote: > On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC > can be discovered by CPUID but Windows driver requires EPC to be exposed in > ACPI table as well. This patch exposes EPC in ACPI table. :( > diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl > index fa8ff317b2..25ce196028 100644 > --- a/tools/libacpi/dsdt.asl > +++ b/tools/libacpi/dsdt.asl > @@ -441,6 +441,55 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0) > } > } > } > + > + Device (EPC) Would it not be better to put this into an SSDT, and only expose it to the guest if SGX is advertised? ~Andrew
On 7/12/2017 11:05 PM, Andrew Cooper wrote: > On 09/07/17 10:16, Kai Huang wrote: >> On physical machine EPC is exposed in ACPI table via "INT0E0C". >> Although EPC >> can be discovered by CPUID but Windows driver requires EPC to be >> exposed in >> ACPI table as well. This patch exposes EPC in ACPI table. > > :( > >> diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl >> index fa8ff317b2..25ce196028 100644 >> --- a/tools/libacpi/dsdt.asl >> +++ b/tools/libacpi/dsdt.asl >> @@ -441,6 +441,55 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", >> "HVM", 0) >> } >> } >> } >> + >> + Device (EPC) > > Would it not be better to put this into an SSDT, and only expose it to > the guest if SGX is advertised? You mean to create dedicated ssdt_epc.asl? I thought about this, but I am not quite sure if we can, because new EPC device will need to refer \_SB.EMIN, and \_SB.ELEN, which are in acpi_info, to get EPC base and size. Can we refer acpi_info in dedicated ssdt_epc.asl? Thanks, -Kai > > ~Andrew > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> 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. > @@ -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. > --- 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. > @@ -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). Jan
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 >
On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote: > On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC > can be discovered by CPUID but Windows driver requires EPC to be exposed in > ACPI table as well. This patch exposes EPC in ACPI table. > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > --- > tools/firmware/hvmloader/util.c | 23 +++++++++++++++++++ > tools/firmware/hvmloader/util.h | 3 +++ Is there any reason this needs to be done in hvmloader instead of libacpi? I'm mostly asking this because PVH guests can also get ACPI tables, so it would be good to be able to expose EPC to them using ACPI. Thanks, Roger.
On 7/17/2017 10:54 PM, Roger Pau Monné wrote: > On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote: >> On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC >> can be discovered by CPUID but Windows driver requires EPC to be exposed in >> ACPI table as well. This patch exposes EPC in ACPI table. >> >> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >> --- >> tools/firmware/hvmloader/util.c | 23 +++++++++++++++++++ >> tools/firmware/hvmloader/util.h | 3 +++ > > Is there any reason this needs to be done in hvmloader instead of > libacpi? I'm mostly asking this because PVH guests can also get ACPI > tables, so it would be good to be able to expose EPC to them using > ACPI. Hi Roger, Thanks for comments. I didn't deliberately choose to do in hvmloader instead of libacpi. It seems libxl only builds ACPI table when guest is HVM, and it doesn't use any device model, and I think I have covered this part (see changes to init_acpi_config). Is there anything that I missed? Thanks, -Kai > > Thanks, Roger. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel >
On Tue, Jul 18, 2017 at 08:36:15PM +1200, Huang, Kai wrote: > > > On 7/17/2017 10:54 PM, Roger Pau Monné wrote: > > On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote: > > > On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC > > > can be discovered by CPUID but Windows driver requires EPC to be exposed in > > > ACPI table as well. This patch exposes EPC in ACPI table. > > > > > > Signed-off-by: Kai Huang <kai.huang@linux.intel.com> > > > --- > > > tools/firmware/hvmloader/util.c | 23 +++++++++++++++++++ > > > tools/firmware/hvmloader/util.h | 3 +++ > > > > Is there any reason this needs to be done in hvmloader instead of > > libacpi? I'm mostly asking this because PVH guests can also get ACPI > > tables, so it would be good to be able to expose EPC to them using > > ACPI. > > Hi Roger, > > Thanks for comments. I didn't deliberately choose to do in hvmloader instead > of libacpi. It seems libxl only builds ACPI table when guest is HVM, and it > doesn't use any device model, and I think I have covered this part (see > changes to init_acpi_config). Is there anything that I missed? dsdt.asl is only used for HVM guests, PVH guests basically get an empty dsdt + dsdt_acpi_info + processor objects populated by make_dsdt (see Makefile in libacpi), so they end up without the EPC Device block. It would be good if a new empty dsdt is created, that contains the Device EPC block, or a ssdt is used, and it's added to both HVM/PVH guests. Alternatively you could also code the EPC Device block in mk_dsdt, but that's going to be cumbersome IMHO. Thanks, Roger.
On 7/18/2017 10:21 PM, Roger Pau Monné wrote: > On Tue, Jul 18, 2017 at 08:36:15PM +1200, Huang, Kai wrote: >> >> >> On 7/17/2017 10:54 PM, Roger Pau Monné wrote: >>> On Sun, Jul 09, 2017 at 08:16:05PM +1200, Kai Huang wrote: >>>> On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC >>>> can be discovered by CPUID but Windows driver requires EPC to be exposed in >>>> ACPI table as well. This patch exposes EPC in ACPI table. >>>> >>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com> >>>> --- >>>> tools/firmware/hvmloader/util.c | 23 +++++++++++++++++++ >>>> tools/firmware/hvmloader/util.h | 3 +++ >>> >>> Is there any reason this needs to be done in hvmloader instead of >>> libacpi? I'm mostly asking this because PVH guests can also get ACPI >>> tables, so it would be good to be able to expose EPC to them using >>> ACPI. >> >> Hi Roger, >> >> Thanks for comments. I didn't deliberately choose to do in hvmloader instead >> of libacpi. It seems libxl only builds ACPI table when guest is HVM, and it >> doesn't use any device model, and I think I have covered this part (see >> changes to init_acpi_config). Is there anything that I missed? > > dsdt.asl is only used for HVM guests, PVH guests basically get an > empty dsdt + dsdt_acpi_info + processor objects populated by make_dsdt > (see Makefile in libacpi), so they end up without the EPC Device > block. > > It would be good if a new empty dsdt is created, that contains the > Device EPC block, or a ssdt is used, and it's added to both HVM/PVH > guests. > > Alternatively you could also code the EPC Device block in mk_dsdt, but > that's going to be cumbersome IMHO. Hi Roger, I got your point. I think it's definitely better to cover PVH if we can. Let me see whether it is possible to do. Thanks, -Kai > > Thanks, Roger. >
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c index db5f240bb9..4a1da2d63a 100644 --- 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, + uint32_t *ebx, uint32_t *ecx, uint32_t *edx) +{ + asm volatile ( + "cpuid" + : "=a" (*eax), "=b" (*ebx), "=c" (*ecx), "=d" (*edx) + : "0" (idx), "c" (count) ); +} + static const char hex_digits[] = "0123456789abcdef"; /* Write a two-character hex representation of 'byte' to digits[]. @@ -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); + config->epc_size = (((uint64_t)(edx & 0xfffff)) << 32) | + (uint64_t)(ecx & 0xfffff000); +} + void hvmloader_acpi_build_tables(struct acpi_config *config, unsigned int physical) { @@ -920,6 +941,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config, config->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start; } + get_epc_info(config); + s = xenstore_read("platform/generation-id", "0:0"); if ( s ) { diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 6062f0b8cf..deac0abb86 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -112,6 +112,9 @@ int hpet_exists(unsigned long hpet_base); void cpuid(uint32_t idx, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); +void cpuid_count(uint32_t idx, uint32_t count, uint32_t *eax, + uint32_t *ebx, uint32_t *ecx, uint32_t *edx); + /* Read the TSC register. */ static inline uint64_t rdtsc(void) { diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c index f9881c9604..9d64856e26 100644 --- a/tools/libacpi/build.c +++ b/tools/libacpi/build.c @@ -54,6 +54,7 @@ struct acpi_info { uint32_t madt_lapic0_addr; /* 16 - Address of first MADT LAPIC struct */ uint32_t vm_gid_addr; /* 20 - Address of VM generation id buffer */ uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */ + uint64_t epc_min, epc_len; /* 40, 48 - EPC region */ }; static void set_checksum( @@ -535,6 +536,8 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config) acpi_info->pci_hi_min = config->pci_hi_start; acpi_info->pci_hi_len = config->pci_hi_len; } + acpi_info->epc_min = config->epc_base; + acpi_info->epc_len = config->epc_size; /* * Fill in high-memory data structures, starting at @buf. diff --git a/tools/libacpi/dsdt.asl b/tools/libacpi/dsdt.asl index fa8ff317b2..25ce196028 100644 --- 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 + Store(\_SB.EMIN, EMIN) + Store(\_SB.ELEN, ELEN) + Add(EMIN, ELEN, EMAX) + Subtract(EMAX, One, EMAX) + + Return (RBUF) + } + + Method(_STA, 0, NotSerialized) // _STA: Status + { + IF ((\_SB.ELEN != Zero)) + { + Return (0x0F) + } + + Return (Zero) + } + } } /* _S3 and _S4 are in separate SSDTs */ Name (\_S5, Package (0x04) { diff --git a/tools/libacpi/dsdt_acpi_info.asl b/tools/libacpi/dsdt_acpi_info.asl index 0136dce55c..ac6b14f82f 100644 --- a/tools/libacpi/dsdt_acpi_info.asl +++ b/tools/libacpi/dsdt_acpi_info.asl @@ -5,7 +5,7 @@ * BIOS region must match struct acpi_info in build.c and * be located at ACPI_INFO_PHYSICAL_ADDRESS = 0xFC000000 */ - OperationRegion(BIOS, SystemMemory, 0xFC000000, 40) + OperationRegion(BIOS, SystemMemory, 0xFC000000, 56) Field(BIOS, ByteAcc, NoLock, Preserve) { UAR1, 1, UAR2, 1, @@ -21,6 +21,8 @@ LMIN, 32, HMIN, 32, LLEN, 32, - HLEN, 32 + HLEN, 32, + EMIN, 64, + ELEN, 64, } } diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h index 2ed1ecfc8e..5645e0866b 100644 --- a/tools/libacpi/libacpi.h +++ b/tools/libacpi/libacpi.h @@ -63,6 +63,7 @@ struct acpi_config { /* PCI I/O hole */ uint32_t pci_start, pci_len; uint64_t pci_hi_start, pci_hi_len; + uint64_t epc_base, epc_size; uint32_t table_flags; uint8_t acpi_revision; diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c index c0a6e321ec..0d62a76590 100644 --- a/tools/libxl/libxl_x86_acpi.c +++ b/tools/libxl/libxl_x86_acpi.c @@ -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); + rc = 0; out: return rc;
On physical machine EPC is exposed in ACPI table via "INT0E0C". Although EPC can be discovered by CPUID but Windows driver requires EPC to be exposed in ACPI table as well. This patch exposes EPC in ACPI table. Signed-off-by: Kai Huang <kai.huang@linux.intel.com> --- tools/firmware/hvmloader/util.c | 23 +++++++++++++++++++ tools/firmware/hvmloader/util.h | 3 +++ tools/libacpi/build.c | 3 +++ tools/libacpi/dsdt.asl | 49 ++++++++++++++++++++++++++++++++++++++++ tools/libacpi/dsdt_acpi_info.asl | 6 +++-- tools/libacpi/libacpi.h | 1 + tools/libxl/libxl_x86_acpi.c | 3 +++ 7 files changed, 86 insertions(+), 2 deletions(-)