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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 March 2019 14:23
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim
> (Xen.org) <tim@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.Durrant@citrix.com> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@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