> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 13 March 2019 14:23
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Julien Grall <julien.gr...@arm.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Roger Pau
> Monne
> <roger....@citrix.com>; Wei Liu <wei.l...@citrix.com>; Stefano Stabellini
> <sstabell...@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.w...@oracle.com>; Tim
> (Xen.org) <t...@xen.org>
> Subject: RE: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of
> synthetic interrupt MSRs
>
> >>> On 13.03.19 at 14:25, <paul.durr...@citrix.com> wrote:
> >> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] 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.
Ah, ok. So, having talked to Andrew it would seem better to immediately
calculate the pointer into the array and use that. I can do that here. In other
cases, as long as the stack variable is only used once, it would seem the
better to way to construct the code.
>
> >> > @@ -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.
>
Given the confusion, I think keeping the flag within the viridian code may well
be better.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
