Re: [PATCH v2 09/39] xen/riscv: implement virtual APLIC MMIO emulation

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/2/26 2:31 PM, Baptiste Le Duc wrote:
>> +
>> +static bool vaplic_emulate_store(const struct vcpu *curr, paddr_t addr,
>> +                                 uint32_t value)
>> +{
>> +    const struct domain *currd = curr->domain;
>> +    unsigned int offset = addr & APLIC_CTRL_REGION_OFFSET_MASK;
>> +
>> +    ASSERT(curr == current);
>> +
>> +    switch ( offset )
>> +    {
>> +    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
>> +    case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
>> +    case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
>> +    case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
>> +    {
>> +        unsigned int word_idx =
>> +            regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
>> +
>> +        value &= generate_auth_mask(currd, word_idx);
>> +
>> +        break;
>> +    }
>> +
>> +    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
>> +        if ( value & APLIC_SOURCECFG_D )
>> +        {
>> +            dprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
>> +
>> +            goto fail;
>> +        }
>> +
>> +        /*
>> +         * 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 subtracting
>> +         * APLIC_DOMAINCFG instead of APLIC_SOURCECFG_BASE.
>> +         */
>> +        if ( !AUTH_IRQ_BIT(currd,
>> +                           regoffset_to_word_idx(offset - APLIC_DOMAINCFG)) )
>> +            /* Interrupt not enabled, ignore it */
>> +            return true;
>> +
>> +        if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
> This compares the whole value against 7, not the extracted SM field
> (bits [2:0]). A perfectly legal SM=0 write with any bit set in the
> reserved [9:3] range (e.g. value=8) gets rejected here even though the
> actual field is fine Should be MASK_EXTR(value, APLIC_SOURCECFG_SM) >
> APLIC_SOURCECFG_SM_LEVEL_LOW.

Agree, if() written in this way is incorrect in the way what is going 
before this if().

I think that we don't need it at all and what we want instead is 
ignoring write to others bits then D (bit10) and SM(bits 2:0) as they 
are reserved and read as zeros.

         /*
          * Only D (bit 10) and SM (bits 2:0) are implemented, the rest 
of the
          * bits are reserved and read as zero, so ignore what a guest 
writes
          * to them.
          */
         value &= (APLIC_SOURCECFG_D | APLIC_SOURCECFG_SM);

Probably it make sense to introduce and use it above:

/*
  * All other bits of sourcecfg[] are reserved and read as zero, so drop 
them on a write.
  */
#define  APLIC_SOURCECFG_WMASK          (APLIC_SOURCECFG_D | 
APLIC_SOURCECFG_SM)

And ...

> 
> SM is WARL. If SM invalid but other fields valid, shouldn't reject whole
> write. Instead override value.SM with current valid SM in this branch,
> so other valid fields still get written.

... considering that SM is WARL it means that technically any value 
could be written to this field but read of this register should be 
return always something valid. So considering that in the case of 
SOURCECFG we don't have a shadow copy for vAPLIC and use just real h/w 
we could ignore fully the value it is trying to write to SM field as 
even it is something illegal h/w will choose something legal instead.

If one day we will need a copy of SOURCECFG for vAPLIC we will need to 
do something like this to emulate behavior of real SOURCECFG:

         /*
          * SM is a WARL field. If the guest wrote reserved values (2 or 3),
          * optionally coerce them to a supported default (e.g., 
Inactive/0).
          */
         if ( value == 2 || value == 3 )
             value = APLIC_SOURCECFG_SM_INACTIVE;

For now we can do nothing. I can put TODO:

         /*
          * SM is WARL, so the reserved values 0x2 and 0x3 need no handling
          * here: vAPLIC keeps no shadow copy of sourcecfg[], the value is
          * written straight to the h/w register and is read back from 
it, so
          * it is the h/w which substitutes a legal value for an illegal 
one.
          *
          * TODO: when vAPLIC starts to shadow sourcecfg[], the WARL 
behaviour
          * will have to be emulated here instead, e.g.:
          *   if ( value == 0x2 || value == 0x3 )
          *       value = APLIC_SOURCECFG_SM_INACTIVE;
          */

Note that here it is okay not to use MASK_EXTR as we have always D=0 so 
sourcecfg[i] value is basically SM field (as other bits are reserved and 
are read only)

So my final suggestion is:

      case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
+        /*
+         * Only D (bit 10) and SM (bits 2:0) are implemented, the rest 
of the
+         * bits are reserved and read as zero, so ignore what a guest 
writes
+         * to them.
+         */
+        value &= APLIC_SOURCECFG_WMASK;
+
          if ( value & APLIC_SOURCECFG_D )
          {
              dprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
@@ -169,6 +176,18 @@ static bool vaplic_emulate_store(const struct vcpu 
*curr, paddr_t addr,
              goto fail;
          }

+        /*
+         * SM is WARL, so the reserved values 0x2 and 0x3 need no handling
+         * here: vAPLIC keeps no shadow copy of sourcecfg[], the value is
+         * written straight to the h/w register and is read back from 
it, so
+         * it is the h/w which substitutes a legal value for an illegal 
one.
+         *
+         * TODO: when vAPLIC starts to shadow sourcecfg[], the WARL 
behaviour
+         * will have to be emulated here instead, e.g.:
+         *   if ( value == 0x2 || value == 0x3 )
+         *       value = APLIC_SOURCECFG_SM_INACTIVE;
+         */
+
          /*
           * As sourcecfg register starts from 1:
           *   0x0000 domaincfg
@@ -184,15 +203,6 @@ static bool vaplic_emulate_store(const struct vcpu 
*curr, paddr_t addr,
              /* Interrupt not enabled, ignore it */
              return true;

-        if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
-        {
-            gdprintk(XENLOG_ERR,
-                     "value(%#x) is incorrect for sourcecfg register\n",
-                     value);
-
-            return true;
-        }
-
          break;

Does it make sense? Or I still missing something.




>> +        {
>> +            gdprintk(XENLOG_ERR,
>> +                     "value(%#x) is incorrect for sourcecfg register\n",
>> +                     value);
>> +
>> +            return true;
>> +        }
>> +
>> +        break;
>> +
>> +    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
>> +    {
>> +        struct vaplic *vaplic = to_vaplic(currd);
>> +        struct vcpu *target_vcpu;
>> +        unsigned int guest_hart_idx = MASK_EXTR(value, APLIC_TARGET_HART_IDX);
>> +        /*
>> +         * Look at vaplic_emulate_load() for explanation why APLIC_GENMSI is
>> +         * subtracted.
>> +         */
>> +        unsigned int srcn = regoffset_to_word_idx(offset - APLIC_GENMSI);
>> +
>> +        if ( !AUTH_IRQ_BIT(currd, srcn) )
>> +            /* Interrupt not enabled, ignore it */
>> +            return true;
>> +
>> +        target_vcpu = domain_vcpu(currd, guest_hart_idx);
>> +
>> +        if ( !target_vcpu )
>> +        {
>> +            dprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
>> +
>> +            /* Ignore such writings */
>> +            return true;
>> +        }
>> +
>> +        if ( vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM )
>> +        {
>> +            /*
>> +             * A non-zero guest index asks for delivery to an interrupt file of
>> +             * nested guest. The vIMSIC node has no riscv,guest-index-bits
>> +             * property, so a guest is told its harts have no guest interrupt
>> +             * files and the field is read-only zero for them. The write isn't
>> +             * rejected (that would throw away a valid hart index and EIID);
>> +             * instead the field is dropped, which is also what
>> +             * aplic_msi_target_gen() does with it when programming the h/w.
>> +             */
>> +            if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
>> +            {
>> +                printk_once(XENLOG_WARNING
>> +                            "%pd: vAPLIC target guest index != 0 is unsupported\n",
>> +                            currd);
>> +
>> +                /* Ignore such writes ... */
>> +                return true;
>> +            }
> Comment above this says "The write isn't rejected ... instead the field
> is dropped, which is also what aplic_msi_target_gen() does with it." But
> the code doesn't follow it as it returns true immediately here before
> the write occurred and without zeroing the guest index field.

It looks like the same question in the other thread [1] at the end.

If you don't mind lets continue discussion there. I responded there.

[1] 
https://lore.kernel.org/xen-devel/cover.1787838835.git.oleksii.kurochko@gmail.com/T/#m0de75013bd31481a2f6abd6f36ccccb8ede87a20

>> +
>> +            write_atomic(&vaplic->regs.target[srcn], value);
>> +
>> +            value = aplic_msi_target_gen(target_vcpu, value);
>> +        }
>> +        else
>> +        {
>> +            /*
>> +             * IPRIO is WARL and zero isn't a legal value for it, so normalize
>> +             * it once: the guest then reads back exactly what it gets.
>> +             */
>> +            unsigned int iprio = MASK_EXTR(value, APLIC_TARGET_IPRIO) ?:
>> +                                 APLIC_TARGET_IPRIO_DEFAULT;
>> +            unsigned long h = cpuid_to_hartid(guest_hart_idx);
>> +
>> +            value = MASK_INSR(guest_hart_idx, APLIC_TARGET_HART_IDX) |
>> +                    MASK_INSR(iprio, APLIC_TARGET_IPRIO);
>> +
>> +            write_atomic(&vaplic->regs.target[srcn], value);
>> +
>> +            value = MASK_INSR(h, APLIC_TARGET_HART_IDX) |
>> +                    MASK_INSR(iprio, APLIC_TARGET_IPRIO);
>> +        }
>> +
>> +        break;
>> +    }
>> +
>> +    case APLIC_SETIPNUM:
>> +    case APLIC_SETIPNUM_LE:
>> +    case APLIC_CLRIPNUM:
>> +    case APLIC_SETIENUM:
>> +    case APLIC_CLRIENUM:
>> +        if ( !value || !AUTH_IRQ_BIT(currd, value) )
>> +            return true;
>> +
>> +        break;
>> +
>> +    case APLIC_DOMAINCFG:
>> +    {
>> +        struct vaplic *vaplic = to_vaplic(currd);
>> +
>> +        vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO |
>> +                                 (value & APLIC_DOMAINCFG_WMASK);
>> +
> APLIC_DOMAINCFG_WMASK includes APLIC_DOMAINCFG_DM, so
> the guest can clear DM through this write. But aplic.c:
> aplic_init_hw_interrupts() sets the real hardware APLIC's domaincfg to IE|DM
> exactly once and never touches it again. Is this expected?

Yes, it is expected as we are supporting now only APLIC+IMSIC in Xen and 
it is the reason why we here started to provided a shadow copy of 
domaincfg register for vAPLIC instead of using real APLIC domaincfg 
register.

> 
> Moreover, I saw that d8fbe0bbc7's commit message claims: "a guest's
> domaincfg.DM reads back as a fixed one, so is there situation where we would
> allow direct delivery mode? If not, the else branch should be dropped.
> 

At the moment, we started with a support only when we are working in MSI 
mode but commonly it is possible that IMSIC will be absent and we don't 
have any other choice as started to support delivery mode.

Thanks for review!

~ Oleksii