Re: [PATCH v2 3/7] x86/sev: add support for RMPOPT instruction

[Andrew Cooper <andrew.cooper3@citrix.com>]

>> +/* + * 'val' is a system physical address aligned to 1GB OR'ed with
>> + * a function selection. Currently supported functions are 0 + *
>> (verify and report status) and 1 (report status). + */ +static void
>> rmpopt(void *val) +{ + asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc" +
>> : : "a" ((u64)val & PUD_MASK), "c" ((u64)val & 0x1) + : "memory",
>> "cc"); +}
> Doesn't this belong in:
>
> arch/x86/include/asm/special_insns.h
>
> Also, it's not reporting *any* status here, right? So why even talk
> about it if the kernel isn't doing any status checks? It just makes it
> more confusing.

The "c" (val & 0x1) constraint encodes whether this is a query or a
mutation, but both forms produce an answer via the carry flag.

Because it's void, it's a useless helper, and the overloading via one
parameter makes specifically poor code generation.

It should be:

static inline bool __rmpopt(unsigned long addr, unsigned int fn)
{
    bool res;

    asm volatile (".byte 0xf2, 0x0f, 0x01, 0xfc"
                 : "=ccc" (res)
                 : "a" (addr), "c" (fn));

    return res;
}

with:

    static inline bool rmpopt_query(unsigned long addr)
    static inline bool rmpopt_set(unsigned long addr)

built on top.

Logic asking hardware to optimise a 1G region because of no guest memory
should at least WARN() if hardware comes back and says "well hang on now..."

The memory barrier isn't necessary and hinders the optimiser.

~Andrew