Re: [PATCH v1 19/27] xen/riscv: emulate guest writes to virtual APLIC MMIO

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/16/26 3:19 PM, Jan Beulich wrote:
> On 14.04.2026 18:04, Oleksii Kurochko wrote:
> > On 4/2/26 4:18 PM, Jan Beulich wrote:
> > > On 10.03.2026 18:08, Oleksii Kurochko wrote:
> > > > +static int cf_check vaplic_emulate_store(const struct vcpu *vcpu,
> > > > +                                         unsigned long addr, uint32_t value)
> > > > +{
> > > > +    struct vaplic *vaplic = to_vaplic(vcpu->domain->arch.vintc);
> > > > +    struct aplic_priv *priv = vaplic->base.info->private;
> > > > +    uint32_t offset = addr & APLIC_REG_OFFSET_MASK;
> > > See ./CODING_STYLE as to uses of fixed-width types.
> > >
> > > > +    unsigned long aplic_addr = addr - priv->paddr_start;
> > > > +    const uint32_t *auth_irq_bmp = vcpu->domain->arch.vintc->private;
> > > > +
> > > > +    switch ( offset )
> > > > +    {
> > > > +    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
> > > And (taking this just as example) any misaligned accesses falling in this range
> > > are fine?
> > Do you mean something like 0x1C02 instead of 0x1C00 or 0x1C04?
> Yes.
>
> > > > +        /*
> > > > +         * As sourcecfg register starts from 1:
> > > > +         *   0x0000 domaincfg
> > > > +         *   0x0004 sourcecfg[1]
> > > > +         *   0x0008 sourcecfg[2]
> > > > +         *    ...
> > > > +         *   0x0FFC sourcecfg[1023]
> > > > +         * It is necessary to calculate an interrupt number by substracting
> > > Nit: subtracting
> > >
> > > > +         * of APLIC_DOMAINCFG instead of APLIC_SOURCECFG_BASE.
> > > > +         */
> > > > +        if ( !AUTH_IRQ_BIT(regval_to_irqn(offset - APLIC_DOMAINCFG)) )
> > > > +            /* interrupt not enabled, ignore it */
> > > Throughout the series: Please adhere to ./CODING_STYLE.
> > >
> > > > +            return 0;
> > > > +
> > > > +        break;
> > > And any value is okay to write?
> > No, it should be in a range
> > [APLIC_SOURCECFG_SM_INACTIVE,APLIC_SOURCECFG_SM_LEVEL_LOW].
> >
> > I will add the check before break:
> >           if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
> >           {
> >               gdprintk(XENLOG_WARNING,
> >                        "value(%u) is incorrect for sourcecfg register\n",
> > value);
> >               value = APLIC_SOURCECFG_SM_INACTIVE;
> >           }
> And why would writing APLIC_SOURCECFG_SM_INACTIVE be any better, when
> that's not what the guest wanted? Simply ignore such writes, unless the
> spec mandates specific behavior for out-of-range avlues?
The spec doesn't mandate specific behavior for out-of-range values but I 
thought it would be better to make irq inactive instead of just ignoring 
so it won't affect somehow potential occurrence of this interrupt.
> > > > +    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
> > > > +        struct vcpu *target_vcpu = NULL;
> > > > +
> > > > +        /*
> > > > +         * Look at vaplic_emulate_load() for explanation why
> > > > +         * APLIC_GENMSI is substracted.
> > > > +         */
> > > There's no vaplic_emulate_load() - how can I go look there?
> > It is introduced in the next patch.
> As before - it should be possible to review patch series strictly
> sequentially. Further, what if this patch gets committed, and the other
> gets delayed by several months?
Got you, I will re-order patches.

> > > > +        if ( !AUTH_IRQ_BIT(regval_to_irqn(offset - APLIC_GENMSI)) )
> > > > +            /* interrupt not enabled, ignore it */
> > > > +            return 0;
> > > > +
> > > > +        for ( int i = 0; i < vcpu->domain->max_vcpus; i++ )
> > > unsigned int
> > >
> > > > +        {
> > > > +            struct vcpu *v = vcpu->domain->vcpu[i];
> > > > +
> > > > +            if ( v->vcpu_id == (value >> APLIC_TARGET_HART_IDX_SHIFT) )
> > > > +            {
> > > > +                target_vcpu = v;
> > > > +                break;
> > > > +            }
> > > > +        }
> > > > +
> > > > +        ASSERT(target_vcpu);
> > > What guarantees the pointer to be non-NULL? The incoming value can be
> > > arbitrary, afaict.
> > I didn't understand your point. It is just checking that target_vcpu has
> > been found. If after for() loop the value of target_vcpu is still NULL
> > then something wrong in Xen.
> If that's true, then the assertion is fine to have. I can't help the
> impression though that a guest could pick a value such that you can't
> possibly find the target vCPU. Asserting on guest controlled input is
> not okay, as was said several times before.
I will then do domain_crash() that as a value is incorrect in case if 
target_vcpu is NULL, I missed that guest could put wrong value.
> > > > +        if ( !(vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM) )
> > > > +        {
> > > > +            vaplic_dm_update_target(cpuid_to_hartid(target_vcpu->processor),
> > > > +                                    &value);
> > > > +        }
> > > > +        else
> > > > +            vaplic_update_target(priv->imsic_cfg,
> > > > +                                 vcpu_guest_file_id(target_vcpu),
> > > > +                                 cpuid_to_hartid(target_vcpu->processor),
> > > > +                                 &value);
> > > I'm struggling with the naming here: When DM is clear, a function with "dm"
> > > in the name is called.
> > it means direct (delivery) mode. Maybe it is better to put dm at the end
> > of the function name? Or it is just better to change it to something else?
> Without a better understanding of what is wanted, all I can say is that
> calling something with "dm" in its name when the condition says it's not
> "dm" is confusing.
Basically it should be the following. If domaincfg.DM (here dm is 
delivery mode according to spec) is 0 then it means that APLIC works in 
direct delivery mode, if DM bit is 1 then MSI delivery mode is used.
So just for clarity I will rename:
- vaplic_dm_update_target -> vaplic_ddm_update_target
- vaplic_update_target -> vaplic_mdm_update_target

Or maybe just s/ddm/direct and s/mdm/msi will be just better in the 
function names.
> > > > +    default:
> > > > +        panic("%s: unsupported register offset: %#x\n", __func__, offset);
> > > Crashing the host for the guest doing something odd? It's odd that the function
> > > only ever returns 0 anyway - it could simply return an error here (if the
> > > itention is to not ignore such writes).
> > But maybe it is a legal offset and we really want to support it?
> Still not a reason to crash the entire host?
Agree, domain crash will be more then enough.

Thanks.

~ Oleksii