Re: [RFC 04/14] x86/apic: Initialize APIC backing page for Secure AVIC

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

On 11/7/2024 8:58 PM, Borislav Petkov wrote:
> On Fri, Sep 13, 2024 at 05:06:55PM +0530, Neeraj Upadhyay wrote:
>> From: Kishon Vijay Abraham I <kvijayab@amd.com>
>>
>> Secure AVIC lets guest manage the APIC backing page (unlike emulated
>> x2APIC or x2AVIC where the hypervisor manages the APIC backing page).
>>
>> However the introduced Secure AVIC Linux design still maintains the
>> APIC backing page in the hypervisor to shadow the APIC backing page
>> maintained by guest (It should be noted only subset of the registers
>> are shadowed for specific usecases and registers like APIC_IRR,
>> APIC_ISR are not shadowed).
>>
>> Add sev_ghcb_msr_read() to invoke "SVM_EXIT_MSR" VMGEXIT to read
>> MSRs from hypervisor. Initialize the Secure AVIC's APIC backing
>> page by copying the initial state of shadow APIC backing page in
>> the hypervisor to the guest APIC backing page. Specifically copy
>> APIC_LVR, APIC_LDR, and APIC_LVT MSRs from the shadow APIC backing
>> page.
> 
> You don't have to explain what the patch does - rather, why the patch exists
> in the first place and perhaps mention some non-obvious stuff why the code
> does what it does.
> 
> Check your whole set pls.

I will improve on this in the next version.


>> -static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>> +static enum es_result __vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt, bool write)
> 
> Yeah, this one was bugging me already during Nikunj's set so I cleaned it up
> a bit differently:
> 
> https://git.kernel.org/tip/8bca85cc1eb72e21a3544ab32e546a819d8674ca
> 

Ok nice! I will rebase.

>> +enum es_result sev_ghcb_msr_read(u64 msr, u64 *value)
> 
> Why is this a separate function if it is called only once from x2avic_savic.c?
> 

As sev_ghcb_msr_read() work with any msr and is not limited to reading
x2apic msrs, I created a global sev function for it.

> I think you should merge it with read_msr_from_hv(), rename latter to
> 
> x2avic_read_msr_from_hv()
> 
> and leave it here in sev/core.c.
> 

Ok sure, I will leave generalizing this to future use cases (if/when they come up)
and provide a secure avic specific function here (will do the same for
sev_ghcb_msr_write(), which comes later in this series).

"x2avic" terminology is not used in guest code. As this function only has secure
avic user, does secure_avic_ghcb_msr_read() work?



>> +enum lapic_lvt_entry {
> 
> What's that enum for?

It's used in init_backing_page()

for (i = LVT_THERMAL_MONITOR; i < APIC_MAX_NR_LVT_ENTRIES; i++) {
        val = read_msr_from_hv(APIC_LVTx(i));
        set_reg(backing_page, APIC_LVTx(i), val);
}

> 
> Oh, you want to use it below but you don't. Why?
> 

As LVT_TIMER is unused, I will remove it:

enum lapic_lvt_entry {
       LVT_THERMAL_MONITOR = 1,
       LVT_PERFORMANCE_COUNTER,
       LVT_LINT0,
       LVT_LINT1,
       LVT_ERROR,

       APIC_MAX_NR_LVT_ENTRIES,
};


>> +	LVT_TIMER,
>> +	LVT_THERMAL_MONITOR,
>> +	LVT_PERFORMANCE_COUNTER,
>> +	LVT_LINT0,
>> +	LVT_LINT1,
>> +	LVT_ERROR,
>> +
>> +	APIC_MAX_NR_LVT_ENTRIES,
>> +};
>> +
>> +#define APIC_LVTx(x) (APIC_LVTT + 0x10 * (x))
>> +
>>  static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>  {
>>  	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
>> @@ -35,6 +49,22 @@ static inline void set_reg(char *page, int reg_off, u32 val)
>>  	WRITE_ONCE(*((u32 *)(page + reg_off)), val);
>>  }
>>  
>> +static u32 read_msr_from_hv(u32 reg)
> 
> A MSR's contents is u64. Make this function generic enough and have the
> callsite select only the lower dword.
> 

Ok sure, will update.

>> +{
>> +	u64 data, msr;
>> +	int ret;
>> +
>> +	msr = APIC_BASE_MSR + (reg >> 4);
>> +	ret = sev_ghcb_msr_read(msr, &data);
>> +	if (ret != ES_OK) {
>> +		pr_err("Secure AVIC msr (%#llx) read returned error (%d)\n", msr, ret);
> 
> Prepend "0x" to the format specifier.
> 

Using '#' prepends "0x". Am I missing something here?


- Neeraj