On 11/6/2024 11:46 PM, Borislav Petkov wrote:
> On Fri, Sep 13, 2024 at 05:06:54PM +0530, Neeraj Upadhyay wrote:
>> @@ -24,6 +25,108 @@ 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);
>> }
>>
>> +static inline u32 get_reg(char *page, int reg_off)
>
> Just "reg" like the other APICs.
>
Ok sure.
>> +static u32 x2apic_savic_read(u32 reg)
>> +{
>> + void *backing_page = this_cpu_read(apic_backing_page);
>> +
>> + switch (reg) {
>> + case APIC_LVTT:
>> + case APIC_TMICT:
>> + case APIC_TMCCT:
>> + case APIC_TDCR:
>> + case APIC_ID:
>> + case APIC_LVR:
>> + case APIC_TASKPRI:
>> + case APIC_ARBPRI:
>> + case APIC_PROCPRI:
>> + case APIC_LDR:
>> + case APIC_SPIV:
>> + case APIC_ESR:
>> + case APIC_ICR:
>> + case APIC_LVTTHMR:
>> + case APIC_LVTPC:
>> + case APIC_LVT0:
>> + case APIC_LVT1:
>> + case APIC_LVTERR:
>> + case APIC_EFEAT:
>> + case APIC_ECTRL:
>> + case APIC_SEOI:
>> + case APIC_IER:
>
> I'm sure those can be turned into ranges instead of enumerating every single
> APIC register...
>
Below are the offset of these, as per "Table 16-6. x2APIC Register" in
APM vol2:
#Reg #offset
APIC_LVTT - 0x320
APIC_TMICT - 0x380
APIC_TMCCT - 0x390
APIC_TDCR - 0x3E0
Above timer related registers are read from HV when we reach the end of this patch
series.
APIC_ID - 20h
APIC_LVR - 30h
APIC_TASKPRI - 80h
APIC_ARBPRI - 90h
APIC_PROCPRI - A0h
APIC_LDR - D0h
APIC_SPIV - F0h
APIC_ESR - 280h
APIC_ICR - 300h
APIC_LVTTHMR - 330h
APIC_LVTPC - 340h
APIC_LVT0 - 350h
APIC_LVT1 - 360h
APIC_LVTERR - 370h
APIC_EFEAT - 0x400h
APIC_ECTRL - 0x410h
APIC_SEOI - 0x420h
APIC_IER - 0x480h
These are few registers like part of LVT (APIC_LVTTHMR ... APIC_LVTERR) ,
priority (APIC_TASKPRI ... APIC_PROCPRI), extended APIC
(APIC_EFEAT ... APIC_ECTRL) which can be grouped.
Intention of doing per reg is to be explicit about which registers
are accessed from backing page, which from hv and which are not allowed
access. As access (and their perms) are per-reg and not range-based, this
made sense to me. Also, if ranges are used, I think 16-byte aligned
checks are needed for the range. If using ranges looks more logical grouping
here, I can update it as per the above range groupings.
>> + case APIC_EILVTn(0) ... APIC_EILVTn(3):
>
> Like here.
>
As this case is for EILVT register range, these registers were grouped.
(I need to add a 16-byte alignment check here).
>> + return get_reg(backing_page, reg);
>> + case APIC_ISR ... APIC_ISR + 0x70:
>> + case APIC_TMR ... APIC_TMR + 0x70:
>> + WARN_ONCE(!IS_ALIGNED(reg, 16), "Reg offset %#x not aligned at 16 bytes", reg);
>
> What's the point of a WARN...
>
>> + return get_reg(backing_page, reg);
>
> ... and then allowing the register access anyway?
>
I will skip access for non-aligned case.
>> + /* IRR and ALLOWED_IRR offset range */
>> + case APIC_IRR ... APIC_IRR + 0x74:
>> + /*
>> + * Either aligned at 16 bytes for valid IRR reg offset or a
>> + * valid Secure AVIC ALLOWED_IRR offset.
>> + */
>> + WARN_ONCE(!(IS_ALIGNED(reg, 16) || IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)),
>> + "Misaligned IRR/ALLOWED_IRR reg offset %#x", reg);
>> + return get_reg(backing_page, reg);
>
> Ditto.
>
> And below too.
>
Same reply as above.
- Neeraj
>> + default:
>> + pr_err("Permission denied: read of Secure AVIC reg offset %#x\n", reg);
>> + return 0;
>> + }
>> +}
>> +
