| Message ID | 20220512031803.3315890-23-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX QEMU support | expand |
> +static int tdx_accept_ram_range(uint64_t address, uint64_t length) > +{ > + TdxRamEntry *e; > + int i; > + > + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { > + e = &tdx_guest->ram_entries[i]; > + > + if (address + length < e->address || > + e->address + e->length < address) { > + continue; > + } > + > + if (e->address > address || > + e->address + e->length < address + length) { > + return -EINVAL; > + } if (e->type == TDX_RAM_ADDED) return -EINVAL > + if (e->address == address && e->length == length) { > + e->type = TDX_RAM_ADDED; > + } else if (e->address == address) { > + e->address += length; > + e->length -= length; > + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); > + } else if (e->address + e->length == address + length) { > + e->length -= length; > + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); > + } else { > + TdxRamEntry tmp = { > + .address = e->address, > + .length = e->length, > + }; > + e->length = address - tmp.address; > + > + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); > + tdx_add_ram_entry(address + length, > + tmp.address + tmp.length - (address + length), > + TDX_RAM_UNACCEPTED); > + } I think all this can be simplified, by (1) Change the existing entry to cover the accepted ram range. (2) If there is room before the accepted ram range add a TDX_RAM_UNACCEPTED entry for that. (3) If there is room after the accepted ram range add a TDX_RAM_UNACCEPTED entry for that. take care, Gerd
On 5/24/2022 3:37 PM, Gerd Hoffmann wrote: >> +static int tdx_accept_ram_range(uint64_t address, uint64_t length) >> +{ >> + TdxRamEntry *e; >> + int i; >> + >> + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { >> + e = &tdx_guest->ram_entries[i]; >> + >> + if (address + length < e->address || >> + e->address + e->length < address) { >> + continue; >> + } >> + >> + if (e->address > address || >> + e->address + e->length < address + length) { >> + return -EINVAL; >> + } > > if (e->type == TDX_RAM_ADDED) > return -EINVAL > >> + if (e->address == address && e->length == length) { >> + e->type = TDX_RAM_ADDED; >> + } else if (e->address == address) { >> + e->address += length; >> + e->length -= length; >> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >> + } else if (e->address + e->length == address + length) { >> + e->length -= length; >> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >> + } else { >> + TdxRamEntry tmp = { >> + .address = e->address, >> + .length = e->length, >> + }; >> + e->length = address - tmp.address; >> + >> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >> + tdx_add_ram_entry(address + length, >> + tmp.address + tmp.length - (address + length), >> + TDX_RAM_UNACCEPTED); >> + } > > I think all this can be simplified, by > (1) Change the existing entry to cover the accepted ram range. > (2) If there is room before the accepted ram range add a > TDX_RAM_UNACCEPTED entry for that. > (3) If there is room after the accepted ram range add a > TDX_RAM_UNACCEPTED entry for that. I implement as below. Please help review. +static int tdx_accept_ram_range(uint64_t address, uint64_t length) +{ + uint64_t head_start, tail_start, head_length, tail_length; + uint64_t tmp_address, tmp_length; + TdxRamEntry *e; + int i; + + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { + e = &tdx_guest->ram_entries[i]; + + if (address + length < e->address || + e->address + e->length < address) { + continue; + } + + /* + * The to-be-accepted ram range must be fully contained by one + * RAM entries + */ + if (e->address > address || + e->address + e->length < address + length) { + return -EINVAL; + } + + if (e->type == TDX_RAM_ADDED) { + return -EINVAL; + } + + tmp_address = e->address; + tmp_length = e->length; + + e->address = address; + e->length = length; + e->type = TDX_RAM_ADDED; + + head_length = address - tmp_address; + if (head_length > 0) { + head_start = e->address; + tdx_add_ram_entry(head_start, head_length, TDX_RAM_UNACCEPTED); + } + + tail_start = address + length; + if (tail_start < tmp_address + tmp_length) { + tail_length = e->address + e->length - tail_start; + tdx_add_ram_entry(tail_start, tail_length, TDX_RAM_UNACCEPTED); + } + + return 0; + } + + return -1; +} > take care, > Gerd >
On Thu, May 26, 2022 at 03:33:10PM +0800, Xiaoyao Li <xiaoyao.li@intel.com> wrote: > On 5/24/2022 3:37 PM, Gerd Hoffmann wrote: > > I think all this can be simplified, by > > (1) Change the existing entry to cover the accepted ram range. > > (2) If there is room before the accepted ram range add a > > TDX_RAM_UNACCEPTED entry for that. > > (3) If there is room after the accepted ram range add a > > TDX_RAM_UNACCEPTED entry for that. > > I implement as below. Please help review. > > +static int tdx_accept_ram_range(uint64_t address, uint64_t length) > +{ > + uint64_t head_start, tail_start, head_length, tail_length; > + uint64_t tmp_address, tmp_length; > + TdxRamEntry *e; > + int i; > + > + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { > + e = &tdx_guest->ram_entries[i]; > + > + if (address + length < e->address || > + e->address + e->length < address) { > + continue; > + } > + > + /* > + * The to-be-accepted ram range must be fully contained by one > + * RAM entries > + */ > + if (e->address > address || > + e->address + e->length < address + length) { > + return -EINVAL; > + } > + > + if (e->type == TDX_RAM_ADDED) { > + return -EINVAL; > + } > + > + tmp_address = e->address; > + tmp_length = e->length; > + > + e->address = address; > + e->length = length; > + e->type = TDX_RAM_ADDED; > + > + head_length = address - tmp_address; > + if (head_length > 0) { > + head_start = e->address; > + tdx_add_ram_entry(head_start, head_length, TDX_RAM_UNACCEPTED); tdx_add_ram_entry() increments tdx_guest->nr_ram_entries. I think it's worth for comments why this is safe regarding to this for-loop.
On 5/26/2022 3:33 PM, Xiaoyao Li wrote: > On 5/24/2022 3:37 PM, Gerd Hoffmann wrote: >>> + if (e->address == address && e->length == length) { >>> + e->type = TDX_RAM_ADDED; >>> + } else if (e->address == address) { >>> + e->address += length; >>> + e->length -= length; >>> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >>> + } else if (e->address + e->length == address + length) { >>> + e->length -= length; >>> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >>> + } else { >>> + TdxRamEntry tmp = { >>> + .address = e->address, >>> + .length = e->length, >>> + }; >>> + e->length = address - tmp.address; >>> + >>> + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); >>> + tdx_add_ram_entry(address + length, >>> + tmp.address + tmp.length - (address + >>> length), >>> + TDX_RAM_UNACCEPTED); >>> + } >> >> I think all this can be simplified, by >> (1) Change the existing entry to cover the accepted ram range. >> (2) If there is room before the accepted ram range add a >> TDX_RAM_UNACCEPTED entry for that. >> (3) If there is room after the accepted ram range add a >> TDX_RAM_UNACCEPTED entry for that. > > I implement as below. Please help review. > > +static int tdx_accept_ram_range(uint64_t address, uint64_t length) > +{ > + uint64_t head_start, tail_start, head_length, tail_length; > + uint64_t tmp_address, tmp_length; > + TdxRamEntry *e; > + int i; > + > + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { > + e = &tdx_guest->ram_entries[i]; > + > + if (address + length < e->address || > + e->address + e->length < address) { > + continue; > + } > + > + /* > + * The to-be-accepted ram range must be fully contained by one > + * RAM entries > + */ > + if (e->address > address || > + e->address + e->length < address + length) { > + return -EINVAL; > + } > + > + if (e->type == TDX_RAM_ADDED) { > + return -EINVAL; > + } > + > + tmp_address = e->address; > + tmp_length = e->length; > + > + e->address = address; > + e->length = length; > + e->type = TDX_RAM_ADDED; > + > + head_length = address - tmp_address; > + if (head_length > 0) { > + head_start = e->address; > + tdx_add_ram_entry(head_start, head_length, > TDX_RAM_UNACCEPTED); > + } > + > + tail_start = address + length; > + if (tail_start < tmp_address + tmp_length) { > + tail_length = e->address + e->length - tail_start; > + tdx_add_ram_entry(tail_start, tail_length, > TDX_RAM_UNACCEPTED); > + } > + > + return 0; > + } > + > + return -1; > +} above is incorrect. I implement fixed one: +static int tdx_accept_ram_range(uint64_t address, uint64_t length) +{ + uint64_t head_start, tail_start, head_length, tail_length; + uint64_t tmp_address, tmp_length; + TdxRamEntry *e; + int i; + + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { + e = &tdx_guest->ram_entries[i]; + + if (address + length < e->address || + e->address + e->length < address) { + continue; + } + + /* + * The to-be-accepted ram range must be fully contained by one + * RAM entries + */ + if (e->address > address || + e->address + e->length < address + length) { + return -EINVAL; + } + + if (e->type == TDX_RAM_ADDED) { + return -EINVAL; + } + + tmp_address = e->address; + tmp_length = e->length; + + e->address = address; + e->length = length; + e->type = TDX_RAM_ADDED; + + head_length = address - tmp_address; + if (head_length > 0) { + head_start = tmp_address; + tdx_add_ram_entry(head_start, head_length, TDX_RAM_UNACCEPTED); + } + + tail_start = address + length; + if (tail_start < tmp_address + tmp_length) { + tail_length = tmp_address + tmp_length - tail_start; + tdx_add_ram_entry(tail_start, tail_length, TDX_RAM_UNACCEPTED); + } + + return 0; + } + + return -1; +} > > >> take care, >> Gerd >> >
On 5/27/2022 2:48 AM, Isaku Yamahata wrote: > On Thu, May 26, 2022 at 03:33:10PM +0800, > Xiaoyao Li <xiaoyao.li@intel.com> wrote: > >> On 5/24/2022 3:37 PM, Gerd Hoffmann wrote: >>> I think all this can be simplified, by >>> (1) Change the existing entry to cover the accepted ram range. >>> (2) If there is room before the accepted ram range add a >>> TDX_RAM_UNACCEPTED entry for that. >>> (3) If there is room after the accepted ram range add a >>> TDX_RAM_UNACCEPTED entry for that. >> >> I implement as below. Please help review. >> >> +static int tdx_accept_ram_range(uint64_t address, uint64_t length) >> +{ >> + uint64_t head_start, tail_start, head_length, tail_length; >> + uint64_t tmp_address, tmp_length; >> + TdxRamEntry *e; >> + int i; >> + >> + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { >> + e = &tdx_guest->ram_entries[i]; >> + >> + if (address + length < e->address || >> + e->address + e->length < address) { >> + continue; >> + } >> + >> + /* >> + * The to-be-accepted ram range must be fully contained by one >> + * RAM entries >> + */ >> + if (e->address > address || >> + e->address + e->length < address + length) { >> + return -EINVAL; >> + } >> + >> + if (e->type == TDX_RAM_ADDED) { >> + return -EINVAL; >> + } >> + >> + tmp_address = e->address; >> + tmp_length = e->length; >> + >> + e->address = address; >> + e->length = length; >> + e->type = TDX_RAM_ADDED; >> + >> + head_length = address - tmp_address; >> + if (head_length > 0) { >> + head_start = e->address; >> + tdx_add_ram_entry(head_start, head_length, TDX_RAM_UNACCEPTED); > > tdx_add_ram_entry() increments tdx_guest->nr_ram_entries. I think it's worth > for comments why this is safe regarding to this for-loop. The for-loop is to find the valid existing RAM entry (from E820 table). It will update the RAM entry and increment tdx_guest->nr_ram_entries when the initial RAM entry needs to be split. However, once find, the for-loop is certainly stopped since it returns unconditionally.
Hi, > > tdx_add_ram_entry() increments tdx_guest->nr_ram_entries. I think it's worth > > for comments why this is safe regarding to this for-loop. > > The for-loop is to find the valid existing RAM entry (from E820 table). > It will update the RAM entry and increment tdx_guest->nr_ram_entries when > the initial RAM entry needs to be split. However, once find, the for-loop is > certainly stopped since it returns unconditionally. Add a comment saying so would be good. Or move the code block doing the update out of the loop. That will likewise make clear that finding the entry which must be updated is the only purpose of the loop. take care, Gerd
On 5/30/2022 7:59 PM, Gerd Hoffmann wrote: > Hi, > >>> tdx_add_ram_entry() increments tdx_guest->nr_ram_entries. I think it's worth >>> for comments why this is safe regarding to this for-loop. >> >> The for-loop is to find the valid existing RAM entry (from E820 table). >> It will update the RAM entry and increment tdx_guest->nr_ram_entries when >> the initial RAM entry needs to be split. However, once find, the for-loop is >> certainly stopped since it returns unconditionally. > > Add a comment saying so would be good. > > Or move the code block doing the update out of the loop. That will > likewise make clear that finding the entry which must be updated is > the only purpose of the loop. Good idea. I'll go this way. > take care, > Gerd >
diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index 8bac49419f37..e7071bfe4c9c 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -19,6 +19,7 @@ #include "sysemu/kvm.h" #include "sysemu/sysemu.h" +#include "hw/i386/e820_memory_layout.h" #include "hw/i386/x86.h" #include "hw/i386/tdvf.h" #include "kvm_i386.h" @@ -129,11 +130,105 @@ static void get_tdx_capabilities(void) tdx_caps = caps; } +static void tdx_add_ram_entry(uint64_t address, uint64_t length, uint32_t type) +{ + uint32_t nr_entries = tdx_guest->nr_ram_entries; + tdx_guest->ram_entries = g_renew(TdxRamEntry, tdx_guest->ram_entries, + nr_entries + 1); + + tdx_guest->ram_entries[nr_entries].address = address; + tdx_guest->ram_entries[nr_entries].length = length; + tdx_guest->ram_entries[nr_entries].type = type; + tdx_guest->nr_ram_entries++; +} + +static int tdx_accept_ram_range(uint64_t address, uint64_t length) +{ + TdxRamEntry *e; + int i; + + for (i = 0; i < tdx_guest->nr_ram_entries; i++) { + e = &tdx_guest->ram_entries[i]; + + if (address + length < e->address || + e->address + e->length < address) { + continue; + } + + if (e->address > address || + e->address + e->length < address + length) { + return -EINVAL; + } + + if (e->address == address && e->length == length) { + e->type = TDX_RAM_ADDED; + } else if (e->address == address) { + e->address += length; + e->length -= length; + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); + } else if (e->address + e->length == address + length) { + e->length -= length; + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); + } else { + TdxRamEntry tmp = { + .address = e->address, + .length = e->length, + }; + e->length = address - tmp.address; + + tdx_add_ram_entry(address, length, TDX_RAM_ADDED); + tdx_add_ram_entry(address + length, + tmp.address + tmp.length - (address + length), + TDX_RAM_UNACCEPTED); + } + + return 0; + } + + return -1; +} + +static int tdx_ram_entry_compare(const void *lhs_, const void* rhs_) +{ + const TdxRamEntry *lhs = lhs_; + const TdxRamEntry *rhs = rhs_; + + if (lhs->address == rhs->address) { + return 0; + } + if (le64_to_cpu(lhs->address) > le64_to_cpu(rhs->address)) { + return 1; + } + return -1; +} + +static void tdx_init_ram_entries(void) +{ + unsigned i, j, nr_e820_entries; + + nr_e820_entries = e820_get_num_entries(); + tdx_guest->ram_entries = g_new(TdxRamEntry, nr_e820_entries); + + for (i = 0, j = 0; i < nr_e820_entries; i++) { + uint64_t addr, len; + + if (e820_get_entry(i, E820_RAM, &addr, &len)) { + tdx_guest->ram_entries[j].address = addr; + tdx_guest->ram_entries[j].length = len; + tdx_guest->ram_entries[j].type = TDX_RAM_UNACCEPTED; + j++; + } + } + tdx_guest->nr_ram_entries = j; +} + static void tdx_finalize_vm(Notifier *notifier, void *unused) { TdxFirmware *tdvf = &tdx_guest->tdvf; TdxFirmwareEntry *entry; + tdx_init_ram_entries(); + for_each_tdx_fw_entry(tdvf, entry) { switch (entry->type) { case TDVF_SECTION_TYPE_BFV: @@ -144,12 +239,16 @@ static void tdx_finalize_vm(Notifier *notifier, void *unused) case TDVF_SECTION_TYPE_TEMP_MEM: entry->mem_ptr = qemu_ram_mmap(-1, entry->size, qemu_real_host_page_size(), 0, 0); + tdx_accept_ram_range(entry->address, entry->size); break; default: error_report("Unsupported TDVF section %d", entry->type); exit(1); } } + + qsort(tdx_guest->ram_entries, tdx_guest->nr_ram_entries, + sizeof(TdxRamEntry), &tdx_ram_entry_compare); } static Notifier tdx_machine_done_notify = { diff --git a/target/i386/kvm/tdx.h b/target/i386/kvm/tdx.h index 12bcf25bb95b..5792518afa62 100644 --- a/target/i386/kvm/tdx.h +++ b/target/i386/kvm/tdx.h @@ -15,6 +15,17 @@ typedef struct TdxGuestClass { ConfidentialGuestSupportClass parent_class; } TdxGuestClass; +enum TdxRamType{ + TDX_RAM_UNACCEPTED, + TDX_RAM_ADDED, +}; + +typedef struct TdxRamEntry { + uint64_t address; + uint64_t length; + uint32_t type; +} TdxRamEntry; + typedef struct TdxGuest { ConfidentialGuestSupport parent_obj; @@ -24,6 +35,9 @@ typedef struct TdxGuest { uint64_t attributes; /* TD attributes */ TdxFirmware tdvf; + + uint32_t nr_ram_entries; + TdxRamEntry *ram_entries; } TdxGuest; #ifdef CONFIG_TDX
The RAM of TDX VM can be classified into two types: - TDX_RAM_UNACCEPTED: default type of TDX memory, which needs to be accepted by TDX guest before it can be used and will be all-zeros after being accepted. - TDX_RAM_ADDED: the RAM that is ADD'ed to TD guest before running, and can be used directly. E.g., TD HOB and TEMP MEM that needed by TDVF. Maintain TdxRamEntries[] which grabs the initial RAM info from e820 table and mark each RAM range as default type TDX_RAM_UNACCEPTED. Then turn the range of TD HOB and TEMP MEM to TDX_RAM_ADDED since these ranges will be ADD'ed before TD runs and no need to be accepted runtime. The TdxRamEntries[] are later used to setup the memory TD resource HOB that passes memory info from QEMU to TDVF. Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com> --- target/i386/kvm/tdx.c | 99 +++++++++++++++++++++++++++++++++++++++++++ target/i386/kvm/tdx.h | 14 ++++++ 2 files changed, 113 insertions(+)