lkml.org 
[lkml]   [2023]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v6 1/6] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed
    Date
    > From: Dave Hansen <dave.hansen@intel.com>
    > Sent: Tuesday, May 23, 2023 2:13 PM
    > ...
    > On 5/4/23 15:53, Dexuan Cui wrote:
    > > - if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
    > > + while (1) {
    > > + memset(&args, 0, sizeof(args));
    > > + args.r10 = TDX_HYPERCALL_STANDARD;
    > > + args.r11 = TDVMCALL_MAP_GPA;
    > > + args.r12 = start;
    > > + args.r13 = end - start;
    > > +
    > > + ret = __tdx_hypercall_ret(&args);
    > > + if (ret != TDVMCALL_STATUS_RETRY)
    > > + break;
    > > + /*
    > > + * The guest must retry the operation for the pages in the
    > > + * region starting at the GPA specified in R11. Make sure R11
    > > + * contains a sane value.
    > > + */
    > > + map_fail_paddr = args.r11;
    > > + if (map_fail_paddr < start || map_fail_paddr >= end)
    > > + return false;
    >
    > This should probably also say: "r11" comes from the untrusted VMM.
    > Sanity check it.

    Thanks! I'll use the wording you recommended in the next version.

    > Should this *really* be "map_fail_paddr >= end"? Or is
    > "map_fail_paddr > end" sufficient. In other words, is it really
    > worth failing this if a VMM said to retry a 0-byte region at the end?

    According to the GHCI spec, R13 "must be a multiple of 4KB". My
    understanding is that R13 should not be 0, and a hypervisor is not
    supposed to tell the guest to retry a 0-byte region at the end.

    IMHO it should be a hypervisor bug if a hypervisor returns
    TDVMCALL_STATUS_RETRY and the returned 'map_fail_paddr' equals
    'end' (Note: the valid page range is [start, end - 1]).

    Hyper-V returns "invalid parameter" if the length (i.e. args.r13) is 0,
    so "retry a 0-byte region at the end" would fail anyway. I guess
    other hypervisors may also return an error if the length is 0.

    So I'd like to keep the comparison as-is.

    > > + if (map_fail_paddr == start) {
    > > + retry_cnt++;
    > > + if (retry_cnt > max_retry_cnt)
    >
    > I think we can spare two bytes in a few spots to make these 'count'
    > instead of 'cnt'.

    Ok, I'll rename the variable 'max_retry_cnt' to 'max_retries_per_page',
    and 'retry_cnt' to 'retry_count'.

    > > + return false;
    > > + } else {
    > > + retry_cnt = 0;
    > > + start = map_fail_paddr;
    > > + }
    > > + }
    >
    > this fails the "normal operation should be at the lowest indentation"
    > rule. How about this:
    >
    > while (retry_count < max_retries) {
    > ...
    >
    > /* "Consume" a retry without forward progress: */
    > if (map_fail_paddr == start) {
    > retry_count++;
    > continue;
    > }
    >
    > start = map_fail_paddr;
    > retry_count = 0;
    > }
    >
    > // plus maybe a wee bit different 'ret' processing
    >
    >
    > 'max_retries' also ends up being a misnomer. You can have as many
    > retries as there are pages plus 'max_retries'. It's really "maximum
    > allowed consecutive failures". Maybe it should be "max_retries_per_page".

    Thanks, I'll raname 'max_retries" to 'max_retries_per_page'.

    I'll use the beow in the next version.
    I added "const" to "int max_retries_per_page".
    Please let me know if I missed anything.

    +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
    +{
    + const int max_retries_per_page = 3;
    + struct tdx_hypercall_args args;
    + u64 map_fail_paddr, ret;
    + int retry_count = 0;
    +
    + if (!enc) {
    + /* Set the shared (decrypted) bits: */
    + start |= cc_mkdec(0);
    + end |= cc_mkdec(0);
    + }
    +
    + while (retry_count < max_retries_per_page) {
    + memset(&args, 0, sizeof(args));
    + args.r10 = TDX_HYPERCALL_STANDARD;
    + args.r11 = TDVMCALL_MAP_GPA;
    + args.r12 = start;
    + args.r13 = end - start;
    +
    + ret = __tdx_hypercall_ret(&args);
    + if (ret != TDVMCALL_STATUS_RETRY)
    + return !ret;
    + /*
    + * The guest must retry the operation for the pages in the
    + * region starting at the GPA specified in R11. R11 comes
    + * from the untrusted VMM. Sanity check it.
    + */
    + map_fail_paddr = args.r11;
    + if (map_fail_paddr < start || map_fail_paddr >= end)
    + return false;
    +
    + /* "Consume" a retry without forward progress */
    + if (map_fail_paddr == start) {
    + retry_count++;
    + continue;
    + }
    +
    + start = map_fail_paddr;
    + retry_count = 0;
    + }
    +
    + return false;
    +}
    \
     
     \ /
      Last update: 2023-05-25 04:07    [W:3.597 / U:0.528 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site