Hi Dave,
On 2/10/2023 11:38 AM, Dave Hansen wrote:
> On 2/10/23 08:58, Pavan Kumar Paluri wrote:
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -648,8 +648,8 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
>> unsigned long vaddr_end;
>> int rc;
>>
>> - vaddr = vaddr & PAGE_MASK;
>> - vaddr_end = vaddr + (npages << PAGE_SHIFT);
>> + vaddr_end = PAGE_ALIGN(vaddr + (npages << PAGE_SHIFT));
>> + vaddr = PAGE_ALIGN_DOWN(vaddr);
>
> Could you folks please go take a look at all of the callers that call
> into pvalidate_pages() and set_pages_state()?
>
> I think part of the problem here is that the API is quite inconsistent.
> There have been a few bugs in this area. Ideally, if you have an
> interface that deals in 'pages', then the addresses should be aligned
> already before hitting that interface.
I agree
>
> But, there are messes like this:
>
> static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
> unsigned long paddr, bool decrypt)
> {
> unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> ...
> early_snp_set_memory_shared((unsigned long)__va(paddr), paddr,
> npages);
>
> That's taking a theoretically unaligned 'paddr', page-aligning the size,
> then passing the result into an 'npages' interface. That makes zero
> sense if, for instance, it tried to do:
>
> paddr = 0x0fff
> sz = 2
>
> That would pass along:
>
> paddr = 0x0fff
> npages = 1
>
> npages (the effective end address) is aligned, but paddr is not.
>
> We can keep on hacking around these bugs as they present themselves
> one-by-one. Could you look a bit more widely, please, and see if
> there's something more fundamental that should be done?
We will try to fix paddr and vaddr alignments before they begin to touch
the interfaces.
Thanks,
Pavan
