Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 14:47, Roger Pau Monné wrote:
> On Tue, Apr 15, 2025 at 12:18:04PM +0200, Jan Beulich wrote:
>> On 15.04.2025 12:04, Roger Pau Monné wrote:
>>> On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
>>>> On 15.04.2025 10:34, Roger Pau Monné wrote:
>>>>> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
>>>>>> On 14.04.2025 18:13, Roger Pau Monné wrote:
>>>>>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
>>>>>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
>>>>>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>>>>>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>>>>>>>>>> p2m_mmio_direct.
>>>>>>>>>
>>>>>>>>> But that won't go into hvm_hap_nested_page_fault() when using
>>>>>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
>>>>>>>>> mapped as p2m_mmio_direct)?
>>>>>>>>>
>>>>>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
>>>>>>>>> differently?
>>>>>>>>
>>>>>>>> All true as long as things work as expected (potentially including the 
>>>>>>>> guest
>>>>>>>> also behaving as expected). Also this was explicitly only an example I 
>>>>>>>> could
>>>>>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
>>>>>>>> getting things to handle it's not meant to ever see.
>>>>>>>
>>>>>>> How was access to MMIO r/o regions supposed to be handled before
>>>>>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
>>>>>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
>>>>>>> (~2010), yet I can't figure out how writes would be handled back then
>>>>>>> that didn't result in a p2m fault and crashing of the domain.
>>>>>>
>>>>>> Was that handled at all before said change?
>>>>>
>>>>> Not really AFAICT, hence me wondering how where write accesses to r/o
>>>>> MMIO regions supposed to be handled by (non-priv) domains.  Was the
>>>>> expectation that those writes trigger an p2m violation thus crashing
>>>>> the domain?
>>>>
>>>> I think so, yes. Devices with such special areas weren't (aren't?) supposed
>>>> to be handed to DomU-s.
>>>
>>> Oh, I see.  That makes stuff a bit clearer.  I think we would then
>>> also want to add some checks to {ept_}p2m_type_to_flags()?
>>>
>>> I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
>>> in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
>>> the hardware domain in ~2010 an HVM dom0 was not even in sight?
>>
>> I fear because I was wrong with what I said in the earlier reply: There's
>> one exception - the MSI-X tables of devices. DomU-s (and even Dom0) aren't
>> supposed to access them directly, but we'd permit reads (which, at least
>> back at the time, were also required to keep qemu working).
> 
> Hm, but reads to the MSI-X table for HVM domains will go through QEMU,
> and hence not hit the r/o MMIO path, because the MSI-X table will
> never be mapped to an HVM guest p2m?

Right, just wanted to be on the safe side there.

>>>>>>> I'm happy to look at other ways to handling this, but given there's
>>>>>>> current logic for handling accesses to read-only regions in
>>>>>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
>>>>>>> also handle accesses to MMIO read-only regions.
>>>>>>>
>>>>>>> Arguably it would already be the case that for other reasons Xen would
>>>>>>> need to emulate an instruction that accesses a read-only MMIO region?
>>>>>>
>>>>>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
>>>>>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
>>>>>> needed; while arguably wrong for the introspection case, I'm not sure
>>>>>> that and pass-through actually go together). Hence it's down to
>>>>>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
>>>>>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
>>>>>> which means that if hvm_io_intercept() can't handle it, the access
>>>>>> will be forwarded to the responsible DM, or be "processed" by the
>>>>>> internal null handler.
>>>>>>
>>>>>> Given this, perhaps what you do is actually fine. At the same time
>>>>>> note how several functions in hvm/emulate.c simply fail upon
>>>>>> encountering p2m_mmio_direct. These are all REP handlers though, so
>>>>>> the main emulator would then try emulating the insn the non-REP way.
>>>>>
>>>>> I'm open to alternative ways of handling such accesses, just used what
>>>>> seemed more natural in the context of hvm_hap_nested_page_fault().
>>>>>
>>>>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
>>>>> perspective, that would "just" result in the guest getting a #GP
>>>>> injected.
>>>>
>>>> That's not the part I'm worried about. What worries me is that we open up
>>>> another (or better: we're widening a) way to hit the emulator in the first
>>>> place. (Plus, as said, the issue with the not really tidy P2M type system.)
>>>
>>> But the hit would be limited to domains having r/o p2m_mmio_direct
>>> entries in the p2m, as otherwise the path would be unreachable?
>>
>> I fear I don't follow - all you look for in the newly extended conditional
>> is the type being p2m_mmio_direct. There's no r/o-ness being checked for
>> until we'd make it through the emulator and into subpage_mmio_accept().
> 
> Well, it's a write page-fault of a type with p2m_mmio_direct.  What
> about limiting the path even further by checking for mmio_ro_ranges:
> 
>     if ( (p2mt == p2m_mmio_dm) ||
>          (npfec.write_access &&
>           (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
>            /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. 
> */
>            (p2mt == p2m_mmio_direct &&
>             rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))))) )
>     {

Yes, this would make me quite a bit less nervous.

Jan