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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> ---
>>>  xen/arch/x86/hvm/emulate.c             | 47 +-------------
>>>  xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---
>>
>> It's quite a bit of rather special code you add to this catch-all file. How
>> about introducing a small new one, e.g. mmio.c (to later maybe become home
>> to some further stuff)?
> 
> Yes, I didn't find any suitable place, I was also considering
> hvm/io.c or hvm/intercept.c, but none looked very good TBH.
> 
> The main benefit of placing it in hvm.c is that the functions can all
> be static and confined to hvm.c as it's only user.

I understand that. Still, if we went by that criteria, we'd best put all of
our code in a single file ;-)

>>> +static int cf_check subpage_mmio_write(
>>> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long 
>>> data)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    p2m_type_t t;
>>> +    mfn_t mfn = get_gfn_query(d, addr, &t);
>>> +
>>> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
>>> +    {
>>> +        put_gfn(d, addr);
>>> +        return X86EMUL_RETRY;
>>> +    }
>>> +
>>> +    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
>>> +
>>> +    put_gfn(d, addr);
>>> +    return X86EMUL_OKAY;
>>> +}
>>
>> Unlike the read path this doesn't return RETRY when subpage_mmio_find_page()
>> fails (indicating something must have changed after "accept" said yes).
> 
> Yeah, I've noticed this, but didn't feel like modifying
> subpage_mmio_write_emulate() for this.  The list of partial r/o MMIO
> pages cannot change after system boot, so accept returning yes and not
> finding a page here would likely warrant an ASSERT_UNRECHABLE().
> 
> Would you like me to modify subpage_mmio_write_emulate to make it
> return an error code?

I'd be happy with the two paths being in sync in whichever way that's
achieved. The argument you give equally holds for the read path, after
all.

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

Jan