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

["Roger Pau Monné" <roger.pau@citrix.com>]

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:
> >> On 11.04.2025 12:54, Roger Pau Monne wrote:
> >>> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >>>       */
> >>>      if ( (p2mt == p2m_mmio_dm) ||
> >>>           (npfec.write_access &&
> >>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> >>> +          (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))) )
> >>>      {
> >>>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
> >>>              hvm_inject_hw_exception(X86_EXC_GP, 0);
> >>
> >> Aren't we handing too many things to handle_mmio_with_translation() this
> >> way? At the very least you're losing ...
> >>
> >>> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >>>          goto out_put_gfn;
> >>>      }
> >>>  
> >>> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
> >>
> >> ... the .present check.
> > 
> > Isn't the p2mt == p2m_mmio_direct check already ensuring the entry is
> > present?  Otherwise it's type would be p2m_invalid or p2m_mmio_dm?
> 
> Yes (to the 1st question), it kind of is.
> 
> > It did seem to me the other checks in this function already assume
> > that by having a valid type the entry is present.
> 
> Except for the code above, where we decided to play safe. AT the very least
> if you drop such a check, please say a justifying word in the description.

I've added:

"As part of the fix r/o MMIO accesses are now handled by
handle_mmio_with_translation(), re-using the same logic that was used
for other read-only types part of p2m_is_discard_write().  The page
present check is dropped as type p2m_mmio_direct must have the
present bit set in the PTE."

Let me know if you think that's enough.

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

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?

Thanks, Roger.