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.
