Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of synthetic interrupt MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.19 at 14:25, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
>> Jan Beulich
>> Sent: 13 March 2019 13:15
>> 
>> > +    case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
>> > +    {
>> > +        unsigned int sintx = array_index_nospec(idx - HV_X64_MSR_SINT0,
>> > +                                                ARRAY_SIZE(vv->sint));
>> 
>> While here I can see the usefulness of using the local variable (but
>> you're aware of the remaining issue with going this route?), ...
> 
> I guess I'm not aware. Given that using sintx cannot lead to an 
> out-of-bounds access, what is the risk?

The workaround is effective only as long as the variable stays in a
register. If it gets read from memory before use, mis-speculation
is possible again from all we can tell.

>> > @@ -1328,9 +1343,13 @@ int vlapic_has_pending_irq(struct vcpu *v)
>> >           (irr & 0xf0) <= (isr & 0xf0) )
>> >      {
>> >          viridian_apic_assist_clear(v);
>> > -        return -1;
>> > +        irr = -1;
>> >      }
>> >
>> > +out:
>> > +    if (irr == -1)
>> > +        vlapic->polled_synic = false;
>> 
>> I'm struggling to understand the purpose of this flag, and the
>> situation is no helped by viridian_synic_poll_messages() currently
>> doing nothing. It would be really nice if maintenance of a flag like
>> this - if needed in the first place - could be kept local to Viridian
>> code (but of course not at the expense of adding various new
>> hooks for that purpose).
> 
> The flag is there to stop viridian_synic_poll_messages() being called 
> multiple times as you requested. I can move the flag into the viridian code 
> but I'll have to add add extra call to unblock the poll in this case and in 
> the ack function.

Well, in that case it's perhaps better to keep as is.

As to me having requested this - in an abstract way, yes, but to
be honest I couldn't have deduced that connection from the
name you've chosen. "polled_synic" reads to me like reflecting
some guest property. I admit though that I'm also having difficulty
suggesting a better alternative: "synic_polled", "synic_was_polled",
or "sync_poll_pending" come to mind.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel