Re: [PATCH 2/2] x86/hvm: fix write emulation of RO ranges

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Apr 08, 2025 at 03:57:17PM +0200, Jan Beulich wrote:
> On 08.04.2025 11:31, Roger Pau Monne wrote:
> > When running on AMD hardware in HVM mode the guest linear address (GLA)
> > will not be provided to hvm_emulate_one_mmio(), and instead is
> > unconditionally set of ~0.  As a consequence mmio_ro_emulated_write() will
> > always report an error, as the fault GLA generated by the emulation of the
> > access won't be ~0.
> 
> Which means subpage_mmio_write_accept() is flawed, too, on AMD (or more
> generally whenever .gla_valid isn't set).

Oh, yes, good catch.  I didn't notice that one.  We should move all
those checks to use a paddr rather than a gla.

> > Fix this by only checking for the fault GLA in mmio_ro_emulated_write()
> > when the guest is PV.
> 
> This narrows checking too much, imo. For VT-x we could continue to do so,
> provided we pass e.g. npfec down into hvm_emulate_one_mmio(), i.e. make
> the gla_valid flag visible there.

I don't think we should rely on the gla at all in
mmio_ro_emulated_write(), and instead just use the physical address.

> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5187,7 +5187,12 @@ int cf_check mmio_ro_emulated_write(
> >  
> >      /* Only allow naturally-aligned stores at the original %cr2 address. */
> >      if ( ((bytes | offset) & (bytes - 1)) || !bytes ||
> > -         offset != mmio_ro_ctxt->cr2 )
> > +         /*
> > +          * HVM domains might not have a valid fault GLA in the context, 
> > as AMD
> > +          * NPT faults don't report the faulting GLA.  It's also possible 
> > for
> > +          * the fault to happen in non-paging modes.
> > +          */
> > +         (is_pv_domain(current->domain) && offset != mmio_ro_ctxt->cr2) )
> >      {
> >          gdprintk(XENLOG_WARNING, "bad access (cr2=%lx, addr=%lx, 
> > bytes=%u)\n",
> >                  mmio_ro_ctxt->cr2, offset, bytes);
> 
> Is logging the supposed CR2 value useful then for cases where the GLA
> isn't valid? I fear it might be more confusing than helpful.

Since it was a debug printk I was kind of OK with leaving it, but
given the other comments I think I will have to rework
mmio_ro_emulated_write() so that it doesn't use the gla anymore.

Thanks, Roger.