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

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

On Wed, Apr 23, 2025 at 10:32:33AM +0200, Jan Beulich wrote:
> 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".

OK, let me send v4 with those adjustments then.

Thanks, Roger.