[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of synthetic interrupt MSRs
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 13 March 2019 14:23 > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx> > Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper > <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap > <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau > Monne > <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini > <sstabellini@xxxxxxxxxx>; > xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk > <konrad.wilk@xxxxxxxxxx>; Tim > (Xen.org) <tim@xxxxxxx> > Subject: RE: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of > synthetic interrupt MSRs > > >>> 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. 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |