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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4af5212d-a6db-4f14-ace7-c6deb6d0f676@amd.com>
Date: Fri, 8 Nov 2024 23:38:15 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
 dave.hansen@...ux.intel.com, Thomas.Lendacky@....com, nikunj@....com,
 Santosh.Shukla@....com, Vasant.Hegde@....com, Suravee.Suthikulpanit@....com,
 David.Kaplan@....com, x86@...nel.org, hpa@...or.com, peterz@...radead.org,
 seanjc@...gle.com, pbonzini@...hat.com, kvm@...r.kernel.org
Subject: Re: [RFC 04/14] x86/apic: Initialize APIC backing page for Secure
 AVIC



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@....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