Re: [PATCH v3 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2025 10:21, Roger Pau Monné wrote:
> On Tue, Apr 22, 2025 at 08:46:13AM +0200, Jan Beulich wrote:
>> On 17.04.2025 18:23, Roger Pau Monné wrote:
>>> On Thu, Apr 17, 2025 at 05:38:54PM +0200, Jan Beulich wrote:
>>>> On 17.04.2025 17:25, Roger Pau Monne wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/hvm/mmio.c
>>>>> @@ -0,0 +1,125 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * MMIO related routines.
>>>>> + *
>>>>> + * Copyright (c) 2025 Cloud Software Group
>>>>> + */
>>>>> +
>>>>> +#include <xen/io.h>
>>>>> +#include <xen/mm.h>
>>>>> +
>>>>> +#include <asm/p2m.h>
>>>>> +
>>>>> +static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long 
>>>>> addr)
>>>>> +{
>>>>> +    p2m_type_t t;
>>>>> +    mfn_t mfn = get_gfn_query_unlocked(v->domain, PFN_DOWN(addr), &t);
>>>>> +
>>>>> +    return !mfn_eq(mfn, INVALID_MFN) && t == p2m_mmio_direct &&
>>>>> +           subpage_mmio_find_page(mfn);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * The guest has read access to those regions, and consequently read 
>>>>> accesses
>>>>> + * shouldn't fault.  However read-modify-write operations may take this 
>>>>> path,
>>>>> + * so handling of reads is necessary.
>>>>> + */
>>>>> +static int cf_check subpage_mmio_read(
>>>>> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long 
>>>>> *data)
>>>>> +{
>>>>> +    struct domain *d = v->domain;
>>>>> +    unsigned long gfn = PFN_DOWN(addr);
>>>>> +    p2m_type_t t;
>>>>> +    mfn_t mfn;
>>>>> +    struct subpage_ro_range *entry;
>>>>> +    volatile void __iomem *mem;
>>>>> +
>>>>> +    *data = ~0UL;
>>>>> +
>>>>> +    if ( !IS_ALIGNED(len | addr, len) )
>>>>
>>>> What's the point of doing the | ? len can't be misaligned with itself?
>>>
>>> Hm, it's the same form that's used in mmio_ro_emulated_write(), I
>>> assumed it was to catch illegal access lengths, like 3.
>>
>> Oh, I see. But that's not using IS_ALIGNED(), and imo validly so, despite the
>> apparent open-coding. IS_ALIGNED() requires the 2nd argument to be a power of
>> two. The combined check there is folding the power-of-2 one with the is-
>> aligned one.
> 
> Do you think it's worth keeping those checks then?

Yes, I think we should be as strict as possible in what we (try to) emulate.

>  I could do:
> 
> if ( len & (len - 1) || len > 8 || !IS_ALIGNED(addr, len) )
> 
> As a possibly more complete and easier to parse check?

If you dislike the form mmio_ro_emulated_write() uses, sure. However, you
will want to check len to be non-zero, while I'm unsure you need to check
len > 8 - mmio_ro_emulated_write() doesn't have such. Albeit - perhaps
wrongly so; we'd end at the ASSERT_UNREACHABLE() in
subpage_mmio_write_emulate() if a wider store was used. I guess I ought to
make a patch there, and you want to keep the "len > 8".

Jan