Re: [PATCH] scsi: lpfc: Properly set WC for DPP mapping

[Mathias Krause <minipli@grsecurity.net>]

Hi Justin,

On 16.01.26 23:33, Justin Tee wrote:
> Hi Mathias,
> 
>>> I don't have any hardware to test this on. I just got the report from a
>>> customer of ours regarding the CONFIG_DEBUG_VIRTUAL BUG_ON(). As I don't
>>> have any spec for the hardware either, I assumed a few things, like:
>>> 1/ DPP regions are only supported on SIL4 devices.
>>> 2/ DPP may be shared with other registers (doorbells?) in the same BAR.
>>
>> Sure, we’ll have close look at this patch and test on real hardware.
>> Will report back on our findings.
> 
> This patch has been tested on real hardware and I/O is stalled when
> using DPP.  We can look for an alternative solution.

Hmm, that's bad. However, that makes me think, making the mapping
write-combining may had been a bad idea from the very beginning?

The thing is, the call to set_memory_wc() won't really do what commit
1351e69fc6db ("scsi: lpfc: Add push-to-adapter support to sli4") wanted
it to do. It does change the PTE flags of the mapping, but it operates
on invalid physical addresses as __pa($VMALLOC_ADDR) just won't work.

Given the time when it was developed, memcpy_{from,to}io() was really
slow on modern systems as these would simply be memcpy() and that was
ASM-alternative'd to a 'rep movsb' if the CPU featured X86_FEATURE_ERMS.

However, the LPFC driver already tries to do "big writes" by making use
of __raw_writeq() instead of memcpy_toio(). That should have solved most
of the latency issues back then already which makes me think, simply
dropping the call to set_memory_wc() is probably the next best option.

> 
> Do we happen to have a dmesg log with the call trace observed?

Unfortunately, we don't. We just have a truncated screenshot with RIP in
__phys_addr(), code and register dump. The surrounding symbolization of
register values we have in grsecurity makes it clear, it's lpfc and, in
fact, the set_memory_wc() call in lpfc_wq_create().

Specifically, it's the call to __pa(addr), which is __phys_addr() under
CONFIG_DEBUG_VIRTUAL, triggering the VIRTUAL_BUG_ON(... ||
!phys_addr_valid(x)) which boils down to BUG_ON() under
CONFIG_DEBUG_VIRTUAL.

> 
> I plan on attempting to reproduce what the customer is observing by
> enabling CONFIG_DEBUG_VIRTUAL, and would be helpful to see context
> from a dmesg log.

Just loading lpfc.ko with enabled CONFIG_DEBUG_VIRTUAL on a DPP
supported platform should trigger the bug as the set_memory_wc() is
unconditionally attempted in this case.

Thanks,
Mathias

> 
> Regards,
> Justin